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

Sylvain Audi via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 8 11:02:10 PDT 2021


saudi added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677
+  SwapOpts(Res);
+  bool Success2 = Parse(Res, GeneratedArgs1, Diags);
+
----------------
jansvoboda11 wrote:
> saudi wrote:
> > 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?
> > 
> > > 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`.
> 
> Let me clarify. AFAIK the only read from `clang::CodeGenOptions::CommandLineArgs` appears in `initTargetOptions`, which is **Clang** code that initializes `llvm::{MC,}TargetOptions`. My thinking is that this code could access `CompilerInvocation`, call its `generateCC1CommandLine` and put the generated arguments into an owning `std::vector<std::string>` inside `llvm::MCTargetOptions`. The `CompilerInvocation` data structure could then drop `CodeGenOptions::CommandLineArgs` since it's redundant anyways (it can be re-generated). This way, we can avoid complicating the lifetimes and clean up `CompilerInvocation`. If we really want, we can optimize the `std::string`s away by using `BumpPtrAllocator` - as you mentioned - inside `llvm::{MC,}TargetOptions`. Do you think that makes sense?
> 
> > > 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?
> 
> From Clang's point of view, I don't think using `ArgV` instead of `GeneratedArgs1` would remove the effect of using the round-tripped arguments during the rest of the compilation. The semantics of the generated arguments are captured by the whole `CompilerInvocation` and `clang::CodeGenOptions::CommandLineArgs` don't get used anywhere else in Clang. The question is what LLVM does with these arguments. If it only shows them to the user to provide extra information, I think that's fine too.
> Let me clarify. AFAIK the only read from `clang::CodeGenOptions::CommandLineArgs` appears in `initTargetOptions`, which is **Clang** code that initializes `llvm::{MC,}TargetOptions`. My thinking is that this code could access `CompilerInvocation`, call its `generateCC1CommandLine` and put the generated arguments into an owning `std::vector<std::string>` inside `llvm::MCTargetOptions`. 
After looking a bit, `initTargetOptions` doesn't seem to have access to the `CompilerInvocation`, it is triggered by the clang backend, where only the `{HeaderSearch|CodeGen|Target|Lang}Options` structures are passed; it would require passing the `CompilerInvocation` through several layers.

> From Clang's point of view, I don't think using `ArgV` instead of `GeneratedArgs1` would remove the effect of using the round-tripped arguments during the rest of the compilation. The semantics of the generated arguments are captured by the whole `CompilerInvocation` and `clang::CodeGenOptions::CommandLineArgs` don't get used anywhere else in Clang. The question is what LLVM does with these arguments. If it only shows them to the user to provide extra information, I think that's fine too.

I realized that using `ArgV` instead of `GeneratedArgs1` may miss detecting if `Generate` misses some arguments. In that case, no error would be reported by the round-trip test, as `GeneratedArgs1` and `GeneratedArgs2` would be identical. 

I'm currently trying to add a `BumpPtrAllocator` along with a `SmallVector< const char *>` in `CompilerInvocationRefBase`. I think it makes sense for CompilerInvocation to be the owner of the custom command line arguments generated during the round-trip.
Let me know if there would be a cleaner solution: maybe adding `CompilerInvocation` access in the backend would be simple enough?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472



More information about the cfe-commits mailing list