tests/event_wait_timeout: Fix for 8bit platforms by maribu · Pull Request #12776 · RIOT-OS/RIOT

maribu · 2019-11-21T22:49:42Z

Contribution description

Fix a potential issue with the use of a regular global variable for communication between thread and IRQ context
Fixed US_PER_MS and friends to be safely used when sizeof(unsigned) < sizeof(uint32_t). Citing the commit message:

The macros US_PER_MS and friends are assumed to be 32 bit unsigned integers by users. However, e.g. on AVR a 1000U is only 16 bit long. Thus, e.g. xtimer_usleep(100 * US_PER_MS) will wrap around and only sleep for ~34ms.

This commit ensures that they are treated as 32 bit unsigned integers on all
platforms.

Testing procedure

tests/event_wait_timeout should now work on ATmega

Issues/PRs references

Tick second item in #12651

maribu · 2019-11-21T22:54:21Z

This took me ages to understand why the xtimer which was more distant in the future was added in xtimer_t *timer_list_head before the xtimer that was sooner to fire.

aabadie · 2019-11-22T07:10:55Z

I confirm this PR is fixing tests/event_wait_timeout on AVR (tested on atmega328p and atmega-mega2560).

There are other issues reported by Murdock with tests/xtimer_msg.

Maybe @smlng could also have a look since he already worked on fixing xtimer macros.

kaspar030

We're using "LU" everywhere else instead of casts to type.

kaspar030 · 2019-11-22T08:53:27Z

sys/include/timex.h

 * @brief The number of microseconds per second
 */
-#define US_PER_SEC (1000000U)
+#define US_PER_SEC ((uint32_t)1000000U)


Can we please use (1000000LU)?

I tried that, but that yielded 100+ compilation errors as PRIu32 expects unsigned int, not unsinged long. Even though they are the same. But the cast clearly doesn't work, as other code is using US_PER_SEC in preprocessor #if statements. They don't work with types...

Can you keep the UL here but add the cast where needed in tests/xtimer_msg ?

