[clang] Restrict Inline Builtin to non-static, non-odr linkage

[serge-sans-paille]

Summary

Inline builtins have a very special behavior compared to other
functions, it's better if we keep them restricted to a minimal set of
functions.

Add a linkage check which prevents considering ODR definitions as inline
builtins.

[jyu2]

Hi @serge-sans-paille, thanks for the fix. Could you please also try some test with dllimport/dllexport with inline function? Jennifer

clang/lib/AST/ASTContext.cpp

11538 ↗

(On Diff #514983)

How about declspec(dllimpor/dllexprort) forceinline fexprl?

[erichkeane]

clang/lib/CodeGen/CodeGenModule.cpp

5192 ↗

(On Diff #514983)

Is this an unrelated change?

[efriedma]

This seems like a weird way to fix this. The point of an "inline builtin" is that the inline function is actually the original function; it's just the inline implementation is only used in limited circumstances (in particular, it can't be used recursively). Changing the linkage could have unexpected side-effects.

Maybe it makes sense to restore some form of the "GNUInlineAttr" check in isInlineBuiltinDeclaration. But instead of actually checking for the attribute, check that the function would be emitted with available_externally linkage. So "inline builtins" don't exist on Windows because inline functions are linkonce_odr. But we still detect builtins that are declared inline instead of extern inline __attribute((gnu_inline)).

[serge-sans-paille]

In D148723#4280916, @efriedma wrote:

This seems like a weird way to fix this.

I agree, not a big fan either. But wanting to start the bike shedding in some way.

The point of an "inline builtin" is that the inline function is actually the original function; it's just the inline implementation is only used in limited circumstances (in particular, it can't be used recursively). Changing the linkage could have unexpected side-effects.

