Page MenuHomePhabricator

[clang][Opt] Add NoArgUnusedWith to not warn for unused redundant options
Needs ReviewPublic

Authored by abrachet on Mar 13 2022, 3:34 PM.

Details

Summary

NoArgUnusedWith can be used for options which together are redundant or not meaningful. This patch adds an example for -static-libstdc++ and -nostdlib. The former isn't meaningful with the latter. Projects might elect to build with a flag like -static-libstdc++ globally and use nostdlib for specific TU's, in this case they would get unused args warnings.

Diff Detail

Unit TestsFailed

TimeTest
60,090 msx64 debian > LLVM.CodeGen/NVPTX::wmma.py
Script: -- : 'RUN: at line 5'; "/usr/bin/python3.9" /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/NVPTX/wmma.py --ptx=60 --gpu-arch=70 > /var/lib/buildkite-agent/builds/llvm-project/build/test/CodeGen/NVPTX/Output/wmma.py.tmp-ptx60-sm_70.ll

Event Timeline

abrachet created this revision.Mar 13 2022, 3:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
abrachet requested review of this revision.Mar 13 2022, 3:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 13 2022, 3:34 PM
MaskRay added a comment.EditedMar 13 2022, 3:51 PM

This patch adds an example for -static-libcxx and -nostdlib.

We have -static-libstdc++ but not -static-libc++. (D53238 attempts to add -static and name it -static=c++stdlib)

NoArgUnusedWith can be used for options which together are redundant or not meaningful.

I agree that -Wunused-command-line-argument sometimes gets in the way and removing the warnings is sometimes inconvenient.

You can currently avoid the warning with: --start-no-unused-arguments -static-libstdc++ --end-no-unused-arguments.

For NoArgUnusedWith, intuitively I am unsure it is the desired interface. For -static-libstdc++, you probably want to remove the diagnostic with -nostdlib++ and -nodefaultlibs as well. There are just so many similar options.

clang/lib/Driver/Driver.cpp
4448

Unless you want to capture it somewhere.

abrachet updated this revision to Diff 414965.Mar 13 2022, 4:13 PM
abrachet edited the summary of this revision. (Show Details)

Update example to have multiple options in NoArgUnusedWith

abrachet marked an inline comment as done.Mar 13 2022, 4:17 PM

This patch adds an example for -static-libcxx and -nostdlib.

We have -static-libstdc++ but not -static-libc++. (D53238 attempts to add -static and name it -static=c++stdlib)

Thank you, updated the description

NoArgUnusedWith can be used for options which together are redundant or not meaningful.

I agree that -Wunused-command-line-argument sometimes gets in the way and removing the warnings is sometimes inconvenient.

You can currently avoid the warning with: --start-no-unused-arguments -static-libstdc++ --end-no-unused-arguments.

Yeah, we are currently using this in some places, but if you just globally set -static-libstdc++ as --start-no-unused-arguments -static-libstdc++ --end-no-unused-arguments, then you lose out when it should actually warn. Likewise right now we set -Wno-unused-command-line-argument in a few places where this is an issue

For NoArgUnusedWith, intuitively I am unsure it is the desired interface. For -static-libstdc++, you probably want to remove the diagnostic with -nostdlib++ and -nodefaultlibs as well. There are just so many similar options.

Added to the example both of these flags to show NoArgUnusedWith can support multiple flags. There's a lot more redundant flags, and I'll get to some in follow up patches

clang/lib/Driver/Driver.cpp
4448

Indeed, ConsideredUsed is used in the llvm::any_of call

abrachet updated this revision to Diff 417313.Mar 22 2022, 8:54 AM
abrachet marked an inline comment as done.

clang-format

abrachet updated this revision to Diff 419491.Mar 31 2022, 9:04 AM

Rebase so patch applies

abrachet updated this revision to Diff 420375.Apr 4 2022, 9:20 PM

Actually clang-format...

MaskRay added a comment.EditedApr 8 2022, 10:22 AM

If you need a -static-libstdc++ not subject to unused argument warning, --start-no-unused-arguments and D53238 (-static=c++stdlib) may be better choices.

dexonsmith resigned from this revision.Apr 11 2022, 10:43 AM

If you need a -static-libstdc++ not subject to unused argument warning, --start-no-unused-arguments and D53238 (-static=c++stdlib) may be better choices.

That patch might help for -static-libstdc++ in particular and other -static-* flags, however there are other flags that this will help with in the future. What about -nostdlib -noprofilelib. I'm currently only interested with -nostdlib and haven't looked into other flags but I suspect this could help in other instances too. Could be helpful with sanitizers too, but all their flags get read into SanitizerArgs so there are never warnings there.

I don't think --start-no-unused arguments is as good as a solution here. For a large project build, I think --{start,end}-no-unused-arguments is too heavy a hand. You would just end up putting it everywhere and lose any warnings that might be useful. This patch is specifically trying to reduce the need for those.

MaskRay added a comment.EditedApr 13 2022, 3:08 PM

If you need a -static-libstdc++ not subject to unused argument warning, --start-no-unused-arguments and D53238 (-static=c++stdlib) may be better choices.

That patch might help for -static-libstdc++ in particular and other -static-* flags, however there are other flags that this will help with in the future. What about -nostdlib -noprofilelib. I'm currently only interested with -nostdlib and haven't looked into other flags but I suspect this could help in other instances too. Could be helpful with sanitizers too, but all their flags get read into SanitizerArgs so there are never warnings there.

I don't think --start-no-unused arguments is as good as a solution here. For a large project build, I think --{start,end}-no-unused-arguments is too heavy a hand. You would just end up putting it everywhere and lose any warnings that might be useful. This patch is specifically trying to reduce the need for those.

I have run into situation where I find the unused argument warning for -nostdlib is inconvenient (musl clang wrapper), but I can think of users who actually want to the diagnostic (myself included at times).
I am concerned that the decision suitable for your use case may not be desired by other groups.
This is why I'd like keep suggesting --start-no-unused-arguments: it encodes the intention clearly and cannot get in the way of others while achieving your goals.
It's just longer, less ergonomic. If you want to make the option shorter more ergonomic, I am happy to review such changes.

In addition, I am concerned with a can of worms this change will open: for many options which can be debatable one way or the other, there may be contributors trying to toggling them.
I am worried that folks who generally care about driver changes can't keep up with all such patches.

FWIW I interpreted dexonsmith's resign from the patch as his ambivalence or minor objection to the direction, otherwise I'd expect a LGTM.

smeenai resigned from this revision.Apr 28 2022, 1:40 PM
ormris removed a subscriber: ormris.May 16 2022, 11:27 AM