[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

> So if isInlineBuiltinDeclaration() simply returns false for C++ code, 
> everything should just work, without messing with the linkage.

Unfortunately not. See the example from 
`clang/test/CodeGen/inline-builtin-comdat.c` above. There's no C++ involved, 
the actual issue is that the initial Global Declacaration is stripped from its 
body which is moved to it's `.inline` suffixed version. But during codegen, if 
we don't take into account the fact that it's an inline builtin, we still put 
it in a COMDAT (because it's *inline*) while it shouldn't (because it's a 
declaration. I've uploaded a new patch that should be slightly less intrusive.


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Is there some reason we actually need to do this whole dance in C++?  The whole 
point of "inline builtins" was to handle constructs in the glibc headers that 
involve implementations of libc functions that somehow end up recursively 
calling themselves instead of a real implementation.  In C++, you can't get 
away with that sort of thing anyway: the compiler, in general, doesn't discard 
inline functions, so you can't guarantee that an external implementation will 
be used.

So if isInlineBuiltinDeclaration() simply returns false for C++ code, 
everything should just work, without messing with the linkage.


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D148723#4296111 , @efriedma wrote:

> I'm a little concerned that this will explode in unexpected ways... in 
> particular, it'll fail to link if the function doesn't actually exist 
> externally.  Which it probably doesn't if it would otherwise be linkonce_odr.

Let me summarize current approach and the problem it tries to solve in order to 
find a solution (I'm not claiming current solution is the best).

Some code define an always inline version of existing builtins. This has 
happened in the past in the case of Fortified source, and also for in some 
math.h header. Previous behavior of clang was to ignore the actual definition 
because it knew the builtin behavior and directly replaced it by a builtin 
call. This was not satisfying because this by-passed fortified implementation 
of these builtins.

What we were doing before extending the scope of what is an inline builtin was 
to detect inline, always-inline, gnu-inline functions and rename them into 
`.inline`, updating all call site (except the trivially recursive 
ones) to call this version. The original definition remained but got stripped 
of its body (so that the `.inline` version could call it in a 
non-recursive manner), turning it into a declaration. This declaration kept its 
original attribute, which made it fall into a COMDAT section which is invalid. 
What this patch is trying to achieve is to massage the original definition in 
such a way that it's compatible with the other assumptions.

Writing this I realize that I could also remove its inline / always_inline 
attribute when it's stripped of its body, that maybe enough.

What this p


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D148723#4296112 , @efriedma wrote:

> Really, I'd prefer to keep isInlineBuiltinDeclaration() targeted as narrowly 
> as possible; part of that is making it not trigger for C++ inline functions 
> (which it never did in the past).

I second that. Unfortunately the original bug showcase a situation that no 
longer holds the GNUinline attribute 
https://github.com/llvm/llvm-project/issues/61691


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Really, I'd prefer to keep isInlineBuiltinDeclaration() targeted as narrowly as 
possible; part of that is making it not trigger for C++ inline functions (which 
it never did in the past).


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm a little concerned that this will explode in unexpected ways... in 
particular, it'll fail to link if the function doesn't actually exist 
externally.  Which it probably doesn't if it would otherwise be linkonce_odr.


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

https://reviews.llvm.org/D148723

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 516645.
serge-sans-paille retitled this revision from "[clang] Enforce internal linkage 
for inline builtin" to "[clang] Enforce external linkage for inline builtin 
original declaration".
serge-sans-paille edited the summary of this revision.
serge-sans-paille added a comment.
Herald added a subscriber: mstorsjo.

Change approach and update test case accordingly.


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

https://reviews.llvm.org/D148723

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/CodeGen/asm-label-inline-builtins.c
  clang/test/CodeGen/inline-builtin-asm-name.c
  clang/test/CodeGen/inline-builtin-comdat.c
  clang/test/CodeGen/pr9614.c

Index: clang/test/CodeGen/pr9614.c
===
--- clang/test/CodeGen/pr9614.c
+++ clang/test/CodeGen/pr9614.c
@@ -30,6 +30,8 @@
   memchr("", '.', 0);
 }
 
+// CHECK-LABEL: declare i32 @abs(i32
+//
 // CHECK-LABEL: define{{.*}} void @f()
 // CHECK: call void @foo()
 // CHECK: call i32 @abs(i32 noundef %0)
@@ -37,9 +39,8 @@
 // CHECK: call void @llvm.prefetch.p0(
 // CHECK: call ptr @memchr(
 // CHECK: ret void
-
-// CHECK: declare void @foo()
-// CHECK: declare i32 @abs(i32
-// CHECK: declare ptr @strrchr(ptr noundef, i32 noundef)
-// CHECK: declare ptr @memchr(
-// CHECK: declare void @llvm.prefetch.p0(
+//
+// CHECK-LABEL: declare void @foo()
+// CHECK-LABEL: declare ptr @strrchr(ptr noundef, i32 noundef)
+// CHECK-LABEL: declare ptr @memchr(
+// CHECK-LABEL: declare void @llvm.prefetch.p0(
Index: clang/test/CodeGen/inline-builtin-comdat.c
===
--- /dev/null
+++ clang/test/CodeGen/inline-builtin-comdat.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-windows -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+// Make sure we don't generate definition for Inline Builtins, and that all
+// linkage are compatible with comdat rules.
+
+double __cdecl frexp( double _X, int* _Y);
+inline __attribute__((always_inline))  long double __cdecl frexpl( long double __x, int *__exp ) {
+  return (long double) frexp((double)__x, __exp );
+}
+
+long double pain(void)
+{
+long double f = 123.45;
+int i;
+long double f2 = frexpl(f, );
+return f2;
+}
+
+// CHECK-NOT: define {{.*}} @frexpl(
+//
+// CHECK-LABEL: declare dso_local double @frexpl(
+//
+// CHECK-LABEL: define internal double @frexpl.inline(
+// CHECK:   call double @frexp
+//
+// CHECK-LABEL: declare dso_local double @frexp(
+//
+// CHECK-LABEL: define dso_local double @pain(
+// CHECK:call double @frexpl.inline(
Index: clang/test/CodeGen/inline-builtin-asm-name.c
===
--- clang/test/CodeGen/inline-builtin-asm-name.c
+++ clang/test/CodeGen/inline-builtin-asm-name.c
@@ -1,14 +1,14 @@
 // RUN: %clang_cc1 -triple i686-windows-gnu -emit-llvm -o - %s -disable-llvm-optzns | FileCheck %s
 
-// CHECK: call i32 @"\01_asm_func_name.inline"
-
-// CHECK: declare dso_local i32 @"\01_asm_func_name"(ptr noundef, i32 noundef, ptr noundef, ptr noundef)
-
-// CHECK: define internal i32 @"\01_asm_func_name.inline"
-
+// CHECK-LABEL: declare dso_local i32 @"\01_asm_func_name"(ptr noundef, i32 noundef, ptr noundef, ptr noundef)
+//
+// CHECK-LABEL: define internal i32 @"\01_asm_func_name.inline"(
 // CHECK: call i32 @__mingw_vsnprintf
-
-// CHECK: declare dso_local i32 @__mingw_vsnprintf
+//
+// CHECK-LABEL: declare dso_local i32 @__mingw_vsnprintf
+//
+// CHECK-LABEL: define dso_local void @call(ptr noundef %fmt, ...)
+// CHECK: call i32 @"\01_asm_func_name.inline"
 
 typedef unsigned int size_t;
 
Index: clang/test/CodeGen/asm-label-inline-builtins.c
===
--- clang/test/CodeGen/asm-label-inline-builtins.c
+++ clang/test/CodeGen/asm-label-inline-builtins.c
@@ -25,8 +25,8 @@
   vprintf(fmt, ap);
 }
 
-// CHECK-LABEL: void @test(
-// CHECK: call i32 @__vprintfieee128.inline(
-//
 // CHECK-LABEL: internal i32 @__vprintfieee128.inline(
 // CHECK: call i32 @__vfprintf_chkieee128(
+//
+// CHECK-LABEL: void @test(
+// CHECK: call i32 @__vprintfieee128.inline(
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11552,6 +11552,11 @@
   if (!FD->isExternallyVisible())
 return GVA_Internal;
 
+  // Inline Builtin declaration are forward declared as external and their
+  // actual implementation is provided under the ".inline" declaration.
+  if (FD->isInlineBuiltinDeclaration())
+return GVA_StrongExternal;
+
   // Non-user-provided functions get emitted as weak definitions with every
   // use, no matter whether they've been explicitly instantiated etc.
   if (!FD->isUserProvided())
___
cfe-commits mailing list