Weird strcat() behaviour

[peter-h]

And as usual somebody has been there before, and as usual (especially for the almost useless ST forum) didn't bother to post his solution

https://community.st.com/t5/stm32cubeide-mcus/why-are-calls-to-strcat-not-actually-calling-strcat-and-why-do-i/td-p/158592

So, without help with the above asm, I can either globally disable inlining, or globaly #define a replacement strcat().

What is weird is that this has not come up before. This project has hundreds of strcats. But maybe not hundreds lending themselves to register replacement. And in the very function I have one which works and below it one which doesn't add the zero.

Where is the global inlining disable switch?

Unlike the other guy, I am not getting hard faults.

BTW I realised that strcat with \r\n0000000 will still only move \r\n\0, so I tried it with strcat(buffer, "\r\naaaaaaaaaaaab"); and get much the same weird code.

Now testing -fno-inline-small-functions - does not work on GCC v11, evidently. Testing -fno-inline; also doesn't work.

[brucehoult]

https://community.st.com/t5/stm32cubeide-mcus/why-are-calls-to-strcat-not-actually-calling-strcat-and-why-do-i/td-p/158592

So, without help with the above asm, I can either globally disable inlining, or globaly #define a replacement strcat().

Inlining is not the problem. The generated code is perfectly correct IFF the CPU and memory region supports unaligned accesses. And if not, then you should tell the compiler that in the compiler options.

And can people please, for the love of all that is holy, turn on a little optimisation ... I HATE having to wade through things like "mov r3,r0; mov r2,r3; add.w r3, r7, #12".

Try this:

https://godbolt.org/z/YdTerE3f1

Anyway, the code is fine. Both strcat are copying the null.

What is weird is that this has not come up before. This project has hundreds of strcats. But maybe not hundreds lending themselves to register replacement. And in the very function I have one which works and below it one which doesn't add the zero.

You have yet to show any evidence that you have strcat code that is not copying the null. The code at the link you show correctly copies the null, both times.

Where is the global inlining disable switch?

A thoroughly stupid "solution" that addresses a non problem. (Well, assuming you don't care about code size -- if you care about code size then use -Os)

Unlike the other guy, I am not getting hard faults.

Then your issue -- which you have not demonstrated exists -- is different.

He, apparently has a CPU such as a Cortex-M0 or a Cortex-M33 depending on options chosen that doesn't support unaligned accesses. That problem is solved by telling GCC which CPU you have e.g.

https://godbolt.org/z/eGMjxndsj

That makes GCC inline srcat as a strlan followed by memcpy lol.

BTW I realised that strcat with \r\n0000000 will still only move \r\n\0

Yes, both inlined code and non-inlined will stop at the first null.

[peter-h]

OK I get it. The \07 is \0 and '7' So it is working.

[Nominal Animal]

Even if strcat() was inlined, we can look at gcc/gimple-fold.cc:gimple_fold_builtin_strcat() or gcc/gimple-fold.c:gimple_fold_builtin_strcat() prior to GCC switching from C to C++ or gcc/builtins.c:fold_builtin_strcat() in older GCC versions, to see what code it generates.

Given strcat(s1,s2) where s2 is a non-empty string literal, it generates memcpy(s1+strlen(s1),s2,strlen(s2)+(size_t)1), although this itself is then subject to further optimizations.

Thus, it is extremely unlikely strcat() optimization is the reason.

OK I get it. The \07 is \0 and '7' So it is working.

Perhaps.  You see, in C, "\7", "\07", and "\007" are all one-character strings, or two chars, 0x07 and 0x00.  (See C99-C23 6.4.4.4 Character constants: octal character constants may use one, two, or three octal digits.)

It is impossible to say what escaping scheme the IDE you took the screengrab from uses, because the above fact is an odd quirk not many people are aware of. 

Usually, the only one-digit escape code supported is \0, with all other octal escape codes consisting of exactly three digits.  I personally prefer the hexadecimal character escapes, \xHH, because of that.

[peter-h]

I checked the hex dump and I was wrong. The \07 is really 0x00 0x37.

Let me go back to the comment on the ST forum: "Most Cortex can handle unaligned 16 or 32-bit access, all will fail with LDRD/STRD". Is that relevant here?

The two modules which are not running are ETH and USB, both of which use DMA. Those buffers are explicitly aligned though...

[brucehoult]

Let me go back to the comment on the ST forum: "Most Cortex can handle unaligned 16 or 32-bit access, all will fail with LDRD/STRD". Is that relevant here?

Not unless you show us code expanded from strcat that is using LDRD/STRD to copy 8 chars at a time.

If I make the strcat's string sufficiently long I see ldmia to load the string, but only str, strh, strb to store it, which are all ok on those "Most Cortex".

ALIGNMENT IS NOT YOUR PROBLEM.

Nor is inlining.

And you STILL have yet to show us any evidence that there is actually anything wrong.

[Nominal Animal]

I checked the hex dump and I was wrong. The \07 is really 0x00 0x37.

So, no "weird strcat() behaviour" after all, just a misinterpretation of the data displayed in the IDE?

Let me go back to the comment on the ST forum: "Most Cortex can handle unaligned 16 or 32-bit access, all will fail with LDRD/STRD". Is that relevant here?

I don't think so, because GCC optimizer knows it can only use these when it knows the base address (in a register) is a multiple of 4 (making these instructions safe, never triggering an unaligned access exception on ARMv7-m).

This kind of limitation is common.  For example, x86 SSE has both aligned (movaps) and unaligned (movups) 128-bit four-float load/store instruction, and depending on the target architecture, GCC must only generate the aligned version when it can reliably infer at compile time that the memory address is sufficiently aligned.  Thus, I do believe GCC optimizer gets this kind of thing right, or a lot of code would not compile correctly.  You can find this kind of bug discussed in GCC mailing lists, but that is because they're caught relatively early in adding support for new target arch/extension/cpu/mcu.

[peter-h]

If I had "evidence" I would have fixed it without posting here

The 1 extra byte makes the difference between major parts of the device not working. They are extremely complex. Buffer alignment (both ETH and USB use DMA) has been checked, and anyway would have come up before.

I tried renaming that #include file to .c (which produced duplicate symbol warnings between .c and .o which I was unable to fix) and to .h (which did exactly the same as .ini).

It is an alignment issue because by moving appnamestring upwards in address, I can make the problem disappear. But this is very complex to do because the linkfile allocates objects as it finds them, possibly in alpha order.

[IanB]

If I had "evidence" I would have fixed it without posting here

The 1 extra byte makes the difference between major parts of the device not working. They are extremely complex. Buffer alignment (both ETH and USB use DMA) has been checked, and anyway would have come up before.

I tried renaming that #include file to .c (which produced duplicate symbol warnings between .c and .o which I was unable to fix) and to .h (which did exactly the same as .ini).

It is an alignment issue because by moving appnamestring upwards in address, I can make the problem disappear. But this is very complex to do because the linkfile allocates objects as it finds them, possibly in alpha order.

The include file is handled by the pre-processor, which is a purely textual operation and has no knowledge of the code. Therefore the name of the include file should make no difference to the outcome where the pre-processor is concerned. On the other hand, if you rename a file with a .c extension it might be picked up by the build tool, which might attempt to compile it as a project source file. This would certainly cause chaos.

Correct alignment of objects in memory is one of the core jobs of the compiler and loader. If you correctly tell your tools the architecture you are compiling for, they should get this right.

With the things you are describing in this thread, it suggests you have some deep problems in your code and your project design that you have not understood yet.

[IanB]

Working one step at a time from a working version, I found that replacing
char appnamestring [] = "appname_1.1";
with
char appnamestring [] = {"appname_1.1\0"};
makes it work. The 2nd one is a more explicit initialisation. The context is:

Code: [Select]


// Function returns a pointer to the customer application string.

#include "appname.ini"

char* get_appname(void)
{
return (appnamestring);
}

and appname.ini contains
char appnamestring [] = {"appname_1.1\0"};

The string is outside a function so should end up as initialised data which gets copied to RAM at startup. It should probably be "const". But that blows it up.

Funny thing is that I have another (rare) case of Cube IDE SWD interface getting buggered-up by C code in the project!

Why not something like this?

appname.h

Code: [Select]

#define APPNAME "appname_1.1"
appname function:

Code: [Select]

#include "appname.h"

const char* get_appname(void)
{
    static const char appname[] = APPNAME; 
    return appname;
}


[peter-h]

Indeed; these strings should be static const. In fact one of them is and the other two aren't. Most likely I can make this problem go away by making them const which avoids the data being copied from flash to ram, but I want to find out what exactly is going on here, because it should not be happening.

it suggests you have some deep problems in your code and your project design that you have not understood yet.

Well, yes, which is why I am asking all you experts

Actually, after many hours, I think it is some subtle linker issue, maybe in the form of a Cube bug related to #including files in the sources. I can blow it up by making those strings longer, but the strange thing is that

- both ETH (with LWIP) and USB die at the same point
- both use DMA
- both are unbelievably complex, although casually setting breakpoints shows everything working as it should, superficially

This product has been rock solid for past 3 years plus, although notably the issues are confined to adding strings etc in main.c, which is why this was not previously noticed; relatively huge RTOS applications were developed which are all rock solid, but the linkfile loads main.o first, followed by some specific low level modules.

I am suspecting alignment issues caused by an error in the linkfile. This is the relevant part. I am keen to 4-align each module and its data (both data and bss). It looks ok but maybe something is missing

Code: [Select]

 
/* === Rest of code, loaded at base+32k, starting with a stub and then the real XXX_main() === */

  .XXX_main_stub :
  {
    . = ALIGN(4);
    _code_base = .;
    KEEP(*(.XXX_main_stub))
    *XXX_main_stub.o (.text .text* .rodata .rodata*)
    . = ALIGN(4);
  } >FLASH_APP
 
  /* app code vector table */
  /* most of the KEEP directives do nothing but this one is necessary because
     nothing points to the vector table so it would get optimised away */
  .isr_vector2 :
  {
    . = ALIGN(0x200);
    KEEP(*(.isr_vector2))
    *isr_vector2.o (.text .text* .rodata .rodata*)
    . = ALIGN(4);
  } >FLASH_APP
   
  .XXX_main :
  {
    . = ALIGN(4);
    KEEP(*(.XXX_main))
    *XXX_main.o (.text .text* .rodata .rodata*)
    . = ALIGN(4);
  } >FLASH_APP
 
 
 
/* This is for __libc_init_array() */
/* See https://community.st.com/t5/stm32cubeide-mcus/the-mysterious-libc-init-array-in-cube-ide-is-it-only-for-c/td-p/652829 */

.preinit_array     :
  {
    PROVIDE_HIDDEN (__preinit_array_start = .);
    KEEP (*(.preinit_array*))
    PROVIDE_HIDDEN (__preinit_array_end = .);
  } >FLASH_APP
  .init_array :
  {
    PROVIDE_HIDDEN (__init_array_start = .);
    KEEP (*(SORT(.init_array.*)))
    KEEP (*(.init_array*))
    PROVIDE_HIDDEN (__init_array_end = .);
  } >FLASH_APP
  .fini_array :
  {
    PROVIDE_HIDDEN (__fini_array_start = .);
    KEEP (*(.fini_array*))
    KEEP (*(SORT(.fini_array.*)))
    PROVIDE_HIDDEN (__fini_array_end = .);
  } >FLASH_APP


 
/* Initialized data sections for non boot block code. These go into RAM. LMA copy is loaded after code. */
/* This stuff is copied from FLASH to RAM by C code in the main stub */
. = ALIGN(4);

/* main.c DATA */
  .XXX_main_data :
  {
    . = ALIGN(4);
    _s_nonboot_data = .;        /* create a global symbol at data start */
    KEEP(*(.XXX_main))
    *XXX_main.o (.data .data*)      /* .data sections */
    . = ALIGN(4);
  } >RAM  AT >FLASH_APP

/* Remaining DATA */
.XXX_other_data :
  {
    . = ALIGN(4);
    *(.data .data*)      /* .data sections */
      . = ALIGN(4);
    _e_nonboot_data = .;        /* define a global symbol at data end */
  } >RAM  AT >FLASH_APP

  /* used by the main stub C code to initialize data */
  _si_nonboot_data = LOADADDR(.XXX_main_data);


  /* Uninitialized data section (BSS) for non block boot code */
/* This stuff is zeroed by C code in the main stub */
 
  .XXX_main_bss :
  {
    . = ALIGN(4);
    _s_nonboot_bss = .;        /* create a global symbol at BSS start */
    KEEP(*(.XXX_main))
    *XXX_main.o (.bss .bss* .COMMON .common .common*)      /* .bss sections */
    . = ALIGN(4);
  } >RAM
 
  /* Remaining BSS */
 
  .XXX_other_bss :
  {
      . = ALIGN(4);
    *(.bss .bss* .COMMON .common .common*)
    . = ALIGN(4);
    _e_nonboot_bss = .;          /* define a global symbol at BSS end */
  } >RAM
 
 

 
  /* This collects all other stuff, which gets loaded into FLASH */
    .code_constants_etc :
  {
    . = ALIGN(4);
    *(.text)           /* .text sections (code) */
    *(.text*)          /* .text* sections (code) */
    *(.rodata)         /* .rodata sections (constants, strings, etc.) */
    *(.rodata*)        /* .rodata* sections (constants, strings, etc.) */
    *(.glue_7)         /* glue arm to thumb code */
    *(.glue_7t)        /* glue thumb to arm code */
*(.eh_frame)

    KEEP (*(.init))
    KEEP (*(.fini))
   
    . = ALIGN(4);
    _e_code_constants_etc = .;        /* define a global symbol at end of code */
} >FLASH_APP


[T3sl4co1l]

Curious about your conception of "rock solid" -- what fuzzing has been done on these systems?

If very little, or none, and mostly expected interactions from users/network/etc., that means actually very little!

In particular, with so many hardware and software functions interacting, I would be frankly shocked there aren't, for example, timing interactions, race conditions, packet collision and parsing, those sorts of things, going on.  Who knows -- these can be wildly hard to find, even given all the tools to do it (it's no accident hackers expect (and normally receive) a modest payoff when discovering and reporting such a bug in mainstream infrastructure!).  So it's not like such a thing might be critical, if it exists at all, here -- but just to say, you can't do a minimum of testing, to the average and expected case, and infer total success from that.

Tim

[peter-h]

I've updated my post above.

About 10 units have been running, 24/7, doing pretty complex ETH transactions, with MbedTLS, etc. That is as horrible as it gets!

This is not a subtle issue. It looks like a load of code has vanished! But it hasn't; it's there and I can trace through it. ETH and USB blow up exactly concurrently, too, suggesting it is something basic. But like I say these are very complex modules which do nothing until there is traffic, and the traffic is incredibly complex. It would not surprise me if some buffer was simply deleted b the linker.

[ejeffrey]

The linker doesn't just delete buffers!  Almost certainly if you actually have a problem (which isn't even clear) you just have a bug in your C code.  Probably a buffer overflow of some kind.  It could be almost anywhere in your code.

[brucehoult]

The linker doesn't just delete buffers!  Almost certainly if you actually have a problem (which isn't even clear) you just have a bug in your C code.  Probably a buffer overflow of some kind.  It could be almost anywhere in your code.

And the "experts" have been given absolutely nothing to go on except whatever red herring OP has latched onto at that moment. Without being given access to a reproducible test case it's just a huge waste of time.

[Nominal Animal]

Without being given access to a reproducible test case it's just a huge waste of time.

I wouldn't be that harsh.  True, without a MCVE, we have no way to delve further into the issue.  Yet, this thread also shows what can be done without one (elimination of causes by examining the assembly code generated and the sources for the functions used, for example), so at least as educational material to anyone else looking for help with "weird behaviour" in their code, the thread has some utility and value.

[DiTBho]

Curious about your conception of "rock solid" -- what fuzzing has been done on these systems?

as far as I'm concerned, the only solid things are those that emerge from a series of exhaustive verification sessions, done with different techniques, from module-testing, with and without instrumentation, to tests in system integration, and also considering the coverage, that is ' how many if/else branches are actually verified under all condition, and to design all the test-cases during the testbench we go from normal to abnormal stimulus, so out of range and weird inputs, to test how the system reacts and how it is doing with error propagation.

... and this is still not enough, but we are satisfied enough to say "good enough"

Then there are those who ... leave a system running for 24 hours, perhaps always with the same stimulus, and if it doesn't crash, then it's rock-solid 

[DiTBho]

Without being given access to a reproducible test case it's just a huge waste of time.

Precisely.
Education, perhaps for the things that can be recommended as an approach.

[peter-h]

You said you blacklisted me, DiTBho

I would post a simple case if I had a simple case to post. The code in this, minus MbedTLS which is not needed to show it, is about 200k.

I would put money in the linkfile doing something (posted it above) because it is only when I add stuff to main.c that this happens. Adding code to any other module is fine, which is why this has only just come to light. And the linkfile does main.o first and then a separate section(s) is used for the rest. Main.c has been constant for years - until I decided to put in those two functions to return those strings.

