[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:860
+
+  Diags.Report(diag::note_fe_backend_in) << llvm::demangle(D.getCaller().str());
+
----------------
erichkeane wrote:
> Could we instead just make `demangle` take a `string_view` here?  It takes it by const-ref, which shows that it doesn't really seem to need it to be a string, so I would imagine this would be a minor refactor (to add such an overload).
Do you mean `StringRef` rather than `std::string_view`?  The progammer's manual provides guidance on the former (https://llvm.org/docs/ProgrammersManual.html#the-stringref-class) but not that latter.

Otherwise is this what you had in mind?
```
diff --git a/llvm/include/llvm/Demangle/Demangle.h b/llvm/include/llvm/Demangle/Demangle.h
index 6133d0b95bbf..c47222f883e9 100644
--- a/llvm/include/llvm/Demangle/Demangle.h
+++ b/llvm/include/llvm/Demangle/Demangle.h
@@ -11,6 +11,7 @@
 
 #include <cstddef>
 #include <string>
+#include <string_view>
 
 namespace llvm {
 /// This is a llvm local version of __cxa_demangle. Other than the name and
@@ -68,7 +69,7 @@ char *dlangDemangle(const char *MangledName);
 /// \param MangledName - reference to string to demangle.
 /// \returns - the demangled string, or a copy of the input string if no
 /// demangling occurred.
-std::string demangle(const std::string &MangledName);
+std::string demangle(const std::string_view MangledName);
 
 bool nonMicrosoftDemangle(const char *MangledName, std::string &Result);
 
diff --git a/llvm/lib/Demangle/Demangle.cpp b/llvm/lib/Demangle/Demangle.cpp
index 9d128424cabf..408112b9248e 100644
--- a/llvm/lib/Demangle/Demangle.cpp
+++ b/llvm/lib/Demangle/Demangle.cpp
@@ -26,9 +26,10 @@ static bool isDLangEncoding(const std::string &MangledName) {
          MangledName[1] == 'D';
 }
 
-std::string llvm::demangle(const std::string &MangledName) {
+std::string llvm::demangle(const std::string_view MangledName) {
   std::string Result;
-  const char *S = MangledName.c_str();
+  std::string Mangled(MangledName);
+  const char *S = Mangled.c_str();
 
   if (nonMicrosoftDemangle(S, Result))
     return Result;
@@ -43,7 +44,7 @@ std::string llvm::demangle(const std::string &MangledName) {
     return Result;
   }
 
-  return MangledName;
+  return Mangled;
 }
 
 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
```


================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:12
   foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}}
+         // expected-note@* {{In function 'baz'}}
   if (x())
----------------
aaron.ballman wrote:
> Instead of allowing this note to appear anywhere in the file, I think it's better to use "bookmark" comments. e.g.,
> ```
> void baz() { // #baz_defn
> }
> 
> void foo() {
>   baz(); // expected-note@#baz_defn {{whatever note text}}
> }
> ```
> so that we're testing where the diagnostic is emitted. (This is especially important given that the changes are about location tracking.)
oh, that's new (to me)! will do


================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:29
+               // expected-note@* {{In function 'd'}}
+               // expected-note@* {{which inlined function 'b'}}
+               // expected-note@* {{which inlined function 'a'}}
----------------
erichkeane wrote:
> Same comment re-bookmarks, but this diagnostic seems awkward.  
> 
> Should we instead say :
> 
> `call to 'foo' declared with 'error' attribute: oh no foo`
> `called by function 'a'`
> `inlined by function 'b'`
> `inlined by function 'd'`?
> 
ah, yeah, I have this backwards (in v1, I had fixed this in the alt impl https://reviews.llvm.org/D145994). Let me fix that up here now, too. Good catch!


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:2467-2468
+
+        const Function *Callee = CI->getCalledFunction();
+        if (Callee && (Callee->hasFnAttribute("dontcall-error") ||
+                       Callee->hasFnAttribute("dontcall-warn"))) {
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > arsenm wrote:
> > > > Misses constexpr casts and aliases
> > > The base feature doesn't work with aliases (or ConstantExpr), in GCC or Clang.  I should perhaps fix that first...
> > Perhaps I should use Call.getCalledOperand()->stripPointerCasts() for constantexpr case.
> I've added support for ConstantExpr to the base feature in D142058.  I should still fix this in this PR and add a test case, so not yet marking this thread as done.
marking this thread as done; see other thread below. If someone adds an alias, maybe they _intend_ to call the warning/error attributed function for [[good reason]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141451