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

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

In D94472#2539685 <https://reviews.llvm.org/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).



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:592-594
+#ifdef CLANG_ROUND_TRIP_CC1_ARGS
+  DoRoundTrip = true;
+#endif
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472