crashappsec / libcon4m

Base Compiler and Runtime Support for con4m
Apache License 2.0
0 stars 0 forks source link

build: consider suppressing/resolving `-Watomic-alignment` warnings #68

Closed ee7 closed 3 months ago

ee7 commented 3 months ago

With:

or:

running dev build by default produces about 2200 lines of -Watomic-alignment warnings, like:

../src/hatrack/hash/crown.c:416:25: warning: large atomic operation may incur significant performance penalty; the access size (16 bytes) exceeds the max lock-free size (8  bytes) [-Watomic-alignment]
  416 |         record        = atomic_read(&cur->record);
      |                         ^
../include/hatrack/hatomic.h:35:24: note: expanded from macro 'atomic_read'
   35 | #define atomic_read(x) atomic_load_explicit(x, memory_order_relaxed)
      |                        ^
/usr/lib/clang/17/include/stdatomic.h:141:30: note: expanded from macro 'atomic_load_explicit'
  141 | #define atomic_load_explicit __c11_atomic_load
      |                              ^

I added -Wno-atomic-alignment locally when using Clang, but we should either do that in the repo, or otherwise resolve these warnings.

Some context:

https://github.com/crashappsec/libcon4m/blob/b4fdc8e2e70b0ef18a554562702e2db1e0c9aff0/include/hatrack/hatomic.h#L26-L35

https://github.com/crashappsec/libcon4m/blob/b4fdc8e2e70b0ef18a554562702e2db1e0c9aff0/include/hatrack/hatomic.h#L40-L50

https://github.com/crashappsec/libcon4m/blob/b4fdc8e2e70b0ef18a554562702e2db1e0c9aff0/src/quark/x86_64.s#L6-L19

Where my understanding of the rationale for the last file was, from my Slack messages in January:

viega commented 3 months ago

There's nothing wrong with that code; it's intentional that we use the emulation where it's not supported in hardware, and it still performs well there; the emulation is not actually particularly slow.

Feel free to detect x86 and add the flag in the meson build.