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

Bug #1819187 reported by Morty
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
GNU Arm Embedded Toolchain
Invalid
Undecided
Unassigned

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:

https://godbolt.org/z/WJW4nc

#include <stdint.h>

typedef struct
{
    volatile uint32_t DIFSEL;
} ADC_TypeDef;

#define WRITE_REG(REG, VAL) ((REG) = (VAL))
#define MODIFY_REG(REG, CLEARMASK, SETMASK) WRITE_REG((REG), (((READ_REG(REG)) & (~(CLEARMASK))) | (SETMASK)))

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) {

    LL_ADC_SetChannelSingleDiff(testvar,
    (( (0x01U << (26U))) | (((uint32_t)0x00000000U) | (((uint32_t) 3U) << ((uint32_t)20U))) | ((0x00002U << (0U))) ),
          ( (0x7FU << (0U)))
    );
}

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

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

Revision history for this message
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

Revision history for this message
Morty (morty-rh) wrote :

I'm afraid you're right: http://c0x.coding-guidelines.com/6.5.7.html#1185

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
Revision history for this message
Dominic Plunkett (dp111) wrote :

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

Revision history for this message
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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.