[PATCH] D94472: [clang][cli] Command line round-trip for HeaderSearch options
Jan Svoboda via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 6 02:59:20 PDT 2021
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
More information about the cfe-commits
mailing list