Hi, I've been working on getting regression tests to pass for pic16. Not all working yet, but I'd appreciate some input on whether this is a good direction and where to go next. (These are the support/regression tests, have not looked at the pic-specific tests since it looks like they are mostly intended for pic14 AFAICT.)
I've attached the patches so far, mostly good but still a little WIP. (Details in the patch commit messages too)
Re: setjmp/longjmp - I tested this on the small model, added the necessary tweaks for the large model but did not test since the doc says that model is not fully implemented. There's a TODO - I expected to adjust the stack by 2 bytes for the extra int parameter in longjmp, but it was actually 3 bytes, not sure why. It works with the 3 byte adjustment but I'm not sure where the extra byte came from.
Most tests are working but there are still a significant number of failures:
Running pic16 regression tests
Summary for 'pic16': 94 abnormal stops ( ), 995 failures, 20323 tests, 5810 test cases, 0 bytes, 0 ticks
I haven't checked them all but it looks like 64-bit ints are probably a fair number of them. Where would you suggest to look to implement 64-bit ints in pic16? I can stop it generating E_SHORTLONG for long long on pic16 in SDCCsymt.c, but I'm not sure where to actually implement it, hoping a little advice would save a good chunk of time diving :D
Let me know what you think, thanks!
Good afternoon!
Your work on fixing PIC16 regression testing is very welcome, because it is indeed the first sensible step when it comes to reviving the PIC16 target.
You are correct in assuming that the tests in support/regressions are the relevant ones. AFAIK, the PIC-specific ones are mostly a relic that does not serve much purpose anymore.
64 bit integer support was implemented in SDCC when the PIC backends were already de facto unmaintained.
Unfortunately, I cannot think of anyone who is qualified to give you helpful advice on implementing that for PIC and would therefore recommend to postpone that and to selectively disable the relevant tests.
Either way, the regression test summary you quoted looks very promising, already.
Maybe your fixes can be applied before the code freeze for the planned 4.4.0 release.
Actual bug fixing could then start thereafter.
Let's see what the others think.
Greetings,
Benedikt
For integers wider than 32 bits (I guess
long long
first, maybe later also_BitInt
), you mostly need to check the register allocator (src/pic16/ralloc.c) and code generation (src/pic16/gen.c) for any assumptions about variables being at most 32 bits, and also have any necessary long long support functions in the library.For register allocation, if you're lucky, it might be sufficient to change that
if (sym->nRegs > 4)
in line 2812.Thanks. Your patch 0001 makes the regression test infrastructure work for pic16, so I applied it in [r14489], even though I'm not sure about the crt0i vs crt0iz part.
AFAIK, crt0i does not zero global variables, which would make it non-compliant (according to the C standard, global variables get initialized to 0)? For the non-pic ports, AFAIK, we only zero the memory actually used for global variables. Would it be possible to do that in crt0iz for pic16?
You mention that crt0iz zeroes more memory than what many devices have. Would it make sense to have the emulator emulate a device with more memory (not just because of the zeroing, also because I guess with more memory, we can run more regression tests - especially those long long tests tend to need quite some memory)?
Related
Commit: [r14489]
That's a good idea. I'm not sure if there are any devices with memory all the way up to 0xeff, but maybe crt0iz could just zero up to the limit of the largest-memory device, then we pick one of those devices in the simulator. I'll check it out.
Or try to use the approach outlined here to zero global variables, only.
It looks like gputils does not emit symbols denoting region start/end like sdas does, so I can't zero statics/globals that way (without changing gputils as well, but I'd rather focus on getting the regression tests working).
I can zero the device's actual SRAM range though. I can have sdcc emit a symbol (
_sram_end
, comparable to_stack_end
) and use that in crt0iz.c.Looks like this works, and a few more tests pass for 18f452. However there are ~35 devices saying they have 4096 bytes of SRAM in pic16devices.txt, which isn't right because they should have at least a few SFRs in that space. At the moment they will try to zero SFRs, I can probably go through and fix the RAM amounts for those by comparing with gpsim/datasheets.
There are apparently devices with SFRs below 0xEFF too though, so those should be improved, at least if the ram size is correct or gets fixed.
I've attached the patch but we probably should also get the SRAM sizes fixed for those devices before applying this to avoid breaking anything. I should have a bit of time to go through those soon.
I went through all the pic16 devices that said "ramsize 4096" and corrected them. Mostly used Microchip's product listings since the correct value is right there, in a few cases for products no longer listed I found datasheets and checked the data memory map.
These two patches should be good now if they look good to you also. I still need to check out the reason for the extra byte offset in longjmp.
Well, they don't look obviously bad to me. I'm not familiar enough with pic to state that they are "good". They are in [r14510] now.
Related
Commit: [r14510]
Regarding patch 0003: Where does this 79-character limit on labels come from? The gputils 1.5.2 manual recommends to keep labels less than 256 characters (as this is the limit in .cod files), so I assume it supports labels longer than 79 characters.
The gputils 1.5.0 release notes from 2016 mention a limit on filename length being raised from 64 to 256.
Last edit: Maarten Brock 2023-12-04
Regarding patch 0004: I recommend to put a comment on the
#ifndef
line, e.g. something like#ifndef __SDCC_pic16 // todo: enable when pic16 supports long long
since that is helpful when later looking at which tests to enable when more features get supported by the pic16 port, by doing agrep pic16 tests/*.c
or similar (at least such comments were helpful to me for other features and other ports).Thanks, added that change and rebased it dropping the prior patch that's no longer needed.
Applied in [r14492]. Thanks.
Related
Commit: [r14492]
You're right, I don't think this is needed any more. I had been on Fedora 35 putting off release upgrades when I ran into those problems. Now I'm on Fedora 39 and I no longer need that change, looks like it was addressed in gpasm.
Regarding the 2 vs. 3 question for longjmp: If I had to debug such an issue for a port I'm familiar with, I'd single-step through what happens on setjmp/longjmp in the simulator (ucsim), also comparing to the asm listing (from asxxxx). I guess that is possible with gpsim/gpasm, too?
Last edit: Maarten Brock 2023-12-04
I dug in to learn the details of the pic16 stack model and got setjmp/longjmp implemented properly.
The unexplained 3-byte offset was due to difference in temporary register allocation - setjmp was using 5 bytes (so saved 5 temp registers to the stack), longjmp was using 8 bytes (saved 8 temp registers to the stack). The 3 dummy bytes I pushed were the "missing" saved temp registers, which would then clobber those temp registers in the longjmp epilogue. It didn't have anything to do with the function arguments, those are handled by the caller.
I wrote a test for it and confirmed it, then rewrote setjmp/longjmp as naked functions. The new one passes the new test.
I disabled the new test for other targets that don't pass currently and noted what they seem to be doing. Would you like bugs opened for those? (One for everything or one for each target family?)
If you can merge this patch I think we can close this ticket. I'll look into long long at my next chance, either disabling the tests or implementing it if I can do it in a reasonable amount of time. I'll open another ticket to discuss that when I have something useful. (May be a while with holidays, we'll just have to see how much free time I can muster ;-) )
Thanks!
The patch (without the new extra test) is applied in [r14523].
Related
Commit: [r14523]
Regarding the new test, I think we could come up with a more elegant alternative, and I want to look into the details before opening bug reports for other ports.
In particular, AFAIK the values of non-volatile local variables may change, if they did so after the setjmp. ISO C23 standard, section 7.13.2.1, paragraph 3: "All accessible objects have values, and all other components of the abstract machine) have state, as of the time the longjmp function was called, except that the representation of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call is indeterminate."
For mcs51 the pointers to
buf
anddata
are pushed onto the stack beforesetjmp()
is executed and popped back off after the return. This works OK for the normal call. But whenlongjmp()
returns to thesetjmp()
location those pointers are no longer on the stack. So it's notdata
that is destroyed but the pointer todata
that the compiler thought it could get easily from the stack. These two iTemps should be rematerialized after thesetjmp()
call or properly spilled to the stack before and retrieved after.SDCC does not currently have any keyword or other special internal flag to indicate this.
You have defined STACK_PTR_SIZE in both setjmp.h and setjmp.c. Why not define it in setjmp.c only before including setjmp.h ? There is no need for duplication of identical code.
You're right @spth, I forgot the volatile. That fixes everything except mcs51 without --stack-auto. Attached a new patch for the unit test, but if you have another idea to capture this I'm open to suggestions.
Removed the extra STACK_PTR_SIZE from setjmp.c, thanks for pointing that out @maartenbrock. The header has to define one since it is needed to know the size of jmp_buf, so I just used that one again in setjmp.c, didn't realize I had added that twice.
@maartenbrock Does the mcs51 issue still look the same with volatile?
Fully to my surprise the generated code for the added volatile does look different. It even passes though only by accident.
With
volatile
on the array the address of the array on stack is no longer kept in a register and pushed/popped over thesetjmp()
call. The address of data (BP+5) is now recalculated before every call togenRand()
andcheckRand()
. I see no reason why the compiler should do this as neither BP nor the offset can change.The address of
buf
is still kept in registers and pushed/popped around thesetjmp()
call. But since it is only used after this call in the 0-return case when the popped value is still correct it can properly be passed tolongjmp()
and doesn't create havoc. If thebuf
would be used again at the end the remembered address would be corrupt.This does make me wonder: is the mcs51 backend the only one that tries to keep things in registers by pushing/popping them over function calls?
The stm8, pdk, z80 (and related) ports are all capable of doing so. Since they use the new register allocator, this is guided by the cost function, e.g. when using --opt-code-size, they tend to use register with push/pop only if doing so results in lower code size than spilling to the stack.
I didn't dig deeper into the why, but apparently when
data
andbuf
are volatile their addresses are not saved on the stack, but rematerialized before use. And if I understand @spth correctly, they both should be volatile for the test to be valid.I have now reread C23 7.13 again and my conclusion is that
data
does andbuf
does not need to bevolatile
for them to keep their values.So
setjmp()
needs special attention in the compiler to make sure it doesn´t push/pop intermediate pointers in iTemps over its invocation.