This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] adds `-print-diagnostics`
ClosedPublic

Authored by cjdb on Jun 1 2022, 10:47 AM.

Details

Summary

Prints a list of all the warnings that Clang offers.

Diff Detail

Event Timeline

cjdb created this revision.Jun 1 2022, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 10:47 AM
cjdb requested review of this revision.Jun 1 2022, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 10:47 AM
cjdb added a comment.Jun 1 2022, 10:48 AM

I haven't added a test because I'm not sure how we can reasonably test this without keeping a file that holds all the warnings and using FileCheck. I'm okay with that, but want to make sure it's the best thing to do.

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

cjdb added a comment.Jun 1 2022, 12:46 PM

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

Is diagtool tree something that users widely know about?

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

Is diagtool tree something that users widely know about?

I think no. But any random new option to clang will have the same discoverability problem.

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

Is diagtool tree something that users widely know about?

Doubtful?

I think I may have been vaguely aware of it, but I thought it was a testing utility. Is diagtool part of the default install of Clang?

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

Is diagtool tree something that users widely know about?

I think no. But any random new option to clang will have the same discoverability problem.

Yes, but I think there's a pretty wide discoverability gap between "documented command line option that shows up in Clang" and "random utility that doesn't have any obvious relationship to Clang specifically".

Would a JSON dump help you for your tools?

cjdb added a comment.Jun 1 2022, 3:16 PM

Would a JSON dump help you for your tools?

Do you think a JSON dump would be more appropriate? I was just going for something not unlike --help here.

Would a JSON dump help you for your tools?

Do you think a JSON dump would be more appropriate? I was just going for something not unlike --help here.

I thought that you need this for your diagnostics campaign.

cjdb added a comment.Jun 1 2022, 5:00 PM

Would a JSON dump help you for your tools?

Do you think a JSON dump would be more appropriate? I was just going for something not unlike --help here.

I thought that you need this for your diagnostics campaign.

Ah, gotcha. The idea for this feature came about while @aaron.ballman was reviewing a draft of that RFC, but it's orthogonal to that. I can't immediately see how users would benefit from this outputting JSON, but open to ideas.

Would a JSON dump help you for your tools?

Do you think a JSON dump would be more appropriate? I was just going for something not unlike --help here.

I thought that you need this for your diagnostics campaign.

Ah, gotcha. The idea for this feature came about while @aaron.ballman was reviewing a draft of that RFC, but it's orthogonal to that. I can't immediately see how users would benefit from this outputting JSON, but open to ideas.

+1 -- I think JSON output would be a better feature for diagtool. My goal with proposing the idea @cjdb has graciously worked on is to make warnings more discoverable for users in Clang itself, so I don't think JSON output helps there.

cjdb added a comment.Jun 2 2022, 9:21 AM

FYI diagtool tree dumps the diagnostics. You can even provide -W options and observe the differences.

Is diagtool tree something that users widely know about?

I think no. But any random new option to clang will have the same discoverability problem.

Yes, but I think there's a pretty wide discoverability gap between "documented command line option that shows up in Clang" and "random utility that doesn't have any obvious relationship to Clang specifically".

Agreed. I think a combination of Clang-next's release notes, plus this appearing in --help and web docs will be enough.

On discoverability, I'm thinking --print-all-warning-options might be a better name, since 'diagnostic' is a fairly technical term (and this is also not printing all diagnostics).

If you still decide to add an option to clang, I think "diagnostics" is better than "warnings". There are many diagnostics which are errors by default.

If you still decide to add an option to clang, I think "diagnostics" is better than "warnings". There are many diagnostics which are errors by default.

Good point about warnings which default to errors; those will also be part of the output. I think "diagnostic" is the best term I can think of that's still accurate.

cjdb updated this revision to Diff 433823.EditedJun 2 2022, 11:50 AM
  • renames flag to -print-diagnostic-options
  • adds test
  • adds logic to get as many options to be visible as possible

I'm very unhappy with the last point: getDiagnosticFlags doesn't seem to return *all* diagnostic flags (-W is missing with R0 of this patch), and even now, both -Wgnu-statement-expression-from-macro-expansion and -Wqualified-void-return-type are missing from the list despite being in the webpage.

Doing clang -print-diagnostic-options | wc reveals that there are also some options that I'm seeing as warnings, while the docs consider these remarks:

  1. -Wmodule-build
  2. -Wmodule-import
  3. -Wmodule-lock
  4. -Wpass
  5. -Wpass-analysis
  6. -Wpass-missed
  7. -Wremark-backend-plugin
  8. -Wround-trip-cc1-args
  9. -Wsanitize-address
  10. -Wsearch-path-usage

If Clang promoting these to warning flags is fine, that's okay. Regardless, it reveals a flaw in testing because these ten options aren't being flagged as missing when they probably should be. Is FileCheck capable of checking that all lines correspond to a file, or should I move to using diff?

cjdb added a comment.Jun 2 2022, 11:52 AM

Okay, I understand why -W isn't being printed now, but the missing ones are still a bit baffling.

cjdb updated this revision to Diff 433901.Jun 2 2022, 3:02 PM
  • simplifies code
  • minimises test to only a handful of cases

Thanks for this! Just some fairly minor nits from me. FWIW, the changes should also come with a release note so users know about the new command line option.

clang/include/clang/Driver/Options.td
829

Unintended whitespace change?

clang/lib/Basic/DiagnosticIDs.cpp
656

I presume this change doesn't have any negative effects in Driver::HandleAutocompletions()? (That's currently the only consumer of this API.)

clang/lib/Driver/Driver.cpp
2010

Declare the type as a const reference?

(Btw, when uploading patches, you should give them plenty of patch context -- that's what allows reviewers to suggest changes directly in Phab, but it also gives more information to reviewers so they don't have to leave the review to go to a code editor. FWIW, I usually use -U999 to add context.)

clang/test/Driver/print-diagnostic-options.c
1

"All" warning groups? ;-) Might want to update the comment somewhat.

cjdb updated this revision to Diff 435223.Jun 8 2022, 9:49 AM
cjdb added inline comments.
clang/include/clang/Driver/Options.td
829

My editor trims whitespace. I've added it back, but trimmed lines are nice!

clang/lib/Basic/DiagnosticIDs.cpp
656

All tests pass, so I hope not?

clang/lib/Driver/Driver.cpp
2010

Declare the type as a const reference?

I'm not seeing the advantage of doing this here. Happy to make it const, but the reference doesn't add a benefit and potentially introduces confusion.

(Btw, when uploading patches, you should give them plenty of patch context -- that's what allows reviewers to suggest changes directly in Phab, but it also gives more information to reviewers so they don't have to leave the review to go to a code editor. FWIW, I usually use -U999 to add context.)

Sorry about that. I usually use arc diff to upload patches, but have been having problems with it recently and did a rushed manual upload instead.

aaron.ballman accepted this revision.Jun 8 2022, 10:01 AM

LGTM!

clang/lib/Basic/DiagnosticIDs.cpp
656

Sure, we can go with that. I also spot-checked and I don't think this should have a negative impact there.

clang/lib/Driver/Driver.cpp
2010

I'm not seeing the advantage of doing this here. Happy to make it const, but the reference doesn't add a benefit and potentially introduces confusion.

I still reflexively assume that some older compilers aren't as good at move operations as they should be, and this would be inducing a copy of a pretty large vector. Using a const reference means the result of the call is lifetime extended and there's no chance of a big copy. However, I don't insist as I think we should be able to rely on move operations these days?

Sorry about that. I usually use arc diff to upload patches, but have been having problems with it recently and did a rushed manual upload instead.

Ah, no worries! I just mentioned it in case you weren't aware. :-)

This revision is now accepted and ready to land.Jun 8 2022, 10:01 AM
This revision was landed with ongoing or failed builds.Jun 8 2022, 10:56 AM
This revision was automatically updated to reflect the committed changes.