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

Tue Jan 19 14:00:58 PST 2021

dexonsmith added a comment.

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

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3211-3214
+  auto ResetHS = [](CompilerInvocation &Res) {
+    auto EmptyHeaderSearchOpts = std::make_shared<HeaderSearchOptions>();
+    Res.HeaderSearchOpts.swap(EmptyHeaderSearchOpts);
+  };
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472