[clang] Hide the DiagnosticOptions pointer from CompilerInvocation

[jansvoboda11]

This PR hides the reference-counter pointer that holds DiagnosticOptions from the public API of CompilerInvocation. This gives CompilerInvocation an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior.

The only client that currently accesses that pointer is clangd::buildPreamble() which takes care to reset it so that it's not reset concurrently. This code is made redundant by making the reference count of DiagnosticOptions atomic.

[llvmbot]

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Jan Svoboda (jansvoboda11)

Changes

This PR hides the reference-counter pointer that holds DiagnosticOptions from the public API of CompilerInvocation. This gives CompilerInvocation an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior.

The only client that currently accesses that pointer is clangd::buildPreamble() which takes care to reset it so that it's not reset concurrently. This code is made redundant by making the reference count of DiagnosticOptions atomic.

---

Full diff: https://github.com/llvm/llvm-project/pull/106274.diff

3 Files Affected:

• (modified) clang-tools-extra/clangd/Preamble.cpp (-1)
• (modified) clang/include/clang/Basic/DiagnosticOptions.h (+2-1)
• (modified) clang/include/clang/Frontend/CompilerInvocation.h (-1)

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index dd13b1a9e5613d..51d478cf4efc1d 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -673,7 +673,6 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
-  CI.DiagnosticOpts.reset();
 
   // When building the AST for the main file, we do want the function
   // bodies.
diff --git a/clang/include/clang/Basic/DiagnosticOptions.h b/clang/include/clang/Basic/DiagnosticOptions.h
index 30141c2b8f4475..ea8da70e081f01 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.h
+++ b/clang/include/clang/Basic/DiagnosticOptions.h
@@ -67,7 +67,8 @@ inline DiagnosticLevelMask operator&(DiagnosticLevelMask LHS,
 raw_ostream& operator<<(raw_ostream& Out, DiagnosticLevelMask M);
 
 /// Options for controlling the compiler diagnostics engine.
-class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
+class DiagnosticOptions
+    : public llvm::ThreadSafeRefCountedBase<DiagnosticOptions> {
   friend bool ParseDiagnosticArgs(DiagnosticOptions &, llvm::opt::ArgList &,
                                   clang::DiagnosticsEngine *, bool);
 
diff --git a/clang/include/clang/Frontend/CompilerInvocation.h b/clang/include/clang/Frontend/CompilerInvocation.h
index 9daa0a1ecf9488..1e4d2da86c2bee 100644
--- a/clang/include/clang/Frontend/CompilerInvocation.h
+++ b/clang/include/clang/Frontend/CompilerInvocation.h
@@ -269,7 +269,6 @@ class CompilerInvocation : public CompilerInvocationBase {
   /// @{
   using CompilerInvocationBase::LangOpts;
   using CompilerInvocationBase::TargetOpts;
-  using CompilerInvocationBase::DiagnosticOpts;
   std::shared_ptr<HeaderSearchOptions> getHeaderSearchOptsPtr() {
     return HSOpts;
   }

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

This PR hides the reference-counter pointer that holds DiagnosticOptions from the public API of CompilerInvocation. This gives CompilerInvocation an exclusive control over the lifetime of this member, which will eventually be leveraged to implement a copy-on-write behavior.

The only client that currently accesses that pointer is clangd::buildPreamble() which takes care to reset it so that it's not reset concurrently. This code is made redundant by making the reference count of DiagnosticOptions atomic.

---

Full diff: https://github.com/llvm/llvm-project/pull/106274.diff

3 Files Affected:

• (modified) clang-tools-extra/clangd/Preamble.cpp (-1)
• (modified) clang/include/clang/Basic/DiagnosticOptions.h (+2-1)
• (modified) clang/include/clang/Frontend/CompilerInvocation.h (-1)

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index dd13b1a9e5613d..51d478cf4efc1d 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -673,7 +673,6 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
-  CI.DiagnosticOpts.reset();
 
   // When building the AST for the main file, we do want the function
   // bodies.
diff --git a/clang/include/clang/Basic/DiagnosticOptions.h b/clang/include/clang/Basic/DiagnosticOptions.h
index 30141c2b8f4475..ea8da70e081f01 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.h
+++ b/clang/include/clang/Basic/DiagnosticOptions.h
@@ -67,7 +67,8 @@ inline DiagnosticLevelMask operator&(DiagnosticLevelMask LHS,
 raw_ostream& operator<<(raw_ostream& Out, DiagnosticLevelMask M);
 
 /// Options for controlling the compiler diagnostics engine.
-class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
+class DiagnosticOptions
+    : public llvm::ThreadSafeRefCountedBase<DiagnosticOptions> {
   friend bool ParseDiagnosticArgs(DiagnosticOptions &, llvm::opt::ArgList &,
                                   clang::DiagnosticsEngine *, bool);
 
diff --git a/clang/include/clang/Frontend/CompilerInvocation.h b/clang/include/clang/Frontend/CompilerInvocation.h
index 9daa0a1ecf9488..1e4d2da86c2bee 100644
--- a/clang/include/clang/Frontend/CompilerInvocation.h
+++ b/clang/include/clang/Frontend/CompilerInvocation.h
@@ -269,7 +269,6 @@ class CompilerInvocation : public CompilerInvocationBase {
   /// @{
   using CompilerInvocationBase::LangOpts;
   using CompilerInvocationBase::TargetOpts;
-  using CompilerInvocationBase::DiagnosticOpts;
   std::shared_ptr<HeaderSearchOptions> getHeaderSearchOptsPtr() {
     return HSOpts;
   }

[jansvoboda11]

@kadircet Can you please check my assumption on the clangd part? The .reset() call was introduced in 27ade4b.

[kadircet]

yes turning diagnosticoptions to be a ThreadSafeRefCountedBase pointer should ensure we have similar guarantees. so LG from clangd side, but I am not sure about implications on using thread-aware structs in core parts of clang. so it might be worthwhile to get some opinions from clang maintainers as well.

[jansvoboda11]

yes turning diagnosticoptions to be a ThreadSafeRefCountedBase pointer should ensure we have similar guarantees. so LG from clangd side

Thanks for confirming!

I am not sure about implications on using thread-aware structs in core parts of clang. so it might be worthwhile to get some opinions from clang maintainers as well.

Good point. @AaronBallman any thoughts?

[AaronBallman]

yes turning diagnosticoptions to be a ThreadSafeRefCountedBase pointer should ensure we have similar guarantees. so LG from clangd side

Thanks for confirming!

I am not sure about implications on using thread-aware structs in core parts of clang. so it might be worthwhile to get some opinions from clang maintainers as well.

Good point. @AaronBallman any thoughts?

So long as there are sufficient comments explaining why the thread-safety is needed, I think it's okay.