#468 pic16: Fixing regression tests

[Jonathon Hall]

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)

1. Fix execution of regression tests - hook up missing EMU* variables and use crt0i.o since the simulator does not like zeroing memory that does not exist (crt0iz.o)
2. Implement setjmp/longjmp, a handful of tests needed this (working but still a TODO, below)
3. Adjust some templated tests that exceeded the 79-character label limit in gpasm
4. Disable unsigned long long test in sdcccall.c.in (but maybe better to implement 64-bit ints instead?)

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!

[Benedikt Freisen]

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

[Philipp Klaus Krause]

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.

[Philipp Klaus Krause]

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)?

[Jonathon Hall]

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.

[Benedikt Freisen]

Or try to use the approach outlined here to zero global variables, only.

[Jonathon Hall]

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.

[Jonathon Hall]

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.

[Philipp Klaus Krause]

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.

[Philipp Klaus Krause]

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.

[Philipp Klaus Krause]

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 a grep pic16 tests/*.cor similar (at least such comments were helpful to me for other features and other ports).

[Jonathon Hall]

Thanks, added that change and rebased it dropping the prior patch that's no longer needed.

[Philipp Klaus Krause]

Applied in [r14492]. Thanks.

[Jonathon Hall]

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.

[Philipp Klaus Krause]

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?

[Jonathon Hall]

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!

[Philipp Klaus Krause]

The patch (without the new extra test) is applied in [r14523].

[Philipp Klaus Krause]

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."

[Maarten Brock]

For mcs51 the pointers to buf and data are pushed onto the stack before setjmp() is executed and popped back off after the return. This works OK for the normal call. But when longjmp() returns to the setjmp() location those pointers are no longer on the stack. So it's not data that is destroyed but the pointer to data that the compiler thought it could get easily from the stack. These two iTemps should be rematerialized after the setjmp() 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.

;buf                       Allocated to stack - _bp +1 +4 
;data                      Allocated to stack - _bp +5 +16 
;   genAddrOf
    mov a,_bp
    add a,#0x05
;   genCast
    mov r7,a
    [...]
;   genIpush
    push    ar7                 ; <== push &data
    [...]
;   Peephole 300    pop ar7 removed
;   cases/./../tests/setjmp.c:155: if(!setjmp(buf))
;   genAddrOf
    mov r6,_bp
    inc r6
    [...]
;   Peephole    push ar7 removed
    push    ar6                 ; <== push &buf
    lcall   ___setjmp
    mov a, dpl
    mov b, dph
    pop ar6                     ; <== Oops: pop &data
    pop ar7                     ; <== Oops: pop &buf
                                ; These stack locations may now be overwritten!
                                ; And the ISR will do so!

;   cases/./../tests/setjmp.c:160: longjmp(buf, 1);
    [...]
    lcall   _longjmp
    dec sp                      ; <== Unnecessary, _NoReturn
    dec sp                      ; <== Unnecessary, _NoReturn

[Maarten Brock]

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.

[Jonathon Hall]

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?

[Maarten Brock]

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 the setjmp() call. The address of data (BP+5) is now recalculated before every call to genRand() and checkRand(). 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 the setjmp() 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 to longjmp() and doesn't create havoc. If the buf 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?

[Philipp Klaus Krause]

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.

[Maarten Brock]

I didn't dig deeper into the why, but apparently when data and buf 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.

[Maarten Brock]

I have now reread C23 7.13 again and my conclusion is that data does and buf does not need to be volatile 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.