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

[compiler-rt] [test] Fix using toolchains that rely on Clang default configs #113491

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

mstorsjo
Copy link
Member

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because some Linux distributions had a global default config file, that added flags relating to hardening, which interfere with the sanitizer tests. By setting CLANG_NO_DEFAULT_CONFIG, the global default config files that are found are ignored, and the sanitizers get the expected default compiler behaviour.

(This was #60394, which was fixed in 8ab7625.)

However, some toolchains may rely on default config files for mandatory parts required for functioning at all - setting things like sysroots, -rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such a case we can't forcibly disable any default config, because it will break the otherwise working toolchain.

Add a test for whether the compiler works while passing --no-default-config to it. If the option is accepted and the toolchain still works while that is set, set CLANG_NO_DEFAULT_CONFIG while running tests.

(This adds a little bit of inconsistency, as we're testing for the command line option, while using the environment variable. However doing compile testing with an environment variable isn't quite as easily doable, and passing an extra command line flag to all compile commands while testing, is a bit clumsy - therefore this inconsistency.)

@mstorsjo
Copy link
Member Author

FWIW, I'm not particularly happy about this solution, but I don't really see any other good alternatives either, between toolchains that have "mostly opinion" default configs and toolchains that have a hard dependency on their config files.

Copy link

github-actions bot commented Oct 23, 2024

✅ With the latest revision this PR passed the Python code formatter.

…configs

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because
some Linux distributions had a global default config file, that
added flags relating to hardening, which interfere with the
sanitizer tests. By setting CLANG_NO_DEFAULT_CONFIG, the global
default config files that are found are ignored, and the sanitizers
get the expected default compiler behaviour.

(This was llvm#60394, which
was fixed in 8ab7625.)

However, some toolchains may rely on default config files for
mandatory parts required for functioning at all - setting things
like sysroots, -rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such
a case we can't forcibly disable any default config, because it
will break the otherwise working toolchain.

Add a test for whether the compiler works while passing
--no-default-config to it. If the option is accepted and the
toolchain still works while that is set, set CLANG_NO_DEFAULT_CONFIG
while running tests.

(This adds a little bit of inconsistency, as we're testing for the
command line option, while using the environment variable. However
doing compile testing with an environment variable isn't quite as
easily doable, and passing an extra command line flag to all
compile commands while testing, is a bit clumsy - therefore this
inconsistency.)
@mstorsjo mstorsjo force-pushed the compiler-rt-nodefault branch from b4eb8c9 to 452160c Compare October 23, 2024 20:10
@MaskRay
Copy link
Member

MaskRay commented Oct 23, 2024

Looks fine to me :) I can approve if nobody has strong opinion against this.

Copy link
Member

@thesamesam thesamesam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, thanks, and I'm sorry for the breakage. I should've thought of this.

Second, looks good to me, but I'm also happy to -U_FORTIFY_SOURCE and such instead if someone really dislikes this approach.

@mgorny
Copy link
Member

mgorny commented Oct 24, 2024

Lemme just test quickly that it doesn't break things for us.

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mstorsjo
Copy link
Member Author

First, thanks, and I'm sorry for the breakage. I should've thought of this.

No problem - Clang config files can be used in a multitude of ways.

