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

[Sylvain Audi via Phabricator via llvm-commits]

saudi added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677
+  SwapOpts(Res);
+  bool Success2 = Parse(Res, GeneratedArgs1, Diags);
+
----------------
jansvoboda11 wrote:
> saudi wrote:
> > 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?
> > 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?
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472