STM32F4 GCC v10 to v11 - some weird error messages

[peter-h]

I have just updated Cube IDE, in a VM to avoid messing up my main project, from v12.1 to v13, and it has updated the GCC tools, from
arm-none-eabi-gcc (GNU Tools for STM32 10.3-2021.10.20211105-1100) 10.3.1 20210824 (release)
to


The one I have no idea about is
Project XXXX has no explicit encoding set
Lots of stuff on google but no obvious solutions (some for Eclipse but Cube is a modified version of that).

Next is

in


There is also a lot more buffer size checking like

which isn't necessarily a bug because if the destination code has been written right, it won't be extracting expected valid data past the end of the source buffer. It's interesting because I doubt the compiler is smart enough to see if the destination function is actually accessing the data; I expect it is complaining that you are passing the address of say a 20 byte buffer to a function which uses a 25 byte buffer. I would think this is common in programming since making buffers bigger than needed is common.
Otherwise, it is a worry that this is in MbedTLS!

This one is weird (not my code I should say)


What I don't get is why previous versions didn't complain. I also don't get the double cast - what is it meant to do?

I would much appreciate any pointers (no pun intended). I can turn off those error tests but it is better to fix them. Maybe GCC 11 treats some compiler option (or a default setting) differently?

[DavidAlfa]

Nothing showing inputbuffer usage here?
From GCC:

-Warray-parameter ¶
-Warray-parameter=n
Warn about redeclarations of functions involving arguments of array or pointer types of inconsistent kinds or forms, and enable the detection of out-of-bounds accesses to such parameters by warnings such as -Warray-bounds.

If the first function declaration uses the array form the bound specified in the array is assumed to be the minimum number of elements expected to be provided in calls to the function and the maximum number of elements accessed by it. Failing to provide arguments of sufficient size or accessing more than the maximum number of elements may be diagnosed by warnings such as -Warray-bounds. At level 1 the warning diagnoses inconsistencies involving array parameters declared using the T[static N] form.

For example, the warning triggers for the following redeclarations because the first one allows an array of any size to be passed to f while the second one with the keyword static specifies that the array argument must have at least four elements.

void f (int[static 4]);
void f (int[]);           // warning (inconsistent array form)

void g (void)
{
  int *p = (int *)malloc (4);
  f (p);                  // warning (array too small)
  …
}
At level 2 the warning also triggers for redeclarations involving any other inconsistency in array or pointer argument forms denoting array sizes. Pointers and arrays of unspecified bound are considered equivalent and do not trigger a warning.

void g (int*);
void g (int[]);     // no warning
void g (int[8]);    // warning (inconsistent array bound)
-Warray-parameter=2 is included in -Wall. The -Wvla-parameter option triggers warnings for similar inconsistencies involving Variable Length Array arguments.

So its seems probable you made a mistake somewhere!


Typical warning with sprintf. As long as you know the code is correct...


Pretty obvious, %x converts a 8 bit integer into hex.
But for some strange reason the argument converts it to uint32_t with (uint32_t)(uint8_t), so the format should be %02lx.
I'd just remove the (uint32_t).

[peter-h]

Inputbuffer is referenced later in that function, in the memcpy (which copies 8 bytes)

Code: [Select]


/**
  * Write formatted datablock to STLED316S
  *
  * *** this is the only function which accesses the display ***
  *
  * @param  DisplayData_t data Array of STLED316S_DISPLAY_MEM (6) bytes (starting at address 0)
  *
  * The data is ASCII characters. All 6 get written to the chip but the number which appears obviously depends
  * on how many 7-seg LEDs are installed. The 655 board has an option for 5, if the light sensor position is used.
  *
  * There are two ways to display a decimal point:
  *
  * 1) if bit 7 = 1 then DP will be illuminated for that character
  *
  * 2) if a (non-first) char is '.' then the previous char's bit 7 is set, and remaining bytes are shuffled left, until 0x00
  * Only 1 decimal point is supported within the display string, and it must not be the 1st char (e.g  .1234)
  *
  * Note that sprintf puts a 0x00 at the end, so one needs the source buffer to be longer than the
  * 6 max digits of the STLED316. This function gets an address of the buffer only, so it doesn't care if the
  * source buffer has been overrun :) If printing decimal points, you will need 2 extra bytes (1 for the dp and 1 for the 0x00
  * which sprintf puts in at the end)
  *
  * @param  brightness integer 0-15
  * @return none
  */

void display_formatted_string(uint8_t *inputbuffer, uint8_t brightness)
{
int i;
bool dpfound=false;
uint8_t tempbuffer[STLED316S_DISPLAY_MEM+2];  // use local buffer to avoid corrupting the input buffer
static uint8_t dispbuffer[STLED316S_DISPLAY_MEM+3];  // this holds the raw segment format for the STLED316 chip
static const uint8_t brt_translate[16]  = { 0,1,2,8,3,9,4,5,6,7,10,11,12,13,14,15 };

brightness &= 0x0F;  // limit value 0..15
memcpy(tempbuffer,inputbuffer,STLED316S_DISPLAY_MEM+2);  // make a local copy of the display data

if (tempbuffer[0]==0x00) return;  // abandon if 1st char is a null

// Handle lone DP char detection and left shuffling

i=1;  // skip 1st char

do
{
if ( tempbuffer[i]=='.' ) { dpfound=true; }
i++;
}
while ( ( i < STLED316S_DISPLAY_MEM+2 ) && ( tempbuffer[i] != 0x00 ) && ( dpfound == false ) );

i--;  // if a DP was found, i now points at the DP char

if ( dpfound )
{
tempbuffer[i-1]|=0x80; // set the DP on previous digit
while ( ( i < STLED316S_DISPLAY_MEM+2 ) && ( tempbuffer[i] != 0x00 ) )
{
tempbuffer[i]=tempbuffer[i+1];  // shuffle 1 left
i++;
}
}

// Convert ASCII characters via the font table; without regard for any bit 7 set, and replacing < 20h with blank digits
// We fill dispbuffer after 1st byte

for (i = 0; i < STLED316S_DISPLAY_MEM; i++)
{
if ( tempbuffer[i] >= 0x20 )
{
dispbuffer[i+1] = FONT_7S[ ( tempbuffer[i] & 0x7F ) - 0x20];
}
else  // invalid char
{
dispbuffer[i+1] = 0x00;  // 0x00 to blank the digit
}
if ( tempbuffer[i] & 0x80 )
{
dispbuffer[i+1] |= 0x80;  // if bit 7 was set on input byte, set it now in the output
}
}


// re-order brightness values to get a monotonic rise
brightness = brt_translate[brightness];

// Values 0 to 7 select low current (10mA), 8 to 15 high current (40mA)
display_set_current(brightness > 7);

SPI3_Lock();

// If current SPI3 init is not this device, initialise it
if ( g_spi3_current_config != SPI_MODE_STLED316 )
{
spi3_set_mode(SPI_MODE_STLED316);
g_spi3_current_config = SPI_MODE_STLED316;
}

// STLED316S display duty cycle values for brightness are 0 to 7
display_set_global_brightness(brightness % 8);

// This delay (10us just works) is needed after loading the brightness data, before the rest
hang_around_us(20);

dispbuffer[0] = (STLED316S_ADDR_WR_CMD | STLED316S_IDX(STLED316S_DIG_PAGE, 0x00)); // Set Address at 0

// Write all digits in one go

display_cs(0);
hang_around_us(STLED_GEN_WAIT);

SPI3_DMA_TransmitReceive(dispbuffer, NULL, STLED316S_DISPLAY_MEM+1, true, false, RTOS_YIELD);

hang_around_us(STLED_GEN_WAIT);
display_cs(1);
hang_around_us(STLED_GEN_WAIT); // Min CS=1 time

SPI3_Unlock();

}


The compiler probably doesn't like the call to the function

Code: [Select]

display_formatted_string((uint8_t *) "APP   ", 15);
which supplies a null terminated string of 7 bytes total. But we are only reading the source buffer, not writing it. Yes I know it is bad practice to read past the end of a buffer. On the 32F417 it doesn't matter but would do if the said source buffer happened to sit on the very end of physical RAM and then you get a trap.

Re the buffer sizes I think something has been changed in the checking level. GCC v10 was fine with it. And the code runs fine, FWIW.

Doing some digging around compiler options. Nothing has changed





On the ST forum, some reports of people reverting back to GCC v10 to continue working. And no solution for the "explicit encoding set" - something to do with Java?

[DavidAlfa]

Warnings usually arise from bad coding practices or certain undefined condition.
Still it's just a warning, nothing is keeping you from working.
You can overcome it by manually setting the compiler flags.

[SiliconWizard]

in

Normally this warning is triggered due to an inconsistency in how the function is declared.
Is there a prototype of this function somehwere else (like in a header file) that would happen to have a different declaration?

There is also a lot more buffer size checking like

which isn't necessarily a bug because if the destination code has been written right, (...)

This warning usually "just" means that from the format string passed to sprintf(), the worst case could overflow the buffer passed as a first argument.
It's not ultra-clever, but false positives are relatively rare.
Of course, in practice the code may ensure that the worst case is never reached, but software is full of bugs that should never happen, so.

After making sure the buffer is properly sized, one better approach is to use snprintf() and pass the size of the buffer to it. Avoids any potential overflow.
As Youtubers would say: "Stop using sprintf() !"

This one is weird (not my code I should say)


What I don't get is why previous versions didn't complain. I also don't get the double cast - what is it meant to do?

The %x format expects an 'unsigned int' as an argument.
uint32_t is not equivalent to an unsigned int in general (in a portable way). So you get a warning.
The proper C printf formats for stdint integers '(u)intxx_t' are macros of the type 'PRIxyy' that are found in the inttypes.h standard header, so you need to '#include <inttypes.h>'.

In this piece of code, since buffer's pointer base type is uint8_t, the cast (uint8_t) is useless. (I'm assuming the base type of 'buffer' is uint8_t, but is it? If not, then the cast may be warranted.)
The cast (uint32_t) is, strictly speaking, not correct as said above. You should replace it with (unsigned int).
Can it be done without entirely? In your case, yes. You can remove both casts.

But if you wanted your code to compile keeping the two casts as is without warning, you'd need to  '#include <inttypes.h>', and use the following format string instead:

Code: [Select]

" %02" PRIx32

[peter-h]

Thank you all.

I do understand the reasons for the warnings.

What I don't understand is why not on GCC 10 but yes on GCC 11. Compiler flags have not changed.

The MbedTLS case is more concerning since I would expect the coders to be good

Re that KDE_display_formatted_string() 7 v. 8 issue: the function displays 6 digits on a 7 seg LED display e.g. 123456. If a DP is to be displayed, it becomes 7: 123.456
To handle the worst case, the memcpy copies 8 bytes (the 7 plus 0x00) into a local buffer. Hence I need to modify the calling function to always supply 7 plus the 0x00, to avoid this warning. But this is irrelevant; my point is that GCC default options appear to have changed v10 to v11.

EDIT: I found it here
https://gcc.gnu.org/gcc-11/changes.html

New warnings:
-Wmismatched-dealloc, enabled by default, warns about calls to deallocation functions with pointers returned from mismatched allocation functions.
-Wsizeof-array-div, enabled by -Wall, warns about divisions of two sizeof operators when the first one is applied to an array and the divisor does not equal the size of the array element.
-Wstringop-overread, enabled by default, warns about calls to string functions reading past the end of the arrays passed to them as arguments. In prior GCC releases most instances of his warning are diagnosed by -Wstringop-overflow.

So that leaves just the "Project XXXX has no explicit encoding set". I fixed that also using the Eclipse ctrl-1 feature: highlight the error and this shows the fix. Amazing! The video here
https://stackoverflow.com/questions/72692978/eclipse-project-project-name-has-no-explicit-encoding-set
shows it.

[peter-h]

Getting back to this, for MbedTLS... Just four warnings left now:



Predictably others have been digging around this already
https://github.com/Mbed-TLS/mbedtls/issues/4130

The MbedTLS devs are IMHO somewhat full of themselves in not supporting a widely deployed version of the product and expecting people (who have a real life and a real business and need to sell boxes with frozen firmware at some point) to spend weeks upgrading the thing and doing days or weeks of testing...

Does anyone know whether one can have a compiler option (to disable this check) inside a .c source file? I don't really want to make it global because one day it might pick up a real error of mine. And I don't want to set it up inside Cube IDE because it is messy and is likely to bite somebody in the bum in the future.

[dkonigs]

Does anyone know whether one can have a compiler option (to disable this check) inside a .c source file? I don't really want to make it global because one day it might pick up a real error of mine. And I don't want to set it up inside Cube IDE because it is messy and is likely to bite somebody in the bum in the future.

If you're willing to edit the offending source file, GCC has some pragmas that can be used for this:
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
http://dbp-consulting.com/tutorials/SuppressingGCCWarnings.html

One common trend with GCC over the years is that older versions were more permissive, and newer versions have become more strict. This has generally been happening for over 20 years now.  It is generally a good thing, IMHO, but I can see how it can be annoying if you're not willing to fix the nitpicky issues in your code (or someone else's code you're building).  The solution there may simply be to add compiler flags to disable whatever new warnings they've decided to turn on by default.

[peter-h]

I've been looking around this but it looks like

#pragma "-Wno-stringop-overflow"

or

__attribute__(("-Wno-stringop-overflow")) (various versions)

is not supported.

Or it is wrong

Every imaginable version is online e.g.
https://stackoverflow.com/questions/3378560/how-to-disable-gcc-warnings-for-a-few-lines-of-code

These options are not generic GCC; they are compiler specific. Basically I need

#pragma("-Wno-stringop-overflow")
#pragma("-Wno-array-parameter")

but so far no luck.

[Siwastaja]

See this example how I have silenced specific warnings in the past, successfully:

Code: [Select]

#define IO_SET_ALTFUNC(port, pin, af) do{ _Pragma("GCC diagnostic push") _Pragma("GCC diagnostic ignored \"-Wshift-count-negative\"")  _Pragma("GCC diagnostic ignored \"-Wshift-count-overflow\"") \
if((pin)<8) {uint32_t _tmp_ = (port)->AFR[0]; _tmp_ &= ~(0b1111UL<<((pin)*4));     _tmp_ |= af<<((pin)*4);     (port)->AFR[0] = _tmp_;} \
        else {uint32_t _tmp_ = (port)->AFR[1]; _tmp_ &= ~(0b1111UL<<(((pin)-8)*4)); _tmp_ |= af<<(((pin)-8)*4); (port)->AFR[1] = _tmp_;} _Pragma("GCC diagnostic pop") }while(0)


[peter-h]

No luck



I am putting this at the start of the .c file.

The above error suggests _Pragma and #pragma are equivalent, but this doesn't work either



I can edit the TLS source in a couple of cases, apparently safely, by replacing fred[48] with *fred, but in other places it is much less obvious, and reading the MbedTLS stuff on these GCC 11 warnings it doesn't appear that simple.

Predictably, this works, for about half of it

[Siwastaja]

One quite PRAGMAtic way (pun intended) to get forward is to remove the warning entirely and then try to remember every now and then temporarily enable it for checking if your own code has developed any new such warnings.

[peter-h]

This removes it all entirely, just for the one file



i.e. putting the stuff on the compiler command line does work (the non yellow options were already there).

What a waste of time, issuing a compiler with tightened warnings and not providing a way to disable them. Lots of people have to use 3rd party code which they don't want to hack around due to the amount of regression testing needed.

[SiliconWizard]

Thank you all.

I do understand the reasons for the warnings.

What I don't understand is why not on GCC 10 but yes on GCC 11. Compiler flags have not changed.

From version to version, GCC keeps adding new warnings, and for warnings that were already supported, they keep changing the list of warnings that are enabled: by default, with -Wall, with -Wextra, and so on.
That's all there is to it.

The MbedTLS case is more concerning since I would expect the coders to be good

MbedTLS code sucks.

[peter-h]

I am not qualified to judge but it looks horrible.

But the application is truly horrible; you mutually negotiate the auth crypto, hoping that you will agree on something, and then mutually negotiate the session crypto, hoping that you will agree on something.

On the internet everybody is a security expert and stuff gets "deprecated" because somebody read on the internet that it is insecure and deprecated, so everybody copies this BS on their own website. So you remove e.g. hash algorithm X because it is deprecated and insecure, and then you find you cannot connect to some hugely popular service because they are using exactly that hash to sign their certificate.

So I don't know how anybody can write half decent code for all that.

It's also fairly obvious that they collected the various crypto algos by trawling the internet.

It's been a bit of a lesson for me too, because I would have regarded it as perfectly normal to e.g. sprintf a 5 byte string, 0x00 at the end, into a 10 byte buffer which ultimately ends up on a 10 digit LED display and whose code is written to blank any digits after the 5th one. Well, you can't do that anymore. Both src and dest need to have a 11 byte buffer. Even more fun if you want to support decimal points; to support just one you need a 12 byte buffer.

The compiler writers must have put in a vast amount of work detecting what the code is doing; they are not just comparing static buffer lengths.

[ejeffrey]

Yes, basically 99% of what compiler writers do is try to figure out how to better understand what your code is doing.  Compiling correctly written C code into assembly is something an undergraduate can do for a class project in a few weeks.  The hard part is inspecting the code to try to figure out what it is doing so it can more effectively apply optimizations, provide better error messages when things fail, and provide better warnings about legal but probably wrong code.

It's also probably not as smart as you imagine. 

You should be able to sprintf a short buffer into a long one, but I don't think it usually checks the used length, only the allocated length.  Possibly if you initialize it with a literal it tracks the actual length but anything more complicated and I think it just assumes the whole buffer is used. Passing a buffer to a function prototyped to take an array of a specified size will assume that the function will access the entire array.  If you pass something too short it should complain.  If what you actually mean is that this function takes a null terminated string or variable length string with a maximum of 10 characters, just change the signature to char* or char* plus a length parameter.

[eutectique]

The hard part is inspecting the code to try to figure out what it is doing ...

As the comment in the Open Watcom source files says:

Code: [Select]

* ...
*
* Description:  WHEN YOU FIGURE OUT WHAT THIS FILE DOES, PLEASE
*               DESCRIBE IT HERE!
* ...


[paulca]

As a general rule in static code analysis we would ban all usage of sprintf in favour of snprintf.

"I can prove it will never overflow".  is not accepted as an answer.  Because "all things change".  It's the same with "null checks" on pointers.  You might be able to prove your code path means the function will never be called with a null... the point is "it can be called with a null", so it must protect itself from them.

The same goes for the other functions which have an operating length limited version. There is one of them which has no such variant and it can cause issues, it might be sscanf how no snscanf so can continue to roll through memory, there are other ways to limit it with the extraction format.

[ejeffrey]

Yes, snprintf is the easiest and best solution to all sprintf length issues and there is basically no downside.  Even if you don't add error detection / handling, silently truncating the output is better than overflow.

[paulca]

https://owasp.org/www-community/attacks/Format_string_attack

[peter-h]

For sure; there is an absolutely vast number of attack vectors on a public server.

But nobody should be running an embedded box as a public server. Even if you are running TLS on it, it should be only as an HTTPS client, calling up some private server. That is the usage model for what I am doing. The box will either be behind NAT or be equivalently firewalled. There is an HTTP server (which I wrote, using the netconn API of LWIP, and thus assume to be totally insecure) and that is documented to never be on an open port too (and use a VPN for any remote access).

[paulca]

And yes, that's fine.

As long as you don't need to sell it to any companies with security compliance requirements and ISO certs etc.  Some bureaucracy extends far beyond it's immediately means.  For example any system which processes payment information must comply with various certifications (in the UK and other countries have similar eCommerce requirements).  These expand out to many parts of the system and in some instance require total compliance across the board.  Such as encryption "in flight" and "at rest".  "No unauthenticated end points period."

You don't want people 'UART hacking' card transaction machines or any component in that entire hardware and software chain.

It's sector specific, well not specific, but different sectors place more emphasis on security.  However the industry trends towards  security at every layer and on ever interface.  SSL, Encryption, Authentication and authorisation at each stage, each end point, each application, each database.  The only place you can have data unencrypted is in memory and even then you have to be careful with it.

The expansion of crypto features and encrypted storage on MCUs I'd say the embedded space are well on board with modern security.
 

[peter-h]

as you don't need to sell it to any companies with security compliance requirements and ISO certs etc

That would be totally meaningless in the context of any "industrial" embedded system.

Which is not to say that some customer won't ask the question... Actually I really struggle to think of the use of TLS anyway. It enables you to create systems which stop running one day because some certificate has expired, and then it's panic all around while everybody is going crazy and hunting around whoever might know about it. A simple shared key and AES256 would be much better. But that's a whole other topic.

[paulca]

In my software engineering degree the "Professional ethics" module included a case study.  I could look it up but in short...

Cocky embedded engineer works with his colleges during a company dinner to design algorythms for "bonding box" calculations for a robot....  on a napkin... the court heard.  Said engineer the next morning decided to write the code, upload it to the robotics system and.. a robot instead of turning right 10 degrees, turned left 350 degrees.  Except it didn't.  It encountered first a person and then a wall.  The person was killed instantly.

Cocky embedded engineer by my calculations is still in prison.

[peter-h]

There is a name for arguing against a point nobody is making. Google says it is a Straw Man argument; I am not 100% sure.