That's not my understanding. An inline builtin provides an alternate implementation of the builtin that usually wraps the original builtin in some way. It's not meant to be externally visible (it would probably collide with libc's implementation in that case).

Maybe it makes sense to restore some form of the "GNUInlineAttr" check in isInlineBuiltinDeclaration. But instead of actually checking for the attribute, check that the function would be emitted with available_externally linkage. So "inline builtins" don't exist on Windows because inline functions are linkonce_odr. But we still detect builtins that are declared inline instead of extern inline __attribute((gnu_inline)).

I'll give this some thoughts / experiments. Thanks!

[serge-sans-paille]

clang/lib/AST/ASTContext.cpp

11538 ↗

(On Diff #514983)

Out of curiosity : where does the failing code comes from? A standard library or user-specific code? I assume standard library, in that case would you mind sharing a link to the source? I'm trying to understand the meaning we would like to give to an inline builtin with external linkage but force inline attribute.

[serge-sans-paille]

clang/lib/CodeGen/CodeGenModule.cpp

5192 ↗

(On Diff #514983)

indeed ^^!

[erichkeane]

clang/lib/AST/ASTContext.cpp

11538 ↗

(On Diff #514983)

I believe it comes from the version of math.h that we ship with MKL/our compiler in our downstream. We have our own separate implementation of much of math.h.

So you can pick it up from here: https://www.intel.com/content/www/us/en/developer/tools/oneapi/dpc-compiler.html (free download at least).

[jyu2]

I think something like

#ifdef RINZOCORE_SHARED
#define RINZO_LIB declspec(dllexport)
#else
#define RINZO_LIB declspec(dllimport)
#endif

RINZO_LIB inline func() {}

[efriedma]

In D148723#4283094, @serge-sans-paille wrote:

In D148723#4280916, @efriedma wrote:

The point of an "inline builtin" is that the inline function is actually the original function; it's just the inline implementation is only used in limited circumstances (in particular, it can't be used recursively). Changing the linkage could have unexpected side-effects.

That's not my understanding. An inline builtin provides an alternate implementation of the builtin that usually wraps the original builtin in some way. It's not meant to be externally visible (it would probably collide with libc's implementation in that case).

The "linkage" here is the linkage of the declaration. Any code that refers to the declaration has to treat it like something externally visible. Off the top of my head, not sure how much it matters for C, but the linkage at the AST level has cascading effects in C++.

For normal C99 inline functions, there's a definition somewhere else, but there's an inline body which is supposed to be equivalent. Sort of a substitute for "linkonce", which doesn't normally exist in C. The issue with builtins specifically is that glibc headers do weird things, and the inline version ends up recursively calling itself.

[serge-sans-paille]

Change approach and update test case accordingly.

[efriedma]

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.

[efriedma]

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).

[serge-sans-paille]

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

[serge-sans-paille]

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 <builtin-name>.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 <builtin-name>.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.

[efriedma]

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.

[serge-sans-paille]

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.

[efriedma]

I spoke a little loosely when I said "C++ code". I meant "code using C++ 'inline' semantics", which includes C code on Windows.

[serge-sans-paille]

Add a linkage check to further restrict what's an inline builtin.

[efriedma]

LGTM

[serge-sans-paille]

@jyu2 : could you test that patch in your setup ? Also I can't find the bug id associated with your original bug anymore...

[jyu2]

In D148723#4376946, @serge-sans-paille wrote:

@jyu2 : could you test that patch in your setup ? Also I can't find the bug id associated with your original bug anymore...

I just submit bug in https://github.com/llvm/llvm-project/issues/62958 and assign that to you.

[mstorsjo]

This causes failed asserts with _FORTIFY_SOURCE with the mingw-w64 headers. Here's a reduced reproducer:

$ cat reduced.c
typedef unsigned int size_t;

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

extern __inline__ __attribute__((__always_inline__, __gnu_inline__)) __attribute__((__artificial__)) 
void *memcpy(void *__dst, const void *__src, size_t __n) 
{
  return __builtin___memcpy_chk(__dst, __src, __n, __builtin_object_size((__dst), ((0) > 0) && (2 > 1))); 
} 

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

char *a, *b;
void func(void) {
    memcpy(a, b, 42);
}
$ clang -target i686-w64-mingw32 -c reduced.c -O2
clang: ../../clang/lib/AST/Decl.cpp:3763: bool clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion `(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr<AliasAttr>()) && "Must be a function definition"' failed.

Here, the second declaration of the regular extern version of the function is what is triggering the issue.

Can we revert this to unbreak my builds until we have a fix?

[serge-sans-paille]

In D148723#4380165, @mstorsjo wrote:

This causes failed asserts with _FORTIFY_SOURCE with the mingw-w64 headers. Here's a reduced reproducer:

$ cat reduced.c
typedef unsigned int size_t;

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

extern __inline__ __attribute__((__always_inline__, __gnu_inline__)) __attribute__((__artificial__)) 
void *memcpy(void *__dst, const void *__src, size_t __n) 
{
  return __builtin___memcpy_chk(__dst, __src, __n, __builtin_object_size((__dst), ((0) > 0) && (2 > 1))); 
} 

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

char *a, *b;
void func(void) {
    memcpy(a, b, 42);
}
$ clang -target i686-w64-mingw32 -c reduced.c -O2
clang: ../../clang/lib/AST/Decl.cpp:3763: bool clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion `(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr<AliasAttr>()) && "Must be a function definition"' failed.

Here, the second declaration of the regular extern version of the function is what is triggering the issue.

Can we revert this to unbreak my builds until we have a fix?

I can't reproduce locally. Can you check your reproducer on current main?

[mstorsjo]

In D148723#4381414, @serge-sans-paille wrote:

In D148723#4380165, @mstorsjo wrote:

This causes failed asserts with _FORTIFY_SOURCE with the mingw-w64 headers. Here's a reduced reproducer:

$ cat reduced.c
typedef unsigned int size_t;

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

extern __inline__ __attribute__((__always_inline__, __gnu_inline__)) __attribute__((__artificial__)) 
void *memcpy(void *__dst, const void *__src, size_t __n) 
{
  return __builtin___memcpy_chk(__dst, __src, __n, __builtin_object_size((__dst), ((0) > 0) && (2 > 1))); 
} 

void *memcpy(void *_Dst, const void *_Src, size_t _Size);

char *a, *b;
void func(void) {
    memcpy(a, b, 42);
}
$ clang -target i686-w64-mingw32 -c reduced.c -O2
clang: ../../clang/lib/AST/Decl.cpp:3763: bool clang::FunctionDecl::isInlineDefinitionExternallyVisible() const: Assertion `(doesThisDeclarationHaveABody() || willHaveBody() || hasAttr<AliasAttr>()) && "Must be a function definition"' failed.

Here, the second declaration of the regular extern version of the function is what is triggering the issue.

Can we revert this to unbreak my builds until we have a fix?

I can't reproduce locally. Can you check your reproducer on current main?

Still reproduces at d81ce04587c006b6731198956c522c93d0df1050 (and the commit you referenced on irc). Did you use the exact command line from the repro? Note that it uses a 32 bit target triple - with a 64 bit target, you need to change the definition of size_t.

[serge-sans-paille]

Companion patch that fixes the issue reported by @mstorsjo : https://reviews.llvm.org/D151783

clang/lib/AST/Decl.cpp

bool FunctionDecl::isReplaceableGlobalAllocationFunction(
   return Params == FPT->getNumParams();
 }
 
 bool FunctionDecl::isInlineBuiltinDeclaration() const {
   if (!getBuiltinID())
     return false;
 
   const FunctionDecl *Definition;
-  return hasBody(Definition) && Definition->isInlineSpecified() &&
-         Definition->hasAttr<AlwaysInlineAttr>();
+  if (!hasBody(Definition))
+    return false;
+
+  if (!Definition->isInlineSpecified() ||
+      !Definition->hasAttr<AlwaysInlineAttr>())
+    return false;
+
+  ASTContext &Context = getASTContext();
+  switch (Context.GetGVALinkageForFunction(this)) {
+  case GVA_Internal:
+  case GVA_DiscardableODR:
+  case GVA_StrongODR:
+    return false;
+  case GVA_AvailableExternally:
+  case GVA_StrongExternal:
+    return true;
+  }
 }
 
 bool FunctionDecl::isDestroyingOperatorDelete() const {
   // C++ P0722:
   //   Within a class C, a single object deallocation function with signature
   //     (T, std::destroying_delete_t, <more params>)
   //   is a destroying operator delete.
   if (!isa<CXXMethodDecl>(this) || getOverloadedOperator() != OO_Delete ||

clang/test/CodeGen/inline-builtin-comdat.c

+// RUN: %clang_cc1 -triple x86_64-windows -S -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
+// Inline builtin are not supported for odr linkage
+// CHECK-NOT: .inline
+
+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, &i);
+    return f2;
+}