This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jansvoboda11 on Jan 12 2021, 1:19 AM.

Details

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.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Jan 12 2021, 1:19 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 1:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 edited the summary of this revision. (Show Details)Jan 12 2021, 1:30 AM

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.

Add (improved) round-trip mechanism

Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 9:42 AM
jansvoboda11 retitled this revision from [clang][cli] Manually generate header search arguments to [WIP][clang][cli] Command line round-trip for HeaderSearch options.Jan 15 2021, 9:43 AM
jansvoboda11 added inline comments.Jan 15 2021, 9:53 AM
clang/lib/Frontend/CompilerInvocation.cpp
2029

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.

3092

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

3156

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.)

jansvoboda11 edited the summary of this revision. (Show Details)Jan 15 2021, 9:54 AM

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.

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.

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?

  1. 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
3151–3154

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.

Create function for argument generation

Move boilerplate from CreateFromArgs to separate function

Fix formatting

Add round-tripping arguments

Preserve pointer identity when round-tripping

Undo changes to string allocation

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.

  1. 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?

  1. 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.

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

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...

  1. 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.

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.

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.

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

jansvoboda11 retitled this revision from [WIP][clang][cli] Command line round-trip for HeaderSearch options to [clang][cli] Command line round-trip for HeaderSearch options.Jan 27 2021, 4:08 AM
jansvoboda11 edited the summary of this revision. (Show Details)

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.

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
630

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

dexonsmith added inline comments.Jan 27 2021, 12:44 PM
clang/include/clang/Driver/Options.td
523–524

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
557

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.

566

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

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

jansvoboda11 added inline comments.Jan 28 2021, 7:54 AM
clang/include/clang/Driver/Options.td
523–524

Done.

clang/lib/Frontend/CompilerInvocation.cpp
557

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.

566

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

3151–3154

Resolved by SwapOpts.

dexonsmith requested changes to this revision.Jan 28 2021, 11:57 AM

This is getting close! A few more comments inline.

clang/lib/Frontend/CompilerInvocation.cpp
570–575

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.)

577–582
  • 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?
590–594

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

619

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

640–642

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

649

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

670–671

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.

This revision now requires changes to proceed.Jan 28 2021, 11:57 AM

Improve error handling

jansvoboda11 added inline comments.Jan 29 2021, 5:30 AM
clang/lib/Frontend/CompilerInvocation.cpp
570–575

That's fair.

577–582

Good point, resolved.

590–594

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.

619

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

640–642

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.

649

That's right.

670–671

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 added inline comments.Jan 29 2021, 7:50 AM
clang/lib/Frontend/CompilerInvocation.cpp
670–671

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.

Create proper diagnostics, add remark tests

clang/lib/Frontend/CompilerInvocation.cpp
670–671

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.

Fix formatting and comment wording

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

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 accepted this revision.Feb 3 2021, 1:25 PM

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
592–594

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.

This revision is now accepted and ready to land.Feb 3 2021, 1:25 PM
This revision was landed with ongoing or failed builds.Feb 4 2021, 1:18 AM
This revision was automatically updated to reflect the committed changes.
saudi added a subscriber: saudi.Oct 5 2021, 2:36 PM
saudi added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
677

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 added inline comments.Oct 6 2021, 2:59 AM
clang/lib/Frontend/CompilerInvocation.cpp
677

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 added inline comments.Oct 6 2021, 1:25 PM
clang/lib/Frontend/CompilerInvocation.cpp
677

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 added inline comments.Oct 7 2021, 1:31 AM
clang/lib/Frontend/CompilerInvocation.cpp
677

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 added inline comments.Oct 8 2021, 11:02 AM
clang/lib/Frontend/CompilerInvocation.cpp
677

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?

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.)

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...