[clang] Don't use the AST to display backend diagnostics

[aeubanks]

Summary

We keep a map from function name to source location so we don't have to
do it via looking up a source location from the AST. However, since
function names can be long, we actually use a hash of the function name
as the key.

Additionally, we can't rely on Clang's printing of function names via
the AST, so we just demangle the name instead.

This is necessary to implement
https://lists.llvm.org/pipermail/cfe-dev/2021-September/068930.html.

[dblaikie]

clang/lib/CodeGen/CodeGenAction.cpp

345–346

Could we record these earlier, partly to avoid having to do the lookup by mangled name here? (eg: when the function's created instead)

652–655

Not worth doing anything faster (hash table, etc)?

If it's going to stay linear - could use llvm::find_if perhaps - though I realize it doesn't add a /huge/ amount of value here.

If it's going to stay with the explicit loop, the Pair reference should probably be made const.

clang/test/Frontend/backend-diagnostic.c

18–21

I'm surprised these and other warning updates don't show the signature of the function, I'd have expected demangle to have returned something like 'stackSizeWarning()' rather than just 'stackSizeWarning'?

(also it seems nice to retain the word "function" and quotation marks around the function name in the diagnostic messages, I think? (could end up with weird situations where simple function names could be confused with prose in the error message))

[aeubanks]

address comments

clang/lib/CodeGen/CodeGenAction.cpp

345–346

We don't know if a function will be emitted into the IR when creating its AST node. And GetDeclForMangledName may not work if we haven't fully constructed the AST yet (not sure about this, but this would definitely require a lot of frontend knowledge).

652–655

yeah, find_if doesn't really make it any easier to read/shorter

I'm trying to optimize for construction time, not reporting time since reporting warnings/errors is much rarer. Can't get much better than a vector.

clang/test/Frontend/backend-diagnostic.c

18–21

I've added quotes.
For the word "function", sometimes the demangling prints out actual prose, e.g. in backend-stack-frame-diagnostics.cpp below, we have a invocation function for block in frameSizeBlocksWarning(), and adding "function" would print function 'invocation function for block in frameSizeBlocksWarning()'. Maybe that's fine since those are rarer, WDYT?
For C functions, I believe since there's no overloading, we don't print types/parentheses.

[rnk]

clang/lib/CodeGen/CodeGenAction.cpp

129

As David says, this could be a plain hash map, maybe hash_code isn't hashable, but it must have some way to convert to a plain integer.

This also deserves a comment about why the key here is a hash code and not the full string. The purpose of this is to avoid storing another copy of all the mangled names, which are large. In the case of a hash collision, it is possible that the wrong location will be returned. The probability is roughly #functions / size_t_max, so pretty negligible, especially for a 64-bit platform. In any case, the consequences of a collision are that we use the wrong source location, which is a diagnostic quality issue, which isn't severe.

This would be a good use case for a fast content hash, such as blake3. I believe it would be really useful for LLD to have such a fast content hash for parellelizing symbol resolution and other deduplication actions, so I think it is worth a FIXME or TODO to use a fast content hash when one is available.

651

There are LLVM passes that will rename functions, so this lookup will fail for such functions, but I guess that was already the case in the old code. Nothing to do here.

[aeubanks]

add comments (including why it's a vector and not an actual map)

[aeubanks]

ping

[dblaikie]

Sounds OK to me.

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 def note_fe_inline_asm_here : Note<"instantiated into assembly here">;
 def err_fe_source_mgr : Error<"%0">, CatSourceMgr;
 def warn_fe_source_mgr : Warning<"%0">, CatSourceMgr, InGroup<BackendSourceMgr>;
 def note_fe_source_mgr : Note<"%0">, CatSourceMgr;
 def err_fe_linking_module : Error<"cannot link module '%0': %1">, DefaultFatal;
 def warn_fe_linking_module : Warning<"linking module '%0': %1">, InGroup<LinkerWarnings>;
 def note_fe_linking_module : Note<"linking module '%0': %1">;
 
-def warn_fe_frame_larger_than : Warning<"stack frame size (%0) exceeds limit (%1) in %q2">,
+def warn_fe_frame_larger_than : Warning<"stack frame size (%0) exceeds limit (%1) in '%2'">,
     BackendInfo, InGroup<BackendFrameLargerThan>;
 def warn_fe_backend_frame_larger_than: Warning<"%0">,
     BackendInfo, InGroup<BackendFrameLargerThan>;
 def err_fe_backend_frame_larger_than: Error<"%0">, BackendInfo;
 def note_fe_backend_frame_larger_than: Note<"%0">, BackendInfo;
 
 def warn_fe_backend_plugin: Warning<"%0">, BackendInfo, InGroup<BackendPlugin>;
 def err_fe_backend_plugin: Error<"%0">, BackendInfo;

clang/lib/CodeGen/CodeGenAction.cpp

 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/CodeGen/ModuleBuilder.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/LLVMContext.h"
class BackendConsumer : public ASTConsumer {
     /// can happen when Clang plugins trigger additional AST deserialization.
     bool IRGenFinished = false;
 
     bool TimerIsEnabled = false;
 
     std::unique_ptr<CodeGenerator> Gen;
 
     SmallVector<LinkModule, 4> LinkModules;
 
+    // A map from mangled names to their function's source location, used for
+    // backend diagnostics as the Clang AST may be unavailable. We actually use
+    // the mangled name's hash as the key because mangled names can be very
+    // long and take up lots of space. Using a hash can cause name collision,
+    // but that is rare and the consequences are pointing to a wrong source
+    // location which is not severe. This is a vector instead of an actual map
+    // because we optimize for time building this map rather than time
+    // retrieving an entry, as backend diagnostics are uncommon.
+    std::vector<std::pair<llvm::hash_code, FullSourceLoc>>
+        ManglingFullSourceLocs;
+
     // This is here so that the diagnostic printer knows the module a diagnostic
     // refers to.
     llvm::Module *CurLinkModule = nullptr;
 
   public:
     BackendConsumer(BackendAction Action, DiagnosticsEngine &Diags,
                     const HeaderSearchOptions &HeaderSearchOpts,
                     const PreprocessorOptions &PPOpts,
void HandleTranslationUnit(ASTContext &C) override {
       if (OptRecordFile &&
           CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
         Ctx.setDiagnosticsHotnessRequested(true);
 
       // Link each LinkModule into our module.
       if (LinkInModules())
         return;
 
+      for (auto &F : getModule()->functions()) {
+        if (const Decl *FD = Gen->GetDeclForMangledName(F.getName())) {
+          auto Loc = FD->getASTContext().getFullLoc(FD->getLocation());
+          // TODO: use a fast content hash when available.
+          auto NameHash = llvm::hash_value(F.getName());
+          ManglingFullSourceLocs.push_back(std::make_pair(NameHash, Loc));
+        }
+      }
+
       EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef());
 
       EmitBackendOutput(Diags, HeaderSearchOpts, CodeGenOpts, TargetOpts,
                         LangOpts, C.getTargetInfo().getDataLayoutString(),
                         getModule(), Action, std::move(AsmOutStream));
 
       Ctx.setDiagnosticHandler(std::move(OldDiagnosticHandler));
 
public:
 
     /// Get the best possible source location to represent a diagnostic that
     /// may have associated debug info.
     const FullSourceLoc
     getBestLocationFromDebugLoc(const llvm::DiagnosticInfoWithLocationBase &D,
                                 bool &BadDebugInfo, StringRef &Filename,
                                 unsigned &Line, unsigned &Column) const;
 
+    Optional<FullSourceLoc> getFunctionSourceLocation(const Function &F) const;
+
     void DiagnosticHandlerImpl(const llvm::DiagnosticInfo &DI);
     /// Specialized handler for InlineAsm diagnostic.
     /// \return True if the diagnostic has been successfully reported, false
     /// otherwise.
     bool InlineAsmDiagHandler(const llvm::DiagnosticInfoInlineAsm &D);
     /// Specialized handler for diagnostics reported using SMDiagnostic.
     void SrcMgrDiagHandler(const llvm::DiagnosticInfoSrcMgr &D);
     /// Specialized handler for StackSize diagnostic.

 
 bool
 BackendConsumer::StackSizeDiagHandler(const llvm::DiagnosticInfoStackSize &D) {
   if (D.getSeverity() != llvm::DS_Warning)
     // For now, the only support we have for StackSize diagnostic is warning.
     // We do not know how to format other severities.
     return false;
 
-  if (const Decl *ND = Gen->GetDeclForMangledName(D.getFunction().getName())) {
-    // FIXME: Shouldn't need to truncate to uint32_t
-    Diags.Report(ND->getASTContext().getFullLoc(ND->getLocation()),
-                 diag::warn_fe_frame_larger_than)
-        << static_cast<uint32_t>(D.getStackSize())
-        << static_cast<uint32_t>(D.getStackLimit())
-        << Decl::castToDeclContext(ND);
-    return true;
-  }
+  auto Loc = getFunctionSourceLocation(D.getFunction());
+  if (!Loc)
+    return false;
+
+  Diags.Report(*Loc, diag::warn_fe_frame_larger_than)
+      << llvm::demangle(D.getFunction().getName().str());
 
-  return false;
-}
-
 const FullSourceLoc BackendConsumer::getBestLocationFromDebugLoc(
     const llvm::DiagnosticInfoWithLocationBase &D, bool &BadDebugInfo,
     StringRef &Filename, unsigned &Line, unsigned &Column) const {
   SourceManager &SourceMgr = Context->getSourceManager();
   FileManager &FileMgr = SourceMgr.getFileManager();
   SourceLocation DILoc;
 
   if (D.isLocationAvailable()) {
if (D.isLocationAvailable()) {
     }
     BadDebugInfo = DILoc.isInvalid();
   }
 
   // If a location isn't available, try to approximate it using the associated
   // function definition. We use the definition's right brace to differentiate
   // from diagnostics that genuinely relate to the function itself.
   FullSourceLoc Loc(DILoc, SourceMgr);
-  if (Loc.isInvalid())
-    if (const Decl *FD = Gen->GetDeclForMangledName(D.getFunction().getName()))
-      Loc = FD->getASTContext().getFullLoc(FD->getLocation());
+  if (Loc.isInvalid()) {
+    if (auto MaybeLoc = getFunctionSourceLocation(D.getFunction()))
+      Loc = *MaybeLoc;
+  }
 
   if (DILoc.isInvalid() && D.isLocationAvailable())
     // If we were not able to translate the file:line:col information
     // back to a SourceLocation, at least emit a note stating that
     // we could not translate this location. This can happen in the
     // case of #line directives.
     Diags.Report(Loc, diag::note_fe_backend_invalid_loc)
         << Filename << Line << Column;
 
   return Loc;
 }
 
+Optional<FullSourceLoc>
+BackendConsumer::getFunctionSourceLocation(const Function &F) const {
+  auto Hash = llvm::hash_value(F.getName());
+  for (const auto &Pair : ManglingFullSourceLocs) {
+    if (Pair.first == Hash)
+      return Pair.second;
+  }
+  return Optional<FullSourceLoc>();
+}
+
 void BackendConsumer::UnsupportedDiagHandler(
     const llvm::DiagnosticInfoUnsupported &D) {
   // We only support warnings or errors.
   assert(D.getSeverity() == llvm::DS_Error ||
          D.getSeverity() == llvm::DS_Warning);
 
   StringRef Filename;
   unsigned Line, Column;

clang/test/Frontend/backend-diagnostic.c

 // RUN: FileCheck < %t.err %s --check-prefix=PROMOTE --check-prefix=ASM
 // RUN: not %clang_cc1 %s -fwarn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin -Wno-frame-larger-than 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=IGNORE --check-prefix=ASM
 //
 // RUN: %clang_cc1 %s -S -o - -triple=i386-apple-darwin -verify -no-integrated-as
 
 extern void doIt(char *);
 
-// REGULAR: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in function 'stackSizeWarning'
-// PROMOTE: error: stack frame size ([[#]]) exceeds limit ([[#]]) in function 'stackSizeWarning'
-// IGNORE-NOT: stack frame size ([[#]]) exceeds limit ([[#]]) in function 'stackSizeWarning'
+// REGULAR: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
+// PROMOTE: error: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
+// IGNORE-NOT: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
 void stackSizeWarning() {
   char buffer[80];
   doIt(buffer);
 }
 
 // ASM: inline assembly requires more registers than available
 void inlineAsmError(int x0, int x1, int x2, int x3, int x4,
                     int x5, int x6, int x7, int x8, int x9) {
   __asm__("hello world": : "r" (x0),"r" (x1),"r" (x2),"r" (x3), // expected-error + {{inline assembly requires more registers than available}}
           "r" (x4),"r" (x5),"r" (x6),"r" (x7),"r" (x8),"r" (x9));
 }

clang/test/Misc/backend-stack-frame-diagnostics-fallback.cpp

 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 %s -fwarn-stack-size=0 -emit-codegen-only -triple=i386-apple-darwin 2>&1 | FileCheck %s
 
 // TODO: Emit rich diagnostics for thunks and move this into the appropriate test file.
 // Until then, test that we fall back and display the LLVM backend diagnostic.
 namespace frameSizeThunkWarning {
   struct A {
     virtual void f();
   };
 
   struct B : virtual A {
     virtual void f();
   };
 
-  // CHECK: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in function 'frameSizeThunkWarning::B::f'
+  // CHECK: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in 'frameSizeThunkWarning::B::f()'
   // CHECK: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in function '_ZTv0_n12_N21frameSizeThunkWarning1B1fEv'
   void B::f() {
     volatile int x = 0; // Ensure there is stack usage.
   }
 }

clang/test/Misc/backend-stack-frame-diagnostics.cpp

 #endif
 
 extern void doIt(char *);
 
 void frameSizeWarning(int, int) {}
 
 void frameSizeWarning();
 
-void frameSizeWarning() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in function 'frameSizeWarning'}}
+void frameSizeWarning() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in 'frameSizeWarning()'}}
   char buffer[80];
   doIt(buffer);
 }
 
 void frameSizeWarning();
 
 void frameSizeWarning(int) {}
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wframe-larger-than"
 void frameSizeWarningIgnored() {
   char buffer[80];
   doIt(buffer);
 }
 #pragma GCC diagnostic pop
 
 void frameSizeLocalClassWarning() {
   struct S {
-    S() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in function 'frameSizeLocalClassWarning()::S::S'}}
+    S() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in 'frameSizeLocalClassWarning()::S::S()'}}
       char buffer[80];
       doIt(buffer);
     }
   };
   S();
 }
 
 void frameSizeLambdaWarning() {
   auto fn =
-      []() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in lambda expression}}
+      []() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in 'frameSizeLambdaWarning()::$_0::operator()() const'}}
     char buffer[80];
     doIt(buffer);
   };
   fn();
 }
 
 void frameSizeBlocksWarning() {
   auto fn =
-      ^() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in block literal}}
+      ^() { // expected-warning-re {{stack frame size ({{[0-9]+}}) exceeds limit ({{[0-9]+}}) in 'invocation function for block in frameSizeBlocksWarning()'}}
     char buffer[80];
     doIt(buffer);
   };
   fn();
 }
 
 #else
 
 #define IS_HEADER
 #include __FILE__
 #endif