Page MenuHomePhabricator

[clang][cli] Port DiagnosticOpts to new option parsing system
ClosedPublic

Authored by jansvoboda11 on Jul 27 2020, 9:57 AM.

Details

Summary

This patch introduces additional infrastructure necessary to accommodate DiagnosticOptions.

DiagnosticOptions are unique in that they are parsed by the same function in cc1 AND in the Clang driver. The call to the parsing function from the driver occurs early on in the compilation process, where no proper DiagnosticEngine exists, because the diagnostic options (passed through command line) are not known yet.

To preserve the current behavior, we need to be able to selectively parse:

  • all options (for -cc1),
  • only diagnostic options (for driver).

This patch achieves that in the following way:

  • new MacroPrefix field is added to the Option TableGen class,
  • new IsDiag TableGen mixin sets MacroPrefix to "DIAG_",
  • TableGen backend serializes option records into a macro with the prefix,
  • CompilerInvocation parse/generate methods define the [DIAG_]OPTION_WITH_MARSHALLING macros to handle diagnostic options separately.

Depends on D93700, D93701 & D93702.

Diff Detail

Event Timeline

dang created this revision.Jul 27 2020, 9:57 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 27 2020, 9:57 AM
jansvoboda11 commandeered this revision.Dec 22 2020, 1:55 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

Taking over this patch, as Daniel is no longer involved.

jansvoboda11 retitled this revision from Port DiagnosticOpts to new option parsing system to [clang][cli] Port DiagnosticOpts to new option parsing system.Dec 22 2020, 7:01 AM
jansvoboda11 added a reviewer: dexonsmith.

Rebase, undo NFC changes. Replace the previous diagnostic option tagging system implemented in TableGen with less intrusive backend check. Extract prep patches.

I still need to figure out why Clang built in release mode replaces spaces in an error message by newlines and indents:

$ ./build.noindex/debug/bin/clang -c clang/test/Driver/fmessage-length.c -fmessage-length=nan 
clang-12: error: 
      invalid
      argument
      'nan'
      to
      -fmessage-length=

Where the debug built Clang behaves as expected:

$ ./build.noindex/debug/bin/clang -c clang/test/Driver/fmessage-length.c -fmessage-length=nan
clang-12: error: invalid argument 'nan' to -fmessage-length=

Remove unnecessary if braces

dexonsmith added inline comments.Dec 22 2020, 11:22 PM
llvm/utils/TableGen/OptParserEmitter.cpp
102–107

This seems like a bit of a semantic layering violation. It'd be pretty unexpected if someone renamed DiagnosticOpts in clang that they'd have to update this code in llvm. Is there another way to solve this problem?

jansvoboda11 added inline comments.Jan 4 2021, 4:54 AM
llvm/utils/TableGen/OptParserEmitter.cpp
102–107

I don't like it either, but the alternatives I can think of are worse.

We could add a string MacroPrefix; field to LLVM's Option class and populate it in Clang's TableGen file:

  1. Via something like an IsDiag multiclass that we'd need to remember to apply to each diagnostic option. I don't like it as it seems error prone and introduces duplication.
  2. Put all diagnostic options into a single let MacroPrefix = "DIAG_" in { ... } block. This removes the duplication, but doesn't ensure an option is in that block iff it's a diagnostic option with "DiagnosticOpts.*" keypath.
  3. More involved approach would be to duplicate the LLVM's Option and related stuff in Clang. That would get us a place to put the custom KeyPath.startswith("DiagnosticOpts.") logic and then forward to LLVM's Option with the appropriate MacroPrefix.

I'll think some more about it.

dexonsmith added inline comments.Jan 5 2021, 12:10 PM
llvm/utils/TableGen/OptParserEmitter.cpp
102–107

Doing #1 + #2 seems like an okay tradeoff to me (looking back at the old diff, it seems like that's similar to what @dang originally implemented (except adding a more generic MacroPrefix), and it seems fairly clean / obvious to me).

[...] but doesn't ensure an option is in that block iff it's a diagnostic option with "DiagnosticOpts.*" keypath.

The reason I'm okay with this is that I'm having trouble envisioning how this would go wrong practice.

  • If someone adds somethings to DiagnosticOptions, they're likely to grep for how the adjacent field was defined / marshalled, duplicate the line, and modify it. I'm not seeing a likely path for them to copy/paste from a non-diagnostic option and/or miss adding this to the let block.
  • If someone accidentally adds something to the let block that isn't in DiagnosticOptions, they'll get a compiler error in ParseDiagnosticArgs.

If you're still concerned, I wonder if there's a way to add a check in asserts builds that confirms that ParseDiagnosticArgs fills in DiagnosticOptions equivalently to how createFromCommandLine does? (and/or could the latter call the former as an implementation strategy?)

Rebase on top of prep patches

Introduce IsDiag mixin, add test

Use arrow instead of dot in keypath

jansvoboda11 added inline comments.Jan 6 2021, 8:32 AM
llvm/utils/TableGen/OptParserEmitter.cpp
102–107
  • If someone adds somethings to DiagnosticOptions, they're likely to grep for how the adjacent field was defined / marshalled, duplicate the line, and modify it. I'm not seeing a likely path for them to copy/paste from a non-diagnostic option and/or miss adding this to the let block.

I think that's a fair assumption.

  • If someone accidentally adds something to the let block that isn't in DiagnosticOptions, they'll get a compiler error in ParseDiagnosticArgs.

That's right.

I think it's fine to move from the check in OptParserEmitter to a MacroPrefix then.
I chose the IsDiag mixin as opposed to let MacroPrefix = "_DIAG" in { ... }, as it doesn't require moving options around - cleaner diff.

dexonsmith added inline comments.Jan 6 2021, 12:22 PM
clang/include/clang/Driver/Options.td
1191–1194

There was one thing in the original patch that was a bit of sanity check for whether IsDiag was added to the wrong option: the DiagnosticOpts-> part of the keypath was implicit. What do you think of adding that back? That would make the keypath here ShowCarets.

It highlights that diagnostics are special (since DiagnosticOpts is never mentioned in the file, and any mistakes could be found with a grep).

clang/lib/Frontend/CompilerInvocation.cpp
3215

Making DiagnosticOpts-> implicit requires a little more code here:

#define DIAG_OPTION_WITH_MARSHALLING(                                          \
    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
    HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
    IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
    TABLE_INDEX)                                                               \
  GENERATE_OPTION_WITH_MARSHALLING(                                            \
      Args, SA, KIND, FLAGS, SPELLING, ALWAYS_EMIT, DiagnosticOpts->KEYPATH,   \
      DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, EXTRACTOR,    \
      TABLE_INDEX)

But I think it might be worth it for the sanity check.

Move the "DiagnosticOpts->" prefix from TableGen definitions to macros

Don't create diagnostics_fixit_info via BoolXxxOption to keep the distinstion of Group<f_Group> and Group<f_clang_Group>

jansvoboda11 added inline comments.Jan 7 2021, 7:11 AM
clang/include/clang/Driver/Options.td
1191–1194

I agree that now, when we don't use the keypath as the "source of truth", the original solution where DiagnosticOpts-> is implied by something else is cleaner. Thanks for pointing that out!

This revision is now accepted and ready to land.Jan 7 2021, 11:56 AM
This revision was landed with ongoing or failed builds.Jan 8 2021, 1:45 AM
This revision was automatically updated to reflect the committed changes.

I ran into this commit when integrating commits today (specifically, 97100646d1b4526de1eac3aacdb0b098739c6ec9) -- there's nothing wrong with this patch AFAICT, but I'm wondering if the error messaging/handling could be improved somehow.

tl;dr I reduced an example in D94468

We have an internal user creating a ToolInvocation and attaching a DiagnosticConsumer to it. They were also using -ferror-limit=-1, which if I understand now, may be meaningless -- -ferror-limit=0 is used to mean "unlimited error messages", which is probably what they meant.

Anyway, with the DiagnosticConsumer attached, the test crashed with an assertion failure "Assertion 'SourceMgr && "SourceManager not set!" failed.", which really wasn't helpful at all. After much longer than I care to admit, I thought to remove the setDiagnosticConsumer, and was able to find the "invalid integral value '-1' in '-ferror-limit -1'" error in the logs, and I was able to find the real issue pretty quickly after that.

I don't think the usage was out of the ordinary, so I created D94468 as an example if you have time to take a look, to save the next person that may run into this issue.

Thanks for reporting that, @rupprecht. I'll look into the issue in a couple of days.

Sorry for taking so long. I created a patch that should fix the missing SourceManager: D99414

@rupprecht can you confirm this works for your use-case?