[clang][cli] Command line round-trip for HeaderSearch options

[jansvoboda11]

Summary

This patch implements generation of remaining header search arguments.
It's done manually in C++ as opposed to TableGen, because we need the flexibility and don't anticipate reuse.

This patch also tests the generation of header search options via a round-trip. This way, the code gets exercised whenever Clang is built and tested in asserts mode. All check-clang tests pass.

[dexonsmith]

It'd be much nicer to add the tests as part of the patch. I've added a comment to D94474 with a possible way to do that.

[jansvoboda11]

Add (improved) round-trip mechanism

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

1851

I plan to move the prefixing of the option name and serialization of the value into a separate function, so we don't have to duplicate the logic here.

3101

I'd like to rename this function to something like MaybeRoundTrip and avoid the round-trip in release builds with #ifndef NDEBUG.

3207

I plan to extract this into ParseHeaderSearchArgsMaybeRoundtrip.

llvm/include/llvm/Option/ArgList.h

141

These will be removed once we're able to generate all command line arguments. (We won't need to check which options to copy from the original command line and which to generate.)

[dexonsmith]

A concern I have is that the round-tripping might be too low level since it requires each group of options to opt-in somehow. Having something at the top-level that doesn't have to be touched again would be easier to validate / understand, and might better serve the purpose of verifying that the lower-level details were implemented correctly by not depending on them to turn it on.

A more minor concern is that I don't think this should always or only active when !NDEBUG. It would be ideal if it could be switched on and off in both release builds and asserts builds via a -cc1 flag, maybe something like -round-trip-args. Then the Clang driver can set that flag by default in asserts builds, with a small #ifndef NDEBUG block that adds the -cc1 argument. Search for -discard-value-names or -disable-free for other examples.

If we round-trip at the top level, then I think it's not too hard to implement a -round-trip-args flag with two different modes:

• A "strict" mode that treats the final parse as canonical. I think this is what we want long-term (in asserts mode).
• A "relaxed" mode that treats the first parse as canonical. I think we turn this on now-ish (in asserts mode), even though many args don't yet have generators.

Both modes check that the generated args match for original parse -> generate -> round-tripped parse -> generate. Only "strict" mode returns the result of the round-tripped parse, so in "relaxed" mode we haven't dropped anything from Res.

Here's a possible implementation that assumes most of CreateFromArgs has been moved to CreateFromArgsImpl or something:

bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                        ArrayRef<const char *> CommandLineArgs,
                                        DiagnosticsEngine &Diags,
                                        const char *Argv0) {
  const OptTable &Opts = getDriverOptTable();
  unsigned MissingArgIndex, MissingArgCount;
  DiagnosticsEngine *ActiveDiags = &Diags;
  CompilerInvocation *ActiveInvocation = &Res;
  auto Parse = [&](InputArgList &Args) {
    return CreateFromArgsImpl(*ActiveInvocation, Opts, Args,
                              *ActiveDiags, Argv0,
                              MissingArgIndex, MissingArgCount);
  };

  // Just parse and return if we're not round-tripping.
  const unsigned IncludedFlagsBitmask = options::CC1Option;
  InputArgList OriginalArgs = Opts.ParseArgs(CommandLineArgs, MissingArgIndex,
                                             MissingArgCount, IncludedFlagsBitmask);

  // The driver sets -round-trip-args by default in asserts builds.
  StringRef RoundTrip = OriginalArgs....;
  bool ShouldRoundTrip = !(RoundTrip.empty() || RoundTrip == "off");
  if (!ShouldRoundTrip)
    return Parse(OriginalArgs);

  // Validate -round-trip-args.
  bool IsRoundTripStrict = RoundTrip == "strict"
  if (!IsRoundTripStrict && RoundTrip != "relaxed")) {
    Diags....();
    return false;
  }

  // For -round-trip-args=strict, the first parse is just a temporary used to
  // generate canonical -cc1 args, and the round-tripped parse is canonical. If
  // any used arg is missing generation code, it'll get dropped on the floor.
  //
  // For -round-trip-args=relaxed, the first parse is canonical. The round-trip
  // still checks that when generated args are parsed they'll generate the same
  // args again. However, it doesn't provide coverage for args that aren't
  // generated correctly.
  Optional<DiagnosticsEngine> TempDiags;
  Optional<CompilerInvocation> TempInvocation;
  if (IsRoundTripStrict) {
    TempDiags.emplace(/*Store diags to replay them*/);
    TempInvocation.emplace();
    ActiveDiags = &*TempDiags;
    ActiveInvocation = &*TempInvocation;
  }

  // Parse the provided command-line. If this fails, it does not indicate a program
  // bug, and we don't have a way to test round-tripping.
  if (!Parse(OriginalArgs)) {
    if (IsRoundTripStrict)
      replayDiagnostics(*TempDiags, Diags);
    return false;
  }

  StringPool<> Strings;
  auto Generate = [&](SmallVectorImpl<const char *> GeneratedArgs) {
    return GenerateCommandLine(*ActiveInvocation, GeneratedArgs, Strings);
  };

  // Generate a new -cc1 command-line from the first parse.
  SmallVector<const char *> GeneratedArgs1;
  if (!Generate(GeneratedArgs1))
    llvm::report_fatal_error("Argument generation failed when round-tripping!");

  // Swap out diagnostics.
  if (IsRoundTripStrict) {
    ActiveDiags = &Diags;
    ActiveInvocation = &Res;
    TempDiags = None;
    TempInvocation = None;
  } else {
    TempInvocation.emplace();
    TempDiags.emplace(/*ignore diags*/);
    ActiveDiags = &*TempDiags;
    ActiveInvocation = &*TempInvocation;
  }

  // Fill up Res and Diags with the second parse.
  InputArgList RoundTrippedArgs =
      Opts.ParseArgs(GeneratedArgs1, MissingArgIndex, MissingArgCount,
                     IncludedFlagsBitmask);
  if (!Parse(RoundTrippedArgs))
    llvm::report_fatal_error("Second parse failed when round-tripping!");

  // Generate the arguments again.
  SmallVector<const char *> GeneratedArgs2;
  if (!Generate(GeneratedArgs2))
    llvm::report_fatal_error("Second argument generation failed when round-tripping!");

  // Check that they match exactly.
  if (GeneratedArgs1 != GeneratedArgs2)
    llvm::report_fatal_error("Generated arguments don't match when round-tripping!");
  return true;
}

You could also add something like -round-trip-args-debug-mode to give more details about what went wrong. This could show differences in the generated args, capture the diagnostics on the second parse, or anything else useful. Or maybe we just want that mode *all* the time, since it'll only be triggered when -round-trip-args is on.

[jansvoboda11]

Thanks for putting your time into the comment, @dexonsmith.

I agree that adding a -round-trip-args flag to enable/disable round-tripping is more sensible than hard-coding it via something like #ifndef NDEBUG.
Providing -round-trip-args-debug-mode to help with debugging would be a great addition too. Thanks for pointing that out.

I would be interested to hear why exactly are you concerned by the current approach being too low level.
From my point of view, having to opt-in into round-tripping a part of CompilerInvocation is a feature and I think it should work relatively well with the current roll-out plan:

1. Write documentation on porting existing options to the new marshalling system.
2. Send a message to the mailing list pointing out that we'll be enabling round-tripping in assert builds by default and what that means. Downstream projects can prepare to port their custom options or disable the round-tripping in assert builds.
3. Some time later, commit this patch. Downstream projects already have enough information how to deal with the change. Because we've enabled round-tripping only for HeaderSearchOptions, adopting it isn't that time-consuming if a downstream project decides to do so.
4. (Any patches that add a command line option affecting HeaderSearchOptions will from now on be forced to include the generating/marshalling code.)
5. Slowly commit following patches, each enabling round-tripping for another chunk of CompilerInvocation.
6. After round-tripping has been enabled for all of CompilerInvocation, simplify the code (remove ParseHS, GenerateHS, ResetHS, etc.) and round-trip everything (when told to do so with -round-trip-args).

I'd be interested in hearing your thoughts on that ^.

To help me understand the reasoning behind your proposal, can you answer these questions?

1. What is the benefit or "relaxed" round-tripping compared to the selective strict round-tripping in this patch?
2. What does GeneratedArgs1 != GeneratedArgs2 prove? If we forget to generate some option, it will be in neither GeneratedArgs1 nor GeneratedArgs2. We'll catch that only in "strict" mode anyways. I guess this can help us discover other kinds of lossy generation, but I think the guarantees of "strict" mode are nicer.

[dexonsmith]

In D94472#2507361, @jansvoboda11 wrote:

Thanks for putting your time into the comment, @dexonsmith.

I agree that adding a -round-trip-args flag to enable/disable round-tripping is more sensible than hard-coding it via something like #ifndef NDEBUG.
Providing -round-trip-args-debug-mode to help with debugging would be a great addition too. Thanks for pointing that out.

I would be interested to hear why exactly are you concerned by the current approach being too low level.
From my point of view, having to opt-in into round-tripping a part of CompilerInvocation is a feature and I think it should work relatively well with the current roll-out plan:

1. Write documentation on porting existing options to the new marshalling system.
2. Send a message to the mailing list pointing out that we'll be enabling round-tripping in assert builds by default and what that means. Downstream projects can prepare to port their custom options or disable the round-tripping in assert builds.
3. Some time later, commit this patch. Downstream projects already have enough information how to deal with the change. Because we've enabled round-tripping only for HeaderSearchOptions, adopting it isn't that time-consuming if a downstream project decides to do so.
4. (Any patches that add a command line option affecting HeaderSearchOptions will from now on be forced to include the generating/marshalling code.)
5. Slowly commit following patches, each enabling round-tripping for another chunk of CompilerInvocation.
6. After round-tripping has been enabled for all of CompilerInvocation, simplify the code (remove ParseHS, GenerateHS, ResetHS, etc.) and round-trip everything (when told to do so with -round-trip-args).

I'd be interested in hearing your thoughts on that ^.

I have three concerns with the approach:

1. Churn. This adds a lot of code that will eventually be removed / refactored away. One example is that shifting the NDEBUG logic to the driver requires threading a Boolean (or tristate/enum) for round-trip mode through a few layers; once we're finished, we'll end up stripping that argument back out. A second example is that this seems to require splitting up OPTIONS_WITH_MARSHALLING for each option. Once this work is finished, someone will need to clean them up / unify them, or maybe they'll unnecessarily stay split / duplicated.
2. Boilerplate. Somewhat related to churn; there's a fair bit of additional boilerplate required in this approach. This makes it harder to read / understand / modify the code.
3. Correctness. I'm not sure ResetHS is quite right. It'll probably work for a normal command-line -cc1 invocation, but perhaps not for all tooling applications, since it's changing the pointer identity of HeaderSearchOptions.

On the other hand, one thing I really like about your approach, which I don't see a way of getting with a top-level check, is the ability to turn on "strict" mode for subsets of the command-line, which helps to hold the line in the face of new options being added (I think this is the feature you're (rightly) calling out). I'm not sure how important it is in practice, as long as we still think getting them all is going to happen in a reasonable time frame. There aren't that many new options being added: filtering out [cli] (your patches), [flang], [[D]driver], reverts, and relandings, there are 39 commits to Options.td in the last three months (3 / week). Some of those are deleting options or changing values, and not important to catch. Even for new options, I imagine most people copy/paste nearby options. I think it's critical to hold the line once we have them all, but until then I'm not sure.

Both approaches seem possible for downstreams to adapt / adopt in their own time, although reverting a one line patch switching from "relaxed" to "strict" would be easier than maintaining all the Parse/Generate/Reset infrastructure after upstream deletes it.

(I'm mostly concerned about #1 and #2; I assume #3 can be fixed somehow, although I haven't thought about how.)

To help me understand the reasoning behind your proposal, can you answer these questions?

2. What does GeneratedArgs1 != GeneratedArgs2 prove? If we forget to generate some option, it will be in neither GeneratedArgs1 nor GeneratedArgs2. We'll catch that only in "strict" mode anyways. I guess this can help us discover other kinds of lossy generation, but I think the guarantees of "strict" mode are nicer.

Firstly, GeneratedArgs1 == GeneratedArgs2 proves that generate is an exact inversion of parse, for the subset of args that are handled by the generator (eventually, this will be all args). IOW, it's adding coverage (in one direction) that parse+generate "match" for every option that has generation code. Because of the automation provided by the marshalling infrastructure, I'd expect this to mostly catch bugs in hand-written code (custom marshalling code, manual generation code, or end-of-parse cleanup / follow-up code). Secondly, this adds coverage that command-line generation is deterministic.

strict mode additionally uses the GeneratedArgs1 to fill CompilerInvocation, indirectly checking both directions by requiring tests to pass both with and without this round-trip. However, during development people generally only run the tests one way and the failure mode won't be ideal. If a new option has been left out entirely, it'll usually be clear enough what went wrong and how to fix it; most people build with asserts in development, and the option won't work until they add generation code. But failures from a mismatched parse and generate might not get caught until you compare an Asserts build to a NoAsserts build, which may not happen until the bots run. This kind of failure is also difficult to debug / understand, compared to the assertion on once- and twice-generated args. IOW, even though GeneratedArgs1 == GeneratedArgs2 doesn't catch all issues, it's probably an easier failure to respond to when it does fail.

1. What is the benefit or "relaxed" round-tripping compared to the selective strict round-tripping in this patch?

Mainly, less churn, less boilerplate, and no correctness problem to solve for Reset. I assume you can adapt what you have to also check GeneratedArgs1 == GeneratedArgs2 (it'd be nice to have this at the top level, so you get this check even for options where selective strict mode hasn't yet been enabled), but if that's harder than it sounds then lack of that would miss some coverage and make debugging harder, as described above.

clang/lib/Frontend/CompilerInvocation.cpp

3202–3205

I'm not sure this works correctly. Callers may depend on the identity of the HeaderSearchOptions. It's a shared pointer that could have other references. This will only update / reset one reference.

[jansvoboda11]

Create function for argument generation

[jansvoboda11]

Move boilerplate from CreateFromArgs to separate function

[jansvoboda11]

Fix formatting

[jansvoboda11]

Add round-tripping arguments

[jansvoboda11]

Preserve pointer identity when round-tripping

[jansvoboda11]

Undo changes to string allocation

[jansvoboda11]

In D94472#2508018, @dexonsmith wrote:

I have three concerns with the approach:

1. Churn. This adds a lot of code that will eventually be removed / refactored away. One example is that shifting the NDEBUG logic to the driver requires threading a Boolean (or tristate/enum) for round-trip mode through a few layers; once we're finished, we'll end up stripping that argument back out. A second example is that this seems to require splitting up OPTIONS_WITH_MARSHALLING for each option. Once this work is finished, someone will need to clean them up / unify them, or maybe they'll unnecessarily stay split / duplicated.

I think the driver command line options could be useful even after we're finished, so I'd consider not removing them. For example when working on new Clang feature that requires new command line option, one could implement the parsing first, temporarily disable round-tripping, implement the feature and worry about generation and round-tripping later. I can imagine a situation when downstream projects would prefer to prevent round-tripping from the driver rather than from within CompilerInvocation.

Yes, this requires splitting OPTIONS_WITH_MARSHALLING. I don't think it's a big deal, because we already do that for DiagnosticOpts, LangOpts and CodegenOpts anyway (D84673, D94682). I think it makes more sense to have one parsing function per *Opts, rather than keeping it split up into Parse*Opts and ParseSimpleOpts.

2. Boilerplate. Somewhat related to churn; there's a fair bit of additional boilerplate required in this approach. This makes it harder to read / understand / modify the code.

Boilerplate was added mainly to ArgList, but we can delete that without even showing up in git blame. Another instance is the lambda setup in ParseHeaderSearchArgs, but I think that's pretty isolated as well. I agree the code may be confusing for people not familiar with this effort, but it will be me deleting the code few weeks from now, so I'm not sure how bad this is. Do you worry this will cause confusion during merge conflicts down stream?

3. Correctness. I'm not sure ResetHS is quite right. It'll probably work for a normal command-line -cc1 invocation, but perhaps not for all tooling applications, since it's changing the pointer identity of HeaderSearchOptions.

Nice catch! Some clients make changes to AnalyzerOpts before calling CreateFromArgs, and we would discard those changes. I've updated this patch with a solution: we save the original part of CompilerInvocation, run the first parse on a clean instance, generate arguments and use the original part for the second parse.

On the other hand, one thing I really like about your approach, which I don't see a way of getting with a top-level check, is the ability to turn on "strict" mode for subsets of the command-line, which helps to hold the line in the face of new options being added (I think this is the feature you're (rightly) calling out). I'm not sure how important it is in practice, as long as we still think getting them all is going to happen in a reasonable time frame. There aren't that many new options being added: filtering out [cli] (your patches), [flang], [[D]driver], reverts, and relandings, there are 39 commits to Options.td in the last three months (3 / week). Some of those are deleting options or changing values, and not important to catch. Even for new options, I imagine most people copy/paste nearby options. I think it's critical to hold the line once we have them all, but until then I'm not sure.

That's indeed not too many changes in the area we're working in. Though the biggest reason I'm inclined to stick with this solution is that we can verify the manual generation as soon as we finish one Generate*Args. When committing each Generate*Args separately, this ensures the code really works.

If we go only with the "relaxed" checks, we'd be committing code that might not work as expected and we'd find out only when enabling strict mode for the whole -cc1 interface. Finding a bug in that situation might be harder than with the incremental approach.

strict mode additionally uses the GeneratedArgs1 to fill CompilerInvocation, indirectly checking both directions by requiring tests to pass both with and without this round-trip. However, during development people generally only run the tests one way and the failure mode won't be ideal.

So people build without assertions during development? In that case, I agree that erroring out on GeneratedArgs1 != GeneratedArgs2 (in all kinds of builds) would improve the experience. I don't think there's anything preventing us to incorporate this into the current patch.

[dexonsmith]

Thanks for talking through this with me! The direction sounds good.

In D94472#2519838, @jansvoboda11 wrote:

I think the driver command line options could be useful even after we're finished

You mean the -cc1 options used by the driver right? We shouldn't be adding options to the driver. I agree they should be kept.

Yes, this requires splitting OPTIONS_WITH_MARSHALLING. I don't think it's a big deal, because we already do that for DiagnosticOpts, LangOpts and CodegenOpts anyway (D84673, D94682). I think it makes more sense to have one parsing function per *Opts, rather than keeping it split up into Parse*Opts and ParseSimpleOpts.

Okay, that's a fair point; it probably really is cleaner to have one parse / generate function per option class. In that case, I wonder if we can remove the prefix from the key paths the same way as for DiagnosticOpts...

2. Boilerplate. Somewhat related to churn; there's a fair bit of additional boilerplate required in this approach. This makes it harder to read / understand / modify the code.

Boilerplate was added mainly to ArgList, but we can delete that without even showing up in git blame. Another instance is the lambda setup in ParseHeaderSearchArgs, but I think that's pretty isolated as well. I agree the code may be confusing for people not familiar with this effort, but it will be me deleting the code few weeks from now, so I'm not sure how bad this is. Do you worry this will cause confusion during merge conflicts down stream?

Yes, it's merge conflicts that I think would be most confusing. I think you've convinced me that splitting up the parsers is independently valuable, so I'm less concerned about the churn now.

So people build without assertions during development? In that case, I agree that erroring out on GeneratedArgs1 != GeneratedArgs2 (in all kinds of builds) would improve the experience. I don't think there's anything preventing us to incorporate this into the current patch.

Sounds great. This also checks that generation is deterministic.

[Bigcheese]

In D94472#2519838, @jansvoboda11 wrote:

In D94472#2508018, @dexonsmith wrote:

strict mode additionally uses the GeneratedArgs1 to fill CompilerInvocation, indirectly checking both directions by requiring tests to pass both with and without this round-trip. However, during development people generally only run the tests one way and the failure mode won't be ideal.

So people build without assertions during development? In that case, I agree that erroring out on GeneratedArgs1 != GeneratedArgs2 (in all kinds of builds) would improve the experience. I don't think there's anything preventing us to incorporate this into the current patch.

The only issue I have with this if always parsing twice has a noticeable performance impact for any users of Clang. Can we measure the impact? If it's small (< 100ms ?) then that's fine. I'm also concerned about users like cling and clangd.

[dexonsmith]

In D94472#2523053, @Bigcheese wrote:

In D94472#2519838, @jansvoboda11 wrote:

In D94472#2508018, @dexonsmith wrote:

strict mode additionally uses the GeneratedArgs1 to fill CompilerInvocation, indirectly checking both directions by requiring tests to pass both with and without this round-trip. However, during development people generally only run the tests one way and the failure mode won't be ideal.

So people build without assertions during development? In that case, I agree that erroring out on GeneratedArgs1 != GeneratedArgs2 (in all kinds of builds) would improve the experience. I don't think there's anything preventing us to incorporate this into the current patch.

The only issue I have with this if always parsing twice has a noticeable performance impact for any users of Clang. Can we measure the impact? If it's small (< 100ms ?) then that's fine. I'm also concerned about users like cling and clangd.

I think the idea is only to parse twice when -round-trip-args is on, which the driver will only set by default in asserts mode. Good to measure the impact nevertheless.

[jansvoboda11]

Compare once and twice generated arguments, fix re-ordering of UserEntries

[jansvoboda11]

In D94472#2523053, @Bigcheese wrote:

The only issue I have with this if always parsing twice has a noticeable performance impact for any users of Clang. Can we measure the impact? If it's small (< 100ms ?) then that's fine. I'm also concerned about users like cling and clangd.

I've created a micro-benchmark in D95516. I think we should be fine.

In D94472#2523490, @dexonsmith wrote:

I think the idea is only to parse twice when -round-trip-args is on, which the driver will only set by default in asserts mode. Good to measure the impact nevertheless.

I must've misunderstood you earlier. I've updated the patch so that we round-trip only when cc1 is given -round-trip-args (which happens automatically only in assert builds).
We check that the once- and twice- generated arguments match, and we make sure the CompilerInvocation resulting from the second parse gets used in the rest of compilation.

clang/lib/Frontend/CompilerInvocation.cpp

605

There must be a better way to do this...

[dexonsmith]

clang/include/clang/Driver/Options.td

593–594 ↗

(On Diff #319526)

Can we make this =true vs. =false, or add a -no-round-trip-args? That will allow:

% clang some args -Xclang -no-round-trip-args

to disable it in asserts builds; much better than:

% clang some args -###
# Search for -round-trip-args
% clang -cc1 copy-paste-args-before copy-past-args-after

clang/lib/Frontend/CompilerInvocation.cpp

532

Hmm, I thought we'd found a way to avoid allocating the prefixed name, by adding it to the options table. This unfortunate, but I guess you can fix it another time.

541

Does this allocated string get used, or does "=some-value" get added on?

[jansvoboda11]

Ensure Diags get used only once, add disabling flag, clean up

[jansvoboda11]

clang/include/clang/Driver/Options.td

593–594 ↗

(On Diff #319526)

Done.

clang/lib/Frontend/CompilerInvocation.cpp

532

We can avoid the allocation in the generating macro. I'll come back to this later and figure out a way to do the same here.

541

Both. It depends on the kind/class (Joined/Separate/...) of the option.

3202–3205

Resolved by SwapOpts.

[dexonsmith]

This is getting close! A few more comments inline.

clang/lib/Frontend/CompilerInvocation.cpp

545–550

I suggest inlining Equal and defining the lambda directly in the call to std::equal; I think that'll be easier to read than having this up here. (Nice to have it split out fo rcompile-time if RoundTrip is templated, but I have a suggestion about that below.)

552–557

• I think this would be more clear as a local lambda too.
• I don't think this is the best destination, since llvm::dbgs() goes nowhere when NDEBUG. Should this go in errs()? Or should -round-trip-args-debug= take a filename for where to write extra stuff?
• The arguments won't be correctly formatted for copy-paste if there are any characters that should be escaped. Is the logic from -### reusable?

565–569

Instead of templating this, can we take llvm::function_refs to type-erase the functions?

594

I suggest double-checking that the second parse also fails, and reporting that as an error somehow.

615–617

Does this need to be checked for success / error conditions in any way?

624

Might be better to error here if the second parse fails. We'd want to debug the generated args too.

645–646

Another option vs. report_fatal_error would be to create (fatal) clang error diagnostics. That applies here, and to my previous comments. WDYT? (@Bigcheese, any thoughts?)

Also, I think -round-trip-args-debug might not be super discoverable. Maybe we can add a comment in the code where the errors are reported suggesting adding that.

[jansvoboda11]

Improve error handling

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

545–550

That's fair.

552–557

Good point, resolved.

565–569

Yes, that would be better. I had this templated while I was figuring out what their signature should even be. I've extracted typedefs to keep the signature of RoundTrip readable.

594

I think this should never happen, but being defensive here is a good idea.

615–617

Other callers usually check for missing argument values or unknown arguments. If we did that, we'd need to distinguish between bad arguments we've copied from OriginalArgs and errors we created in Generate. We don't have access to MissingArgIndex and MissingArgsCount of OriginalArgs, so it's not that easy. We could serialize and parse OriginalArgs again, but I think that's not a good idea from performance perspective.

I suggest we omit the checks for now, but put them in place as soon as we round-trip all arguments.

624

That's right.

645–646

If we decide to go with custom diagnostics instead of just using llvm::errs and llvm::report_fatal_error, we'd need to instantiate a custom DiagnosticsEngine here in RoundTrip, because we cannot rely on clients passing us reasonable Diags.

One such example is CompilerInvocationTest, where we don't care what diagnostics get emitted (by using TextDiagnosticBuffer consumer). The errors we're emitting here would be still useful to see when testing though.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

645–646

I'm not entirely following why this would need a custom Diags. If the client wants to ignore diagnostics that should be up to the client, especially in clang-as-a-library situations. In fact, using errs or dbgs at all is pretty iffy when clang is used as a library.

For CompilerInvocationTest, I imagine we could change the consumer / test fixture / etc. somehow to capture this case.

[jansvoboda11]

Create proper diagnostics, add remark tests

clang/lib/Frontend/CompilerInvocation.cpp

645–646

My understanding is that we'd be converting llvm::report_fatal_error into error diagnostics and PrintArgs into notes and remarks.

The hiccup here is that Diags most probably hasn't been configured yet (we're still just parsing the DiagnosticsOptions). And by default, DiagnosticsEngine ignores all remarks.

So my thinking was that to ensure our remarks are not thrown away, we could modify the existing Diags to our liking (too invasive) or set up a custom DiagnosticsEngine instance here in RoundTrip. In the end, the best solution is probably to do this of setup in cc1_main, so that we're emitting diagnostics only when used within compiler.

[jansvoboda11]

Fix formatting and comment wording

[jansvoboda11]

Don't round-trip when the driver says so. Round-trip all the time when built to do so.

[jansvoboda11]

I've changed this patch so that the errors and debugging output goes to DiagnosticsEngine.

Also, I switched back to the original solution, where we always round-trip when Clang is built certain way (this time not when CMake was configured with -DLLVM_ENABLE_ASSERTIONS=ON, but with -DCLANG_ROUND_TRIP_CC1_ARGS=ON). The problem with the previous solution (passing -round-trip-args from the driver to -cc1) is that a lot of tests invoke -cc1 directly, not through the driver. We want to test round-tripping on such tests as well, but shouldn't pollute them with -round-trip-args or force them to go through the driver. I've considered enabling round-tripping through an environment variable, but the problem is that the, the environment variable is not propagated when running lit tests.

[dexonsmith]

In D94472#2539685, @jansvoboda11 wrote:

I've changed this patch so that the errors and debugging output goes to DiagnosticsEngine.

Also, I switched back to the original solution, where we always round-trip when Clang is built certain way (this time not when CMake was configured with -DLLVM_ENABLE_ASSERTIONS=ON, but with -DCLANG_ROUND_TRIP_CC1_ARGS=ON). The problem with the previous solution (passing -round-trip-args from the driver to -cc1) is that a lot of tests invoke -cc1 directly, not through the driver. We want to test round-tripping on such tests as well, but shouldn't pollute them with -round-trip-args or force them to go through the driver. I've considered enabling round-tripping through an environment variable, but the problem is that the, the environment variable is not propagated when running lit tests.

LGTM; I think this is fine as an incremental step, as long as the goal is to get this on in asserts builds once it's complete (we don't want another bot configuration with this...). Please add -cc1 flags to override at least by the time you switch back to doing this for asserts builds. I've noted inline how you can do that (even though the clang driver idea doesn't work for this case).

clang/lib/Frontend/CompilerInvocation.cpp

567–569

It'd be good to the flags still available in -cc1 as overrides.

bool RoundTripDefault = false;

// FIXME: Switch to '#ifndef NDEBUG' once it's ready.
#ifdef CLANG_ROUND_TRIP_CC1_ARGS
RoundTripDefault = true;
#endif

bool DoRoundTrip = Args.hasFlag(OPT_round_trip_args, OPT_no_round_trip_args,
                                RoundTripDefault);

But that's most compelling once this is in NDEBUG. If you'd rather leave that until then I'm fine with that.

[saudi]

clang/lib/Frontend/CompilerInvocation.cpp

652

Hello,

I encountered crashes on Windows targets, related to this line, when rebasing https://reviews.llvm.org/D80833, where CodeGenOpts::CommandLineArgs is read later in the compilation process.

When the function exits, GeneratedArgs1 is destroyed but Res.getCodeGenOpts().CommandLineArgs still references its contents. The code has changed since this patch was submitted, but the logic remains the same in more recent code.

Note that the bug doesn't happen when round-trip is skipped, as CommandLineArgs then refers to ArgV which content is valid for the entire compiler run.

As a solution I considered allowing CodeGenOptions to optionally own the memory by introducing BumpPtrAllocator CodeGenOptions::CommandLineArgsAlloc.
However it has drawbacks:

• need to customize CodeGenOptions copy constructor/operator,
• lifetime for copies of CodeGenOptions::CommandLineArgs (e.g. to other structures) would be bound to the lifetime of the original CodeGenOptions object.

Another solution (slower) would be to use 2 dummy CompilerInvocation to perform the round-trip test, and use the regular arguments on the main instance. It would change the behavior since the main instance currently uses GeneratedArgs1.

WDYT?

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

652

Hello,

I encountered crashes on Windows targets, related to this line, when rebasing https://reviews.llvm.org/D80833, where CodeGenOpts::CommandLineArgs is read later in the compilation process.

When the function exits, GeneratedArgs1 is destroyed but Res.getCodeGenOpts().CommandLineArgs still references its contents. The code has changed since this patch was submitted, but the logic remains the same in more recent code.

Note that the bug doesn't happen when round-trip is skipped, as CommandLineArgs then refers to ArgV which content is valid for the entire compiler run.

Hi, you're right, I can see the lifetime issues here.

As a solution I considered allowing CodeGenOptions to optionally own the memory by introducing BumpPtrAllocator CodeGenOptions::CommandLineArgsAlloc.
However it has drawbacks:

• need to customize CodeGenOptions copy constructor/operator,
• lifetime for copies of CodeGenOptions::CommandLineArgs (e.g. to other structures) would be bound to the lifetime of the original CodeGenOptions object.

Instead of introducing BumpPtrAllocator in CodeGenOptions and dealing with custom copy constructor/assignment, I think it should be possible to use an owning type (e.g. std::vector<std::string>>), WDYT? That's the approach we generally take in CompilerInvocation members.

Another solution (slower) would be to use 2 dummy CompilerInvocation to perform the round-trip test, and use the regular arguments on the main instance. It would change the behavior since the main instance currently uses GeneratedArgs1.

Referring to ArgV instead of GeneratedArgs1 should be fine too, since they are guaranteed to be semantically equivalent anyways.

WDYT?

I think both approaches are valid, but I think the cleanest solution would be to drop CodeGenOptions::CommandLineArgs entirely. It's one of the few cases where CompilerInvocation points to memory owned by someone else. (I thought this only happens in PreprocessorOptions::RemappedFileBuffers, but I stand corrected.) Whenever clients need the original command-line, they can call CompilerInvocation::generateCC1CommandLine and get semantically equivalent list of cc1 arguments. Would that work for you use-case?

[saudi]

clang/lib/Frontend/CompilerInvocation.cpp

652

Hello, thank you for your fast feedback!

Instead of introducing BumpPtrAllocator in CodeGenOptions and dealing with custom copy constructor/assignment, I think it should be possible to use an owning type (e.g. std::vector<std::string>>), WDYT? That's the approach we generally take in CompilerInvocation members.

Actually, CodeGenOptions::CommandLineArgs is an ArrayRef<const char *>, so we need to store an array (e.g. std::vector<const char *>) which entries point inside the owning container. Maintaining this relationship will probably require custom copy constructor/assigment work anyway.

Another solution could be to store the generated command line arguments (BumpPtrAllocator + SmallVector<const char *>) into the CompilerInvocation object. Especially, CompilerInvocationRefBase already has custom copy mechanism.

Note on BumpPtrAllocator vs owner vector: containers like std::string may be optimized when the content is small, embedding the value where the allocated pointer would be. That could break the string::c_str() results when std::vector grows. SmallString, SmallVector etc use the same pattern.

Whenever clients need the original command-line, they can call CompilerInvocation::generateCC1CommandLine and get semantically equivalent list of cc1 arguments. Would that work for you use-case?

Unfortunately, the patch I mentionned accesses CommandLineArguments at CodeGen time (in llvm code, not clang), so the CompilerInvocation is not available. Currently, this is forwarded to CodeGen by storing a reference to CommandLineArguments in MCTargetOptions.

Referring to ArgV instead of GeneratedArgs1 should be fine too, since they are guaranteed to be semantically equivalent anyways.

In the code, I see that the round-trip had two effects:

• Verify that the parse+generate is symetrical and complete (not losing arguments on the way), emitting an error if not
• Use the arguments computed during the round-trip during the rest of compilation

Using ArgV instead of GeneratedArgs1 for the main CompilerInvocation would simplify the round-trip code a bit, however it would remove the latter effect. Is it acceptable?

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

652

Instead of introducing BumpPtrAllocator in CodeGenOptions and dealing with custom copy constructor/assignment, I think it should be possible to use an owning type (e.g. std::vector<std::string>>), WDYT? That's the approach we generally take in CompilerInvocation members.

Actually, CodeGenOptions::CommandLineArgs is an ArrayRef<const char *>, so we need to store an array (e.g. std::vector<const char *>) which entries point inside the owning container. Maintaining this relationship will probably require custom copy constructor/assigment work anyway.

Another solution could be to store the generated command line arguments (BumpPtrAllocator + SmallVector<const char *>) into the CompilerInvocation object. Especially, CompilerInvocationRefBase already has custom copy mechanism.

Note on BumpPtrAllocator vs owner vector: containers like std::string may be optimized when the content is small, embedding the value where the allocated pointer would be. That could break the string::c_str() results when std::vector grows. SmallString, SmallVector etc use the same pattern.

Whenever clients need the original command-line, they can call CompilerInvocation::generateCC1CommandLine and get semantically equivalent list of cc1 arguments. Would that work for you use-case?

Unfortunately, the patch I mentionned accesses CommandLineArguments at CodeGen time (in llvm code, not clang), so the CompilerInvocation is not available. Currently, this is forwarded to CodeGen by storing a reference to CommandLineArguments in MCTargetOptions.

Let me clarify. AFAIK the only read from clang::CodeGenOptions::CommandLineArgs appears in initTargetOptions, which is Clang code that initializes llvm::{MC,}TargetOptions. My thinking is that this code could access CompilerInvocation, call its generateCC1CommandLine and put the generated arguments into an owning std::vector<std::string> inside llvm::MCTargetOptions. The CompilerInvocation data structure could then drop CodeGenOptions::CommandLineArgs since it's redundant anyways (it can be re-generated). This way, we can avoid complicating the lifetimes and clean up CompilerInvocation. If we really want, we can optimize the std::strings away by using BumpPtrAllocator - as you mentioned - inside llvm::{MC,}TargetOptions. Do you think that makes sense?

Referring to ArgV instead of GeneratedArgs1 should be fine too, since they are guaranteed to be semantically equivalent anyways.

In the code, I see that the round-trip had two effects:

• Verify that the parse+generate is symetrical and complete (not losing arguments on the way), emitting an error if not
• Use the arguments computed during the round-trip during the rest of compilation

Using ArgV instead of GeneratedArgs1 for the main CompilerInvocation would simplify the round-trip code a bit, however it would remove the latter effect. Is it acceptable?

From Clang's point of view, I don't think using ArgV instead of GeneratedArgs1 would remove the effect of using the round-tripped arguments during the rest of the compilation. The semantics of the generated arguments are captured by the whole CompilerInvocation and clang::CodeGenOptions::CommandLineArgs don't get used anywhere else in Clang. The question is what LLVM does with these arguments. If it only shows them to the user to provide extra information, I think that's fine too.

[saudi]

clang/lib/Frontend/CompilerInvocation.cpp

652

Let me clarify. AFAIK the only read from clang::CodeGenOptions::CommandLineArgs appears in initTargetOptions, which is Clang code that initializes llvm::{MC,}TargetOptions. My thinking is that this code could access CompilerInvocation, call its generateCC1CommandLine and put the generated arguments into an owning std::vector<std::string> inside llvm::MCTargetOptions.

After looking a bit, initTargetOptions doesn't seem to have access to the CompilerInvocation, it is triggered by the clang backend, where only the {HeaderSearch|CodeGen|Target|Lang}Options structures are passed; it would require passing the CompilerInvocation through several layers.

From Clang's point of view, I don't think using ArgV instead of GeneratedArgs1 would remove the effect of using the round-tripped arguments during the rest of the compilation. The semantics of the generated arguments are captured by the whole CompilerInvocation and clang::CodeGenOptions::CommandLineArgs don't get used anywhere else in Clang. The question is what LLVM does with these arguments. If it only shows them to the user to provide extra information, I think that's fine too.

I realized that using ArgV instead of GeneratedArgs1 may miss detecting if Generate misses some arguments. In that case, no error would be reported by the round-trip test, as GeneratedArgs1 and GeneratedArgs2 would be identical.

I'm currently trying to add a BumpPtrAllocator along with a SmallVector< const char *> in CompilerInvocationRefBase. I think it makes sense for CompilerInvocation to be the owner of the custom command line arguments generated during the round-trip.
Let me know if there would be a cleaner solution: maybe adding CompilerInvocation access in the backend would be simple enough?

[dexonsmith]

Looking at CommandLineArgs, and its eventual forward to MCTargetInfo, the existing design looks like a reference leak, which would also block freeing Clang memory when LLVM is still running. Given that 89ea0b05207d45c145fb525df554b3b986ae379b has no in-tree users, I'd suggest reverting it and creating a different mechanism to pass the command-line through to https://reviews.llvm.org/D80833 that doesn't make MCTargetInfo depend on the lifetime of CompilerInvocation. The current mechanism doesn't look like it'll work correctly for clang -save-temps either.

E.g., Clang could create a global metadata node with the flattened command-line args on the targets / for the users that want this: if and only if requested, CompilerInvocation creates a std::string FlattenedCommandLine that it stores in CodeGenOptions, clang/CodeGen creates a !llvm.commandline metadata node, and llvm/PDB reads the metadata node and creates the entry. (Note that I don't think that should be done by default (certainly not for all targets); I'm a bit skeptical that https://reviews.llvm.org/D80833 is a good idea; see my comment there.)

[dexonsmith]

In D94472#3052604, @dexonsmith wrote:

Looking at CommandLineArgs, and its eventual forward to MCTargetInfo, the existing design looks like a reference leak, which would also block freeing Clang memory when LLVM is still running. Given that 89ea0b05207d45c145fb525df554b3b986ae379b has no in-tree users, I'd suggest reverting it and creating a different mechanism to pass the command-line through to https://reviews.llvm.org/D80833 that doesn't make MCTargetInfo depend on the lifetime of CompilerInvocation. The current mechanism doesn't look like it'll work correctly for clang -save-temps either.

E.g., Clang could create a global metadata node with the flattened command-line args on the targets / for the users that want this: if and only if requested, CompilerInvocation creates a std::string FlattenedCommandLine that it stores in CodeGenOptions, clang/CodeGen creates a !llvm.commandline metadata node, and llvm/PDB reads the metadata node and creates the entry. (Note that I don't think that should be done by default (certainly not for all targets); I'm a bit skeptical that https://reviews.llvm.org/D80833 is a good idea; see my comment there.)

@aprantl helped me dig up how this feature is implemented for DWARF on Darwin. Note that it's off-by-default because it's horrible for reproducible builds.

• Driver argument -grecord-command-line causes the driver to send its args to -cc1 in a flattened string via -dwarf-debug-flags.
• That winds up in the std::string in CodeGenOptions::DwarfDebugFlags.
• Environment variable RC_DEBUG_INFO triggers the same code path.

Seems like you could use the same -grecord-command-line logic on Windows? (Off-by-default I hope...) You could either rename DwarfDebugFlags to DebugFlags or create a separate PDBDebugFlags...

clang/include/clang/Frontend/CompilerInvocation.h

 //===- CompilerInvocation.h - Compiler Invocation Helper Data ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
static void ParseLangArgs(LangOptions &Opts, llvm::opt::ArgList &Args,
                             DiagnosticsEngine &Diags);
 
   /// Parse command line options that map to CodeGenOptions.
   static bool ParseCodeGenArgs(CodeGenOptions &Opts, llvm::opt::ArgList &Args,
                                InputKind IK, DiagnosticsEngine &Diags,
                                const llvm::Triple &T,
                                const std::string &OutputFile,
                                const LangOptions &LangOptsRef);
+
+  static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
+                                       SmallVectorImpl<const char *> &Args,
+                                       CompilerInvocation::StringAllocator SA);
 };
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 createVFSFromCompilerInvocation(const CompilerInvocation &CI,
                                 DiagnosticsEngine &Diags);
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem> createVFSFromCompilerInvocation(
     const CompilerInvocation &CI, DiagnosticsEngine &Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_COMPILERINVOCATION_H

clang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteOptions.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ModuleFileExtension.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FloatingPointMode.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/DebugInfoMetadata.h"
static Optional<bool> normalizeSimpleNegativeFlag(OptSpecifier Opt, unsigned,
   return None;
 }
 
 /// The tblgen-erated code passes in a fifth parameter of an arbitrary type, but
 /// denormalizeSimpleFlags never looks at it. Avoid bloating compile-time with
 /// unnecessary template instantiations and just ignore it with a variadic
 /// argument.
 static void denormalizeSimpleFlag(SmallVectorImpl<const char *> &Args,
-                                  const char *Spelling,
-                                  CompilerInvocation::StringAllocator,
+                                  Twine Spelling,
+                                  CompilerInvocation::StringAllocator SA,
                                   Option::OptionClass, unsigned, /*T*/...) {
-  Args.push_back(Spelling);
+  Args.push_back(SA(Spelling));
 }
 
 template <typename T> static constexpr bool is_uint64_t_convertible() {
   return !std::is_same<T, uint64_t>::value &&
          llvm::is_integral_or_enum<T>::value;
 }
 
 template <typename T,
return [Value](SmallVectorImpl<const char *> &Args, const char *Spelling,
                  CompilerInvocation::StringAllocator, Option::OptionClass,
                  unsigned, bool KeyPath) {
     if (KeyPath == Value)
       Args.push_back(Spelling);
   };
 }
 
 static void denormalizeStringImpl(SmallVectorImpl<const char *> &Args,
-                                  const char *Spelling,
+                                  Twine Spelling,
                                   CompilerInvocation::StringAllocator SA,
                                   Option::OptionClass OptClass, unsigned,
                                   Twine Value) {
   switch (OptClass) {
   case Option::SeparateClass:
   case Option::JoinedOrSeparateClass:
-    Args.push_back(Spelling);
+    Args.push_back(SA(Spelling));
     Args.push_back(SA(Value));
     break;
   case Option::JoinedClass:
-    Args.push_back(SA(Twine(Spelling) + Value));
+    Args.push_back(SA(Spelling + Value));
     break;
   default:
     llvm_unreachable("Cannot denormalize an option with option class "
                      "incompatible with string denormalization.");
   }
 }
 
 template <typename T>
 static void
