[PATCH] D94472: [WIP][clang][cli] Command line round-trip for HeaderSearch options

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94472/new/

https://reviews.llvm.org/D94472