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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
