Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" #19155

Teufelchen1 · 2023-01-16T11:02:31Z

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3) -shortens hotpath"

This reverts commit 5447203.

Contribution description

Compiling examples/gnrc_networking_mac using TOOLCHAIN=llvm yields the following error:

RIOT/sys/pm_layered/pm.c:77:16: error: unknown attribute 'optimize' ignored [-Werror,-Wunknown-attributes]
__attribute__((optimize(3)))

As indicated, this is because the attribute optimize is GCC only and not present in LLVM.
Compare the manpages of GCC and LLVM.

Testing procedure

Since this should only affect performance and not behavior, no special testing is needed. I am not aware of any tests in RIOT which could verify that assumption.

Issues/PRs references

Introduced in #18846

There is another instance of this attribute being used in shell_lock.c. Since the usage is security related, I omit it from this PR.

…ens hotpath" This reverts commit 5447203.

benpicco

@kfessel what difference doesn -O3 even make here?
I can't think of that many different ways to optimize this.

I would assume most optimizations came from the proper alignment?

benpicco · 2023-01-16T11:49:38Z

There is another instance of this attribute being used in shell_lock.c. Since the usage is security related, I omit it from this PR.

shell_lock is more of a tamper protection than real security. There is a wanrning about this in the doc.

benpicco · 2023-01-16T12:54:16Z

bors merge

bors · 2023-01-16T12:54:20Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

riot-ci · 2023-01-16T13:15:02Z

Murdock results

✔️ PASSED

571026d Revert "sys/pm_layered: pm_(un)block add attribute optimize(3) -shortens hotpath"

Success	Failures	Total	Runtime
6785	0	6785	12m:39s

Artifacts

Documentation preview

bors · 2023-01-16T13:15:59Z

Already running a review

18477: gnrc_static: add static network configuration r=miri64 a=benpicco 19155: Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" r=benpicco a=Teufelchen1 Revert "sys/pm_layered: pm_(un)block add attribute optimize(3) -shortens hotpath" This reverts commit 5447203. ### Contribution description Compiling `examples/gnrc_networking_mac` using `TOOLCHAIN=llvm` yields the following error: ``` RIOT/sys/pm_layered/pm.c:77:16: error: unknown attribute 'optimize' ignored [-Werror,-Wunknown-attributes] __attribute__((optimize(3))) ``` As indicated, this is because the attribute `optimize` is GCC only and not present in LLVM. Compare the manpages of [GCC](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html) and [LLVM](https://clang.llvm.org/docs/AttributeReference.html). ### Testing procedure Since this should only affect performance and not behavior, no special testing is needed. I am not aware of any tests in RIOT which could verify that assumption. ### Issues/PRs references Introduced in #18846 There is another instance of this attribute being used in[ shell_lock.c](https://github.com/RIOT-OS/RIOT/blob/6fb340d654ac8da07759cb9199c6aaa478589aa7/sys/shell_lock/shell_lock.c#L80). Since the usage is security related, I omit it from this PR. Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com> Co-authored-by: Teufelchen1 <bennet.blischke@haw-hamburg.de>

maribu · 2023-01-16T13:29:41Z

bors cancel
bors merge

(This should allow another PR to hop onto the merge train 🚂 🚋 🚋 🚋)

bors · 2023-01-16T13:29:44Z

Canceled.

bors · 2023-01-16T13:31:18Z

Already running a review

18477: gnrc_static: add static network configuration r=miri64 a=benpicco 19155: Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" r=maribu a=Teufelchen1 Revert "sys/pm_layered: pm_(un)block add attribute optimize(3) -shortens hotpath" This reverts commit 5447203. ### Contribution description Compiling `examples/gnrc_networking_mac` using `TOOLCHAIN=llvm` yields the following error: ``` RIOT/sys/pm_layered/pm.c:77:16: error: unknown attribute 'optimize' ignored [-Werror,-Wunknown-attributes] __attribute__((optimize(3))) ``` As indicated, this is because the attribute `optimize` is GCC only and not present in LLVM. Compare the manpages of [GCC](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html) and [LLVM](https://clang.llvm.org/docs/AttributeReference.html). ### Testing procedure Since this should only affect performance and not behavior, no special testing is needed. I am not aware of any tests in RIOT which could verify that assumption. ### Issues/PRs references Introduced in #18846 There is another instance of this attribute being used in[ shell_lock.c](https://github.com/RIOT-OS/RIOT/blob/6fb340d654ac8da07759cb9199c6aaa478589aa7/sys/shell_lock/shell_lock.c#L80). Since the usage is security related, I omit it from this PR. Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com> Co-authored-by: Teufelchen1 <bennet.blischke@haw-hamburg.de>

maribu · 2023-01-16T13:33:36Z

One last try to pack the train:

bors cancel
bors merge

bors · 2023-01-16T13:33:38Z

Canceled.

benpicco · 2023-01-16T13:52:42Z

bors merge

bors · 2023-01-16T13:52:46Z

Already running a review

kfessel · 2023-01-16T13:54:31Z

@kfessel what difference doesn -O3 even make here? I can't think of that many different ways to optimize this.

I would assume most optimizations came from the proper alignment?

the attribute shortens the hotpath by reordering instructions preferring the common case of not failing the assertion -> less jumps -> less loss of pipeline

btw this is not an error but an elevated warning

benpicco · 2023-01-16T13:59:23Z

Uh sounds like we need

#define likely(x)       __builtin_expect((x),1)
#define unlikely(x)     __builtin_expect((x),0)

like Linux, then assert() would be

#define assert(cond) (likely(cond) ? (void)0 : _assert_panic())

kfessel · 2023-01-16T16:19:19Z

we could use #pragma GCC optimize(3) (not per function but for the whole file) to achieve the same optimization without clang complaining

kfessel · 2023-01-16T16:28:08Z

another fix would be not to warn for unknown attributes in clang

maribu · 2023-01-16T16:30:12Z

Maybe we should just add -Wno-error=unknown-attributes when compiling with LLVM?

Otherwise this likely will be a whack-a-mole game, as we only compile-test with GCC.

bors · 2023-01-16T16:52:21Z

Build succeeded:

19157: sys/shell_lock: do not call strlen, less jumpy r=kaspar030 a=kfessel ### Contribution description the current `_safe_strcmp` depends on not being optimized and not being inlined (implicitly given by the -O0), this new one does less (would say not since even O3 had compile results that should not return early or show different runtimes for different secrets). the runtime of strlen depend on the length of the string (password) therefor it is removed. `ifs` are very jumpy and depend on the contend of password, this avoids `ifs` sadly clang still compiles some boolean statements to if. with this PR the `__attribute__((optimize("O0")))` should be removable. ### Testing procedure see https://godbolt.org/z/x35b85cEx and run the `<RIOT>/tests/shell_lock` ### Issues/PRs references #19155 made me aware of this function Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>

19157: sys/shell_lock: do not call strlen, less jumpy r=benpicco a=kfessel ### Contribution description the current `_safe_strcmp` depends on not being optimized and not being inlined (implicitly given by the -O0), this new one does less (would say not since even O3 had compile results that should not return early or show different runtimes for different secrets). the runtime of strlen depend on the length of the string (password) therefor it is removed. `ifs` are very jumpy and depend on the contend of password, this avoids `ifs` sadly clang still compiles some boolean statements to if. with this PR the `__attribute__((optimize("O0")))` should be removable. ### Testing procedure see https://godbolt.org/z/x35b85cEx and run the `<RIOT>/tests/shell_lock` ### Issues/PRs references #19155 made me aware of this function Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3) -short…

571026d

…ens hotpath" This reverts commit 5447203.

Teufelchen1 requested a review from kaspar030 as a code owner January 16, 2023 11:02

github-actions bot added the Area: sys Area: System label Jan 16, 2023

miri64 requested review from kfessel and benpicco January 16, 2023 11:24

benpicco approved these changes Jan 16, 2023

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2023

benpicco mentioned this pull request Jan 16, 2023

core/compiler_hints: add likely() / unlikely() hints #19156

Merged

bors bot merged commit 211db05 into RIOT-OS:master Jan 16, 2023

kfessel mentioned this pull request Jan 16, 2023

sys/shell_lock: do not call strlen, less jumpy #19157

Merged

jia200x added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" #19155

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" #19155

Teufelchen1 commented Jan 16, 2023

benpicco left a comment

benpicco commented Jan 16, 2023

benpicco commented Jan 16, 2023

bors bot commented Jan 16, 2023

riot-ci commented Jan 16, 2023

bors bot commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

bors bot commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

benpicco commented Jan 16, 2023

bors bot commented Jan 16, 2023

kfessel commented Jan 16, 2023 •

edited

benpicco commented Jan 16, 2023

kfessel commented Jan 16, 2023

kfessel commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" #19155

Revert "sys/pm_layered: pm_(un)block add attribute optimize(3)" #19155

Conversation

Teufelchen1 commented Jan 16, 2023

Contribution description

Testing procedure

Issues/PRs references

benpicco left a comment

Choose a reason for hiding this comment

benpicco commented Jan 16, 2023

benpicco commented Jan 16, 2023

bors bot commented Jan 16, 2023

riot-ci commented Jan 16, 2023

Murdock results

Artifacts

bors bot commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

bors bot commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

benpicco commented Jan 16, 2023

bors bot commented Jan 16, 2023

kfessel commented Jan 16, 2023 • edited

benpicco commented Jan 16, 2023

kfessel commented Jan 16, 2023

kfessel commented Jan 16, 2023

maribu commented Jan 16, 2023

bors bot commented Jan 16, 2023

kfessel commented Jan 16, 2023 •

edited