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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
