1

I am working on some C++ code for small microcontrollers (AVR). We use a struct MCUto represent the register layout of the MCU. In the past all registers were of type uint8_t and we use the manufacture provided cpp-macros to set inidividual bits. This not the C++ way ...

We introduced the ControlRegister class template: it can be parametrized with the acceptable value type: 

template<typename Component, typename BitType, typename ValueType = uint8_t>
struct __attribute__((packed)) ControlRegister final {
    typedef Component component_type;
    typedef ValueType value_type;    
    typedef BitType bit_type;

    template<typename... Flags>
    void inline set(Flags... f) {
        static_assert((std::is_same<bit_type, Flags>::value && ... && true), "wrong flag type");
        static_assert(sizeof...(Flags) <= 8, "too much flags");
        value_type v = (static_cast<value_type>(f) | ... | 0);
        assert(Util::numberOfOnes(v) == sizeof... (Flags));
        hwRegister = v;
    }
    template<BitType... Flags>
    void inline add() {
        static_assert(sizeof...(Flags) <= 8, "too much flags");
        constexpr auto v = (static_cast<value_type>(Flags) | ... | 0);
        constexpr auto n = Util::numberOfOnes(v);        
        static_assert(n == sizeof... (Flags), "use different flags");
        hwRegister |= v;
    }
private:    
    volatile value_type hwRegister;
};

Some MCU registers have a two-fold usage, e.g. setting a timer prescaler and some other interrupt information. The prescaler bits are used at several places and we thought it would be better to divide the register into two or more parts resembling the structure of the particular register. Since this is essentially the very same memory location the idea to use a union came into mind:

struct MCU final {
    struct CompA {
        enum class Flags1 {
            f1 = (1 << 0),
            f2 = (1 << 1)
        };
        enum class Flags2 {
            f1 = (1 << 1),
            f2 = (1 << 2)
        };
        union {
            ControlRegister<CompA, Flags1> part1;
            ControlRegister<CompA, Flags2> part2;
        } __attribute__ ((packed));
    };    
};

The part2-member can be the forementioned prescaler component: the same value-type Flags can be used in several places.

I've read several posts about unions in C++, but I still have no clear inbterpretation if this code fragment yields to UB.

Looking at the assembler-language output of g++ leads to the assumption, that it is ok. But I'm not sure ...! 

int main() {
    const auto c = reinterpret_cast<MCU::CompA*>(0x28);

    using F1 = MCU::CompA::Flags1;
    c->part1.set(F1::f1, F1::f1); // writing

    using F2 = MCU::CompA::Flags2;
    c->part2.set(F2::f1); 

    c->part2.add<F2::f2>(); // reading

    while(true) {}
}
wimalopaan
  • 4,838
  • 1
  • 21
  • 39
  • I do not see anything which will be undefined in the provided snippet. Why do you suspect undefined behavior? – SergeyA Jan 24 '17 at 15:22
  • Because I'm accessing the physical register via `part1` and `part2` member. – wimalopaan Jan 24 '17 at 15:23
  • But C++ knows nothing of your register. Undefined behavior is a C++ term, and the program as written is well-formed. You might ask if this well-formed program would do what you expect it to do, but this is a different question. – SergeyA Jan 24 '17 at 15:25
  • 1
    I started to write up an answer but I think [this](http://stackoverflow.com/a/11996970/4342498) does it better. Looks UB per that answer. – NathanOliver Jan 24 '17 at 15:29
  • Well, most comments on accessing unions give the impression, that reading and writing from / to different members is UB. Perhaps this is not so clear in the example, but clearly we do read the value back. – wimalopaan Jan 24 '17 at 15:29
  • @NathanOliver: I read that answer but I am still not sure ... – wimalopaan Jan 24 '17 at 15:32
  • I see no reading from inactive union element here. You read from active in the provided snippet. – SergeyA Jan 24 '17 at 15:34
  • Still see no reading from inactive element. – SergeyA Jan 24 '17 at 15:40
  • I am using part1 (writing) and part2 (writing an reading) , well, in real code in random order ... – wimalopaan Jan 24 '17 at 15:42
  • Is it so hard to come up with clear MCVE demonstrating the problem you want to ask us about? – SergeyA Jan 24 '17 at 15:43
  • The different members have an identical, and standard layout, so the standard permits "inspection" of the inactive member. I'm not quite sure permitted "inspection" allows writing. Reading should be fine. – eerorika Jan 24 '17 at 15:46
  • 2
    @SergeyA `part1` and `part2` are different members of the same union. Since the OP calls a `add` which modifies the element and does not destruct the active member and construct the inactive member then it is UB. – NathanOliver Jan 24 '17 at 15:59
  • 1
    @NathanOliver, they are both trivially constructable, so I do not think it applies. You do not have to explictly call constructor on them. – SergeyA Jan 24 '17 at 16:11
  • As a matter of fact, I see undefined behavior here: `const auto c = reinterpret_cast(0x28)` While struct/union are trivially constructible, I do not remember standard giving any guarantee about the state of the trivially constructed object. So this action does not guarantee the state of the register. – SergeyA Jan 24 '17 at 16:13
  • @SergeyA It is really a question of if `part2` is considered alive when it is not the active member. If it is not then accessing it is UB. I'm not sure if there is anything in the standard that has that language and the previous answer I linked to also thinks it is UB. Me personally I would err on the side of caution. – NathanOliver Jan 24 '17 at 16:19
  • Please take into account that the data member is of type `volatile uint8_t`. I am not sure if this changes anything. – wimalopaan Jan 24 '17 at 16:50
  • Sorry for my ignorance: please give me a definition of the term: active member of a union. – wimalopaan Jan 24 '17 at 16:51
  • Additionally, since the types of the union members share the very same layout (only one data member of primitive type), according to http://stackoverflow.com/a/11906997/3359751 it should not be UB. – wimalopaan Jan 24 '17 at 17:13
  • 1
    @wimalopaan That answer says inspecting is legal and I agree with that. You are not inspecting though, you are writing which is a whole different thing. Active member means either the first member if it is default constructed or the member that was last constructed (either through a constructor call or manually doing it with placement new). – NathanOliver Jan 24 '17 at 17:54
  • @wimalopaan - I believe the active member of a union is that member that you have last written to. Writing to an inactive member makes it active and all the other members inactive. Reading from an inactive member is UB. – Omnifarious Mar 20 '17 at 02:01

0 Answers0