-denormalizeString(SmallVectorImpl<const char *> &Args, const char *Spelling,
+denormalizeString(SmallVectorImpl<const char *> &Args, Twine Spelling,
                   CompilerInvocation::StringAllocator SA,
                   Option::OptionClass OptClass, unsigned TableIndex, T Value) {
   denormalizeStringImpl(Args, Spelling, SA, OptClass, TableIndex, Twine(Value));
 }
 
 static Optional<SimpleEnumValue>
 findValueTableByName(const SimpleEnumValueTable &Table, StringRef Name) {
   for (int I = 0, E = Table.Size; I != E; ++I)
if (A->getOption().matches(options::OPT_O)) {
       case 'z':
         return 2;
       }
     }
   }
   return 0;
 }
 
-static std::string GetOptName(llvm::opt::OptSpecifier OptSpecifier) {
-  static const OptTable &OptTable = getDriverOptTable();
-  return OptTable.getOption(OptSpecifier).getPrefixedName();
+static void GenerateArg(SmallVectorImpl<const char *> &Args,
+                        llvm::opt::OptSpecifier OptSpecifier,
+                        CompilerInvocation::StringAllocator SA) {
+  Option Opt = getDriverOptTable().getOption(OptSpecifier);
+  denormalizeSimpleFlag(Args, Opt.getPrefix() + Opt.getName(), SA,
+                        Option::OptionClass::FlagClass, 0);
+}
+
+static void GenerateArg(SmallVectorImpl<const char *> &Args,
+                        llvm::opt::OptSpecifier OptSpecifier,
+                        const Twine &Value,
+                        CompilerInvocation::StringAllocator SA) {
+  Option Opt = getDriverOptTable().getOption(OptSpecifier);
+  denormalizeString(Args, Opt.getPrefix() + Opt.getName(), SA, Opt.getKind(), 0,
+                    Value);
 }
 
 static void addDiagnosticArgs(ArgList &Args, OptSpecifier Group,
                               OptSpecifier GroupWithValue,
                               std::vector<std::string> &Diagnostics) {
   for (auto *A : Args.filtered(Group)) {
     if (A->getOption().getKind() == Option::FlagClass) {
       // The argument is a pure flag (such as OPT_Wall or OPT_Wdeprecated). Add
       // its name (minus the "W" or "R" at the beginning) to the warning list.
       Diagnostics.push_back(
           std::string(A->getOption().getName().drop_front(1)));
     } else if (A->getOption().matches(GroupWithValue)) {
       // This is -Wfoo= or -Rfoo=, where foo is the name of the diagnostic group.
       Diagnostics.push_back(
           std::string(A->getOption().getName().drop_front(1).rtrim("=-")));
     } else {
       // Otherwise, add its value (for OPT_W_Joined and similar).
       for (const auto *Arg : A->getValues())
         Diagnostics.emplace_back(Arg);
     }
   }
 }
 
 // Parse the Static Analyzer configuration. If \p Diags is set to nullptr,
 // it won't verify the input.
 static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
                                  DiagnosticsEngine *Diags);
 
 static void getAllNoBuiltinFuncValues(ArgList &Args,
                                       std::vector<std::string> &Funcs) {
   SmallVector<const char *, 8> Values;
   for (const auto &Arg : Args) {
     const Option &O = Arg->getOption();
     if (O.matches(options::OPT_fno_builtin_)) {
       const char *FuncName = Arg->getValue();
       if (Builtin::Context::isBuiltinFunc(FuncName))
         Values.push_back(FuncName);
     }
   }
   Funcs.insert(Funcs.end(), Values.begin(), Values.end());
 }
 
 static bool ParseAnalyzerArgs(AnalyzerOptions &Opts, ArgList &Args,
                               DiagnosticsEngine &Diags) {
   bool Success = true;
   if (Arg *A = Args.getLastArg(OPT_analyzer_store)) {
     StringRef Name = A->getValue();
     AnalysisStores Value = llvm::StringSwitch<AnalysisStores>(Name)
 #define ANALYSIS_STORE(NAME, CMDFLAG, DESC, CREATFN) \
       .Case(CMDFLAG, NAME##Model)
 #include "clang/StaticAnalyzer/Core/Analyses.def"
       .Default(NumStores);
     if (Value == NumStores) {
       Diags.Report(diag::err_drv_invalid_value)
         << A->getAsString(Args) << Name;
       Success = false;
     } else {
       Opts.AnalysisStoreOpt = Value;
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_analyzer_constraints)) {
     StringRef Name = A->getValue();
     AnalysisConstraints Value = llvm::StringSwitch<AnalysisConstraints>(Name)
 #define ANALYSIS_CONSTRAINTS(NAME, CMDFLAG, DESC, CREATFN) \
       .Case(CMDFLAG, NAME##Model)
 #include "clang/StaticAnalyzer/Core/Analyses.def"
       .Default(NumConstraints);
     if (Value == NumConstraints) {
       Diags.Report(diag::err_drv_invalid_value)
         << A->getAsString(Args) << Name;
       Success = false;
     } else {
       Opts.AnalysisConstraintsOpt = Value;
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_analyzer_output)) {
     StringRef Name = A->getValue();
     AnalysisDiagClients Value = llvm::StringSwitch<AnalysisDiagClients>(Name)
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATFN) \
       .Case(CMDFLAG, PD_##NAME)
 #include "clang/StaticAnalyzer/Core/Analyses.def"
       .Default(NUM_ANALYSIS_DIAG_CLIENTS);
     if (Value == NUM_ANALYSIS_DIAG_CLIENTS) {
       Diags.Report(diag::err_drv_invalid_value)
         << A->getAsString(Args) << Name;
       Success = false;
     } else {
       Opts.AnalysisDiagOpt = Value;
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_analyzer_purge)) {
     StringRef Name = A->getValue();
     AnalysisPurgeMode Value = llvm::StringSwitch<AnalysisPurgeMode>(Name)
 #define ANALYSIS_PURGE(NAME, CMDFLAG, DESC) \
       .Case(CMDFLAG, NAME)
 #include "clang/StaticAnalyzer/Core/Analyses.def"
       .Default(NumPurgeModes);
     if (Value == NumPurgeModes) {
       Diags.Report(diag::err_drv_invalid_value)
         << A->getAsString(Args) << Name;
       Success = false;
     } else {
       Opts.AnalysisPurgeOpt = Value;
     }
   }
 
   if (Arg *A = Args.getLastArg(OPT_analyzer_inlining_mode)) {
     StringRef Name = A->getValue();
     AnalysisInliningMode Value = llvm::StringSwitch<AnalysisInliningMode>(Name)
 #define ANALYSIS_INLINING_MODE(NAME, CMDFLAG, DESC) \
       .Case(CMDFLAG, NAME)
 #include "clang/StaticAnalyzer/Core/Analyses.def"
       .Default(NumInliningModes);
     if (Value == NumInliningModes) {
       Diags.Report(diag::err_drv_invalid_value)

 
 std::string CompilerInvocation::GetResourcesPath(const char *Argv0,
                                                  void *MainAddr) {
   std::string ClangExecutable =
       llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
   return Driver::GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
 }
 
-static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
-                                     SmallVectorImpl<const char *> &Args,
-                                     CompilerInvocation::StringAllocator SA) {
+void CompilerInvocation::GenerateHeaderSearchArgs(
+    const HeaderSearchOptions &Opts, SmallVectorImpl<const char *> &Args,
   const HeaderSearchOptions *HeaderSearchOpts = &Opts;
 #define HEADER_SEARCH_OPTION_WITH_MARSHALLING(                                 \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, 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, KEYPATH, DEFAULT_VALUE,    \
       IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, EXTRACTOR, TABLE_INDEX)
 #include "clang/Driver/Options.inc"
 #undef HEADER_SEARCH_OPTION_WITH_MARSHALLING
+
+  if (Opts.UseLibcxx)
+    GenerateArg(Args, OPT_stdlib_EQ, "libc++", SA);
+
+  if (!Opts.ModuleCachePath.empty())
+    GenerateArg(Args, OPT_fmodules_cache_path, Opts.ModuleCachePath, SA);
+
+  for (const auto &File : Opts.PrebuiltModuleFiles)
+    GenerateArg(Args, OPT_fmodule_file, File.first + "=" + File.second, SA);
+
+  for (const auto &Path : Opts.PrebuiltModulePaths)
+    GenerateArg(Args, OPT_fprebuilt_module_path, Path, SA);
+
+  for (const auto &Macro : Opts.ModulesIgnoreMacros)
+    GenerateArg(Args, OPT_fmodules_ignore_macro, Macro.val(), SA);
+
+  auto matches = [](const HeaderSearchOptions::Entry &Entry,
+                    frontend::IncludeDirGroup Group, bool IsFramework,
+                    bool IgnoreSysRoot) {
+    return Entry.Group == Group && Entry.IsFramework == IsFramework &&
+           Entry.IgnoreSysRoot == IgnoreSysRoot;
+  };
+
+  for (const HeaderSearchOptions::Entry &E : Opts.UserEntries) {
+    // Add -I..., -F..., and -index-header-map options in order.
+
+    if (E.Group == frontend::IndexHeaderMap)
+      GenerateArg(Args, OPT_index_header_map, SA);
+
+    Optional<OptSpecifier> Opt;
+
+    if (matches(E, frontend::IndexHeaderMap, true, true))
+      Opt = OPT_F;
+    else if (matches(E, frontend::IndexHeaderMap, false, true))
+      Opt = OPT_I;
+    else if (matches(E, frontend::Angled, true, true))
+      Opt = OPT_F;
+    else if (matches(E, frontend::Angled, false, true))
+      // Also handles [-iprefix=xx] -iwithprefixbefore=yy as -I[xx]yy.
+      Opt = OPT_I;
+
+    if (Opt) {
+      GenerateArg(Args, *Opt, E.Path, SA);
+      continue;
+    }
+
+    if (matches(E, frontend::After, false, true))
+      // Also handles [-iprefix=xx] -iwithprefix=yy as -idirafter=[xx]yy.
+      Opt = OPT_idirafter;
+    else if (matches(E, frontend::Quoted, false, true))
+      Opt = OPT_iquote;
+    else if (matches(E, frontend::System, false, true))
+      Opt = OPT_isystem;
+    else if (matches(E, frontend::System, false, false))
+      Opt = OPT_iwithsysroot;
+    else if (matches(E, frontend::System, true, true))
+      Opt = OPT_iframework;
+    else if (matches(E, frontend::System, true, false))
+      Opt = OPT_iframeworkwithsysroot;
+
+    else if (matches(E, frontend::CSystem, false, true))
+      // Add the paths for the various language specific isystem flags.
+      Opt = OPT_c_isystem;
+    else if (matches(E, frontend::CXXSystem, false, true))
+      Opt = OPT_cxx_isystem;
+    else if (matches(E, frontend::ObjCSystem, false, true))
+      Opt = OPT_objc_isystem;
+    else if (matches(E, frontend::ObjCXXSystem, false, true))
+      Opt = OPT_objcxx_isystem;
+
+    else if (matches(E, frontend::System, false, true))
+      // Add the internal paths from a driver that detects standard include
+      // paths.
+      Opt = OPT_internal_isystem;
+    else if (matches(E, frontend::ExternCSystem, false, true))
+      Opt = OPT_internal_externc_isystem;
+    else
+      llvm_unreachable("Tried to marshall invalid HeaderSearchOptions::Entry");
+
+    GenerateArg(Args, *Opt, E.Path, SA);
+  }
+
+  // Add the path prefixes which are implicitly treated as being system headers.
+  for (const auto &P : Opts.SystemHeaderPrefixes) {
+    OptSpecifier Opt = P.IsSystemHeader ? OPT_system_header_prefix
+                                        : OPT_no_system_header_prefix;
+    GenerateArg(Args, Opt, P.Prefix, SA);
+  }
+
+  for (const std::string &F : Opts.VFSOverlayFiles)
+    GenerateArg(Args, OPT_ivfsoverlay, F, SA);
 }
 
 static void ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
                                   DiagnosticsEngine &Diags,
                                   const std::string &WorkingDir) {
   HeaderSearchOptions *HeaderSearchOpts = &Opts;
   bool Success = true;
 
static const StringRef GetInputKindName(InputKind IK) {
   }
   llvm_unreachable("unknown input language");
 }
 
 static void GenerateLangArgs(const LangOptions &Opts,
                              SmallVectorImpl<const char *> &Args,
                              CompilerInvocation::StringAllocator SA) {
   if (Opts.IncludeDefaultHeader)
-    Args.push_back(SA(GetOptName(OPT_finclude_default_header)));
+    GenerateArg(Args, OPT_finclude_default_header, SA);
   if (Opts.DeclareOpenCLBuiltins)
-    Args.push_back(SA(GetOptName(OPT_fdeclare_opencl_builtins)));
+    GenerateArg(Args, OPT_fdeclare_opencl_builtins, SA);
 }
 
 void CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
                                        InputKind IK, const llvm::Triple &T,
                                        std::vector<std::string> &Includes,
                                        DiagnosticsEngine &Diags) {
   // FIXME: Cleanup per-file based stuff.
   LangStandard::Kind LangStd = LangStandard::lang_unspecified;
if (Arg *A = Args.getLastArg(options::OPT_target_sdk_version_EQ)) {
     if (Version.tryParse(A->getValue()))
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
     else
       Opts.SDKVersion = Version;
   }
 }
 
+template <typename ParseFn, typename GenerateFn, typename ResetFn>
+void RoundTrip(ParseFn &&Parse, GenerateFn &&Generate, ResetFn &&Reset,
+               CompilerInvocation &Res, InputArgList &OriginalArgs) {
+  // Set up the string allocator for generating command line arguments.
+  SmallVector<std::string, 32> GeneratedArgsStorage;
+  auto SA = [&GeneratedArgsStorage](const Twine &Arg) -> const char * {
+    return GeneratedArgsStorage.emplace_back(Arg.str()).c_str();
+  };
+
+  // Parse the original arguments and records which options the parser queried.
+  llvm::DenseSet<unsigned> QueriedBefore = OriginalArgs.getQueriedOpts();
+  Parse(Res, OriginalArgs);
+  llvm::DenseSet<unsigned> QueriedAfter = OriginalArgs.getQueriedOpts();
+  llvm::DenseSet<unsigned> QueriedDuring =
+      llvm::set_difference(QueriedAfter, QueriedBefore);
+
+  // Create an empty string list.
+  ArgStringList GeneratedArgStringList;
+  // Add all original arguments that were not queried by the parser.
+  OriginalArgs.AddAllArgsExcept(GeneratedArgStringList, QueriedDuring);
+  // Invoke the generator. If implemented correctly, it should complete the
+  // string list with arguments the parser queried, and have the same
+  // semantics as the original arguments.
+  Generate(Res, GeneratedArgStringList, SA);
+
+  // Construct InputArgList instance from the generated string list.
+  const OptTable &Opts = getDriverOptTable();
+  unsigned MissingArgIndex, MissingArgCount;
+  InputArgList GeneratedArgs =
+      Opts.ParseArgs(GeneratedArgStringList, MissingArgIndex, MissingArgCount,
+                     options::CC1Option);
+
+  // Reset the part of CompilerInvocation that the parser set up.
+  Reset(Res);
+
+  // Run the parser again. If the generated arguments have the same semantics
+  // as the original arguments, this should set up the CompilerInvocation part
+  // in the same way the first parser call did. Let's verify that by using this
+  // for the rest of the compilation process.
+  Parse(Res, GeneratedArgs);
+}
+
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                         ArrayRef<const char *> CommandLineArgs,
                                         DiagnosticsEngine &Diags,
                                         const char *Argv0) {
   bool Success = true;
 
   // Parse the arguments.
   const OptTable &Opts = getDriverOptTable();
bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
   Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, &Diags,
                                  /*DefaultDiagColor=*/false);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
                                       LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   llvm::Triple T(Res.getTargetOpts().Triple);
+
+  auto ParseHS = [&Diags](CompilerInvocation &Res, InputArgList &Args) {
     ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args, Diags,
                           Res.getFileSystemOpts().WorkingDir);
+  };
+  auto GenerateHS = [](CompilerInvocation &Res,
+                       SmallVectorImpl<const char *> &Args,
+                       StringAllocator SA) {
+    GenerateHeaderSearchArgs(Res.getHeaderSearchOpts(), Args, SA);
+  };
+  auto ResetHS = [](CompilerInvocation &Res) {
+    auto EmptyHeaderSearchOpts = std::make_shared<HeaderSearchOptions>();
+    Res.HeaderSearchOpts.swap(EmptyHeaderSearchOpts);
+  };
+
+  RoundTrip(ParseHS, GenerateHS, ResetHS, Res, Args);
   if (DashX.getFormat() == InputKind::Precompiled ||
       DashX.getLanguage() == Language::LLVM_IR) {
     // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
     // PassManager in BackendUtil.cpp. They need to be initializd no matter
     // what the input type is.
     if (Args.hasArg(OPT_fobjc_arc))
       LangOpts.ObjCAutoRefCount = 1;
     // PIClevel and PIELevel are needed during code generation and this should be

llvm/include/llvm/Option/ArgList.h

 //===- ArgList.h - Argument List Management ---------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
private:
   arglist_type Args;
 
   using OptRange = std::pair<unsigned, unsigned>;
   static OptRange emptyRange() { return {-1u, 0u}; }
 
   /// The first and last index of each different OptSpecifier ID.
   DenseMap<unsigned, OptRange> OptRanges;
 
+  /// The OptSpecifiers that were queried from this argument list.
+  mutable DenseSet<unsigned> QueriedOpts;
+
+  /// Record the queried OptSpecifiers.
+  template <typename... OptSpecifiers>
+  void recordQueriedOpts(OptSpecifiers... Ids) const {
+    SmallVector<unsigned, 4> OptsSpecifiers({toOptSpecifier(Ids).getID()...});
+    QueriedOpts.insert(OptsSpecifiers.begin(), OptsSpecifiers.end());
+  }
+
   /// Get the range of indexes in which options with the specified IDs might
   /// reside, or (0, 0) if there are no such options.
   OptRange getRange(std::initializer_list<OptSpecifier> Ids) const;
 
 protected:
   // Make the default special members protected so they won't be used to slice
   // derived objects, but can still be used by derived objects to implement
   // their own special members.
public:
   const_iterator end() const { return {Args.end(), Args.end()}; }
 
   const_reverse_iterator rbegin() const { return {Args.rbegin(), Args.rend()}; }
   const_reverse_iterator rend() const { return {Args.rend(), Args.rend()}; }
 
   template<typename ...OptSpecifiers>
   iterator_range<filtered_iterator<sizeof...(OptSpecifiers)>>
   filtered(OptSpecifiers ...Ids) const {
+    recordQueriedOpts(Ids...);
     OptRange Range = getRange({toOptSpecifier(Ids)...});
     auto B = Args.begin() + Range.first;
     auto E = Args.begin() + Range.second;
     using Iterator = filtered_iterator<sizeof...(OptSpecifiers)>;
     return make_range(Iterator(B, E, {toOptSpecifier(Ids)...}),
                       Iterator(E, E, {toOptSpecifier(Ids)...}));
   }
 
   template<typename ...OptSpecifiers>
   iterator_range<filtered_reverse_iterator<sizeof...(OptSpecifiers)>>
   filtered_reverse(OptSpecifiers ...Ids) const {
+    recordQueriedOpts(Ids...);
     OptRange Range = getRange({toOptSpecifier(Ids)...});
     auto B = Args.rend() - Range.second;
     auto E = Args.rend() - Range.first;
     using Iterator = filtered_reverse_iterator<sizeof...(OptSpecifiers)>;
     return make_range(Iterator(B, E, {toOptSpecifier(Ids)...}),
                       Iterator(E, E, {toOptSpecifier(Ids)...}));
   }
 
public:
 
   /// Render only the last argument match \p Id0, if present.
   template<typename ...OptSpecifiers>
   void AddLastArg(ArgStringList &Output, OptSpecifiers ...Ids) const {
     if (Arg *A = getLastArg(Ids...)) // Calls claim() on all Ids's Args.
       A->render(*this, Output);
   }
 
+  /// AddAllArgsExcept - Render all arguments not matching any of the excluded
+  /// ids.
+  void AddAllArgsExcept(ArgStringList &Output,
+                        const DenseSet<unsigned> &ExcludeIds) const;
   /// AddAllArgsExcept - Render all arguments matching any of the given ids
   /// and not matching any of the excluded ids.
   void AddAllArgsExcept(ArgStringList &Output, ArrayRef<OptSpecifier> Ids,
                         ArrayRef<OptSpecifier> ExcludeIds) const;
   /// AddAllArgs - Render all arguments matching any of the given ids.
   void AddAllArgs(ArgStringList &Output, ArrayRef<OptSpecifier> Ids) const;
 
   /// AddAllArgs - Render all arguments matching the given ids.
public:
   /// ClaimAllArgs - Claim all arguments which match the given
   /// option id.
   void ClaimAllArgs(OptSpecifier Id0) const;
 
   /// ClaimAllArgs - Claim all arguments.
   ///
   void ClaimAllArgs() const;
 
+  /// Return the OptSpecifiers queried from this argument list.
+  const DenseSet<unsigned> &getQueriedOpts() const { return QueriedOpts; }
+
   /// @}
   /// @name Arg Synthesis
   /// @{
 
   /// Construct a constant string pointer whose
   /// lifetime will match that of the ArgList.
   virtual const char *MakeArgStringRef(StringRef Str) const = 0;
   const char *MakeArgString(const Twine &Str) const {

llvm/lib/Option/ArgList.cpp

 //===- ArgList.cpp - Argument List Management -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 
 StringRef ArgList::getLastArgValue(OptSpecifier Id, StringRef Default) const {
   if (Arg *A = getLastArg(Id))
     return A->getValue();
   return Default;
 }
 
 std::vector<std::string> ArgList::getAllArgValues(OptSpecifier Id) const {
+  recordQueriedOpts(Id);
   SmallVector<const char *, 16> Values;
   AddAllArgValues(Values, Id);
   return std::vector<std::string>(Values.begin(), Values.end());
 }
 
 void ArgList::AddAllArgsExcept(ArgStringList &Output,
+                               const DenseSet<unsigned> &ExcludeIds) const {
+  for (const Arg *Arg : *this) {
+    if (!ExcludeIds.contains(Arg->getOption().getID())) {
+      Arg->claim();
+      Arg->render(*this, Output);
+    }
+  }
+}
+
+void ArgList::AddAllArgsExcept(ArgStringList &Output,
                                ArrayRef<OptSpecifier> Ids,
                                ArrayRef<OptSpecifier> ExcludeIds) const {
   for (const Arg *Arg : *this) {
     bool Excluded = false;
     for (OptSpecifier Id : ExcludeIds) {
       if (Arg->getOption().matches(Id)) {
         Excluded = true;
         break;