Page MenuHomePhabricator

[AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode
ClosedPublic

Authored by mstorsjo on Sep 10 2019, 1:52 PM.

Details

Summary

This matches how GCC handles it, see e.g. https://gcc.godbolt.org/z/HPplnl.

The previous behaviour of gnu_inline in C++, without the extern keyword, can be traced back to the original commit that added support for gnu_inline, SVN r69045.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Sep 10 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 1:52 PM
lib/AST/Decl.cpp
3283 ↗(On Diff #219599)

I would have thought the existing case here would handle your change. If it doesn't, why not? Should your change also remove this (essentially moving it earlier)?

3381 ↗(On Diff #219599)

Ditto.

mstorsjo marked 2 inline comments as done.Sep 12 2019, 1:44 PM
mstorsjo added inline comments.
lib/AST/Decl.cpp
3283 ↗(On Diff #219599)

With the gnu_inline attribute on a function, the block above can end up returning FoundBody = true.

So yes, I guess one would get the same effect by just moving the existing if statement up past the gnu inline block.

3381 ↗(On Diff #219599)

Hmm, if moved above, I guess we should convert the assert to a plain if (CPlusPlus) return false; instead?

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

Is Clang's current behavior actually causing problems for some real use case? Changing this behavior is not without risk, and the old behavior is certainly defensible.

If we want to match GCC's behavior for gnu_inline in C++ mode, we should think about whether we also want to permit things like:

__attribute__((__gnu_inline__)) extern inline int externinlinefunc() { return 0; }
int externinlinefunc() { return 1; }

(with or without the extern), which g++ allows and clang rejects.

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

I think it's a rather uncommon combination (I think most use of gnu_inline is together with extern), so I wouldn't think that there's a lot of users relying on the current behaviour (in clang-only codebases), but I have no data to back it up with.

Is Clang's current behavior actually causing problems for some real use case? Changing this behavior is not without risk, and the old behavior is certainly defensible.

It did come up in mingw-w64 recently; new header additions (that were developed with gcc) broke when built with clang (due to multiple definitions of a symbol). It's been rectified for now by avoiding this combination (for compatibility with existing clang versions) though.

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

I think it's a rather uncommon combination (I think most use of gnu_inline is together with extern), so I wouldn't think that there's a lot of users relying on the current behaviour (in clang-only codebases), but I have no data to back it up with.

Is Clang's current behavior actually causing problems for some real use case? Changing this behavior is not without risk, and the old behavior is certainly defensible.

It did come up in mingw-w64 recently; new header additions (that were developed with gcc) broke when built with clang (due to multiple definitions of a symbol). It's been rectified for now by avoiding this combination (for compatibility with existing clang versions) though.

OK. If this is causing compatibility pain in practice, following GCC here seems reasonable. We should at least produce a warning on gnu_inline without extern in C++ mode, though.

OK. If this is causing compatibility pain in practice, following GCC here seems reasonable. We should at least produce a warning on gnu_inline without extern in C++ mode, though.

Yes, a warning probably is good. As these things tend to be in headers, it'd be quite spammy, but it would alert the user that while the construct does work in current (future) clang versions, it doesn't in older ones, allowing users to steer clear of it.

Is there any existing DiagGroup which would be a good fit for such a warning, and what would be good wording for it?

"gnu_inline without extern in C++ treated as if externally available" (which is exactly what the GCC docs for the attribute says, which isn't too understandable unless you know the history)

"gnu_inline without extern in C++ changed behaviour in Clang 10" (doesn't say much about what changed and how)

mstorsjo updated this revision to Diff 220053.Sep 13 2019, 3:33 AM

Adapted based on the feedback so far, suggestions on naming and grouping the warning are welcome.

The warning did trigger in an existing CUDA test as well - I'm not familiar with cuda and how it relates to other languages, so suggestions on what to do wrt it, if anything, are also welcome.

mstorsjo marked an inline comment as done.Sep 13 2019, 3:36 AM
mstorsjo added inline comments.
clang/lib/AST/Decl.cpp
3334 ↗(On Diff #220053)

As this method only expects to be called for gnu_inline in C++ mode ("nline function definition in C, or for a gnu_inline function in C++" from the comment), and we're changing the C++ case to a plain if (C++) return false;, one could also consider updating the caller and making this method only be used for C mode, but maybe that should be left as a separate refactoring step (if it's worth doing at all)?

tra added a subscriber: tra.Sep 13 2019, 9:21 AM
tra added inline comments.
clang/test/SemaCUDA/gnu-inline.cu
8 ↗(On Diff #220053)

The warning did trigger in an existing CUDA test as well - I'm not familiar with cuda and how it relates to other languages, so suggestions on what to do wrt it, if anything, are also welcome.

I believe what we care about here is whether gnu_inline is handled at all.
If gnu_inline needs extern, you should add it here and revert the warning check.

mstorsjo marked an inline comment as done.Sep 13 2019, 9:24 AM
mstorsjo added inline comments.
clang/test/SemaCUDA/gnu-inline.cu
8 ↗(On Diff #220053)

Ok, that makes sense, will change it.

mstorsjo updated this revision to Diff 221086.Fri, Sep 20, 12:12 PM

Updated the CUDA test based on the suggestion.

@rsmith - What do you think of this version, the warning message, and the invariants for the isInlineDefinitionExternallyVisible method?

rsmith accepted this revision.Thu, Sep 26, 3:20 PM

Looks good to me, thanks.

This revision is now accepted and ready to land.Thu, Sep 26, 3:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 27, 5:25 AM