warnings generated by retain attribute in sel4test library header

[kent-mcleod]

When support for lld was added in #75, this retain attribute was added. However this is causing warnings now when compiling with gcc-12:

/tmp/tmp.BhrhA5NZPe/projects/seL4_libs/libsel4test/include/sel4test/test.h:145:12: error: 'retain' attribute ignored [-Werror=attributes]
  145 |     struct testcase TEST_ ## _name = { \
      |            ^~~~~~~~
/tmp/tmp.BhrhA5NZPe/projects/seL4_libs/libsel4test/include/sel4test/test.h:155:73: note: in expansion of macro 'DEFINE_TEST_WITH_TYPE'
  155 | #define DEFINE_TEST_BOOTSTRAP(_name, _description, _function, _enabled) DEFINE_TEST_WITH_TYPE(_name, _description, _function, BOOTSTRAP, _enabled)
      |                                                                         ^~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.BhrhA5NZPe/projects/sel4test/apps/sel4test-driver/src/tests/syscall.c:237:5: note: in expansion of macro 'DEFINE_TEST_BOOTSTRAP'
  237 |     DEFINE_TEST_BOOTSTRAP(_test, "Basic " #_syscall "() testing", test_ ## _syscall, _enabled)
      |     ^~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.BhrhA5NZPe/projects/sel4test/apps/sel4test-driver/src/tests/syscall.c:239:5: note: in expansion of macro 'GENERATE_SYSCALL_TEST_MAYBE'
  239 |     GENERATE_SYSCALL_TEST_MAYBE(_test, _syscall, _code, true)
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.BhrhA5NZPe/projects/sel4test/apps/sel4test-driver/src/tests/syscall.c:245:1: note: in expansion of macro 'GENERATE_SYSCALL_TEST'
  245 | GENERATE_SYSCALL_TEST(SYSCALL0000, seL4_Yield,
      | ^~~~~~~~~~~~~~~~~~~~~
/tmp/tmp.BhrhA5NZPe/projects/seL4_libs/libsel4test/include/sel4test/test.h:145:12: error: 'retain' attribute ignored [-Werror=attributes]
  145 |     struct testcase TEST_ ## _name = { \
      |            ^~~~~~~~

There doesn't seem to be a way to reliably test if gcc supports the retain attribute as __has_attribute(retain) returns 1 anyway.
Is there an alternative approach here? (ping @heshamelmatary)

[Indanz]

binutils 2.36 or newer is required according to this, otherwise you get that warning. There is a bug report by the same llvm dev for gcc 11 here with some more info: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

[heshamelmatary]

@kent-mcleod I believe @Indanz is right. I also left a comment back then to address this issue that might solve those warnings? seL4/seL4_tools#173 (comment)

Please let me know if this issue still holds

[midnightveil]

This makes the current sel4test manifest fail to build now, when building with arm-gnu-toolchain-12.2.rel1-x86_64-aarch64-none-elf, because of -Werror.

Reverting 12d0d97 (in seL4_libs) and be8277a84c0c59b472909877540d55d53739001e. (in seL4_tools) allows this to build.

[Indanz]

We have multiple options:

• Just not support toolchains with mismatching binutils.
• Revert those commits.
• Add retain only for ldd builds.
• Disable link time optimisations.
• Add -Wno-error=attributes (or whatever the correct option to turn it into a warning is.)
• Find an alternative solution to retain to solve the original problem.

I have no opinion about the way forward.

12.2 was released in 2022, do any newer versions have the same problem? Current release is 14.2.

[kent-mcleod]

It works on the versions of the toolchains provided by the docker containers, which is why the changes haven't been reverted yet.

If there's an easy way to conditionally compile based on lld then that would be good, but I'm not sure what the condition would be. Unless it's a conditional on clang vs gcc.

We could add some cmake tests that checks if the toolchain supports retain and then conditionally use it based on that. But that's more maintenance effort.

The documentation on the used attribute 1, explains that for ELF targets the retain attribute is needed to prevent unused sections from being stripped during linker section GC.

[heshamelmatary]

We could probably, in cmake, check for the binutils version and conditionally add -z nostart-stop-gc for older versions

[Ivan-Velickovic]

12.2 was released in 2022, do any newer versions have the same problem? Current release is 14.2.

Tried with aarch64-none-elf-gcc (GCC) 14.2.1 20250322 and GNU ld (GNU Binutils) 2.44 but still the same issue.

[Indanz]

We could probably, in cmake, check for the binutils version and conditionally add -z nostart-stop-gc for older versions

Considering the problems this gives with gcc, I propose we remove the newly added retain keyword altogether and just use -z nostart-stop-gc to fix the original llvm problem. Any downsides to that?

[kent-mcleod]

Alternatively we could unwind the previous commit and make the retain keyword conditional of the compiler-linker selection that's known in CMake scope. In the draft PR i just conditioned on Clang or !Clang because I don't have toolchains to test with in front of me: #102

Considering the problems this gives with gcc, I propose we remove the newly added retain keyword altogether and just use -z nostart-stop-gc to fix the original llvm problem. Any downsides to that?

Won't we then have to detect and handle cases where nostart-stop-gc isn't supported by the linker?

[Indanz]

Alternatively we could unwind the previous commit and make the retain keyword conditional of the compiler-linker selection that's known in CMake scope. In the draft PR i just conditioned on Clang or !Clang because I don't have toolchains to test with in front of me: #102

That seems the best short-term fix, yes. Except if we can find a better solution.

Considering the problems this gives with gcc, I propose we remove the newly added retain keyword altogether and just use -z nostart-stop-gc to fix the original llvm problem. Any downsides to that?

Won't we then have to detect and handle cases where nostart-stop-gc isn't supported by the linker?

Yes, but if the linker ignores unknown options it's an improvement. We'd add it only for llvm, until gcc also enables LTO by default. Anyway, limiting retain to Clang is probably the better solution, but this is something we need to keep in mind.

This post is very illuminating.

Network manager had the same issue and they solved it by wrapping retain around a __clang__check too.

[midnightveil]

Just for the record: elfloader-tool also needs to be fixed https://github.com/seL4/seL4_tools/blob/34bb2ca78418f3347e7b3aad7e4b5035339f8a5d/elfloader-tool/include/drivers/common.h#L46