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

configurator: make c file name last in compiler args to workaround clang bug #11123

Closed
wants to merge 1 commit into from

Conversation

paparodeo
Copy link

@paparodeo paparodeo commented Nov 14, 2024

Re-order the compiler arguments so the c file name is last. This works around a clang bug which will suppress errors for unsupported flags if linker arguments come after the c file name. This suppression is a problem because it indicates that some argument is supported when it is not which can result in build errors.

llvm/llvm-project#116278

after this change the base module will build again with clang > 16 on systems without popcnt.
https://github.com/janestreet/base/blob/6ddf3438c79184fa443727058b623ccfe1d82ead/src/discover/discover.ml#L13-L20

eg: on aarch64 darwin and clang > 16 -mpopcnt will generate an error

$ clang -mpopcnt hello.c
clang: error: unsupported option '-mpopcnt' for target 'aarch64-apple-darwin'
$ echo $?
1

however, if appending -lpthread:

$ clang -mpopcnt hello.c -lpthread
$ echo $?
0

Which is a problem as code will assume -mpopcnt is supported and will then break later when compiling with the unsupported flag

@paparodeo paparodeo marked this pull request as draft November 14, 2024 23:58
@paparodeo paparodeo marked this pull request as ready for review November 15, 2024 00:15
@paparodeo paparodeo changed the title configurator: don't append link flags to workaround clang bug configurator: c file name is last in compiler args Nov 15, 2024
@paparodeo paparodeo changed the title configurator: c file name is last in compiler args configurator: make c file name is last in compiler args to workaround clang bug Nov 15, 2024
@paparodeo paparodeo changed the title configurator: make c file name is last in compiler args to workaround clang bug configurator: make c file name last in compiler args to workaround clang bug Nov 15, 2024
@paparodeo
Copy link
Author

paparodeo commented Nov 15, 2024

index 986cee9..3ca7459 100644
--- a/_build/.sandbox/beae43a6828cc6a280ceffe7f90d7970/default/test/blackbox-tests/test-cases/configurator-c-libraries.t/run.t
+++ b/_build/.sandbox/beae43a6828cc6a280ceffe7f90d7970/default/test/blackbox-tests/test-cases/configurator-c-libraries.t/run.t.corrected
@@ -10,4 +10,4 @@ link successfully
 
   $ dune exec -- ./discover.exe
   $ cat out
-  1
+  0

this is wrong. locally if I run this I get 1 with a variety of compilers and when running the test suite. not sure what the problem is on the CI but moving to draft till rectify situation.

@paparodeo paparodeo marked this pull request as draft November 15, 2024 21:35
Re-order the compiler arguments so that the c file is last. This works
around a clang bug which will suppress errors for unsupported flags if
linker arguments come after the c file name. This suppression is a
problem because it indicates that some argument is supported when it is
not which can result in build errors.

`https://github.com/llvm/llvm-project/issues/116278`

eg: on aarch64 darwin and clang > 16 `-mpopcnt` will generate an error

```
$ clang -mpopcnt hello.c
clang: error: unsupported option '-mpopcnt' for target 'aarch64-apple-darwin'
$ echo $?
1
```

however, if appending `-lpthread`:

```
$ clang -mpopcnt hello.c -lpthread
$ echo $?
0
```

Which is a problem as code will assume `-mpopcnt` is supported and will
then break later when compiling with the unsupported flag

Signed-off-by: Reno Dakota <paparodeo@proton.me>
@paparodeo paparodeo marked this pull request as ready for review November 16, 2024 00:10
@paparodeo paparodeo marked this pull request as draft November 16, 2024 07:34
@paparodeo
Copy link
Author

logged into the runner and sure enough gcc requires the args to be in order -- must succeed locally due to interaction with the gcc compiler wrapper on nix. anyway, this change won't work then.

@paparodeo paparodeo closed this Nov 16, 2024
@paparodeo paparodeo deleted the clang-workaround branch November 16, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant