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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472