[PATCH] D111009: Update inline builtin handling to honor gnu inline attribute

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

hmm...this seems to cause a bunch of ICEs building the kernel:

  Global is external, but doesn't have external or weak linkage!
  i64 (i8*)* @strlen
  fatal error: error in backend: Broken module found, compilation aborted!
  Global is external, but doesn't have external or weak linkage!
  i8* (i8*, i8*, i64)* @strncpy
  Global is external, but doesn't have external or weak linkage!
  i8* (i8*, i8*)* @strcpy
  Global is external, but doesn't have external or weak linkage!
  i64 (i8*)* @strlen
  Global is external, but doesn't have external or weak linkage!
  i64 (i8*, i8*, i64)* @strlcpy
  Global is external, but doesn't have external or weak linkage!
  i8* (i8*, i32, i64)* @memchr



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4895
+
+    // Replaceable builtin provide their own implementation of a builtin. If we
+    // are in an inline builtin implementation, avoid trivial infinite
----------------
s/Replaceable builtin/Replaceable builtins/


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4898
     // recursion.
+    StringRef FDInlineName = (FD->getName() + ".inline").str();
     if (!FD->isInlineBuiltinDeclaration() ||
----------------
When building this patch (with Clang, via cmake vars `-DCMAKE_C_COMPILER=` and `-DCMAKE_CXX_COMPILER=`), I observe the following warning:

```
[99/110] Building CXX object tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CGExpr.cpp.o
/android0/llvm-project/clang/lib/CodeGen/CGExpr.cpp:4898:30: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
    StringRef FDInlineName = (FD->getName() + ".inline").str();
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

This leads to crashes when using the resulting image built with assertions enabled.

```
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index afed918c5c7f..cb6469d58b4c 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4895,9 +4895,9 @@ static CGCallee EmitDirectCallee(CodeGenFunction &CGF, GlobalDecl GD) {
     // Replaceable builtin provide their own implementation of a builtin. If we
     // are in an inline builtin implementation, avoid trivial infinite
     // recursion.
-    StringRef FDInlineName = (FD->getName() + ".inline").str();
+    Twine FDInlineName = FD->getName() + ".inline";
     if (!FD->isInlineBuiltinDeclaration() ||
-        CGF.CurFn->getName() == FDInlineName) {
+        CGF.CurFn->getName() == FDInlineName.str()) {
       return CGCallee::forBuiltin(builtinID, FD);
     }
     // When directing calling an inline builtin, call it through it's mangled
@@ -4906,7 +4906,7 @@ static CGCallee EmitDirectCallee(CodeGenFunction &CGF, GlobalDecl GD) {
       llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
       llvm::Function *Fn = llvm::cast<llvm::Function>(CalleePtr);
       llvm::Module &M = CGF.CGM.getModule();
-      llvm::Function *Clone = M.getFunction(FDInlineName);
+      llvm::Function *Clone = M.getFunction(FDInlineName.str());
       if (!Clone) {
         Clone = llvm::Function::Create(Fn->getFunctionType(), Fn->getLinkage(),
                                        Fn->getAddressSpace(),
```


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4901
+        CGF.CurFn->getName() == FDInlineName) {
       return CGCallee::forBuiltin(builtinID, FD);
+    }
----------------
I think if you flip the order of the branches by inverting the logic in the conditional, then you could avoid needing braces for the single statement branch. ie.

```
if (FD->isInlineBuiltinDeclaration() && CGF.CurFn->getName() != FDInlineName) {
  llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
  ...
} else
  return CGCallee::forBuiltin(builtinID, FD);
```


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4908
+      llvm::Function *Fn = llvm::cast<llvm::Function>(CalleePtr);
+      llvm::Module &M = CGF.CGM.getModule();
+      llvm::Function *Clone = M.getFunction(FDInlineName);
----------------
If you have a handle to a `Function`, you can get a pointer to it's module via `Function::getParent()`.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4913
+                                       Fn->getAddressSpace(),
+                                       FD->getName() + ".inline", &M);
+        Clone->addFnAttr(llvm::Attribute::AlwaysInline);
----------------
Can we reuse `FDInlineName` here for the 4th param?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1302
+  if (FD->isInlineBuiltinDeclaration() && Fn) {
+    llvm::Module &M = CGM.getModule();
+    llvm::Function *Clone = M.getFunction((Fn->getName() + ".inline").str());
----------------
could use `Fn->getParent()` here, too.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1307
+                                     Fn->getAddressSpace(),
+                                     (Fn->getName() + ".inline").str(), &M);
+      Clone->addFnAttr(llvm::Attribute::AlwaysInline);
----------------
`Fn->getName() + ".inline"` appears twice. Want to save it in a local variable then reuse it, similar to what you do above in clang/lib/CodeGen/CGExpr.cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111009