Let me first try to weasel around this by using U on systems with sizeof(unsinged) == sizeof(uint32_t)andLUon other systems. With usingLU` unconditionally, the number of compilation issues was huge...

Also: I think users can rightfully expect that PRIu32 should work with something like e.g. US_PER_SEC * 200. The only way to solve this (except for the casts to uint32_t) is to make the postfix dependent on the integer width.

OK. The current approach at least works locally. Let's wait for Murdock to confirm.

uff, but it is mega hacky and different to everywhere else in the code.

... but also quite contained and practical...

Still fails (which I currently fail to understand). I need to think about this for some time. Maybe there is a cleaner solution that just didn't occur to me yet.

OK. On ESP32 without an explicit #include <limits.h> is needed. (I was pretty sure that an #include <stdint.h> should have also provided UINT_MAX, but maybe I was wrong.)

maribu · 2019-11-22T20:23:50Z

OK. With the code now (hopefully!) compiling on all platforms, so that MS_PER_USEC * 100 would yield reliably an uint32_t on every platform, it is time to decide if we want this solution.

The only two other solutions I have in mind are:

Use C casts to uint32_t. But that would break some other code that does things like #if MS_PER_USED * 50 < FOO, in which casts are not valid. And some MAC layer code actually does something like this, so this is not hypothetical.
Always use LU as postfix. But e.g. xtimer_usleep() requires uint32_t. At least clang is very unhappy if you pass a long instead of an int; even when they happen to have the same representation and value range. And on 32 bit platforms uint32_t is defined as unsigned int, so this would also potentially bite us later.

maribu · 2019-11-26T11:43:41Z

@kaspar030: What is the verdict on this?

Summing up the approaches to fix the issue that were discussed so far, and their pros and cons:

Consistently define US_PER_MS and friends as (1000LU)
- 👍 Simple, readable
- 👍 Yields 32 bit unsigned integers on all supported platforms (at least I believe so)
- 👎 Compiler errors on printf() with PRIu32, as uint32_t is unsigned int and not unsigned long on 32 bit architectures
- 👎 Potential compiler errors on calls to xtimer_usleep(42 * US_PER_MS), as unsigned long and uint32_t are formally not the same on 32 bit platforms. (At least I had clang complying about this at some point in time, but I don't remember the warning config.)
Use a c style cast to uint32_t, e.g. #define US_PER_MS ((uint32_t)1000)
- 👍 Reliably yields uint32_t on every platform, so printf("%" PRIu32, US_PER_MS) with PRIu32 and xtimer_usleep(42 * US_PER_MS) will just work
- 👎 Cannot be used in pre-prosessor statements
- 👎 Might not be usable for assignments of const variables / struct member (initializer not constant) on some compilers
- 👎 Ugly
Keep US_PER_MS and friends as they are, every uses needs to ensure that no integer width issue appear
- 👍 no changes are needed
- 👎👎👎 latent bugs all over the place
Use postfix LU for non-32bit platforms, and U for 32-bit platforms
- 👍 Reliably yield uint32_t on every supported platform, so printf("%" PRIu32, US_PER_MS) with PRIu32 and xtimer_usleep(42 * US_PER_MS) will just work
- 👍 Can be used reliably in per-processor statements and as constant initializer even with prehistoric toolchains (looking at AVR and MSP430)
- 👎 Ugly

kaspar030 · 2019-11-26T12:13:17Z

Compiler errors on printf() with PRIu32, as uint32_t is unsigned int and not unsigned long on 32 bit architectures

How many are there? Maybe fixing those is the best option?

Throughout the codebase, we've been using integer literals with type suffixes as needed.

-1 Potential compiler errors on calls to xtimer_usleep(42 * US_PER_MS), as unsigned long and uint32_t are formally not the same on 32 bit platforms. (At least I had clang complying about this at some point in time, but I don't remember the warning config.)

This shouldn't happen, as C has legal implicit type conversions, right?

maribu · 2019-11-26T12:25:50Z

This shouldn't happen, as C has legal implicit type conversions, right?

For unsigned this should be indeed be safe. It seems I had the issue with signed int and long. Citing cppreference on this:

if the target type can represent the value, the value is unchanged

otherwise, if the target type is signed, the behavior is implementation-defined (which may include raising a signal)

So in the current case uint32_t can represent the value of an unsigned long and it is not signed. So just using LU and getting rid of of PRIu32 should indeed work here.

maribu · 2020-04-23T19:15:15Z

I have to rebase in order to be able to fix issues in tests/ztimer_msg, which was not yet part of RIOT when this PR was created.

The test currently uses static globals for communication without any protection. This could lead optimizing C compilers to deduce that access to those are not needed (e.g. with LTO enabled). Using C11 atomics is the easiest way to tell the compiler that those accesses are used for communication between two different threads of execution (here: between the ISR and the main thread).

The macros US_PER_MS and friends are assumed to be 32 bit unsigned integers by users. However, e.g. on AVR a `1000U` is only 16 bit long. Thus, e.g. `xtimer_usleep(100 * US_PER_MS)` will wrap around and only sleep for ~34ms. This commit declares them as unsigned long, which is on all currently supported platforms 32 bit wide.

maribu · 2020-04-23T19:43:11Z

All green now. Time to cross this issue of the list :-)

maribu · 2020-05-05T19:28:47Z

@kaspar030: You're comments are addressed and all is green.

kaspar030

ACK.

maribu · 2020-05-08T06:11:11Z

Thanks for the review!

maribu requested a review from aabadie November 21, 2019 22:49

maribu removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Nov 22, 2019

kaspar030 requested changes Nov 22, 2019

View reviewed changes

maribu mentioned this pull request Nov 22, 2019

tests/isr_yield_higher: Make use of US_PER_MS #12791

Merged

aabadie mentioned this pull request Nov 23, 2019

Failing tests on AVR (tested with atmega256rfr2-xpro) #12651

Closed

13 tasks

maribu force-pushed the test_event_wait_timeout branch from b82ec01 to b9376f0 Compare November 26, 2019 12:35

maribu requested review from MichelRottleuthner, PeterKietzmann, cgundogan, fjmolinas, haukepetersen, leandrolanzieri, miri64 and smlng as code owners April 23, 2020 15:36

maribu added 2 commits April 23, 2020 21:16

maribu added 3 commits April 23, 2020 21:16

sys/net/gnrc: Use %lu to print times

7b67e74

tests/xtimer_msg: Use %lu to print times

1b44066

tests/ztimer_msg: Use %lu to print times

9f75501

maribu force-pushed the test_event_wait_timeout branch from 37a8bf2 to 9f75501 Compare April 23, 2020 19:18

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 5, 2020

kaspar030 approved these changes May 5, 2020

View reviewed changes

kaspar030 merged commit 8ffd34d into RIOT-OS:master May 5, 2020

maribu deleted the test_event_wait_timeout branch May 8, 2020 06:10

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

This was referenced Jul 15, 2020

tests/gnrc_dhcpv6_client: fix expected output #14524

Merged

tests/gnrc_dhcpv6_client: fix expected output [backport 2020.07] #14528

Merged

Conversation

maribu commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Nov 21, 2019

Uh oh!

aabadie commented Nov 22, 2019

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu commented Nov 22, 2019

Uh oh!

maribu commented Nov 26, 2019

Uh oh!

kaspar030 commented Nov 26, 2019

Uh oh!

maribu commented Nov 26, 2019

Uh oh!

maribu commented Apr 23, 2020

Uh oh!

maribu commented Apr 23, 2020

Uh oh!

maribu commented May 5, 2020

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented May 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maribu commented Nov 21, 2019 •

edited

Loading