This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings
ClosedPublic

Authored by mstorsjo on Jan 2 2022, 2:52 PM.

Details

Summary

When passing a set of flags to configure defaults for a specific
target (similar to the cmake settings CLANG_DEFAULT_RTLIB,
CLANG_DEFAULT_UNWINDLIB, CLANG_DEFAULT_CXX_STDLIB and
CLANG_DEFAULT_LINKER, but without hardcoding it in the binary),
some of the flags may cause warnings (e.g. -stdlib= when compiling C
code). Allow requesting selectively ignoring unused arguments among
some of the arguments on the command line, without needing to resort
to -Qunused-arguments or -Wno-unused-command-line-argument.

Fix up the existing diagnostics.c testcase. It was added in
response to PR12181 to fix handling of
-Werror=unused-command-line-argument, but the command line option
in the test (-fzyzzybalubah) now triggers "error: unknown argument"
instead of the intended warning. Change it into a linker input
(-lfoo) which triggers the intended diagnostic. Extend the
existing test case to check more cases and make sure that it keeps
testing the intended case.

Add testing of the new option to this existing test.

Diff Detail

Unit TestsFailed

Event Timeline

mstorsjo created this revision.Jan 2 2022, 2:52 PM
mstorsjo requested review of this revision.Jan 2 2022, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2022, 2:52 PM

I know some folks who dislike aggressive unused command line option diagnostics but also worry about -Qunused-arguments -Wno-unused-command-line-argument affecting the entire command line. start/end looks good to me.

clang/docs/ClangCommandLineReference.rst
1342

The sentence will be scrubbed.

Use path/to/clang-tblgen -gen-opt-docs -I llvm/include -I clang/include/clang/Driver clang/include/clang/Driver/ClangOptionDocs.td > clang/docs/ClangCommandLineReference.rst to regenerate this rst.

clang/include/clang/Driver/Options.td
3918

Drop NoXarchOption. The option can be used with Darwin -Xarch (test/Driver/Xarch.c) though it is unnecessary to test it.

clang/test/Driver/diagnostics.c
21

Looks like no command has an output. In case there is one, make sure -o /dev/null

MaskRay added a comment.EditedJan 4 2022, 4:59 PM

Add arguments for silencing unused argument warnings for some but not all arguments

A more searchable subject (mentioning the new option in the subject is useful for archaeology) may be Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

A more searchable subject (mentioning the new option in the subject is useful for archaeology) may be Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings

Thanks, that's indeed a better summary (although the subject line ends up even longer than now). Will change it to that form.

clang/docs/ClangCommandLineReference.rst
1342

Ok, will do. There's lots of other changes in the file though, from other options that haven't been added here, but I'm just including the changes for the options I'm adding in this commit.

clang/include/clang/Driver/Options.td
3918

Ok, will do.

clang/test/Driver/diagnostics.c
21

As all commands use -fsyntax-only, I presume there should be no output. So do I read your comment correctly that we should have -o /dev/null specifically _if_ we'd have a test that lack -fsyntax-only, or do you want me to add it just in case? (The preexisting test doesn't have that and just use -fsyntax-only.)

mstorsjo updated this revision to Diff 397476.Jan 5 2022, 1:29 AM

Regenerated ClangCommandLineReference.rst (with the relevant changes), removed NoXarchOption.

mstorsjo retitled this revision from [clang] Add arguments for silencing unused argument warnings for some but not all arguments to [clang] Add --start-no-unused-arguments/--end-no-unused-arguments to silence some unused argument warnings.Jan 5 2022, 1:29 AM
MaskRay accepted this revision.Jan 5 2022, 11:37 AM

I forwarded this to some groups who want this functionality. Jannik2099 from Gentoo likes the change.

But make sure to wait a bit to see what others think.

clang/test/Driver/diagnostics.c
21

-fsyntax-only doesn't need -o /dev/null.

Some downstream users (including Google) may run the test in the source directory and make the source directory in a read-only filesystem, so printing files other than somewhere in %t* will lead to an error.

This revision is now accepted and ready to land.Jan 5 2022, 11:37 AM
MaskRay added inline comments.Jan 5 2022, 11:38 AM
clang/test/Driver/diagnostics.c
34

You may change some x86_64-apple-darwin10 to other triples, probably including clang-cl (to test CoreOption).

The canonical spelling for -target is ---target=. -target (space separated driver option) is just abused so much that it is unrealistic to remove support now.

MaskRay added inline comments.Jan 5 2022, 11:40 AM
clang/test/Driver/diagnostics.c
2

If you are going to update comments, remove this PR12181. It says an issue about -Werror=unused-command-line-argument but your comment below has covered the information.

phosek accepted this revision.Jan 5 2022, 1:05 PM

LGTM this is something we have a use case for this in Fuchsia as well.

mstorsjo updated this revision to Diff 397719.Jan 5 2022, 2:55 PM
mstorsjo marked 2 inline comments as done.

Add testcases for using the new option with clang-cl, use --target= everywhere.

But make sure to wait a bit to see what others think.

Do you think it's ok to go ahead and land this change now, or should I wait a bit more? (I'd like to have it landed with enough margin before the 14.x branch so that it has time to settle before that.)

MaskRay accepted this revision.Jan 10 2022, 4:26 PM

But make sure to wait a bit to see what others think.

Do you think it's ok to go ahead and land this change now, or should I wait a bit more? (I'd like to have it landed with enough margin before the 14.x branch so that it has time to settle before that.)

The wait time has been sufficiently long to me:) Go head.

This revision was landed with ongoing or failed builds.Jan 10 2022, 11:22 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 8:54 AM
Herald added a subscriber: StephenFan. · View Herald Transcript