[SOLVED] Why is gcc producing code with an infinite loop in it ?

[rteodor]

I had an putc function ported from AVR to ARM and now on RISC-V. Works fine on all of them.
Its putting chars into a circular buffer from where the UART ISR consumes. Buffer overwrites occurs if there is too much data incoming.

Now on CH32V003 due to small RAM, I wanted to add a loop and make the putc function blocking until there is space in the buffer.
Should be simple yet the build counter increased by over 100 and I could not produce a solution. As my build of riscv-openocd-wch/bin/openocd is almost unusable, I went to debug with GPIO's. At some point just flipping a GPIO in the right place made the loop code working as expected. Time to dig the asm (I kept avoiding that because RISC-V is new for me).

Here is the code, the not working disassembly, and the working disassembly side by side. When line 186 is commented the loop does not work. Looking at the ASM - maybe I should wash my eyes - the execution is most likely locked at:

Code: [Select]

00000226 <.L28>:
    while (FALSE == free_space);
 226: c381                beqz a5,226 <.L28>




Making free_space variable volatile does not help. Not a fragment of an idea on what is that PD0 clear lines changes for the compiler.

Details:

Code: [Select]

$ riscv-none-elf-gcc --version
riscv-none-elf-gcc (GCC) 14.2.0
Tried 15.2.0 too. Same result.

Code: [Select]

riscv-none-elf-gcc -c -Wall -O1 -g -fpack-struct -fshort-enums -funsigned-char -funsigned-bitfields -I src -I src/ch32v003 -march=rv32ec -mabi=ilp32e -o out/obj/term.o src/term.c


Code: [Select]

#define TERM_UART_BUFFER_SIZE       (200u)

typedef struct
{
    uint8_t buffer[TERM_UART_BUFFER_SIZE];
    uint8_t rd;
    uint8_t wr;
    bool_t msg_pending;
} term_buffer_t;

static term_buffer_t term_tx_buffer;


[ejeffrey]

Making free_space variable volatile does not help. Not a fragment of an idea on what is that PD0 clear lines changes for the compiler.

Right idea, wrong variable.

The term_tx_buffer needs to be volatile.  Thats the variable that is changed from another context.

The more modern way to do this is to make just the read and write offsets atomic, but for this purpose that prbably doesn't provide any real advantage.

The gpio access just adds a compiler barrier that accidentally avoids the problem.

[brucehoult]

Why would anyone even try to understand what you've done wrong — no, it will NOT be such a common and standard compiler — without having a full compilable example code with the problem? And not just photos of code.

However I'm feeling generous so I'll try to fill in the gaps of what you should have given in the first place: https://godbolt.org/z/c1rnh8738

With that code I don't know why you're not just getting a single "ret" instruction, or "bx lr" in Arm.

Adding "volatile" gives you what I assume you want.

And the asm code seems perfectly readable to me. (I had to do RV32IC since Godbolt doesn't seem to like RV32E)

Code: [Select]

term_uart_putc:
        lla     a5,.LANCHOR0
        li      a3,198
        j       .L5
.L2:
        lbu     a4,200(a5)
        bne     a4,zero,.L6
.L5:
        lbu     a4,201(a5)
        bgtu    a4,a3,.L2
        lbu     a4,201(a5)
        lbu     a2,200(a5)
        addi    a4,a4,1
        beq     a4,a2,.L5
        ret
.L6:
        ret
        .set    .LANCHOR0,. + 0
term_tx_buffer:
        .zero   203

You really should use some local variables rather than loading from RAM multiple times, ESPECIALLY given that the value might change between the different uses.

[rteodor]

Making free_space variable volatile does not help. Not a fragment of an idea on what is that PD0 clear lines changes for the compiler.

Right idea, wrong variable.

The term_tx_buffer needs to be volatile.  Thats the variable that is changed from another context.

The more modern way to do this is to make just the read and write offsets atomic, but for this purpose that prbably doesn't provide any real advantage.

The gpio access just adds a compiler barrier that accidentally avoids the problem.

Yes you are right. Its the rd buffer index that is in fact modified by the ISR.

At first I had the code something like:

Code: [Select]

    bool_t free_space;
    volatile uint8_t rd;
    volatile uint8_t wr;

    do
    {
        free_space = TRUE;

        __disable_irq ();
        rd = term_tx_buffer.rd;
        wr = term_tx_buffer.wr;
        __enable_irq ();
...

Here the compiler placed only the store into local variables into the protected section. Without volatile at the source variable it does not know that its the loading, not the storing, that must be protected and most likely optimizes that loading out of the section.

Anyway, making any of the following works:

Code: [Select]

static volatile term_buffer_t term_tx_buffer;
or

Code: [Select]

typedef struct
{
    uint8_t buffer[TERM_UART_BUFFER_SIZE];
    volatile uint8_t rd;
    uint8_t wr;
    volatile bool_t msg_pending;
} term_buffer_t;

Thanks.
Maybe I should wash my eyes ...

[rteodor]

Why would anyone even try to understand what you've done wrong — no, it will NOT be such a common and standard compiler — without having a full compilable example code with the problem? And not just photos of code.

Its a bit complicated ... if I had to post the whole project after some many builds ... its a bit messy and I am not sure I want the world to see all the stupid things I have in there. If it were some basic code - meaning not containing an ISR and platform specific code - I would most probably reduce it to a single compilable file and post it here. However I will try to add it at the end of the post, just for the sake of trying ... because currently I do not know how to make it compilable AND be sure my problem is still contained in there. AND I did not want to flood the forum with ballast code and loose people focus.

You really should use some local variables rather than loading from RAM multiple times, ESPECIALLY given that the value might change between the different uses.

As I responded above to @ejeffrey I tried that in a protected section, but somehow, because of the missing volatile's in the right place, did not work either.

This is the code that has shown the problem, no need to debug it further. I am posting it just to discuss how to make it "compilable".

Code: [Select]

#include <stdint.h>
#include <string.h>
#include <ch32v003/ch32v003.h>

#define TERM_UART_BUFFER_SIZE       (200u)

typedef struct
{
    uint8_t buffer[TERM_UART_BUFFER_SIZE];
    uint8_t rd;
    uint8_t wr;
    bool_t msg_pending;
} term_buffer_t;

static term_buffer_t term_tx_buffer;

__attribute__((interrupt)) void USART1_IRQHandler (void)
{
    if (USART1->STATR & USART_STATR_RXNE)
    {
        /* data was received */
        term_receive_char (USART1->DATAR);
    }

    if (USART1->STATR & USART_STATR_TXE)
    {
        /* there is still some more data to send */
        if (term_tx_buffer.wr != term_tx_buffer.rd)
        {
            USART1->DATAR = term_tx_buffer.buffer[term_tx_buffer.rd];

            term_tx_buffer.rd++;
            if (term_tx_buffer.rd >= sizeof (term_tx_buffer.buffer))
            {
                term_tx_buffer.rd = 0;
            }
        }
        else
        {
            term_tx_buffer.msg_pending = FALSE;

            /* disable TX complete interrupt otherwise its ISR will be executed continuously (and keep CPU fully busy) */
            USART1->CTLR1 &= ~USART_CTLR1_TXEIE;
        }
    }
}

void term_uart_putc (uint8_t byte)
{
    bool_t free_space;

    do
    {
        free_space = TRUE;

        if (term_tx_buffer.wr < (TERM_UART_BUFFER_SIZE - 1))
        {
            if ((term_tx_buffer.wr + 1) == term_tx_buffer.rd)
            {
                free_space = FALSE;
            }
        }
        else
        {
            if (0 == term_tx_buffer.rd)
            {
                free_space = FALSE;
            }
        }
    }
    while (FALSE == free_space);

    term_tx_buffer.buffer[term_tx_buffer.wr] = byte;

    term_tx_buffer.wr++;
    if (term_tx_buffer.wr >= sizeof (term_tx_buffer.buffer))
    {
        term_tx_buffer.wr = 0;
    }

    if (FALSE == term_tx_buffer.msg_pending)
    {
        term_tx_buffer.msg_pending = TRUE;

        term_tx_buffer.rd++;
        if (term_tx_buffer.rd >= sizeof (term_tx_buffer.buffer))
        {
            term_tx_buffer.rd = 0;
        }

        USART1->DATAR = byte;

        USART1->CTLR1 |= USART_CTLR1_TXEIE;
    }
}


[ejeffrey]

Yes you are right. Its the rd buffer index that is in fact modified by the ISR.

You need volatile on any variable that is shared between the ISR and the main thread.  That includes ones written by the main thread and read by the ISR.

At first I had the code something like:
Here the compiler placed only the store into local variables into the protected section. Without volatile at the source variable it does not know that its the loading, not the storing, that must be protected and most likely optimizes that loading out of the section.

The actual variable that is shared is the one that must be marked volatile.  Creating a local volatile copy doesn't do anything useful.

Anyway, making any of the following works:

Code: [Select]

static volatile term_buffer_t term_tx_buffer;


This is the correct way.

[HwAoRrDk]

You need volatile on any variable that is shared between the ISR and the main thread.  That includes ones written by the main thread and read by the ISR.

It's also probably worth mentioning that if you have local pointer to a volatile shared variable, that pointer also needs to be defined as volatile (or, I should say, a pointer to a volatile).

[ejeffrey]

True.  Compilers will do their level best to warn you about that with messages like "pointer assignment discards volatile"  If you persist in ignoring those warnings you are probably going to have a bad day.

[Siwastaja]

True.  Compilers will do their level best to warn you about that with messages like "pointer assignment discards volatile"  If you persist in ignoring those warnings you are probably going to have a bad day.

Simply: always compile with -Wall -Werror, and you are forced to deal with these issues. Large chunk of hard-to-find problems just goes away.

[Alien Brother]

It also depends how __disable_irq() and __enable_irq() are defined. Ideally, they should include something like `asm volatile ... memory` to prevent the compiler from reordering stuff. In general it's not feasible to put `volatile` on all data that is shared between "threads".

[rteodor]

Its the scope of the 'volatile' is where I got it wrong. What I had in mind is that any assignment from a volatile would be done fully without optimizations. Said otherwise: if volatile were to be present on any one side of an assignment, accesses on both sides would be done without optimizations and thus directly to/from memory. Bad assumption.

[ejeffrey]

In general it's not feasible to put `volatile` on all data that is shared between "threads".

Why not?

Also we aren't talking about general threads.  Interrupt handlers need special treatment.  You should not have so many shared variables in an ISR that you cannot enumerate them and analyze them individually.

For sharing between multiple scheduled threads there are othet options and volatile is basically never correct.

[Alien Brother]

Why not?

Because code that implements data structures rarely uses blanket volatile, especially if you're using 3rd party code or stuff written by colleagues. Here, if OP were to encapsulate push() and pop() operations into functions, these functions would have to accept volatile term_buffer_t*, and the same would need to be done for all data structures that may get used by interrupt handlers. It's annoying and unnecessary and people tend to not do things this way. Normally, we'd want to either wrap calls to push() and pop() into critical sections, in which case we'd want to fix the fact that __disable_irq()/__enable_irq() in OP's case apparently don't implement a critical section. Or, we'd want to change the queue to make blanket volatile unnecessary. A single producer single consumer queue should be using "stronger" read and write operations on the its head and tail pointers, for example C11 atomic_load() and atomic_store() that get correctly ordered with the data reads and writes (but that you already mentioned). Here there's an example that uses C++11 std::atomic https://github.com/free-audio/clap-helpers/blob/main/include/clap/helpers/param-queue.hh .

[brucehoult]

A single producer single consumer queue should be using "stronger" read and write operations on the its head and tail pointers, for example C11 atomic_read() and atomic_write() that get correctly ordered with the data reads and writes. Here there's an example that uses C++11 std::atomic https://github.com/free-audio/clap-helpers/blob/main/include/clap/helpers/param-queue.hh .

We're talking about the smallest microcontrollers here ... AVR, Cortex-M0, CH32V003 ... not big machines with caches, store buffers, OoO execution. None of them have any special atomic operations only regular loads and stores.

Note the bigger ARMv7-M Cortex-M do have LDREX/STREX, and CH32V103 and up have AMOADD etc.

[Alien Brother]

The role of atomic_load()/atomic_store() is to provide the barrier for both processor and compiler (which is always out there to get you regardless how small the processor is) without putting volatile qualifier on all data. On M0, GCC compiles them into ldr/str surrounded by dmb. Maybe dmb is redundant in this case, I don't know.

[rteodor]

in which case we'd want to fix the fact that __disable_irq()/__enable_irq() apparently don't implement a critical section.

I would have expected for the compiler to get this code:

Code: [Select]

        __disable_irq ();
        rd = (volatile uint8_t)(term_tx_buffer.rd);
        wr = (volatile uint8_t)(term_tx_buffer.wr);
        __enable_irq ();

But it doesn't. After a few tries I convinced it with this:

Code: [Select]

        __disable_irq ();
        rd = *(volatile uint8_t *)(&(term_tx_buffer.rd));
        wr = *(volatile uint8_t *)(&(term_tx_buffer.wr));
        __enable_irq ();


[Alien Brother]

I would have expected for the compiler to get this code:

Can you look up the implementations of __disable_irq() and __enable_irq() that you have?

In ARM's CMSIS, they have

Code: [Select]

__STATIC_FORCEINLINE void __disable_irq(void)
{
  __ASM volatile ("cpsid i" : : : "memory");
}

I think the volatile ... memory is supposed to prevent the compiler from moving memory accesses across this.

[rteodor]

The role of atomic_load()/atomic_store() is to provide the barrier for both processor and compiler (which is always out there to get you regardless how small the processor is) without putting volatile qualifier on all data. On M0, GCC compiles them into ldr/str surrounded by dmb. Maybe dmb is redundant in this case, I don't know.

No solution is the best but some solutions fit a certain purpose better than others. If I would expect to port this code to a multi-MHz MCU or SoC with sophisticated D-cache, the atomic_load()/atomic_store() would be a proper solution. But if I stay confined into a few KB of flash I would rather use volatile in the right places. This I found to give me the smallest code (short of going to asm).

[rteodor]

Can you look up the implementations of __disable_irq() and __enable_irq() that you have?

Sure, its straight from ch32fun"

Code: [Select]

/* Enable Global Interrupt */
RV_STATIC_INLINE void __enable_irq ()
{
    uint32_t result; __ASM volatile (ADD_ARCH_ZICSR "csrr %0," "mstatus": "=r" (result));
    result |= 0x88;  __ASM volatile (ADD_ARCH_ZICSR "csrw mstatus, %0" : : "r" (result));
}

/* Disable Global Interrupt */
RV_STATIC_INLINE void __disable_irq ()
{
    uint32_t result; __ASM volatile (ADD_ARCH_ZICSR "csrr %0," "mstatus": "=r" (result));
    result &= ~0x88; __ASM volatile (ADD_ARCH_ZICSR "csrw mstatus, %0" : : "r" (result));
}


[Alien Brother]

I think this code will benefit from adding `: "memory"` to the asm instructions so that they become barriers for the compiler.

I am not familiar with RISC-V, but another suspicious thing is that this code seems to be updating a flag in a register mstatus (?) in a non-atomic way, so the ISRs must not be writing to mstatus. But maybe that's not an issue.

[ejeffrey]

Because code that implements data structures rarely uses blanket volatile, especially if you're using 3rd party code or stuff written by colleagues. Here, if OP were to encapsulate push() and pop() operations into functions, these functions would have to accept volatile term_buffer_t*, and the same would need to be done for all data structures that may get used by interrupt handlers.

Implelenting a specific "interrupt safe queue" is a totally normal and correct thing to do.  And again "all data structures thet get used by interrupt handlers" should be a very small list.  If this is a problem, your issue isn't making them all volatile it is that your interrupt handlers are way to complex.

in which case we'd want to fix the fact that __disable_irq()/__enable_irq() in OP's case apparently don't implement a critical section.

The OP isn't disabling interrupts.  That is of course another way to fix this, but its totally unnecessary for a simple single producer single consumer queue.

should be using "stronger" read and write operations on the its head and tail pointers, for example C11 atomic_load() and atomic_store() that get correctly ordered with the data reads and writes (but that you already mentioned).

Atomics are a much better way in general to implement multithreaded queues but usually not for interrupt contexts on small microcontrollers.  The problem is that smal MCUs lack the cortex M0+ lack the RMW operations needed to implement lock free atomics.  You dont need RMW operations for a simple queue, but without them you cant have a conforming implementation of C11 atomics.

It would be nice if C would add "light weight atomics" that only have store/load operations with the needed barriers. But
 mostly its an edge use case where volatile works fine. 

[Alien Brother]

And again "all data structures thet get used by interrupt handlers" should be a very small list.  If this is a problem, your issue isn't making them all volatile it is that your interrupt handlers are way to complex.

If you rely on blanket volatile, you won't be able to even call memcpy() without implementing your own version that accepts pointers to volatile, and not only in an ISR - in any code that accesses the data structure.

The problem is that smal MCUs lack the cortex M0+ lack the RMW operations needed to implement lock free atomics.  You dont need RMW operations for a simple queue...

You are responding to your own argument here. Single producer single consumer queue does not need atomic rmw, it needs that accesses head/tail pointers are acquire/release, which atomic_load() and atomic_store() will ensure even on M0. But the same C code will also work on other architectures and on multi-core. Apart from being able to move to a different MCU (for a hobbyist it's normal to try things out), you'll be able to test some of your code on PC with threads.

The OP isn't disabling interrupts.  That is of course another way to fix this, but its totally unnecessary for a simple single producer single consumer queue.

Thinking more about it, it's not obvious whether buffered IO can be implemented with only a queue. You need to start the transmission when the data enters the queue for the first time and stop when the queue is empty (in rteodor's code, msg_pending and USART1->CTLR1 manage this). I personally wouldn't bet my life on making it work without a critical section.

[HwAoRrDk]

I am not familiar with RISC-V, but another suspicious thing is that this code seems to be updating a flag in a register mstatus (?) in a non-atomic way, so the ISRs must not be writing to mstatus. But maybe that's not an issue.

Yeah, it's not atomic doing a read-modify-write sequence like that. I don't know why they're doing it that way. Why not do it with a single csrrs and csrrc instruction?

Code: [Select]

RV_STATIC_INLINE void __enable_irq ()
{
    uint32_t tmp;
    __ASM volatile (ADD_ARCH_ZICSR "csrrs %0," "mstatus, %1" : "=r" (tmp) : "i" (0x88));
}

RV_STATIC_INLINE void __disable_irq ()
{
    uint32_t tmp;
    __ASM volatile (ADD_ARCH_ZICSR "csrrc %0," "mstatus, %1" : "=r" (tmp) : "i" (0x88));
}


[ejeffrey]

And again "all data structures thet get used by interrupt handlers" should be a very small list.  If this is a problem, your issue isn't making them all volatile it is that your interrupt handlers are way to complex.

If you rely on blanket volatile, you won't be able to even call memcpy() without implementing your own version that accepts pointers to volatile, and not only in an ISR - in any code that accesses the data structure.

The problem is that smal MCUs lack the cortex M0+ lack the RMW operations needed to implement lock free atomics.  You dont need RMW operations for a simple queue...

You are responding to your own argument here. Single producer single consumer queue does not need atomic rmw, it needs that accesses head/tail pointers are acquire/release, which atomic_load() and atomic_store() will ensure even on M0. But the same C code will also work on other architectures and on multi-core. Apart from being able to move to a different MCU (for a hobbyist it's normal to try things out), you'll be able to test some of your code on PC with threads.

My point was this:

The C standard doesn't make this distinction.  You either have atomics or you don't.  Since it's not possible to implement C11 atomics in a lock-free manner on Cortex M0/M0+, compiler support is varied and not reliable. In principle a compiler could just guard all atomic operations into locked access which is an allowed implementation but will deadlock if used in an ISR.

I just did some testing on godbolt to see what actual compilers do:

gcc 16 emits standard load/store instructions with memory barriers for atomic load/store.  It emits calls to libatomic for atomic_swap and atomic_compare_exchange, which will fail since libatomic doesn't exist.  That's fine behavior if you want to use it for a simple queue from an interrupt handler, and you will get an error if you try to call unsupported functions.  But if you are using an RTOS and want to use the rest of the atomic operations for inter thread communication you are kind of stuck.  The only valid way to implement libatomic here is by disabling interrupts -- you can't do it with ordinary locks because the load/store operations won't respect the lock.  But disabling interrupts might be more intrusive than you want for inter-thread communication.

clang emits a warning that you have used an atomic type larger than the maximum lock-free type and always emits calls to libatomic.  This is the most flexible, since you can implement the functions either with locks or disabling interrupts depending on your use case.   You still have to pick, but it's your choice.  However, without mucking around in the platform defintiion, "is_always_lock_free" will be false even if you choose disabling interrupts.  clang even emits this warning (atomic object too larger for lock-free behavior) on atomic_flag.  This is a violation of the standard which requires that atomic_flag always be lock-free but there just isn't a way to implement that.

gcc 13 (which is only a couple years old), when faced with atomic_flag_test_and_set simply turns it into a load and a store!  This is an even worse violation of the standard since the sequence is definitely not atomic.

All this is why I don't recommend people to use C/C++ atomics for on CPUs that don't have RMW instructions, but instead just use volatile for ISRs.  It's not ideal, but it works, it's universally supported, and with good code design the amount of code that has to directly interact with the volatile variables is minimized.  It's easy enough to convert to a more modern atomics based operations if you ever port to a multi-core system.

[SiliconWizard]

I am not familiar with RISC-V, but another suspicious thing is that this code seems to be updating a flag in a register mstatus (?) in a non-atomic way, so the ISRs must not be writing to mstatus. But maybe that's not an issue.

Yeah, it's not atomic doing a read-modify-write sequence like that. I don't know why they're doing it that way. Why not do it with a single csrrs and csrrc instruction?

Code: [Select]

RV_STATIC_INLINE void __enable_irq ()
{
    uint32_t tmp;
    __ASM volatile (ADD_ARCH_ZICSR "csrrs %0," "mstatus, %1" : "=r" (tmp) : "i" (0x88));
}

RV_STATIC_INLINE void __disable_irq ()
{
    uint32_t tmp;
    __ASM volatile (ADD_ARCH_ZICSR "csrrc %0," "mstatus, %1" : "=r" (tmp) : "i" (0x88));
}


This is how they do it in the CH32V30x SDK from WCH, eg:

Code: [Select]

__attribute__( ( always_inline ) ) RV_STATIC_INLINE void __enable_irq()
{
  __asm volatile ("csrs 0x800, %0" : : "r" (0x88) );
}
ch32fun is cool but it's probably still pretty immature and I personally don't see the point of using it compared to WCH's SDKs which are pretty lean overall.
Of course, anyone who finds ch32fun useful and using it can create a ticket or PR so that they improve those two functions.