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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:336-337
 
+      for (auto &F : getModule()->functions()) {
+        if (const Decl *FD = Gen->GetDeclForMangledName(F.getName())) {
+          auto Loc = FD->getASTContext().getFullLoc(FD->getLocation());
----------------
dblaikie wrote:
> Could we record these earlier, partly to avoid having to do the lookup by mangled name here? (eg: when the function's created instead)
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).


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:642-645
+  for (auto &Pair : ManglingFullSourceLocs) {
+    if (Pair.first == Hash)
+      return Pair.second;
+  }
----------------
dblaikie wrote:
> 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.
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.


================
Comment at: clang/test/Frontend/backend-diagnostic.c:18-20
+// 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
----------------
dblaikie wrote:
> 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))
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110665