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

[mstorsjo]

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.

[nickdesaulniers]

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]

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?

[rsmith]

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.

[mstorsjo]

In D67414#1668443, @rsmith wrote:

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.

[rsmith]

In D67414#1668475, @mstorsjo wrote:

In D67414#1668443, @rsmith wrote:

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.

[mstorsjo]

In D67414#1668579, @rsmith wrote:

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]

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]

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]

clang/test/SemaCUDA/gnu-inline.cu

8 ↗

(On Diff #220053)

In D67414#1669156, @mstorsjo wrote:

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]

clang/test/SemaCUDA/gnu-inline.cu

8 ↗

(On Diff #220053)

Ok, that makes sense, will change it.

[mstorsjo]

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]

Looks good to me, thanks.

[smeenai]

It looks like our behavior still differs from gcc in the case of a static inline __attribute__((gnu_inline)) function: https://gcc.godbolt.org/z/cY9-HQ. We emit it and gcc doesn't. I don't think that combination makes a lot of sense, but I ran into it in some internal code while testing LLVM 10.

[efriedma]

https://gcc.godbolt.org/z/cY9-HQ

gcc's behavior for your testcase makes no sense. We have to emit the definition of a static function: it can't be defined in any other translation unit because it's impossible to name in any other translation unit. Note the "_ZL" prefix. (Given the way ELF works, I guess you could get around that limitation if the function is extern "C", but still...)

[smeenai]

In D67414#1873445, @efriedma wrote:

https://gcc.godbolt.org/z/cY9-HQ

gcc's behavior for your testcase makes no sense. We have to emit the definition of a static function: it can't be defined in any other translation unit because it's impossible to name in any other translation unit. Note the "_ZL" prefix. (Given the way ELF works, I guess you could get around that limitation if the function is extern "C", but still...)

You're right. Perhaps we should just not warn for the combination of static and gnu_inline then? On my end I'm just planning to drop the gnu_inline in the internal code though, since I can't fathom a reason for wanting the combination of the two.

[efriedma]

I agree, it doesn't make sense to warn on static functions; the behavior didn't change, and there's only one reasonable result.

cfe/trunk/docs/ReleaseNotes.rst

 C11 Feature Support
 ^^^^^^^^^^^^^^^^^^^
 
 ...
 
 C++ Language Changes in Clang
 -----------------------------
 
-- ...
+- The behaviour of the `gnu_inline` attribute now matches GCC, for cases
+  where used without the `extern` keyword. As this is a change compared to
+  how it behaved in previous Clang versions, a warning is emitted for this
+  combination.
 
 C++1z Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 ...
 
 Objective-C Language Changes in Clang
 -------------------------------------

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_type_attribute_wrong_type : Warning<
   InGroup<IgnoredAttributes>;
 def warn_incomplete_encoded_type : Warning<
   "encoding of %0 type is incomplete because %1 component has unknown encoding">,
   InGroup<DiagGroup<"encode-type">>;
 def warn_gnu_inline_attribute_requires_inline : Warning<
   "'gnu_inline' attribute requires function to be marked 'inline',"
   " attribute ignored">,
   InGroup<IgnoredAttributes>;
+def warn_gnu_inline_cplusplus_without_extern : Warning<
+  "'gnu_inline' attribute without 'extern' in C++ treated as externally"
+  " available, this changed in Clang 10">,
+  InGroup<DiagGroup<"gnu-inline-cpp-without-extern">>;
 def err_attribute_vecreturn_only_vector_member : Error<
   "the vecreturn attribute can only be used on a class or structure with one member, which must be a vector">;
 def err_attribute_vecreturn_only_pod_record : Error<
   "the vecreturn attribute can only be used on a POD (plain old data) class or structure (i.e. no virtual functions)">;
 def err_cconv_change : Error<
   "function declared '%0' here was previously declared "
   "%select{'%2'|without calling convention}1">;
 def warn_cconv_unsupported : Warning<

cfe/trunk/lib/AST/Decl.cpp

bool FunctionDecl::doesDeclarationForceExternallyVisibleDefinition() const {
 
   if (Context.getLangOpts().MSVCCompat) {
     const FunctionDecl *Definition;
     if (hasBody(Definition) && Definition->isInlined() &&
         redeclForcesDefMSVC(this))
       return true;
   }
 
+  if (Context.getLangOpts().CPlusPlus)
+    return false;
+
   if (Context.getLangOpts().GNUInline || hasAttr<GNUInlineAttr>()) {
     // With GNU inlining, a declaration with 'inline' but not 'extern', forces
     // an externally visible definition.
     //
     // FIXME: What happens if gnu_inline gets added on after the first
     // declaration?
     if (!isInlineSpecified() || getStorageClass() == SC_Extern)
       return false;
while ((Prev = Prev->getPreviousDecl())) {
       } else if (Prev->isInlineSpecified() &&
                  Prev->getStorageClass() != SC_Extern) {
         return false;
       }
     }
     return FoundBody;
   }
 
-  if (Context.getLangOpts().CPlusPlus)
-    return false;
-
   // C99 6.7.4p6:
   //   [...] If all of the file scope declarations for a function in a
   //   translation unit include the inline function specifier without extern,
   //   then the definition in that translation unit is an inline definition.
   if (isInlineSpecified() && getStorageClass() != SC_Extern)
     return false;
   const FunctionDecl *Prev = this;
   bool FoundBody = false;
bool FunctionDecl::isInlineDefinitionExternallyVisible() const {
 
   if (Context.getLangOpts().GNUInline || hasAttr<GNUInlineAttr>()) {
     // Note: If you change the logic here, please change
     // doesDeclarationForceExternallyVisibleDefinition as well.
     //
     // If it's not the case that both 'inline' and 'extern' are
     // specified on the definition, then this inline definition is
     // externally visible.
+    if (Context.getLangOpts().CPlusPlus)
+      return false;
     if (!(isInlineSpecified() && getStorageClass() == SC_Extern))
       return true;
 
     // If any declaration is 'inline' but not 'extern', then this definition
     // is externally visible.
     for (auto Redecl : redecls()) {
       if (Redecl->isInlineSpecified() &&
           Redecl->getStorageClass() != SC_Extern)

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

 
 static void handleGNUInlineAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   const auto *Fn = cast<FunctionDecl>(D);
   if (!Fn->isInlineSpecified()) {
     S.Diag(AL.getLoc(), diag::warn_gnu_inline_attribute_requires_inline);
     return;
   }
 
+  if (S.LangOpts.CPlusPlus && Fn->getStorageClass() != SC_Extern)
+    S.Diag(AL.getLoc(), diag::warn_gnu_inline_cplusplus_without_extern);
+
   D->addAttr(::new (S.Context) GNUInlineAttr(S.Context, AL));
 }
 
 static void handleCallConvAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (hasDeclarator(D)) return;
 
   // Diagnostic is emitted elsewhere: here we store the (valid) AL
   // in the Decl node for syntactic reasoning, e.g., pretty-printing.

cfe/trunk/test/CodeGen/inline.c

 // CHECK2-LABEL: define available_externally void @gnu_ei_inline()
 
 // RUN: echo "C++ tests:"
 // RUN: %clang_cc1 -x c++ %s -triple i386-unknown-unknown -O1 -disable-llvm-passes -emit-llvm -o - -std=c++98 | FileCheck %s --check-prefix=CHECK3
 // RUN: %clang_cc1 -x c++ %s -triple i386-unknown-unknown -fexperimental-new-pass-manager -O1 -disable-llvm-passes -emit-llvm -o - -std=c++98 | FileCheck %s --check-prefix=CHECK3
 // CHECK3-LABEL: define i32 @_Z3barv()
 // CHECK3-LABEL: define linkonce_odr i32 @_Z3foov()
 // CHECK3-NOT: unreferenced
-// CHECK3-LABEL: define void @_Z10gnu_inlinev()
+// CHECK3-LABEL: define available_externally void @_Z10gnu_inlinev()
 // CHECK3-LABEL: define available_externally void @_Z13gnu_ei_inlinev()
 // CHECK3-NOT: @_Z5testCv
 // CHECK3-LABEL: define linkonce_odr i32 @_Z2eiv()
 
 // RUN: echo "MS C Mode tests:"
 // RUN: %clang_cc1 %s -triple i386-pc-win32 -O1 -disable-llvm-passes -emit-llvm -o - -std=c99 | FileCheck %s --check-prefix=CHECK4
 // RUN: %clang_cc1 %s -triple i386-pc-win32 -fexperimental-new-pass-manager -O1 -disable-llvm-passes -emit-llvm -o - -std=c99 | FileCheck %s --check-prefix=CHECK4
 // CHECK4-NOT: define weak_odr void @_Exit(

 
 int bar() { return foo(); }
 
 
 __inline void unreferenced1() {}
 extern __inline void unreferenced2() {}
 
 __inline __attribute((__gnu_inline__)) void gnu_inline() {}
+void (*P1)() = gnu_inline;
 
 // PR3988
 extern __inline __attribute__((gnu_inline)) void gnu_ei_inline() {}
 void (*P)() = gnu_ei_inline;
 
 // <rdar://problem/6818429>
 int test1();
 __inline int test1() { return 4; }

cfe/trunk/test/SemaCUDA/gnu-inline.cu

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 #include "Inputs/cuda.h"
 
 // expected-no-diagnostics
 
 // Check that we can handle gnu_inline functions when compiling in CUDA mode.
 
 void foo();
-inline __attribute__((gnu_inline)) void bar() { foo(); }
+extern inline __attribute__((gnu_inline)) void bar() { foo(); }

cfe/trunk/test/SemaCXX/gnu_inline.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+extern inline
+__attribute__((__gnu_inline__))
+void gnu_inline1() {}
+
+inline
+__attribute__((__gnu_inline__)) // expected-warning {{'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10}}
+void gnu_inline2() {}

cfe/trunk/test/SemaCXX/undefined-inline.cpp

 
 namespace test7 {
   void f();  // expected-warning{{inline function 'test7::f' is not defined}}
   void test() { f(); } // no used-here note.
   inline void f();
 }
 
 namespace test8 {
-  inline void foo() __attribute__((gnu_inline));
+  inline void foo() __attribute__((gnu_inline)); // expected-warning {{'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10}}
   void test() { foo(); }
 }
 
 namespace test9 {
   void foo();
   void test() { foo(); }
-  inline void foo() __attribute__((gnu_inline));
+  inline void foo() __attribute__((gnu_inline)); // expected-warning {{'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10}}
 }
 
 namespace test10 {
   inline void foo();
   void test() { foo(); }
-  inline void foo() __attribute__((gnu_inline));
+  inline void foo() __attribute__((gnu_inline)); // expected-warning {{'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10}}
 }
 
 namespace test11 {
   inline void foo() __attribute__((dllexport));
   inline void bar() __attribute__((dllimport));
   void test() { foo(); bar(); }
 }
 
 namespace test12 {
   template<typename> constexpr int _S_chk(int *);
   decltype(_S_chk<int>(nullptr)) n;
 }