I wonder whether the align 4 directives are done right?

[peter-h]

I am looking for debugging suggestions.

I have a system which works perfectly. I am #including a file, appname.ini, containing

static char appnamestring [] = "appname_1.1";  // max 32 bytes

It is returned by this function

Code: [Select]

// Function returns a pointer to the customer application string.
// This is the customer application name - max 32 bytes

char * get_appname(void)
{
#include "appname.ini"
return (char*) (appnamestring);
}
It is read-only so it would be sensible to make it "static const" but that blows up large chunks of the project, namely ETH and USB.

I know what "const" does: it leaves the string in flash and does not copy it to ram. So all the workspace locations in RAM shift down by 8 bytes (in this case - I used a shorter string)



Doing a diff on two .map files, this is all I see. Everything is there on the duff version. Just the addresses are lower on the duff one (the upper one).

The weird thing is that both ETH and USB die at the same time. All buffers that should be aligned are 4-aligned; in fact lots of stuff is 4-aligned (see the linkfile above). This box has been in development (in various guises; starting with the ST DISCO board) for about 8 years. Other issues e.g. DMA cannot access CCM (on the 32F4xx) should be OK by a) design and b) extensive devt over time.

I am sure this is something very simple, but ETH and USB cannot be debugged from first principles, with breakpoints, partly due to protocol timeouts and partly due to sheer complexity. ETH, like LWIP, runs as an RTOS task. USB is entirely interrupt-driven and there is an RTOS task which implements a VCP and MSC (the latter with FatFS).

Simplistically, the use of "const" breaks a huge amount of stuff. But there is another function which works on a similar string, but longer, which uses "static const", and that works. I also use const all over the place, for R/O strings.

Putting debugs in ETH shows no incoming data is seen by this function, which is pretty basic! Data is seen fine otherwise

Code: [Select]

HAL_StatusTypeDef IF_HAL_ETH_GetReceivedFrame(ETH_HandleTypeDef *heth)
{
uint32_t framelength = 0U;

/* Check if segment is not owned by DMA */
/* if (((heth->RxDesc->Status & ETH_DMARXDESC_OWN) == (uint32_t)RESET) && ((heth->RxDesc->Status & ETH_DMARXDESC_LS) != (uint32_t)RESET)) */
//__DMB();
if(((heth->RxDesc->Status & ETH_DMARXDESC_OWN) == (uint32_t)RESET))
{
/* Check if last segment */
if(((heth->RxDesc->Status & ETH_DMARXDESC_LS) != (uint32_t)RESET))
{
/* increment segment count */
(heth->RxFrameInfos).SegCount++;

/* Check if last segment is first segment: one segment contains the frame */
if ((heth->RxFrameInfos).SegCount == 1U)
{
(heth->RxFrameInfos).FSRxDesc =heth->RxDesc;
}

heth->RxFrameInfos.LSRxDesc = heth->RxDesc;

/* Get the Frame Length of the received packet: substruct 4 bytes of the CRC */
framelength = (((heth->RxDesc)->Status & ETH_DMARXDESC_FL) >> ETH_DMARXDESC_FRAMELENGTHSHIFT) - 4U;
heth->RxFrameInfos.length = framelength;

/* Get the address of the buffer start address */
heth->RxFrameInfos.buffer = ((heth->RxFrameInfos).FSRxDesc)->Buffer1Addr;
/* point to next descriptor */
heth->RxDesc = (ETH_DMADescTypeDef*) ((heth->RxDesc)->Buffer2NextDescAddr);

/* Return function status */
return HAL_OK;
}
/* Check if first segment */
else if((heth->RxDesc->Status & ETH_DMARXDESC_FS) != (uint32_t)RESET)
{
(heth->RxFrameInfos).FSRxDesc = heth->RxDesc;
(heth->RxFrameInfos).LSRxDesc = NULL;
(heth->RxFrameInfos).SegCount = 1U;
/* Point to next descriptor */
heth->RxDesc = (ETH_DMADescTypeDef*) (heth->RxDesc->Buffer2NextDescAddr);
}
/* Check if intermediate segment */
else
{
(heth->RxFrameInfos).SegCount++;
/* Point to next descriptor */
heth->RxDesc = (ETH_DMADescTypeDef*) (heth->RxDesc->Buffer2NextDescAddr);
}
}

/* Return function status */
return HAL_ERROR;
}
That suggests that ETH is just broken in some way. I am suspecting DMA...

Obviously I will poke about lots more but is this correct GCC format for alignment?

Code: [Select]

__ALIGN_BEGIN ETH_DMADescTypeDef  DMARxDscrTab[ETH_RXBUFNB] __ALIGN_END;/* Ethernet Rx MA Descriptor */
__ALIGN_BEGIN ETH_DMADescTypeDef  DMATxDscrTab[ETH_TXBUFNB] __ALIGN_END;/* Ethernet Tx DMA Descriptor */
__ALIGN_BEGIN uint8_t Rx_Buff[ETH_RXBUFNB][ETH_RX_BUF_SIZE] __ALIGN_END; /* Ethernet Receive Buffer */
__ALIGN_BEGIN uint8_t Tx_Buff[ETH_TXBUFNB][ETH_TX_BUF_SIZE] __ALIGN_END; /* Ethernet Transmit Buffer */

[IanB]

You are defining and initializing an object in a header file. This is not something that is normally done. Normally, header files contain shared declarations, but not  definitions. If you include such a header file in more than one place it may cause problems.

This is not necessarily the cause of your problem, but it is a sign of an unusual code pattern that may be indicative of other anti-patterns in the code.

[peter-h]

OK; I took that #include out and nothing changes.

Also there is another function just below the appname one which does a "static const" with a much bigger string - about 1k) and that's working fine. Been there for years. So it doesn't point to a problem with the compiler disliking an initialised string in an include file.

This suggests the DMA buffers etc are indeed 4-aligned


A couple more data points:

The Build Analyser shows just the expected difference in ram size.
Putting that static const string at the end of another very similar file containing another static const string doesn't bomb it, suggesting that any movement in memory addresses is not the problem.

I started another thread under Microcontrollers in the hope that I could check some ETH config register bits.

[ajb]

Putting that static const string at the end of another very similar file containing another static const string doesn't bomb it, suggesting that any movement in memory addresses is not the problem.

Correct, the problem isn't where the string is in memory.  The problem is that something is corrupting your memory, but the symptoms depend on what data is getting corrupted, which depends on how the program is linked.  A shift of a single byte may cause wildly different behavior, which can have you chasing after all kinds of wild geese for days until you figure out what is actually causing the corruption. 

It is read-only so it would be sensible to make it "static const" but that blows up large chunks of the project, namely ETH and USB.

Changing the qualification of this object isn't blowing up ETH and USB.  It's just causing the linker to put the string somewhere else, and now whatever was corrupting your string is instead corrupting eth/usb data, because that's what the linker put where the string used to be. 

Fortunately, if you are seeing that string be consistently corrupted a certain way, then you are relatively well positioned to identify the culprit, for example by setting a data watchpoint on the address (or address range) in question.  Other kinds of corruption can be much harder to pin down -- like if some uninitialized data is used as an offset for a subsequent access, the behavior would be much more chaotic. 

[peter-h]

I have narrowed it down to a simpler case of USB almost certainly not getting initialised properly. The whole thing is interrupt-driven. Various registers are loaded with constant values and I suspect one or more of these are done incorrectly. The same process would also bugger up ETH but I have sidestepped that since it happens the same with just USB.

[T3sl4co1l]

Might as well cross-link this here too, FYI: https://www.eevblog.com/forum/programming/the-use-of-the-const-qualifier-gcc/

Tim