Btw, in a closely related matter, I'm aware of (at least) one other case where Clang users want to specify --no-default-config (which I think may be related to Gentoo default configs): Wine can use any Clang installation as cross compiler for windows targets - they don't need any runtime libs or such (they provide all that themselves), they just need a plain Clang executable. But if that Clang is set up with host environment specific defaults, those defaults may conflict with their use of Clang as a cross compiler for another entirely different target. Therefore, Wine also tries to add --no-default-config when using Clang in this role (and we're trying to resolve this same issue there as well, that the config file actually may be essential).

To reduce the need for that, if applicable, you could consider switching from a blanket general config file (like clang.cfg or what it may be called) to a target specific config file (like x86_64-unknown-linux-gnu.cfg), so that it only applies when actually compiling for the intended host, but not if the user specifies any other -target option.

Second, looks good to me, but I'm also happy to -U_FORTIFY_SOURCE and such instead if someone really dislikes this approach.

Thanks for the input. Yeah I'm not entirely sure which way forward is better - we can try to land this and see if it works, and if it seems to require more complex workarounds elsewhere, perhaps we can reconsider other approaches.

@mstorsjo mstorsjo merged commit a14a83d into llvm:main Oct 24, 2024
7 checks passed
@mstorsjo mstorsjo deleted the compiler-rt-nodefault branch October 24, 2024 20:45
@mstorsjo
Copy link
Member Author

FWIW, I'd like to backport this to 19.x if possible. But I'll wait for a little while to see whether this turns up any issues, before filing the backport request.

@vitalybuka
Copy link
Collaborator

it breaks several bots like this https://lab.llvm.org/buildbot/#/builders/164/builds/3908

@mstorsjo
Copy link
Member Author

it breaks several bots like this https://lab.llvm.org/buildbot/#/builders/164/builds/3908

To clarify for posteriority here as well - it seems like #108357 was the one that broke a buildbot, not this one.

@frobtech frobtech mentioned this pull request Oct 25, 2024
@mstorsjo mstorsjo added this to the LLVM 19.X Release milestone Oct 30, 2024
@mstorsjo
Copy link
Member Author

/cherry-pick a14a83d

@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2024

/pull-request #114229

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…configs (llvm#113491)

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because some
Linux distributions had a global default config file, that added flags
relating to hardening, which interfere with the sanitizer tests. By
setting CLANG_NO_DEFAULT_CONFIG, the global default config files that
are found are ignored, and the sanitizers get the expected default
compiler behaviour.

(This was llvm#60394, which was
fixed in 8ab7625.)

However, some toolchains may rely on default config files for mandatory
parts required for functioning at all - setting things like sysroots,
-rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such a case we can't
forcibly disable any default config, because it will break the otherwise
working toolchain.

Add a test for whether the compiler works while passing
--no-default-config to it. If the option is accepted and the toolchain
still works while that is set, set CLANG_NO_DEFAULT_CONFIG while running
tests.

(This adds a little bit of inconsistency, as we're testing for the
command line option, while using the environment variable. However doing
compile testing with an environment variable isn't quite as easily
doable, and passing an extra command line flag to all compile commands
while testing, is a bit clumsy - therefore this inconsistency.)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Nov 12, 2024
…configs (llvm#113491)

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because some
Linux distributions had a global default config file, that added flags
relating to hardening, which interfere with the sanitizer tests. By
setting CLANG_NO_DEFAULT_CONFIG, the global default config files that
are found are ignored, and the sanitizers get the expected default
compiler behaviour.

(This was llvm#60394, which was
fixed in 8ab7625.)

However, some toolchains may rely on default config files for mandatory
parts required for functioning at all - setting things like sysroots,
-rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such a case we can't
forcibly disable any default config, because it will break the otherwise
working toolchain.

Add a test for whether the compiler works while passing
--no-default-config to it. If the option is accepted and the toolchain
still works while that is set, set CLANG_NO_DEFAULT_CONFIG while running
tests.

(This adds a little bit of inconsistency, as we're testing for the
command line option, while using the environment variable. However doing
compile testing with an environment variable isn't quite as easily
doable, and passing an extra command line flag to all compile commands
while testing, is a bit clumsy - therefore this inconsistency.)

(cherry picked from commit a14a83d)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Nov 15, 2024
…configs (llvm#113491)

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because some
Linux distributions had a global default config file, that added flags
relating to hardening, which interfere with the sanitizer tests. By
setting CLANG_NO_DEFAULT_CONFIG, the global default config files that
are found are ignored, and the sanitizers get the expected default
compiler behaviour.

(This was llvm#60394, which was
fixed in 8ab7625.)

However, some toolchains may rely on default config files for mandatory
parts required for functioning at all - setting things like sysroots,
-rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such a case we can't
forcibly disable any default config, because it will break the otherwise
working toolchain.

Add a test for whether the compiler works while passing
--no-default-config to it. If the option is accepted and the toolchain
still works while that is set, set CLANG_NO_DEFAULT_CONFIG while running
tests.

(This adds a little bit of inconsistency, as we're testing for the
command line option, while using the environment variable. However doing
compile testing with an environment variable isn't quite as easily
doable, and passing an extra command line flag to all compile commands
while testing, is a bit clumsy - therefore this inconsistency.)

(cherry picked from commit a14a83d)
nikic pushed a commit to rust-lang/llvm-project that referenced this pull request Nov 20, 2024
…configs (llvm#113491)

The use of CLANG_NO_DEFAULT_CONFIG in the tests was added because some
Linux distributions had a global default config file, that added flags
relating to hardening, which interfere with the sanitizer tests. By
setting CLANG_NO_DEFAULT_CONFIG, the global default config files that
are found are ignored, and the sanitizers get the expected default
compiler behaviour.

(This was llvm#60394, which was
fixed in 8ab7625.)

However, some toolchains may rely on default config files for mandatory
parts required for functioning at all - setting things like sysroots,
-rtlib, -unwindlib, -stdlib, -fuse-ld etc. In such a case we can't
forcibly disable any default config, because it will break the otherwise
working toolchain.

Add a test for whether the compiler works while passing
--no-default-config to it. If the option is accepted and the toolchain
still works while that is set, set CLANG_NO_DEFAULT_CONFIG while running
tests.

(This adds a little bit of inconsistency, as we're testing for the
command line option, while using the environment variable. However doing
compile testing with an environment variable isn't quite as easily
doable, and passing an extra command line flag to all compile commands
while testing, is a bit clumsy - therefore this inconsistency.)

(cherry picked from commit a14a83d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants