GCC v11 32F4: compiler warning depends on optimisation level

[Nominal Animal]

In arm32 I think it's up to 32 bit size objects?

It is true that for composite objects (structures with multiple members) at most 32-bit ones are returned in registers, with larger ones stored at an address provided by the caller in r0, but for vectors up to 128 bits can be returned in registers (r0-r3).  See here for details.

When software supports both IPv4 and IPv6 addresses, then the address part is 128-bit, and IPv4 addresses have the first 80 bits zeroes, next 16 bits ones, and the last 32 bits contain the IPv4 address.  (This is standard IPv6-mapped IPv4 address, typically described using ::ffff:a.b.c.d notation, where a.b.c.d is the corresponding IPv4 address.)

Thus, if you have

Code: [Select]

typedef uint32_t  ip_addr_t  __attribute__((__vector_size__(16)));

ip_addr_t zero_address(void) { return (ip_addr_t){0}; }
the function sets registers r0-r4 to zero; whereas

Code: [Select]

typedef union {
    uint64_t  u64[2];
    uint32_t  u32[4];
    uint16_t  u16[8];
    uint8_t   u8[16];
} ip_addr_t;

ip_addr_t zero_address(void) { return (ip_addr_t){ .u64 = { 0, 0 } }; }
stores zeroes at r0+0..r0+15, inclusive.

The non-standard vector_size attribute is supported by both GCC and Clang.

If the vector definition is used, then

Code: [Select]

typedef uint32_t  ip_addr_t  __attribute__((__vector_size__(16)));

ip_addr_t ipv4_address(const uint8_t a, const uint8_t b, const uint8_t c, const uint8_t d) {
    return (ip_addr_t){ 0x00000000,
                        0x00000000,
                        0x0000FFFF,
                        (((uint32_t)a) << 24) |
                        (((uint32_t)b) << 16) |
                        (((uint32_t)c) << 8)  |
                          (uint32_t)d };
}
returns the IPv6-mapped IPv4 address in registers r0-r3 on 32-bit arm-none-eabi.  Clang generates nice code for it, but GCC uses r4 and r6 needlessly, and pushes r4-r10 and fp temporarily to stack.  (When using godbolt.org, remember to choose "unknown-eabi" or "none" GCC versions, as the "Linux" one uses SysV ABI instead.)

[peter-h]

I posted the typedefs earlier e.g.

Code: [Select]

/** This is the aligned version of ip4_addr_t,
   used as local variable, on the stack, etc. */
struct ip4_addr {
  u32_t addr;
};

So I am not using the more generic form of ipaddr which is a struct which can hold the single uint32_t (ipv4) or the ipv6 version.

To support ipv6 I would need to rebuild LWIP with a different #define and re-code lots of code which processes and displays the IP. The decision was made to not support ipv6.

[Nominal Animal]

I posted the typedefs earlier e.g.

Code: [Select]

/** This is the aligned version of ip4_addr_t,
   used as local variable, on the stack, etc. */
struct ip4_addr {
  u32_t addr;
};


Apologies, I missed that.

Yes, this will be returned in r0 on arm-none-eabi AKA aacs32 (which is what I believe you are using), and passed in a single register when supplied as a parameter (ip4_addr_t addr).


To repeat, the warning issue you are seeing is not because the cause varies at different optimization levels; it is just that at some optimization levels the compiler just happens to not detect the pattern and thus omits the warning, most likely because the pattern is spread over too much code (which gets eliminated and thus pattern more compact when optimizations are enabled).

You can see something similar when you examine the code GCC generates for the last function in my previous message: it pushes a lot of extra registers on stack, because when not optimized, it uses those registers.  Register use is a bit of a weak point in GCC, and has always been, so current optimization schemes still cannot fully optimize it in this case.  Plus, it apparently does not notice those registers are no longer modified, so doesn't remove them from the saved+restored set.  So, when looking at say -O2 optimized code, those extra registers it pushes and pops look insane.

Similarly, because GCC doesn't see the warnings as something it has to check, but something it may do, it doesn't spend much resources in trying to detect the pattern causing the warning; therefore only when the pattern is easily discernible, will it detect and emit the warning.  (In other words, the lack of the warning is not an indication that the return value is initialized.  It is only best-effort warning, nothing conclusive.)

Also, whether ipaddr is initialized in the caller or not does not and should not affect the warning, because the warning is about returning an uninitialized value from a function.  It does not matter that the ABI might pass the result by reference and the immediate caller clears it to zero first, because the compiler must not assume it sees all callers while compiling this file.  (If the function had only local linkage, i.e. it was static, then it would be different, as the compiler would indeed see all callers while compiling this file.)

[peter-h]

OK; I accept all this, as much of it is above my pay grade

Let me just say that file_get_config_value_default() is in the same .c file as the function under discussion above. But not everything that file_get_config_value_default() itself calls is in the same .c file.

[Nominal Animal]

OK; I accept all this, as much of it is above my pay grade

No, no, it really is a simple thing: GCC does not and cannot guarantee it emits the warning, as it is best-effort (or rather, "some effort") only.  Basically, the warning is emitted only when GCC detects the pattern, but it doesn't really try that hard to detect it.  Thus, the difference between optimization levels is the effort needed to detect the problem in the internal representation.

So, it's just like a human proofreader: it may still miss it, especially if the code involved yields a complex internal representation (in GENERIC or GIMPLE), just like a proofreader may miss a minor problem in a complex sentence.  If someone else cleans up the sentence structure first, that same proofreader is much more likely to detect that minor problem.

Also, I forgot that ##netconf_get_ip_config_value() was indeed marked static, so the last two sentences in my above post should be replaced with "Even if the ABI is such that the return value is allocated by the caller, the address passed to this function, this function just conditionally filling the value, does not change the fact that the problem is in this function and not the caller."

You would need to change the function prototype to
    static void ##netconf_get_ip_config_value(ip_addr_t *dst, char *name, char *default_name, int len)
replacing number with len, &ipaddr with dst, and omitting the return line, for the warning to occur in the caller.  The simple reason for this is that the warning is intended to help the developer to check the source code validity (with respect to and in terms of the abstract machine as defined in the C standard), not to check whether the generated machine code is sane or not.  Thus, the ABI used should not affect the warnings.  (It will somewhat, though, because the ABI affects the complexity in the internal representation.)

[SiliconWizard]

I'll add that while GCC warnings should still not be ignored, they don't replace a full-blown static analyzer. Clang already does a much better job at it.
Using a dedicated analyzer is always useful IMO. Some are free, others can be commercial and very expensive.
One cheap (it's open & free & moderately good, but still very useful) option is CppCheck. Highly recommended. It will catch much more than GCC and (usually) than Clang as well, although on some specific cases, I've found Clang to catch stuff that CppCheck didn't.

[peter-h]

How does one run one of these? The project is after all a makefile, which is in this case hidden inside Cube IDE. Or maybe it just looks at all the .c and .h files it can find?

[Siwastaja]

How does one run one of these? The project is after all a makefile, which is in this case hidden inside Cube IDE. Or maybe it just looks at all the .c and .h files it can find?

cppcheck just processes the source files you give to it. It couldn't be much simpler to run:

cppcheck .
checks the source files in that directory and below.

Note it doesn't catch everything either but is a good addition. If you are serious about product quality then adding as many tools as you can imagine is kinda low-hanging fruit (maybe except those that generate way too much false positives and therefore produce workload for you which could be better spent testing traditionally). It doesn't replace proper automated (and manual) testing, good planning, reviews etc. of course.

[SiliconWizard]

Yep. You can customize the files it checks, but with just a '.', it will check all .c/.cpp/.h files automatically. You may want to run it at the appropriate level of directory hierarchy if you want it to avoid checking some files, like maybe some third-party library that you don't intend to bother checking and even less so fixing.

[peter-h]

I've installed an run cppcheck (the GUI mode) and it goes through the project (I added the inc and src directories) but it seems to report nothing. I see a few popups like



but no way to view anything. Under Warning details there is nothing, and under the Analysis log there is nothing but a list of modules. It doesn't find the .h files; a config of the inc path doesn't help.

Edit: it does show some warnings; nothing important. Stuff like implicit casts. It finds some curious stuff e.g. "sportability: Shifting a negative value is technically undefined behaviour [shiftNegativeLHS]" but why is this a problem? Probably a new thread topic

It picks up a ton of stuff which has no effect e.g. "Variable 'long_deg' is assigned a value that is never used. [unreadVariable]" when I zero stack variables (good practice, surely?) but the init value (0) is never used.

[Siwastaja]

"sportability: Shifting a negative value is technically undefined behaviour [shiftNegativeLHS]" but why is this a problem? Probably a new thread topic

Are you seriously asking why UB is a problem? See nasal daemons.

Finding UB is one of the primary reason tools like cppcheck would be used. Just fix if you have time. If not, freeze your tools and hope for the best.

Otherwise than that, don't overthink it. Language rules are what they are and you have to live with that. Be happy it's standardized unlike some "replacements"!

[peter-h]

I don't mean that UB is not worth checking out. I mean why an int32_t cannot be shifted. Isn't it just a pattern of 32 bits, for the purposes of the shift? Should it first be cast into a uint32_t?

If the left shift was implemented as a x2 x4 etc signed multiply then yes it won't make sense. But I would expect it is done with a barrel shifter which just grabs the whole 32 bits and moves it left. That seems to be what actually happens e.g.

int32_t pkt_value = -32767
pkt_value = pkt_value << 13
pkt_value &= 0x1fffe000;

works as expected. How would this be done correctly? Presumably a cast before the shift. The application is weird: assembling arinc429 packets.

I can see why somebody decided shifting signed ints should be UB but is there any CPU on which that is actually true, and why? I'd like to know since why would any compiler writer do anything other than just shift the 32 bit field presented? The intent of the programmer is a shift, simply.

Every day is a school day

[JPortici]

I mean why an int32_t cannot be shifted. Isn't it just a pattern of 32 bits, for the purposes of the shift? Should it first be cast into a uint32_t?

Yes.
Because, what do you do with the sign bit? Do you keep it? Do you zero it? You are assuming that the underlying instruction set has an "arithmetic shift right" instruction (right shift, if previous MSB is 1 shift 1 in) which may not be the case, and that you are using >> for modulo division in this context, or to shift bits in another context, but when which is which? You know it, the compiler doesn't.

var /= 2 is unambigous (and will definetly be mapped to an arithmetic right shift if such instruction is available). var >>= 1, not so much.

this is why it's undefined behaviour (or implementation defined? on microchip's compilers they have an implementation-defined behaviour chapter, sign of right shift is there)

[peter-h]

if previous MSB is 1 shift 1 in

No. AIUI, C shifts are always "shift in zeroes" so (taking an 8 bit example) if you shift 10000000 1 right, you get 01000000. I think that is called a logical shift right.

I had one C coder explain this to me. C doesn't have any of the handy asm shifts like shifts/rotates via the carry, etc. You have to hack them all. Shifts always fill in zeroes.

The thing is that really both these lines should be UB for a signed int

pkt_value = pkt_value << 13
pkt_value &= 0x1fffe000;

because both mess with the sign bit in a way which makes no sense if you want a meaningful signed int afterwards.

Should it be like this

pkt_value = (uint32_t) pkt_value << 13
pkt_value &= 0x1fffe000;

but then the result of line 1 becomes really subtle.

OK here's a funny one. This piece of code is all over the internet

Code: [Select]

// Utility for the file listing - get file size units string

static void printsize(size_t size, char *out)
{
    static const char *SIZES[] = { "B", "kB", "MB", "GB" };
    size_t div = 0;
    size_t rem = 0;

    while (size >= 1024 && div < (sizeof SIZES / sizeof *SIZES)) {
        rem = (size % 1024);
        div++;
        size /= 1024;
    }

    if(div == 0)
    sprintf(out, "%iB\n", size);
    else
        sprintf(out, "%.1f%s\n", (float)size + (float)rem / 1024.0f, SIZES[div]);
}
but it doesn't like it:
warning: Either the condition 'div<(sizeof(SIZES)/sizeof(*SIZES))' is redundant or the array 'SIZES[4]' is accessed at index 4, which is out of bounds. [arrayIndexOutOfBoundsCond]

What is wrong with it? Is it that it works only up to a certain number of GB (perhaps 1024) and then it bombs?

[JPortici]

No. AIUI, C shifts are always "shift in zeroes" so (taking an 8 bit example) if you shift 10000000 1 right, you get 01000000. I think that is called a logical shift right.

Note that i explicitely mentioned "arithmetic" and not logical.

Also, from my favourite compiler's manual "Implementation Defined Behavior" chapter

What is the result of a right shift of a negative-valued signed integral type? (ISO 6.3.7)
The sign is retained.

[Siwastaja]

I mean why an int32_t cannot be shifted. Isn't it just a pattern of 32 bits,

But shifting is not just shifting bits! Many people expect shifting to be equal to division or multiplication by 2^n, which for signed types is different than just the literal meaning of shifting bits. And this assumption is sometimes wrong, sometimes correct.

If you really want to "just shift" the bits for whatever reason, then cast into unsigned. If you want to multiply/divide, then it's a good idea to use * or / operator for that, and let the compiler optimize it to the most suitable shift operation (if that exists on the target architecture).

In the end, this does not matter. Standard saying it's UB means it's UB. You should not do it, if you do, you take the risk.

[Siwastaja]

No. AIUI, C shifts are always "shift in zeroes" so (taking an 8 bit example) if you shift 10000000 1 right, you get 01000000. I think that is called a logical shift right.

This is not true. Whoever taught you this was simply wrong.

Don't use right shift on signed types if you care about which type of shift it is. And you probably do care.

[peter-h]

So a right shift of a negative signed value introduces 1s. And a left shift of a negative signed value introduces zeroes, and just drops off bits at the left end.

It turns out this is OK in my code because I mask off those bits anyway and merge in other ones (which are not related to the number in question). The reason this was not picked up earlier is because I exhaustively tested the various boundary conditions and all worked fine.

pkt_value = pkt_value >> 3;
pkt_value &= 0x1fffffff;  // this clears any 1s introduced from the left

Like I say - every day one learns something.

[JPortici]

So a right shift of a negative signed value introduces 1s.

Nooooooooooooooooooooooooooooooooooo

it's IMPLEMENTATION-DEFINED BEHAVIOUR
It retains the sign for this particular compiler, for this particular version, for this target architecture. No guarantees that it could change in the future, no guarantees it's going to be the same for another target.

[peter-h]

OK; last 2 posts crossed. I do actually mask all "new" bits after a right shift, so it doesn't matter if they are 1s or 0s. Just checked all the code...

That is not the same as saying the right shift is not UB. But at least it doesn't matter what the new bits are.

Cppcheck is very handy - thank you all!

[coppice]

So a right shift of a negative signed value introduces 1s.

Nooooooooooooooooooooooooooooooooooo

it's IMPLEMENTATION-DEFINED BEHAVIOUR
It retains the sign for this particular compiler, for this particular version, for this target architecture. No guarantees that it could change in the future, no guarantees it's going to be the same for another target.

People have gotten so used to modern C compilers treating signed values with arithmetic shifts and unsigned values with logical shifts, most have lost sight of this being undefined behaviour. it is, however, something you can rely on quite well.. A vast amount of code relies on it, and would break if compiler behaviour changes.

[peter-h]

Interesting explanation: https://www.mathworks.com/help/fixedpoint/ug/use-of-shifts-by-c-coders.html

[golden_labels]

A lot of arguing about how bits could be shifted, if 0s or 1s should be added, what is “reasonable”, what is undefined, strictly-defined, or implementation-defined. But seemingly everybody forgotten, that in C operators << and >> are not defined in terms of bits. While “bit shift” is mentioned, giving “the feel” of the operation, the actual behavior is defined in terms of numeric value and traditional arithmetic. All that reasoning and arguing is meaningless in C.

Operation x << B is multiplication by 2^B, while x >> B is division by 2^B and taking the quotient. Both cases are strictly defined exclusively for non-negative numbers, with the latter case also allowing implementation-defined behavior for negative ones. There is no bits involved at all.

Both the original C manual and “The C programming language” described those operations in terms of bit shifts. However, also left them underspecified, allowing for either binary or arithmetic interpretation, limiting to particular type representations, and — in contradiction to bit shift perspective — putting a limitation on negative right operands. This notion has been dropped when C became adapted for a wider range of systems, choosing the arithmetic definition.

[ejeffrey]

But seemingly everybody forgotten, that in C operators << and >> are not defined in terms of bits. While “bit shift” is mentioned, giving “the feel” of the operation, the actual behavior is defined in terms of numeric value and traditional arithmetic. sing the arithmetic definition.

That's a pretty silly argument.  The operators are called right and left shift, and then further defined in a mathematically equivalent fashion.

It's also incorrect.  From the ISO standard, under the shift operator semantics heading:

The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has
an unsigned type, the value of the result is E1 x 2^E2 , wrapped around. If E1 has a signed type and
nonnegative value, and E1 x 2^E2 is representable in the result type, then that is the resulting value;
otherwise, the behavior is undefined.

The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a
signed type and a nonnegative value, the value of the result is the integral part of the quotient of
E1/2^E2 . If E1 has a signed type and a negative value, the resulting value is implementation defined.

It's clearly explained both in terms of bitwise shifts and the mathematically equivalent multiplication.

[golden_labels]

That’s “pretty silly” because? Like with most “it’s silly” reactions: because it doesn’t agree with the mental model you hold?

You know, what I find silly? You see: you may call me a total idiot, if you wish. But neither cherry picking phrases from the standard nor pretending I didn’t refer to those phrases is going to help. Because what we face is a much simpler thing: those operators don’t work as bit shifts would. We wouldn’t be even talking right now, if they would. They work in numerical sense only, and that is behavior chosen from two options offered by K&R. I may only speculate on the reasons for chosing the arithmetic “bit-shift,” but C having trap representations seems a good candidate for the culprit.