Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: Remove strict-aliasing warning #4184

Merged
merged 1 commit into from
Apr 20, 2017

Conversation

geky
Copy link
Contributor

@geky geky commented Apr 13, 2017

If you have compiled mbed-os with gcc version 6 you may have seen this warning:

Compile [ 10.9%]: equeue_mbed.cpp
[Warning] equeue_mbed.cpp@51,13: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

Short story:

This pr removes the warning.

Long story:

Several opaque buffers are used to to wrap c++ classes to pass to the c layer. The reinterpret cast to c++ classes is fine as long as the underlying buffer is not interpreted as different incompatible types, or else the behaviour is undefined.

In the equeue_tick_init function, placement new is used to initialize the buffers. However, this interprets the buffer as a simple array of bytes, not the actual class type. Later the buffer is casted to the class type. From the point of view of the compiler, these two types are incompatible, and the compiler is free to reorder the operations under the assumption that they can't affect each other.

Reinterpet casting the buffer to a class pointer before using placement new insures that the buffer is only interpreted as a single type. Or simple using the return value from placement new will handle the aliasing appropriately.

cc @pan-

Several opaque buffers are used to to wrap c++ classes to pass
to the c layer. The reinterpret cast to c++ classes is fine as long
as the underlying buffer is not interpreted as different incompatible
types, or else the behaviour is undefined.

In the equeue_tick_init function, placement new is used to initialize
the buffers. However, this interprets the buffer as a simple array
of bytes, not the actual class type. Later the buffer is casted to
the class type. From the point of view of the compiler, these two
types are incompatible, and the compiler is free to reorder the
operations under the assumption that they can't affect each other.

Reinterpet casting the buffer to a class pointer before using
placement new insures that the buffer is only interpreted as a single
type. Or simple using the return value from placement new will handle
the aliasing appropriately.
@geky geky added the needs: CI label Apr 13, 2017
@pan-
Copy link
Member

pan- commented Apr 13, 2017

I've approved the PR.

I want to add that it would have been simpler to wrap equeue_timer and equeue_ticker into dedicated functions:

static Timer* equeue_timer() { 
    static Timer timer;
    return &timer;
}

static Ticker* equeue_ticker() { 
    static Ticker ticker;
    return &ticker;
}

static void equeue_tick_update() {
    equeue_minutes += equeue_timer()->read_ms();
    equeue_timer()->reset();
}

static void equeue_tick_init() {
    equeue_minutes = 0;
    equeue_timer()->start();
    equeue_ticker()->attach_us(equeue_tick_update, 1000 << 16);
    equeue_tick_inited = true;
}

unsigned equeue_tick() {
    if (!equeue_tick_inited) {
        equeue_tick_init();
    }

    unsigned minutes;
    unsigned ms;

    do {
        minutes = equeue_minutes;
        ms = equeue_timer()->read_ms();
    } while (minutes != equeue_minutes);

    return minutes + ms;
}

The timer and ticker remains garbage collected if not used, are constructed at their first access and doesn't require cast.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 18, 2017

/morph test

1 similar comment
@bridadan
Copy link
Contributor

/morph test

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 28

All builds and test passed!

@adbridge adbridge merged commit 2ae34c4 into ARMmbed:master Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants