Compiler produces orr instead of bic using (O2 and Os)

Bug #1819187 reported by Morty on 2019-03-08
This bug affects 1 person
Affects Status Importance Assigned to Milestone
GNU Arm Embedded Toolchain

Bug Description

The code is more or less form the SMT32 lib, I did some of the preprocesser-replacements to simplify the code.
I noticed this bug using 7-2018-q2 on windows, but research shows it's more wide spread.

Using -O2/-Os and up GCC 6.4 produces the expected bic, while gcc 7.3 and later produces an orr. -O1 seems to be fine:

#include <stdint.h>

typedef struct
    volatile uint32_t DIFSEL;
} ADC_TypeDef;

#define WRITE_REG(REG, VAL) ((REG) = (VAL))

static inline void LL_ADC_SetChannelSingleDiff(ADC_TypeDef *ADCx, uint32_t Channel, uint32_t SingleDiff)
    (((ADCx->DIFSEL)) = ((((((ADCx->DIFSEL))) & (~(Channel & (((0x7FFFFU << (0U))))))) | ((Channel & (((0x7FFFFU << (0U))))) & ((0x7FFFFU << (0U)) << (SingleDiff & ((0x20U << (0U)))))))));

int test(ADC_TypeDef * testvar) {

    (( (0x01U << (26U))) | (((uint32_t)0x00000000U) | (((uint32_t) 3U) << ((uint32_t)20U))) | ((0x00002U << (0U))) ),
          ( (0x7FU << (0U)))

gcc-6.4 -Os:
        ldr r3, [r0]
        bic r3, r3, #2
        str r3, [r0]
        bx lr

gcc-7.3 -Os:
        ldr r3, [r0]
        orr r3, r3, #2
        str r3, [r0]
        bx lr

Dominic Plunkett (dp111) wrote :

I suspect the issue is : 0x7FFFU<< (SingleDiff & ((0x20U << (0U))))

which is a shift by 32 , this is undefined so the compiler can do what it likes

If you change the 0x7FFFFU to 0x7FFFFLL you get what you would expect. I would suggest there are betters to code this function than using 0x7FFFFLL

Morty (morty-rh) wrote :

I'm afraid you're right:

None the less I think this behavior is pretty dangerous without a warning, as I have seen this kind of stuff pretty often.
From the STM32-Hal where I got this code from:

  /* Bits of channels in single or differential mode are set only for */
  /* differential mode (for single mode, mask of bits allowed to be set is */
  /* shifted out of range of bits of channels in single or differential mode. */

As this is a gcc issue and not arm-gcc (the behavior can be seen with x86, too), I discuss that there.

Changed in gcc-arm-embedded:
status: New → Invalid
Dominic Plunkett (dp111) wrote :

Running cppcheck ( static analysis ) would show up this issue as being undefined. I think the STM Hal should be improved.

Morty (morty-rh) wrote :

I actually run cppcheck on our code once in a while, but there are just too many warnings in the STM HAL. Gcc already throws plenty of warnings.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers