[PATCH] D143418: [libclang] Add API to override preamble storage path

Thu Mar 9 05:54:44 PST 2023

aaron.ballman added a comment.

In D143418#4175887 <https://reviews.llvm.org/D143418#4175887>, @vedgy wrote:

> In D143418#4175628 <https://reviews.llvm.org/D143418#4175628>, @aaron.ballman wrote:
>
>> Hmmm, don't relaxed loads and stores still have the potential to be racey? I thought you needed a release on the store and an acquire on the load (or sequential consistency), but this is definitely not my area of expertise.
>
> The acquire/release semantics is needed if the atomic variable guards access to another non-atomic variable or the atomic pointer guards access to its non-atomic pointed-to value. The relaxed order means no guarantees about this variable's interaction with other atomic or non-atomic variables. But even the relaxed order prevents data races on the variable itself, which is sufficient here.

Ah, thank you for the explanation!

>>> The option can also be added to `CXIndexOptions` in order to allow setting its initial value reliably (not worrying whether it could be used before the setter gets called after index construction).
>>>
>>> Is adding both the setter and the `CXIndexOptions` member OK or would you prefer only one of these two?
>>
>> It's a bit odd to me that we're deprecating the global option setters to turn around and add a new global option setter. The old interfaces are not thread safe and the new one is thread safe, so I get why the desire exists and is reasonable, but (personally) I'd like to get rid of the option state setters entirely someday.
>
> I also thought about the deprecated old interfaces today. `clang_CXIndex_setGlobalOptions()` could be undeprecated by similarly making `CIndexer::Options` atomic. Safely setting `std::string` members would require a mutex.

Yeah, we could make it thread safe, but I still don't think setters are a good design approach. To me, these global options should be ones that are set when creating the index and be immutable from there on. (This mirrors how language options work in Clang itself -- we have a ton of language options, but they get set up by the compiler frontend and are (generally) immutable from there on. When we need to allow for different options, the interface accepts a `LangOptions` object, as in: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Analysis/CFG.h#L1081)

>> What I was envisioning was that the index creation would get global options and if we wanted per-TU options, we'd add an interface akin to `clang_parseTranslationUnit()` which takes another options struct for per-TU options. (Perhaps the default values for those options come from the global options -- e.g., `DisplayDiagnostics` is set at the global level but could be overridden for a single TU). Do you have thoughts about that approach?
>
> This would allow to specify whether to store each individual preamble in memory, which is more specific than necessary for my use case. This would shift the burden of passing around the `StorePreamblesInMemory` value to libclang users. Certainly more difficult to implement both in libclang and in KDevelop.
>
> Advantages of getting rid of the option state setters entirely and passing options to per-TU APIs:
>
> 1. More flexibility (per-TU option specification).
> 2. Possibly more efficient (no need for atomics and mutexes).
> 3. Clearer to API users which TUs the modified options apply to and which TUs (created earlier) keep the original options.
> 4. Less risk of inconsistencies and bugs in libclang. Such as somehow passing a new option value to an old TU with unpredictable consequences.
>
> Do the listed advantages explain your preference?

Yes, thank you! #3/#4 are really the biggest advantages, to me. By passing in the options explicitly to the TU instead of having setters, we reduce the complexity of the interface because it no longer becomes possible for an option to change mid-parse. This in turn makes testing far easier because we don't have to come up with test coverage for "what if the option was X and it got switched to Y before calling this function" kind of situations.

> I am not sure what I would prefer from a hypothetical standpoint of a libclang maintainer. And you definitely have more experience in this area. So I won't argue for the index option state setters.
>
> I'll probably implement the uncontroversial `CXIndexOptions` member first. And then, if I think implementing it is worth the effort, continue discussing `clang_parseTranslationUnitWithOptions()`.

I think that's a good approach. You are a consumer of libclang, so having your perspective about what makes the interface easier for you to use is also really helpful in figuring out a good design. Thank you for being willing to share your experiences with using libclang on KDevelop!

================
Comment at: clang/include/clang-c/Index.h:365
+   */
+  int ExcludeDeclarationsFromPCH : 1;
+  /**
----------------
vedgy wrote:
> Assigning `true` to `int : 1` bit-fields in C++ code produces a GCC warning:
> ```
> warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes value from ‘1’ to ‘-1’ [-Woverflow]
> ```
> 
> Following a suggestion in a comment to https://github.com/llvm/llvm-project/issues/53253, I replaced this `int` with `unsigned` and the warning disappeared. Same for `int DisplayDiagnostics : 1`. Should this type change be included in the upcoming `StorePreamblesInMemory` revision?
> Assigning true to int : 1 bit-fields in C++ code produces a GCC warning:
>
> `warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes value from ‘1’ to ‘-1’ [-Woverflow]`

Ugh, I forgot that the C standard allows that. (C2x 6.7.2.1p12: "A bit-field member is interpreted as having a signed or unsigned integer type consisting of the specified number of bits" -- GCC decided to turn our `int` into `signed char` which is nice for packing data together, but not as nice when it comes to boolean-like bit-fields.)

> Should this type change be included in the upcoming StorePreamblesInMemory revision?

It'd probably be the cleanest to fix that separately. Given that it's NFC and you don't have commit privileges, I can make the change on your behalf and land it today if that's what you'd like.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418