APSR_Type and other registers in core_cm7.h and other headers files assume little-endianness

[amosnier]

/**
  \brief  Union type to access the Application Program Status Register (APSR).
 */
typedef union
{
  struct
  {
    uint32_t _reserved0:16;              /*!< bit:  0..15  Reserved */
    uint32_t GE:4;                       /*!< bit: 16..19  Greater than or Equal flags */
    uint32_t _reserved1:7;               /*!< bit: 20..26  Reserved */
    uint32_t Q:1;                        /*!< bit:     27  Saturation condition flag */
    uint32_t V:1;                        /*!< bit:     28  Overflow condition code flag */
    uint32_t C:1;                        /*!< bit:     29  Carry condition code flag */
    uint32_t Z:1;                        /*!< bit:     30  Zero condition code flag */
    uint32_t N:1;                        /*!< bit:     31  Negative condition code flag */
  } b;                                   /*!< Structure used for bit  access */
  uint32_t w;                            /*!< Type      used for word access */
} APSR_Type;

If that code is compiled according to the ARM ABI, the bit-field part will only correctly map the APSR on a little-endian system. Does ARM assume that no big-endian Cortex-M MCU will ever exist? In that case, it should at least be commented. It would be even better to somehow provide support for an assertion of that condition (I guess it's impossible to assert that at compile-time, so it would have to be at runtime).

[JonatanAntoni]

Hi @amosnier,

thanks for pointing this out. Typically, Cortex-M cores are used in little-endian systems. Let me check, if we can add clarification, here. Do you have a concrete big-endian device in mind?

Cheers, Jonatan

[amosnier]

@JonatanAntoni, maybe I should give you some background.

I'm currently working on an application that generates C++ code from SVD files.

In the generated code, I use bit-fields extensively, in a way which is similar to the ARM code I refer to in this issue. While such an approach provides the most convenient register mapping, it also gives a field layout which is implementation-defined from a C/C++ perspective. If I want my code-generation to work for any input SVD file, I need to be very careful. However, if I assume that the Procedure Call Standard for the Arm® Architecture is fulfilled by the toolchain that builds the code, I believe the problem boils down to an endianness issue.

My strategy to handle this issue, until further notice, is to only support little-endian, because all the cases I know about are little-endian MCUs. However, if my generated code was run on an MCU/CPU that does not meet my assumptions, I do not want it to silently fail.

When I read the ARM®v7-M ArchitectureReference Manual, I conclude that I cannot just assume that everything is little-endian, even for Cortex-M:

ENDIANNESS, bit[15]
    Indicates the memory system endianness:
    0   Little endian.
    1   Big endian.
    This bit is static or configured by a hardware input on reset.
    This bit is read only.

If endianness information is present in the SVD-file (it is unfortunately not always the case for Cortex-M MCUs) I assert little-endianness. I also provide a runtime bit-field/endianness assert function, and I strongly recommend my users to invoke it.

Finally, I reviewed ARM's library files in search for additional guidance, and came across core_cm7.h and the like.

I was surprised to see neither precaution nor comment in these files, in code that appears to assume little endianness. I started by asking the question on Stack Overflow, where I was recommended to contact ARM. I then posted the same question on the ARM community forums, and created this issue, since this is after all an implementation which is not strictly compliant to ARM's own specification. Unless, of course, I am missing something, in which case I very much would like to know what.

As a side note, I am also curious to hear what ARM has to say about one comment I got on Stack Overflow:

Nobody would start building new big-endian architectures today. Little-endian has won, and since the choice is entirely arbitrary, there is no value proposition to using the less-common variant. It's much better to include a single byte-order swapping instruction (REV Rd, Rm in cortexm3/m4) to enable efficient processing of legacy (network) protocols that require the wrong endianness. - EOF

Best regards, Alain Mosnier

[kjbracey]

I'm not going to address the endianness - I'm not actually sure how baked-in little-endianness support is to the CMSIS headers overall.

That APSR_Type clearly assumes little-endianness, but it is not used by the CMSIS code anywhere.

Which is probably just as well, as that sort of aliasing is not legal, unless I'm missing something.

This code invokes undefined behaviour:

 APSR_type apsr;
 apsr.w = __get_APSR();
 if (apsr.b.Q) {
    ....

You cannot write to one member of a union and read it as a different one, with a few exceptions of which this is not one.

That should be written as

uint32_t apsr;
apsr = __get_APSR();
if (apsr & APSR_Q_Msk) {
    ....

I would personally suggest that any such union types in CMSIS be deprecated then dropped.

Aside from those unions, I'm not sure what other little-endian assumptions there are.

You can legally use a bitfield to access a value, but it's really clunky. There would have to be at least 1 memcpy in there, and a union won't help you. Only stops being clunky in C++20 where you could use bit_cast.

struct APSR_bitfield {
    ...
 uint32_t Q:1;
    ...
 uint32_t N:1;
};
uint32_t apsr_raw = __get_APSR();
auto apsr = std::bit_cast<APSR_bitfield>(apsr_raw);
if (apsr.Q) {
}

Still nastier than just & APSR_Q_Msk, unless you wrap it into a function that just returns the APSR_bitfield. I guess such a function could hide a memcpy too.

APSR_bitfield __get_APSR_as_bitfield()
{
    uint32_t raw = __get_APSR();
    APSR_bitfield ret;
    memcpy(&ret, &raw, sizeof ret);
    return ret;
}

That would be legal, and you then just need to deal with getting the layout right.

You /could/ use the existing APSR_Type, but the union aliasing would be a trap. Better to not have the union as an undefined behaviour trap to fall into.

// Returns value in union's b - accessing return as w would be undefined behaviour.
APSR_Type __get_APSR_as_bitfield()
{
    uint32_t raw = __get_APSR();
    APSR_Type ret;
    ret.b.N = 0; // "activate" the b member - not clear if memcpy alone will do that.
    memcpy(&ret.b, &raw, sizeof ret);
    return ret;
}

[amosnier]

@kjbracey-arm, thanks for confirming the issue, albeit just for starters. I agree with the rest of your analysis too, and the issue you are pointing out is more serious than the one I reported. The APSR is not the only register mapped in that way either.

While I clearly see the point of using bit-fields for memory-mapped registers (which is not even the case for the APSR and friends), I do not advocate including them in a union. I think this is just confusing to the user, when a library is unclear about how a register should be accessed, and even encourages code with undefined behaviour.

Out of curiosity, I have just run, arbitrarily:

$ ./SVDConv ./STM32H7/STM32H743x.svd --generate=header --fields=struct-ansic

The peripheral declarations in the result file start with:

/**
  * @brief COMP1 (COMP1)
  */

typedef struct {                                /*!< (@ 0x58003800) COMP1 Structure                                            */

  union {
    __IM  uint32_t reg;                         /*!< (@ 0x00000000) Comparator status register                                 */

    struct {
      __IM  uint32_t C1VAL      : 1;            /*!< [0..0] COMP channel 1 output status bit                                   */
      __IM  uint32_t C2VAL      : 1;            /*!< [1..1] COMP channel 2 output status bit                                   */
            uint32_t            : 14;
      __IM  uint32_t C1IF       : 1;            /*!< [16..16] COMP channel 1 Interrupt Flag                                    */
      __IM  uint32_t C2IF       : 1;            /*!< [17..17] COMP channel 2 Interrupt Flag                                    */
            uint32_t            : 14;
    } bit;
  } SR;

So ARM indeed likes to include bit-fields in unions, and the little-endian assumption is all over the place. Time for a broader code review?

[jkrech]

I think it is more likely that you will find a note that Big Endian devices are not supported than changes risking to break code compatibility. Given the fact that we have not seen a single issue raised for an existing Arm Cortex-M based device being operated in Big Endian mode using one of the supported compilers, I would like you to put a relevant use case forward before investing into a broader review.

[amosnier]

@jkrech, please read @kjbracey-arm's analysis above. Most of it has nothing to do with endianness. And that analysis also applies to the code generated by SVDConv, as I have shown. When it comes to endianness, a clear statement about the assumptions made by the code would be fine, I think. You could also point out for your customers how easy it is to automatically check that assumption at runtime (in code that should probably be deactivated in the field). That is the strategy I use for ecg.

[kjbracey]

Given the fact that we have not seen a single issue raised for an existing Arm Cortex-M based device being operated in Big Endian mode using one of the supported compilers

That could mean either that

a) no-one's ever attempted it or b) it actually works fine, as long as you don't use those unions

I've never seen a Cortex-M configured big-endian, but I've also never seen those unions used, so I think either is possible. It may be that the unions are the only endian-ness issue. @amosnier - are you aware of any endianness issues other than the unions? (And are there any bitfield types not in a union?)

(I also wasn't aware of the SVDConv tool, so it's not like I'm a CMSIS expert - I live more at the tools end, which is why I'm chiming in on the union thing)

I would personally say that the unions should be deprecated altogether, due to the undefined behaviour trap, but you'd want to think about what, if anything, to replace them with first. In the meantime you could maybe just make the b member conditional on little-endian. They've clearly never worked in big-endian, so that can't break any currently-working code. (Unless someone is deliberately reading the "wrong" field knowing the layout is wrong!)

[amosnier]

@kjbracey-arm, I do not see the unions as an endianness issue. Assuming all MCUs are little-endian, your analysis above based on

You cannot write to one member of a union and read it as a different one

is still valid.

Other than that, I am not aware of any practical issue.

I would personally say that the unions should be deprecated altogether

That makes sense to me.

In the meantime you could maybe just make the b member conditional on little-endian.

Now you got my attention. How would you do that? Do you have a compile time solution? Otherwise, what runtime solution would you recommend?

[kjbracey]

make the b member conditional

I believe every toolchain will have some pre-defined macro indicating big or little endian config that you could #if on. I'm not in the habit of writing endianness-dependent code, so I can't tell you what those would be from memory. You could make a common one from those to test in "cmsis_compiler.h" and its subheaders.

[amosnier]

every toolchain will have some pre-defined macro indicating big or little endian config

@kjbracey-arm, that makes perfect sense, and I could find it on my usual compiler. I had ruled that out because endianness can be configured at runtime on some processors (non-Cortex-M), but of course the compiler still needs to know how it shall generate memory accesses.

Thanks a lot.

[amosnier]

I can confirm, after having tested it, that implementing compile time assertion of endianness as recommended by @kjbracey-arm above is trivial. That is the solution for this issue. It should also be implemented for SVDConv-generated code.

Additionally, I think ARM should follow @kjbracey-arm's recommendation about union-deprecation (see previous posts). This should probably be documented as a separate issue.