[PATCH] D123308: Handle a subtle hole in inline builtin handling
serge-sans-paille added a comment. @nickdesaulniers changes taken into account and pushed, thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123308/new/ https://reviews.llvm.org/D123308 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D123308: Handle a subtle hole in inline builtin handling
This revision was automatically updated to reflect the committed changes.
Closed by commit rG301e0d91354b: [Clang][Fortify] drop inline decls when
redeclared (authored by serge-sans-paille).
Changed prior to commit:
https://reviews.llvm.org/D123308?vs=421187=421438#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123308/new/
https://reviews.llvm.org/D123308
Files:
clang/lib/CodeGen/CGExpr.cpp
clang/test/CodeGen/fread-inline-builtin-late-redecl.c
Index: clang/test/CodeGen/fread-inline-builtin-late-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/fread-inline-builtin-late-redecl.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s |
FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an
+// external definition, even when that definition appears at the end of the
+// file.
+
+// CHECK-NOT: strlen.inline
+
+extern unsigned long strlen(char const *s);
+
+extern __inline __attribute__((__always_inline__))
__attribute__((__gnu_inline__)) unsigned long strlen(char const *s) {
+ return 1;
+}
+
+static unsigned long chesterfield(char const *s) {
+ return strlen(s);
+}
+static unsigned long (*_strlen)(char const *ptr);
+
+unsigned long blutch(char const *s) {
+ return chesterfield(s);
+}
+
+unsigned long strlen(char const *s) {
+ return _strlen(s);
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4948,6 +4948,16 @@
return EmitCall(E->getCallee()->getType(), Callee, E, ReturnValue);
}
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// everywhere. That's GCC behavior too.
+static bool OnlyHasInlineBuiltinDeclaration(const FunctionDecl *FD) {
+ for (const FunctionDecl *PD = FD; PD; PD = PD->getPreviousDecl())
+if (!PD->isInlineBuiltinDeclaration())
+ return false;
+ return true;
+}
+
static CGCallee EmitDirectCallee(CodeGenFunction , GlobalDecl GD) {
const FunctionDecl *FD = cast(GD.getDecl());
@@ -4955,8 +4965,8 @@
std::string FDInlineName = (FD->getName() + ".inline").str();
// When directing calling an inline builtin, call it through it's mangled
// name to make it clear it's not the actual builtin.
-if (FD->isInlineBuiltinDeclaration() &&
-CGF.CurFn->getName() != FDInlineName) {
+if (CGF.CurFn->getName() != FDInlineName &&
+OnlyHasInlineBuiltinDeclaration(FD)) {
llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
llvm::Function *Fn = llvm::cast(CalleePtr);
llvm::Module *M = Fn->getParent();
Index: clang/test/CodeGen/fread-inline-builtin-late-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/fread-inline-builtin-late-redecl.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an
+// external definition, even when that definition appears at the end of the
+// file.
+
+// CHECK-NOT: strlen.inline
+
+extern unsigned long strlen(char const *s);
+
+extern __inline __attribute__((__always_inline__)) __attribute__((__gnu_inline__)) unsigned long strlen(char const *s) {
+ return 1;
+}
+
+static unsigned long chesterfield(char const *s) {
+ return strlen(s);
+}
+static unsigned long (*_strlen)(char const *ptr);
+
+unsigned long blutch(char const *s) {
+ return chesterfield(s);
+}
+
+unsigned long strlen(char const *s) {
+ return _strlen(s);
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4948,6 +4948,16 @@
return EmitCall(E->getCallee()->getType(), Callee, E, ReturnValue);
}
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// everywhere. That's GCC behavior too.
+static bool OnlyHasInlineBuiltinDeclaration(const FunctionDecl *FD) {
+ for (const FunctionDecl *PD = FD; PD; PD = PD->getPreviousDecl())
+if (!PD->isInlineBuiltinDeclaration())
+ return false;
+ return true;
+}
+
static CGCallee EmitDirectCallee(CodeGenFunction , GlobalDecl GD) {
const FunctionDecl *FD = cast(GD.getDecl());
@@ -4955,8 +4965,8 @@
std::string FDInlineName = (FD->getName() + ".inline").str();
// When directing calling an inline builtin, call it through it's mangled
// name to make it clear it's not the actual builtin.
-if (FD->isInlineBuiltinDeclaration() &&
-CGF.CurFn->getName() != FDInlineName) {
+if (CGF.CurFn->getName() != FDInlineName &&
+
[PATCH] D123308: Handle a subtle hole in inline builtin handling
nickdesaulniers added a comment. Also, for the initial lines of commit messages, please try to make them less ambiguous about which parts of the project they touch. For example `[Clang][Fortify] drop inline decls when redeclared` is more descriptive should this get bucketed with other commits in a buildbot failure. The online helps triagers quickly ascertain whether failures they observe look related to which change or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123308/new/ https://reviews.llvm.org/D123308 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D123308: Handle a subtle hole in inline builtin handling
nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.
Thanks for the quick fix!
Comment at: clang/lib/CodeGen/CGExpr.cpp:4968-4969
// name to make it clear it's not the actual builtin.
-if (FD->isInlineBuiltinDeclaration() &&
+if (OnlyHasInlineBuiltinDeclaration(FD) &&
CGF.CurFn->getName() != FDInlineName) {
llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
If `OnlyHasInlineBuiltinDeclaration` returning `false` is unusual, can we get
away with switching the order of the checks here, so that `CGF.CurFn->getName()
!= FDInlineName` which might be more likely might short circuit the
conditional? That might help us avoid walking the redecl list in
`OnlyHasInlineBuiltinDeclaration` occasionally.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123308/new/
https://reviews.llvm.org/D123308
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D123308: Handle a subtle hole in inline builtin handling
serge-sans-paille created this revision.
serge-sans-paille added reviewers: nickdesaulniers, aaron.ballman, tstellar.
Herald added a project: All.
serge-sans-paille requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
When an inline builtin declaration is shadowed by an actual declaration, we must
reference the actual declaration, even if it's not the last, following GCC
behavior.
This fixes #54715
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D123308
Files:
clang/lib/CodeGen/CGExpr.cpp
clang/test/CodeGen/fread-inline-builtin-late-redecl.c
Index: clang/test/CodeGen/fread-inline-builtin-late-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/fread-inline-builtin-late-redecl.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s |
FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an
+// external definition, even when that definition appears at the end of the
+// file.
+
+// CHECK-NOT: strlen.inline
+
+extern unsigned long strlen(char const *s);
+
+extern __inline __attribute__((__always_inline__))
__attribute__((__gnu_inline__)) unsigned long strlen(char const *s) {
+ return 1;
+}
+
+static unsigned long chesterfield(char const *s) {
+ return strlen(s);
+}
+static unsigned long (*_strlen)(char const *ptr);
+
+unsigned long blutch(char const *s) {
+ return chesterfield(s);
+}
+
+unsigned long strlen(char const *s) {
+ return _strlen(s);
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4948,6 +4948,16 @@
return EmitCall(E->getCallee()->getType(), Callee, E, ReturnValue);
}
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// everywhere. That's GCC behavior too.
+static bool OnlyHasInlineBuiltinDeclaration(const FunctionDecl *FD) {
+ for (const FunctionDecl *PD = FD; PD; PD = PD->getPreviousDecl())
+if (!PD->isInlineBuiltinDeclaration())
+ return false;
+ return true;
+}
+
static CGCallee EmitDirectCallee(CodeGenFunction , GlobalDecl GD) {
const FunctionDecl *FD = cast(GD.getDecl());
@@ -4955,7 +4965,7 @@
std::string FDInlineName = (FD->getName() + ".inline").str();
// When directing calling an inline builtin, call it through it's mangled
// name to make it clear it's not the actual builtin.
-if (FD->isInlineBuiltinDeclaration() &&
+if (OnlyHasInlineBuiltinDeclaration(FD) &&
CGF.CurFn->getName() != FDInlineName) {
llvm::Constant *CalleePtr = EmitFunctionDeclPointer(CGF.CGM, GD);
llvm::Function *Fn = llvm::cast(CalleePtr);
Index: clang/test/CodeGen/fread-inline-builtin-late-redecl.c
===
--- /dev/null
+++ clang/test/CodeGen/fread-inline-builtin-late-redecl.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64 -S -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+//
+// Verifies that clang-generated *.inline are removed when shadowed by an
+// external definition, even when that definition appears at the end of the
+// file.
+
+// CHECK-NOT: strlen.inline
+
+extern unsigned long strlen(char const *s);
+
+extern __inline __attribute__((__always_inline__)) __attribute__((__gnu_inline__)) unsigned long strlen(char const *s) {
+ return 1;
+}
+
+static unsigned long chesterfield(char const *s) {
+ return strlen(s);
+}
+static unsigned long (*_strlen)(char const *ptr);
+
+unsigned long blutch(char const *s) {
+ return chesterfield(s);
+}
+
+unsigned long strlen(char const *s) {
+ return _strlen(s);
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4948,6 +4948,16 @@
return EmitCall(E->getCallee()->getType(), Callee, E, ReturnValue);
}
+// Detect the unusual situation where an inline version is shadowed by a
+// non-inline version. In that case we should pick the external one
+// everywhere. That's GCC behavior too.
+static bool OnlyHasInlineBuiltinDeclaration(const FunctionDecl *FD) {
+ for (const FunctionDecl *PD = FD; PD; PD = PD->getPreviousDecl())
+if (!PD->isInlineBuiltinDeclaration())
+ return false;
+ return true;
+}
+
static CGCallee EmitDirectCallee(CodeGenFunction , GlobalDecl GD) {
const FunctionDecl *FD = cast(GD.getDecl());
@@ -4955,7 +4965,7 @@
std::string FDInlineName = (FD->getName() + ".inline").str();
// When directing calling an inline builtin, call it through it's mangled
// name to make it clear it's not the actual builtin.
-if (FD->isInlineBuiltinDeclaration() &&
+if (OnlyHasInlineBuiltinDeclaration(FD) &&
CGF.CurFn->getName() !=
