#3672 mcs51: gptr comparison ignores high byte

[Oleg Endo]

The following function

bool check_ptr (const void* gptr)
{
  return gptr != 0;
}

as per SVN r14396, copmiled with

sdcc -c --std-sdcc2x -mmcs51 --model-large --stack-auto --fomit-frame-pointer

    mov r5,dpl
    mov r6,dph
    mov a,r5         <<<<
    orl a,r6         <<<<
    cjne    a,#0x01,00103$
0103$:
    cpl c
    mov b0,c
    clr a
    rlc a
    mov dpl,a
    ret

Only the two lower bytes of the gptr are checked for zero and the highest 3rd byte is ignored. This seems wrong. It will wrongly return false if the gptr is e.g. a code pointer like 0x810000, which is clearly not 0.

Also related https://sourceforge.net/p/sdcc/feature-requests/892/

[Maarten Brock]

But it is...
SDCC treats the different mcs51 memory spaces as address spaces as defined in the Embedded C standard.
And any null pointer (pointing to an address zero in its own address space) equals any other null pointer.

[Oleg Endo]

So a memory space of __code in bank 0 is considered a different memory space than __code in bank 1? I can hardly imagine. Intuitively I'd think of the whole __code address space as a single space, but it's just larger than 64 KByte.

With code banking enabled, if we do something like this:

bool check_ptr (const void* gptr) __banked
{
  return gptr != 0;
}

void some_other_func (void) __banked
{
  return check_ptr ("hello bogus nullptr check");
}

If the string literal constant falls onto a 64K bank boundary it will return wrongly false. With this kind of stuff around the whole banking support in SDCC is quite difficult to make use of.

Please reconsider re-opening this issue.
You can move it to "feature request", just like https://sourceforge.net/p/sdcc/feature-requests/892/

[Maarten Brock]

Are you saying that you have a construction where you swap out the reset and interrupt vectors of the mcs51? Remember the DPTR part points to the memory map as the mcs51 sees it, not the underlying memory chip. So if you have e.g. 128kB flash with A0-A15 wired directly and A16 on a gpio, then I strongly recommend to fill the first part of each 64kB with identical code for the IVT and HOME areas.

Also note that this code is only generated for comparison against a constant null pointer. All other generic/banked pointer comparisons are handled by gptr_cmp.asm which does compare B for non-null pointers.

[Oleg Endo]

Are you saying that you have a construction where you swap out the reset and interrupt vectors of the mcs51?

Yes, in fact I'm working on/with such a system where the whole 64 KByte bank is switched (although I'm not sure how it matters here). The common bank thing has to be implemented in software. It's been working ok-ish, although SDCC could definitely use some improvements to make it less painful to use.

Also note that this code is only generated for comparison against a constant null pointer. All other generic/banked pointer comparisons are handled by gptr_cmp.asm which does compare B for non-null pointers.

Thanks, good point. I saw your comment in the code. I get your idea now.

[Maarten Brock]

It matters because you could then reuse address 0 to e..g store the constant string as you described. I strongly recommend not to do that. Keep the reset and interrupt vectors available at all times. In a case like this that means copying them to the start of every bank. The same goes for everything in the HOME area such as gptr_get/put and the bankswitching trampoline.

[Oleg Endo]

Well yeah, that's obviously already in place. Otherwise it wouldn't work at all (almost).

My case was actually having only part of the flash used for banked code while some other parts used for other data. I was trying to use void gptr to have a more efficient representation of the 23-bit pointer. E.g. something would return either 0x060000 or nullptr and it would always fail the != nullptr check. Using uint32_t is the better choice here.

[Maarten Brock]

If you already have that in place then why are you asking about placing "hello bogus nullptr check" at code address 0x0000? If the reset vector is always present at 0x0000 no matter which code bank is active then how can you have a text string there?

Also note that a generic pointer with value 0x060000 points to __xdata, not to __code. There is no support for xdata bankswitching in SDCC yet.
And even then 0x060000 would still be a null pointer. It points to something that needs bank 6 selected in xdata and then can be accessed with DPTR=0x0000.

Instead of uint32_t you can also use uintptr_t which in the future might change to use a 24 bit integer representation. Or you can use _BitInt(24) directly already.

[Oleg Endo]

If you already have that in place then why are you asking about placing "hello bogus nullptr check" at code address 0x0000? If the reset vector is always present at 0x0000 no matter which code bank is active then how can you have a text string there?

Yes, you're right. The example was a bad one.

Also note that a generic pointer with value 0x060000 points to __xdata, not to __code.

Yes, I know that. Forgot to set the MSB when writing it. It should have been 0x860000.

And even then 0x060000 would still be a null pointer. It points to something that needs bank 6 selected in xdata and then can be accessed with DPTR=0x0000.

Yes, I get it now. Looking at it that way makes total sense.

Instead of uint32_t you can also use uintptr_t which in the future might change to use a 24 bit integer representation. Or you can use _BitInt(24) directly already.

That's cool. Thanks for the hint!