[PATCH] D95514: [clang][cli] Teach CompilerInvocation to allocate strings on its own

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D95514#2526064 <https://reviews.llvm.org/D95514#2526064>, @dexonsmith wrote:

> Well, the compiler developers are the users, since `-cc1` isn't for end-users. I acknowledge it's a tradeoff. IMO, without actually seeing it in practice, it seems a bit nicer to design this away entirely. But maybe having a `=` is more valuable than I think.



> For string options:
>
> - If `-cc1` accepts only `-opt string`, no need for allocation.
> - If `-cc1` accepts only `-opt=string`, the fully-spelled option is on the original command-line. `ExistingStrings.find()` should turn it up when calling `Strings.save()`.
> - If `-cc1` accepts both `-opt=string` and `-opt string` canonicalize to `-opt string` when generating. No need for allocation.
>
> For numeric / enum options we'll need to allocate, but the allocation doesn't have to live past running the parser.
>
> The only problem would be if `string` gets canonicalized / rewritten somehow during parse + generate, or if an enum value is additionally saved as a string value.

We can definitely ask around how would people feel about that.

>>> Consider that some callers may parse the full command-line and then drop everything but one options class. That pattern wouldn’t work anymore since the StringRefs won’t be valid.
>>
>> Ah, that's right. So if we don't go with the allocation-less approach, we'd need to take `StringAllocator` from the client that also ensures proper lifetimes.
>
> Maybe it wouldn't be terrible to have a `RoundTrip` arg on each options class. When you assign to the CompilerInvocation, it copies the shared ptr into each of the options classes, so that the pool stays alive as long as one of them has it.

I think this is the most robust solution.

Moving `BumpPtrAllocator`/`StringSaver` into `CompilerInvocation` will become problematic once we start round-tripping more option classes.

I've looked through all option classes in `CompilerInvocation`, and `AnalyzerOptions::Config` seems to be the only member that stores non-owning references to command line arguments.
I think the best path forward is to change `AnalyzerOptions` to take ownership and avoid dealing with complicated lifetimes. We can look into removing allocations later, as an optional optimization.
What do you think?



================
Comment at: clang/include/clang/Frontend/CompilerInvocation.h:144
+  /// Container holding strings allocated during round-trip.
+  mutable std::forward_list<std::string> AllocatedStrings;
+
----------------
dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > I suggest using llvm::StringPool instead, which also uniques the strings and is used elsewhere for this kind of thing. Probably in an Optional to make it more clear it’s not always used.
> > > 
> > > Maybe the name RoundTrippedStrings? 
> > From the documentation of `StringPool`, it seems like we would need to store `PooledStringPtr`s in order to keep them alive. How would that work with the `const char *` API we have?
> Oops; I meant `StringSet<>` / `StringMap<NoneType>` / etc., which is what I've always used in the past. This is essentially a `StringMap<void>`; I just always call the variable `StringPool`, hence my error. It has the properties you need (stable, kept alive indefinitely, null-terminated).
> 
> Looking around, there are a couple of options designed for this:
> - `StringSaver`, which is an optimized version of your list (uses a bumpptrallocator, no uniquing).
> - `UniqueStringSaver`, which is essentially equivalent to `StringSet<>`; might be slightly more optimized, and if it's not, we should fix it.
> 
> I suggest using one of those since they exactly match what you need.
Thanks for mentioning `StringSaver`s, they fit our needs perfectly (now used in D94472).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95514