Compiler/MSP430F149: gcc 9.2.0.50 miscompilation, with test case

I think the problem is with using just plain "addr" as the loop condition. I tried a couple of variants.

int vectors_used( void )
{
    int *addr;
    int used = 0;

    for (addr=(void *)0xfffe; addr != (void *)0xffe0; addr--)
      if ( *addr != 0xffff )
	used++;

    return used;
}

Works fine. On the other hand, this results in code for "jump $+0":

int vectors_used2( void )
{
    int *addr;
    int used = 0;

    addr=(void *)0xffe0;
    while(addr)
      {
	if ( *addr != 0xffff )
	  used++;
	addr++;
      }

    return used;
}

An empty main also results in a "jump $+0" for it as well.

> I think the problem is with using just plain "addr" as the loop condition

so obviously i tried "addr != 0" which made no difference to plain "addr". i thought perhaps something funny with address, since some devices have >16 bit addressing (i'm using the 'f149, which is old-school 16-bit). but that made no sense, as you'd still get a wraparound at *some* address.

it is almost as if the compiler is not generating a control depencency for addr++ (amusingly there are several threads on lkml talking about exactly this subject). but again that isn't the whole story, because writing addr not as a pointer but as a regular u16 works:

// this version generates correct code
u16 __attribute__((used)) vectors_used( void )
{
    u16 addr;
    u16 used = 0;

    for (addr=0xffe0; addr; addr+=2) {
        if ( *(u16 *)addr != 0xffff ) {
            P5OUT = 1;
            used++;
            P5OUT = 0;
        }
    }

    return used;
}

so it's clearly something to do with control dependency and pointers, but heck knows what

i did question whether my original code was valid. i'm reasonably experienced in C, and stared at the short test case for a long time trying to figure out what was wrong with the code as written. i came up blank - not seeing anything obvious at all

i dunno. as i wrote, i'm not a compiler expert. but i've recently inherited a 10+yo 13kLOC project that i'm trying to get going with gcc because reasons. and having fixed this bug (which was obvious because it gave a lockup) i'm hitting another lockup much further down the production test sequence. which begs the question what *other* undetected miscompilations there may be in my codebase (i've not yet tried the "-fno-tree-dominator-opts -fno-tree-vrp" trick on the full codebase yet, though i do see it increases code size by slightly over 5%, which is probably fine for my particular case, though it'd be nice if it didn't)

s.

Steve Landy said:

so it's clearly something to do with control dependency and pointers, but heck knows what

u16 __attribute__((used)) vectors_used( void )
{
    const u16 * addr;
    u16 used = 0;

    for (addr=(const u16 *)0xffe0; addr != (const u16 *)(0xfffe + 2); addr++) {
        if ( *addr != 0xffff ) {
            P5OUT = 1;
            used++;
            P5OUT = 0;
        }
    }

    return used;
}

Whereas changing the address terminating the loop to the following doesn't generate an infinite loop:

u16 __attribute__((used)) vectors_used( void )
{
    const u16 * addr;
    u16 used = 0;

    for (addr=(const u16 *)0xffe0; addr != (const u16 *)(0xfffe + 0); addr++) {
        if ( *addr != 0xffff ) {
            P5OUT = 1;
            used++;
            P5OUT = 0;
        }
    }

    return used;
}

Steve Landy said:

i did question whether my original code was valid. i'm reasonably experienced in C, and stared at the short test case for a long time trying to figure out what was wrong with the code as written.

It is worse than just generating an infinite loop. In the tests I did the entire function is reduced to simply "jump $". No other code at all, not even a return.

I wouldn't mind that so much if issued a warning.

Changing things up so the type of "addr" is "int" also lets it work. (Using a cast to make the memory reference work.)

int vectors_used( void )
{
    int addr;
    int used = 0;

    for (addr= 0xffe0; addr; addr++)
      if ( *((int *)addr) != 0xffff )
	used++;

    return used;
}

But I think the best solution is to avoid all of that with:

int vectors_used( void )
{
    int *addr;
    int i, used = 0;
    
    for (addr = (void *)0xffe0, i = 0; i < 16; i++)
      if ( *addr++ != 0xffff )
	used++;

    return used;
}

When I look at the code generated I find that the variable "i" has been vanished and the compiler uses the pointer as the loop variable. Getting the termination at zero correct even if it does issue a redundant comparison to zero after the increment.

Chester Gillon said:

From a quick search it looks like it is undefined behaviour if a pointer wraps - see Pointer overflow check

firstly, thanks for that link - that's a really interesting project that may turn into a rabbit hole for me! it's a shame it references the standard but not a specific section so i dug out my copy. i think the relevant bit is where it talks about additive operators:

ISO-9989 section 6.3.6 said:

For the purposes  of these operators.  a pointer  to a nonarray  object  behaves  the same as a
pointer  to the first element  of an array of length  one with  the type of the object  as its element
type.

...

If both  the pointer  operand  and the result  point  to elements  of the same array
object.  or one past the last element  of the array  object,  the evaluation  shall  not produce  an
overflow;  otherwise,  the behavior  is undefined. 

so i think i lost as soon as i wrote addr=(const u16 *)0xffe0

i get that i specifically asked the compiler to optimise for size, and i accept that my code as written relies on undefined behaviour, but damn... that's not very friendly from a quality of implementation point of view. i agree the lack of a warning is quite egregious.

s.

well

to paraphrase, what i'm hearing back is "yes, gcc miscompiles. but your code invokes undefined behaviour, so tough"

the real problem is that it miscomples silently

so unless a gcc person is willing to step up and own this - and based upon past form I'm not going to hold my breath - then I think the correct solution is to just simply not use gcc

s.

Don't read too much into it. TI employees are so keen on closing out threads that I suspect job performance ratings are tied to it.

The byte op problem has been around a long time. I compared the code (tc-msp430.c and opcode/msp430.h) from the current version of binutils to one from the mspgcc days. Neither checks to see if a byte operation makes sense. The instruction table appears to contain no information that would make this check simple so it would have to be a special case applied to:

 2816     case 'b':
 2817       /* Byte operation.  */
 2818       bin |= BYTE_OPERATION;
 2819       byte_op = TRUE;
 2820       check = TRUE;
 2821       break;

This is a generic choice by GCC to not warn and optimize in this way. If you change "u16" to uintptr_t you can observe the same behavior on your host system.

GCC warns about a lot of undefined behavior, with -Wall and -Wextra, I'm not sure why this case isn't one of the things it warns about. It should warn about signed pointer overflow, but not unsigned pointer overflow.

You could make a post on GCC bugzilla (make it generic to an x86 or x86_64 system), and you may either get some background on why GCC specifically does not warn for this, or perhaps it just went under the radar and someone will add the warning.

I'll also put this on the MSP430-GCC backlog of things to look at.

It's also worth noting that the complaints about these pointer overflow optimizations have been around for a while. You may find the discussion in the comments of this article interesting: https://lwn.net/Articles/278137/

Given this issue has been a point of contention for a while, I suspect a warning was never added because it generates too much noise. Perhaps the runtime sanitizer catches it with -fsanitize=undefined, however, the GCC runtime sanitizers are not supported for MSP430.

that lwn article is interesting, thanks for the link! (there's a few tangential but enlightening bits in there, such as "C is not so low level that integer types wrap around")

i guess if gcc doesn't emit a warning then the question becomes "what sca tools would flag this as being questionable?". or, i guess, delegate testing to our customer... that's what they're for, right? :) but at this point i guess we're heading offtopic

(brief history: this has been in production for over a decade, and hasn't even been touched for 9+ years. it's only come back to life due to obsolete hardware. originally built using quadravox, with a large-but-not-complete effort to get it going with crossworks somewhere along the line. and now i get to pick up the pieces...)

Steve Landy said:

i guess if gcc doesn't emit a warning then the question becomes "what sca tools would flag this as being questionable?".

I don't have any recommendations for that I'm afraid.

GCC has a built-in static analyzer (-fanalyzer) which is in its infancy so doesn't detect too much yet. Perhaps that could be extended to warn about this.

Or maybe it is possible to add a warning when GCC spots this opportunity for optimization, and removes the code it thinks will never execute.

The latter might not be possible, given that the optimizations performed on GCC's internal representation of the code are modular, by the time this optimization is performed the code may not be able to be traced back to the specific undefined behaviour you used in your code. What I mean is, if earlier optimizations coerce valid code into a form that looks the same as your code that invokes undefined behavior, GCC can't warn about it because it would generate a lot of noise and/or invalid warnings.

I am only speculating for now, I'll need to have a proper look at the mechanics of this optimization before I can give an accurate answer.