This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by serge-sans-paille on Apr 19 2023, 9:12 AM.

Details

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 9:12 AM
serge-sans-paille requested review of this revision.Apr 19 2023, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 9:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jyu2 added a comment.Apr 19 2023, 9:58 AM

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 added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
5192 ↗(On Diff #514983)

Is this an unrelated change?

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

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!

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.

clang/lib/CodeGen/CodeGenModule.cpp
5192 ↗(On Diff #514983)

indeed ^^!

erichkeane added inline comments.Apr 20 2023, 8:30 AM
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 added a comment.Apr 20 2023, 8:46 AM

I think something like

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

RINZO_LIB inline func() {}

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 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. (Show Details)

Change approach and update test case accordingly.

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.

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

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 added a comment.EditedMay 2 2023, 9:20 AM

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.

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.

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.

serge-sans-paille retitled this revision from [clang] Enforce external linkage for inline builtin original declaration to [clang] Fix comdat for InlineBuiltin declarations.
serge-sans-paille edited the summary of this revision. (Show Details)

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 retitled this revision from [clang] Fix comdat for InlineBuiltin declarations to [clang] Restrict Inline Builtin to non-static, non-odr linkage.
serge-sans-paille edited the summary of this revision. (Show Details)

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

This revision is now accepted and ready to land.May 26 2023, 10:03 AM

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

jyu2 added a comment.May 26 2023, 1:37 PM

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

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?

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?

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.