Skip to content

cc-wrapper: use -fwrapv instead of -fno-strict-overflow in clang#243595

Merged
vcunat merged 1 commit intoNixOS:stagingfrom
tjni:hardening-strict-overflow
Jul 26, 2023
Merged

cc-wrapper: use -fwrapv instead of -fno-strict-overflow in clang#243595
vcunat merged 1 commit intoNixOS:stagingfrom
tjni:hardening-strict-overflow

Conversation

@tjni
Copy link
Copy Markdown
Contributor

@tjni tjni commented Jul 15, 2023

Description of changes

Please consider this a proof of concept that still needs discussion and testing. I'd appreciate opinions on whether this is the right thing to do. If it is, I'd also like thoughts on how to implement it in a simple, elegant way.

Background and Motivation

Reported in #39687, builds on macOS can encounter warnings like :

clang-11: error: argument unused during compilation: '-fno-strict-overflow' [-Werror,-Wunused-command-line-argument]

If we want to suppress this warning (such as in Kitty's build), we can add:

hardeningDisable = lib.optionals stdenv.cc.isClang ["strictoverflow"];

I find these warnings distracting when I debug builds, so I'd like to fix them once and for all (if possible).

Why does Clang emit this warning?

I think it is related to code at https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L6449:

// -fno-strict-overflow implies -fwrapv if it isn't disabled, but
// -fstrict-overflow won't turn off an explicitly enabled -fwrapv.
if (Arg *A = Args.getLastArg(options::OPT_fwrapv, options::OPT_fno_wrapv)) {
  if (A->getOption().matches(options::OPT_fwrapv))
    CmdArgs.push_back("-fwrapv");
} else if (Arg *A = Args.getLastArg(options::OPT_fstrict_overflow,
                                    options::OPT_fno_strict_overflow)) {
  if (A->getOption().matches(options::OPT_fno_strict_overflow))
    CmdArgs.push_back("-fwrapv");
}

That is, -fno-strict-overflow does nothing more than imply -fwrapv. When upstream adds -fwrapv (or even -fno-wrapv) itself, then -fno-strict-overflow does nothing, and I suspect it gets reported as unused.

What does this PR do?

It simply checks if cc-wrapper is wrapping Clang and uses -fwrapv instead of -fno-strict-overflow if so. However, I don't think the way I've done it is so elegant.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@tjni tjni requested a review from Ericson2314 as a code owner July 15, 2023 05:55
@tjni tjni requested a review from a user July 15, 2023 05:55
@tjni
Copy link
Copy Markdown
Contributor Author

tjni commented Jul 15, 2023

This doesn't even work as-is because @isClang@ is not always being substituted properly. I'll leave it as-is pending initial feedback.

@tjni tjni marked this pull request as draft July 15, 2023 06:55
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 16, 2023
@tjni tjni marked this pull request as ready for review July 17, 2023 04:10
@tjni
Copy link
Copy Markdown
Contributor Author

tjni commented Jul 17, 2023

I'm marking this as ready for review now because I think it could be committed. On aarch64-darwin, I built kitty without disabling strictoverflow hardening, and the warning is no longer emitted. I'm still open to feedback on the implementation.

@ghost
Copy link
Copy Markdown

ghost commented Jul 18, 2023

LGTM but I don't use clang much so I'm not really qualified to approve this... it affects quite a lot of builds.

@nixos-discourse
Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2466

@vcunat vcunat merged commit 88dec0c into NixOS:staging Jul 26, 2023
@tjni tjni deleted the hardening-strict-overflow branch July 26, 2023 10:08
@tjni tjni mentioned this pull request Nov 6, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants