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

Build with -Werror=implicit-function-declaration #627

Merged

Conversation

XrXr
Copy link
Contributor

@XrXr XrXr commented May 30, 2024

To fail-fast in situations like
rust-lang/rust#125619, where an upstream
source compiles but creates a link error way downstream.

@XrXr
Copy link
Contributor Author

XrXr commented May 30, 2024

This is revealing the issue in rust-lang/rust#125619 indeed (aarch64-unknown-linux-gnu is failing). Worth considering once that's solved.

@tgross35
Copy link
Contributor

tgross35 commented Sep 4, 2024

This is revealing the issue in rust-lang/rust#125619 indeed (aarch64-unknown-linux-gnu is failing). Worth considering once that's solved.

Does that just need llvm/llvm-project#93890 to work its way into our LLVM, or do we need to make changes to what .c files we include here?

@XrXr
Copy link
Contributor Author

XrXr commented Oct 2, 2024

It should be good once our LLVM updates.

@tgross35
Copy link
Contributor

tgross35 commented Oct 2, 2024

Mind rebasing so we get an up to date CI run? I'd like to see what the errors are.

llvm/llvm-project#93890 probably could have been requested for a cherry pick but it's likely too late now :/

@XrXr XrXr force-pushed the werror-implicit-function-declaration branch from 97c266e to 3912be0 Compare October 2, 2024 16:58
@tgross35
Copy link
Contributor

tgross35 commented Oct 2, 2024

Oh I guess that did make it to our LLVM somehow, interesting.

One thing I am not sure about is whether we want this check all the time or only for testing. Are there any cases where this could cause a build failure where the user doesn't expect it? I wonder if it would be better to add something like a COMPILER_BUILTINS_STRICT_C flag that we set in CI to enable this and (later) possibly some other warnings/werror.

@Amanieu I'll defer to you for what would be best here.

@XrXr XrXr marked this pull request as ready for review October 2, 2024 17:26
@XrXr
Copy link
Contributor Author

XrXr commented Oct 2, 2024

It looks like CI runs with LLVM 18.0-2024-02-13, which doesn't include llvm/llvm-project#93890, but with 3032f49, we're now using a new enough GCC to dodge the build error. (By the way, "updating the compiler" was also the route rustc took to fix the build issue for the time being.)

@tgross35
Copy link
Contributor

tgross35 commented Oct 2, 2024

We can upgrade LLVM so I put up a PR to do that #703.

@Amanieu
Copy link
Member

Amanieu commented Oct 3, 2024

I think we should probably enable this, at the very least it will notify users that their GCC and/or LLVM needs to be updated to avoid the issue.

To prevent fail-fast in situations like
rust-lang/rust#125619, where an upstream
source compiles but creates a link error way downstream.
@tgross35 tgross35 force-pushed the werror-implicit-function-declaration branch from 3912be0 to 64282a7 Compare October 3, 2024 15:24
@tgross35 tgross35 merged commit 8bc5b9c into rust-lang:master Oct 3, 2024
25 checks passed
@tgross35
Copy link
Contributor

tgross35 commented Oct 3, 2024

Released with the update #700. Could you make a PR to rust-lang/rust updating to 0.1.131?

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
This commit updates compiler-builtins from 0.1.130 to 0.1.131.
The only change in this bump is
<rust-lang/compiler-builtins#627>.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
Update compiler-builtins to 0.1.131

This commit updates compiler-builtins from 0.1.130 to 0.1.131.
The only change in this bump is
<rust-lang/compiler-builtins#627>.
XrXr added a commit to XrXr/rust that referenced this pull request Oct 4, 2024
This commit updates compiler-builtins from 0.1.130 to 0.1.131.

PRs in the delta:
 - rust-lang/compiler-builtins#698
 - rust-lang/compiler-builtins#699
 - rust-lang/compiler-builtins#701
 - rust-lang/compiler-builtins#704
 - rust-lang/compiler-builtins#627

rust-lang/compiler-builtins#627
("Build with -Werror=implicit-function-declaration") should be the only
user-facing change. The rest are changes to tests and the crate's CI
setup.
XrXr added a commit to XrXr/compiler-builtins that referenced this pull request Oct 4, 2024
Testing in <rust-lang/rust#131221>, we found
that <rust-lang#627> is
unusable with the current LLVM version.
XrXr added a commit to XrXr/rust that referenced this pull request Oct 4, 2024
This commit updates compiler-builtins from 0.1.130 to 0.1.132.

PRs in the delta:
 - rust-lang/compiler-builtins#698
 - rust-lang/compiler-builtins#699
 - rust-lang/compiler-builtins#701
 - rust-lang/compiler-builtins#704
 - rust-lang/compiler-builtins#627
 - rust-lang/compiler-builtins#706

rust-lang/compiler-builtins#627
("Build with -Werror=implicit-function-declaration") and
rust-lang/compiler-builtins#706 ("Allow implicit function decl on A64")
should be the only user-facing changes. The rest are test and CI
changes.
XrXr added a commit to XrXr/rust that referenced this pull request Oct 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2024
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 14, 2024
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.

3 participants