[clang] Deduplicate the logic that only warns once when stack is almost full #112371

bricknerb · 2024-10-15T14:48:25Z

Zero diff in behavior.

llvmbot · 2024-10-15T14:48:57Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang-modules

Author: Boaz Brickner (bricknerb)

Changes

Zero diff in behavior.

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

10 Files Affected:

(modified) clang/include/clang/Basic/Stack.h (+27)
(modified) clang/include/clang/Sema/Sema.h (+2-4)
(modified) clang/include/clang/Serialization/ASTReader.h (+4-2)
(modified) clang/lib/Basic/Stack.cpp (+20)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+2-10)
(modified) clang/lib/CodeGen/CodeGenModule.h (+2-4)
(modified) clang/lib/Sema/Sema.cpp (+2-10)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+1-2)
(modified) clang/lib/Serialization/ASTReader.cpp (+10-11)
(modified) clang/lib/Serialization/ASTReaderDecl.cpp (+1-2)

diff --git a/clang/include/clang/Basic/Stack.h b/clang/include/clang/Basic/Stack.h
index 30ebd94aedd1f7..64367d8519bf84 100644
--- a/clang/include/clang/Basic/Stack.h
+++ b/clang/include/clang/Basic/Stack.h
@@ -16,6 +16,8 @@
 
 #include <cstddef>
 
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Compiler.h"
 
@@ -50,6 +52,31 @@ namespace clang {
     Fn();
 #endif
   }
+
+class SingleWarningStackAwareExecutor {
+public:
+  SingleWarningStackAwareExecutor(DiagnosticsEngine &diags)
+      : DiagsRef(diags) {}
+
+  /// Run some code with "sufficient" stack space. (Currently, at least 256K
+  /// is guaranteed). Produces a warning if we're low on stack space and
+  /// allocates more in that case. Use this in code that may recurse deeply to
+  /// avoid stack overflow.
+  void runWithSufficientStackSpace(SourceLocation Loc,
+                                   llvm::function_ref<void()> Fn);
+
+  /// Check to see if we're low on stack space and produce a warning if we're
+  /// low on stack space (Currently, at least 256Kis guaranteed).
+  void warnOnStackNearlyExhausted(SourceLocation Loc);
+
+private:
+  /// Warn that the stack is nearly exhausted.
+  void warnStackExhausted(SourceLocation Loc);
+
+  DiagnosticsEngine &DiagsRef;
+  bool WarnedStackExhausted = false;
+};
+
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_STACK_H
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 0faa5aed4eec3b..9c846f2b6b12f0 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -49,6 +49,7 @@
 #include "clang/Basic/PragmaKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Basic/TemplateKinds.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Basic/TypeTraits.h"
@@ -546,9 +547,6 @@ class Sema final : public SemaBase {
   /// Print out statistics about the semantic analysis.
   void PrintStats() const;
 
-  /// Warn that the stack is nearly exhausted.
-  void warnStackExhausted(SourceLocation Loc);
-
   /// Run some code with "sufficient" stack space. (Currently, at least 256K is
   /// guaranteed). Produces a warning if we're low on stack space and allocates
   /// more in that case. Use this in code that may recurse deeply (for example,
@@ -1183,7 +1181,7 @@ class Sema final : public SemaBase {
   std::optional<std::unique_ptr<DarwinSDKInfo>> CachedDarwinSDKInfo;
   bool WarnedDarwinSDKInfoMissing = false;
 
-  bool WarnedStackExhausted = false;
+  SingleWarningStackAwareExecutor StackAwareExecutor;
 
   Sema(const Sema &) = delete;
   void operator=(const Sema &) = delete;
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index ee4e897b248882..2c6251ce0190c6 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -19,6 +19,7 @@
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/OpenCLOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/ExternalPreprocessorSource.h"
 #include "clang/Lex/HeaderSearch.h"
@@ -445,7 +446,7 @@ class ASTReader
   DiagnosticsEngine &Diags;
   // Sema has duplicate logic, but SemaObj can sometimes be null so ASTReader
   // has its own version.
-  bool WarnedStackExhausted = false;
+  SingleWarningStackAwareExecutor StackAwareExecutor;
 
   /// The semantic analysis object that will be processing the
   /// AST files and the translation unit that uses it.
@@ -2180,7 +2181,8 @@ class ASTReader
   /// Report a diagnostic.
   DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const;
 
-  void warnStackExhausted(SourceLocation Loc);
+  void runWithSufficientStackSpace(SourceLocation Loc,
+                                   llvm::function_ref<void()> Fn);
 
   IdentifierInfo *DecodeIdentifierInfo(serialization::IdentifierID ID);
 
diff --git a/clang/lib/Basic/Stack.cpp b/clang/lib/Basic/Stack.cpp
index aa15d8e66950fa..476612e281422b 100644
--- a/clang/lib/Basic/Stack.cpp
+++ b/clang/lib/Basic/Stack.cpp
@@ -72,3 +72,23 @@ void clang::runWithSufficientStackSpaceSlow(llvm::function_ref<void()> Diag,
     Fn();
   }, DesiredStackSize);
 }
+
+void clang::SingleWarningStackAwareExecutor::runWithSufficientStackSpace(
+    SourceLocation Loc, llvm::function_ref<void()> Fn) {
+  clang::runWithSufficientStackSpace([&] { warnStackExhausted(Loc); }, Fn);
+}
+
+void clang::SingleWarningStackAwareExecutor::warnOnStackNearlyExhausted(
+    SourceLocation Loc) {
+  if (isStackNearlyExhausted())
+    warnStackExhausted(Loc);
+}
+
+void clang::SingleWarningStackAwareExecutor::warnStackExhausted(
+    SourceLocation Loc) {
+  // Only warn about this once.
+  if (!WarnedStackExhausted) {
+    DiagsRef.Report(Loc, diag::warn_stack_exhausted);
+    WarnedStackExhausted = true;
+  }
+}
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index b05ab3606a698b..3153ec8a2d00e3 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -341,7 +341,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
     : Context(C), LangOpts(C.getLangOpts()), FS(FS), HeaderSearchOpts(HSO),
       PreprocessorOpts(PPO), CodeGenOpts(CGO), TheModule(M), Diags(diags),
       Target(C.getTargetInfo()), ABI(createCXXABI(*this)),
-      VMContext(M.getContext()), VTables(*this),
+      VMContext(M.getContext()), VTables(*this), StackAwareExecutor(diags),
       SanitizerMD(new SanitizerMetadata(*this)) {
 
   // Initialize the type cache.
@@ -1594,17 +1594,9 @@ void CodeGenModule::ErrorUnsupported(const Decl *D, const char *Type) {
   getDiags().Report(Context.getFullLoc(D->getLocation()), DiagID) << Msg;
 }
 
-void CodeGenModule::warnStackExhausted(SourceLocation Loc) {
-  // Only warn about this once.
-  if (!WarnedStackExhausted) {
-    getDiags().Report(Loc, diag::warn_stack_exhausted);
-    WarnedStackExhausted = true;
-  }
-}
-
 void CodeGenModule::runWithSufficientStackSpace(SourceLocation Loc,
                                                 llvm::function_ref<void()> Fn) {
-  clang::runWithSufficientStackSpace([&] { warnStackExhausted(Loc); }, Fn);
+  StackAwareExecutor.runWithSufficientStackSpace(Loc, Fn);
 }
 
 llvm::ConstantInt *CodeGenModule::getSize(CharUnits size) {
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index fa82a81b05dd53..f4de284369384e 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -26,6 +26,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/NoSanitizeList.h"
 #include "clang/Basic/ProfileList.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/XRayLists.h"
 #include "clang/Lex/PreprocessorOptions.h"
@@ -336,7 +337,7 @@ class CodeGenModule : public CodeGenTypeCache {
   std::unique_ptr<llvm::IndexedInstrProfReader> PGOReader;
   InstrProfStats PGOStats;
   std::unique_ptr<llvm::SanitizerStatReport> SanStats;
-  bool WarnedStackExhausted = false;
+  SingleWarningStackAwareExecutor StackAwareExecutor;
 
   // A set of references that have only been seen via a weakref so far. This is
   // used to remove the weak of the reference if we ever see a direct reference
@@ -1298,9 +1299,6 @@ class CodeGenModule : public CodeGenTypeCache {
   /// Print out an error that codegen doesn't support the specified decl yet.
   void ErrorUnsupported(const Decl *D, const char *Type);
 
-  /// Warn that the stack is nearly exhausted.
-  void warnStackExhausted(SourceLocation Loc);
-
   /// Run some code with "sufficient" stack space. (Currently, at least 256K is
   /// guaranteed). Produces a warning if we're low on stack space and allocates
   /// more in that case. Use this in code that may recurse deeply to avoid stack
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index f0d1634af529f0..edb13226758ad9 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -220,7 +220,7 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
       AnalysisWarnings(*this), ThreadSafetyDeclCache(nullptr),
       LateTemplateParser(nullptr), LateTemplateParserCleanup(nullptr),
       OpaqueParser(nullptr), CurContext(nullptr), ExternalSource(nullptr),
-      CurScope(nullptr), Ident_super(nullptr),
+      StackAwareExecutor(Diags), CurScope(nullptr), Ident_super(nullptr),
       AMDGPUPtr(std::make_unique<SemaAMDGPU>(*this)),
       ARMPtr(std::make_unique<SemaARM>(*this)),
       AVRPtr(std::make_unique<SemaAVR>(*this)),
@@ -562,17 +562,9 @@ Sema::~Sema() {
   SemaPPCallbackHandler->reset();
 }
 
-void Sema::warnStackExhausted(SourceLocation Loc) {
-  // Only warn about this once.
-  if (!WarnedStackExhausted) {
-    Diag(Loc, diag::warn_stack_exhausted);
-    WarnedStackExhausted = true;
-  }
-}
-
 void Sema::runWithSufficientStackSpace(SourceLocation Loc,
                                        llvm::function_ref<void()> Fn) {
-  clang::runWithSufficientStackSpace([&] { warnStackExhausted(Loc); }, Fn);
+  StackAwareExecutor.runWithSufficientStackSpace(Loc, Fn);
 }
 
 bool Sema::makeUnavailableInSystemHeader(SourceLocation loc,
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 8c7f694c09042e..8fb503c6f7e639 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -806,8 +806,7 @@ void Sema::pushCodeSynthesisContext(CodeSynthesisContext Ctx) {
 
   // Check to see if we're low on stack space. We can't do anything about this
   // from here, but we can at least warn the user.
-  if (isStackNearlyExhausted())
-    warnStackExhausted(Ctx.PointOfInstantiation);
+  StackAwareExecutor.warnOnStackNearlyExhausted(Ctx.PointOfInstantiation);
 }
 
 void Sema::popCodeSynthesisContext() {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ecc5d3c59a3549..70b682dcbe68c4 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -64,6 +64,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/SourceManagerInternals.h"
 #include "clang/Basic/Specifiers.h"
+#include "clang/Basic/Stack.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/TokenKinds.h"
@@ -9648,18 +9649,15 @@ DiagnosticBuilder ASTReader::Diag(SourceLocation Loc, unsigned DiagID) const {
   return Diags.Report(Loc, DiagID);
 }
 
-void ASTReader::warnStackExhausted(SourceLocation Loc) {
+void ASTReader::runWithSufficientStackSpace(SourceLocation Loc,
+                                            llvm::function_ref<void()> Fn) {
   // When Sema is available, avoid duplicate errors.
   if (SemaObj) {
-    SemaObj->warnStackExhausted(Loc);
+    SemaObj->runWithSufficientStackSpace(Loc, Fn);
     return;
   }
 
-  if (WarnedStackExhausted)
-    return;
-  WarnedStackExhausted = true;
-
-  Diag(Loc, diag::warn_stack_exhausted);
+  StackAwareExecutor.runWithSufficientStackSpace(Loc, Fn);
 }
 
 /// Retrieve the identifier table associated with the
@@ -10509,13 +10507,14 @@ ASTReader::ASTReader(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
                      bool AllowConfigurationMismatch, bool ValidateSystemInputs,
                      bool ValidateASTInputFilesContent, bool UseGlobalIndex,
                      std::unique_ptr<llvm::Timer> ReadTimer)
-    : Listener(bool(DisableValidationKind &DisableValidationForModuleKind::PCH)
+    : Listener(bool(DisableValidationKind & DisableValidationForModuleKind::PCH)
                    ? cast<ASTReaderListener>(new SimpleASTReaderListener(PP))
                    : cast<ASTReaderListener>(new PCHValidator(PP, *this))),
       SourceMgr(PP.getSourceManager()), FileMgr(PP.getFileManager()),
-      PCHContainerRdr(PCHContainerRdr), Diags(PP.getDiagnostics()), PP(PP),
-      ContextObj(Context), ModuleMgr(PP.getFileManager(), ModuleCache,
-                                     PCHContainerRdr, PP.getHeaderSearchInfo()),
+      PCHContainerRdr(PCHContainerRdr), Diags(PP.getDiagnostics()),
+      StackAwareExecutor(Diags), PP(PP), ContextObj(Context),
+      ModuleMgr(PP.getFileManager(), ModuleCache, PCHContainerRdr,
+                PP.getHeaderSearchInfo()),
       DummyIdResolver(PP), ReadTimer(std::move(ReadTimer)), isysroot(isysroot),
       DisableValidationKind(DisableValidationKind),
       AllowASTWithCompilerErrors(AllowASTWithCompilerErrors),
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 1ccc810f415eb4..d4e392dcc6bcd0 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4168,8 +4168,7 @@ Decl *ASTReader::ReadDeclRecord(GlobalDeclID ID) {
   D->setDeclContext(Context.getTranslationUnitDecl());
 
   // Reading some declarations can result in deep recursion.
-  clang::runWithSufficientStackSpace([&] { warnStackExhausted(DeclLoc); },
-                                     [&] { Reader.Visit(D); });
+  runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); });
 
   // If this declaration is also a declaration context, get the
   // offsets for its tables of lexical and visible declarations.

github-actions · 2024-10-15T14:52:04Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/include/clang/Sema/Sema.h

clang/include/clang/Basic/Stack.h

ilya-biryukov · 2024-10-16T09:06:27Z

Adding @ChuanqiXu9 to get another pair of eyes on it (for naming, code structure, etc). I believe he was involved in reviewing some changes touching runWithSufficientStackSpace I've previously sent.

Please give him a chance to review this.

ChuanqiXu9

Took a pretty quick scanning and this looks good to me.

clang/include/clang/Sema/Sema.h

bricknerb · 2024-10-16T14:27:55Z

Recreated in #112552.

bricknerb requested a review from Endilll as a code owner October 15, 2024 14:48

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:codegen IR generation bugs: mangling, exceptions, etc. labels Oct 15, 2024

ilya-biryukov reviewed Oct 16, 2024

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

ilya-biryukov reviewed Oct 16, 2024

View reviewed changes

clang/include/clang/Basic/Stack.h Outdated Show resolved Hide resolved

ilya-biryukov requested a review from ChuanqiXu9 October 16, 2024 09:05

ChuanqiXu9 reviewed Oct 16, 2024

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

bricknerb requested review from ftynse, makslevental, stellaraccident, nicolasvasilache, kuhar, banach-space, dcaballe, rengolin, JDevlieghere, nikic and a team as code owners October 16, 2024 11:25

bricknerb mentioned this pull request Oct 16, 2024

[clang] When code causes stack to be almost full in both Sema and CodeGen, two warnings are triggered #112522

Open

Endilll removed their request for review October 16, 2024 12:49

kuhar removed their request for review October 16, 2024 13:08

bricknerb closed this Oct 16, 2024

bricknerb force-pushed the warn branch from ab1f6d8 to b333edd Compare October 16, 2024 14:15

ldionne removed the request for review from a team October 16, 2024 20:00

bricknerb mentioned this pull request Oct 17, 2024

[clang] Deduplicate the logic that only warns once when stack is almost full #112552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Deduplicate the logic that only warns once when stack is almost full #112371

[clang] Deduplicate the logic that only warns once when stack is almost full #112371

Uh oh!

bricknerb commented Oct 15, 2024

Uh oh!

llvmbot commented Oct 15, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Oct 15, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov commented Oct 16, 2024

Uh oh!

ChuanqiXu9 left a comment

Uh oh!

Uh oh!

bricknerb commented Oct 16, 2024

Uh oh!

Uh oh!

[clang] Deduplicate the logic that only warns once when stack is almost full #112371

[clang] Deduplicate the logic that only warns once when stack is almost full #112371

Uh oh!

Conversation

bricknerb commented Oct 15, 2024

Uh oh!

llvmbot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov commented Oct 16, 2024

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bricknerb commented Oct 16, 2024

Uh oh!

Uh oh!

llvmbot commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading