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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67414



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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67414



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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67414



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

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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373078: [clang] [AST] Treat inline gnu_inline 
the same way as extern inline… (authored by mstorsjo, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67414?vs=221086=222145#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67414

Files:
  cfe/trunk/docs/ReleaseNotes.rst
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/Decl.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/CodeGen/inline.c
  cfe/trunk/test/SemaCUDA/gnu-inline.cu
  cfe/trunk/test/SemaCXX/gnu_inline.cpp
  cfe/trunk/test/SemaCXX/undefined-inline.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3008,6 +3008,10 @@
   "'gnu_inline' attribute requires function to be marked 'inline',"
   " attribute ignored">,
   InGroup;
+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>;
 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<
Index: cfe/trunk/test/SemaCUDA/gnu-inline.cu
===
--- cfe/trunk/test/SemaCUDA/gnu-inline.cu
+++ cfe/trunk/test/SemaCUDA/gnu-inline.cu
@@ -7,4 +7,4 @@
 // 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(); }
Index: cfe/trunk/test/SemaCXX/gnu_inline.cpp
===
--- cfe/trunk/test/SemaCXX/gnu_inline.cpp
+++ cfe/trunk/test/SemaCXX/gnu_inline.cpp
@@ -0,0 +1,9 @@
+// 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() {}
Index: cfe/trunk/test/SemaCXX/undefined-inline.cpp
===
--- cfe/trunk/test/SemaCXX/undefined-inline.cpp
+++ cfe/trunk/test/SemaCXX/undefined-inline.cpp
@@ -40,20 +40,20 @@
 }
 
 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 {
Index: cfe/trunk/test/CodeGen/inline.c
===
--- cfe/trunk/test/CodeGen/inline.c
+++ cfe/trunk/test/CodeGen/inline.c
@@ -52,7 +52,7 @@
 // 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()
@@ -85,6 +85,7 @@
 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() {}
Index: cfe/trunk/lib/AST/Decl.cpp
===
--- cfe/trunk/lib/AST/Decl.cpp
+++ cfe/trunk/lib/AST/Decl.cpp
@@ -3261,6 +3261,9 @@
   return true;
   }
 
+  if (Context.getLangOpts().CPlusPlus)
+return false;
+
   if (Context.getLangOpts().GNUInline || hasAttr()) {
 // With GNU inlining, a declaration with 'inline' but not 'extern', forces
 // an externally visible definition.
@@ -3289,9 +3292,6 @@
 return 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks.


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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 221086.
mstorsjo added a comment.

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?


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

https://reviews.llvm.org/D67414

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/inline.c
  clang/test/SemaCUDA/gnu-inline.cu
  clang/test/SemaCXX/gnu_inline.cpp
  clang/test/SemaCXX/undefined-inline.cpp

Index: clang/test/SemaCXX/undefined-inline.cpp
===
--- clang/test/SemaCXX/undefined-inline.cpp
+++ clang/test/SemaCXX/undefined-inline.cpp
@@ -40,20 +40,20 @@
 }
 
 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 {
Index: clang/test/SemaCXX/gnu_inline.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/gnu_inline.cpp
@@ -0,0 +1,9 @@
+// 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() {}
Index: clang/test/SemaCUDA/gnu-inline.cu
===
--- clang/test/SemaCUDA/gnu-inline.cu
+++ clang/test/SemaCUDA/gnu-inline.cu
@@ -7,4 +7,4 @@
 // 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(); }
Index: clang/test/CodeGen/inline.c
===
--- clang/test/CodeGen/inline.c
+++ clang/test/CodeGen/inline.c
@@ -52,7 +52,7 @@
 // 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()
@@ -85,6 +85,7 @@
 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() {}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4253,6 +4253,9 @@
 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));
 }
 
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3251,6 +3251,9 @@
   return true;
   }
 
+  if (Context.getLangOpts().CPlusPlus)
+return false;
+
   if (Context.getLangOpts().GNUInline || hasAttr()) {
 // With GNU inlining, a declaration with 'inline' but not 'extern', forces
 // an externally visible definition.
@@ -3279,9 +3282,6 @@
 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,
@@ -3361,6 +3361,8 @@
 // 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;
 
Index: 

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/test/SemaCUDA/gnu-inline.cu:8
 void foo();
-inline __attribute__((gnu_inline)) void bar() { foo(); }
+inline __attribute__((gnu_inline)) void bar() { foo(); } // expected-warning 
{{'gnu_inline' attribute without 'extern' in C++ treated as externally 
available, this changed in Clang 10}}

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


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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.



Comment at: clang/test/SemaCUDA/gnu-inline.cu:8
 void foo();
-inline __attribute__((gnu_inline)) void bar() { foo(); }
+inline __attribute__((gnu_inline)) void bar() { foo(); } // expected-warning 
{{'gnu_inline' attribute without 'extern' in C++ treated as externally 
available, this changed in Clang 10}}

tra wrote:
> >>! 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.
Ok, that makes sense, will change it.


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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.



Comment at: clang/lib/AST/Decl.cpp:3334
 /// For an inline function definition in C, or for a gnu_inline function
 /// in C++, determine whether the definition will be externally visible.
 ///

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


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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 220053.
mstorsjo added a comment.

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.


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

https://reviews.llvm.org/D67414

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/inline.c
  clang/test/SemaCUDA/gnu-inline.cu
  clang/test/SemaCXX/gnu_inline.cpp
  clang/test/SemaCXX/undefined-inline.cpp

Index: clang/test/SemaCXX/undefined-inline.cpp
===
--- clang/test/SemaCXX/undefined-inline.cpp
+++ clang/test/SemaCXX/undefined-inline.cpp
@@ -40,20 +40,20 @@
 }
 
 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 {
Index: clang/test/SemaCXX/gnu_inline.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/gnu_inline.cpp
@@ -0,0 +1,9 @@
+// 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() {}
Index: clang/test/SemaCUDA/gnu-inline.cu
===
--- clang/test/SemaCUDA/gnu-inline.cu
+++ clang/test/SemaCUDA/gnu-inline.cu
@@ -2,9 +2,7 @@
 
 #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(); }
+inline __attribute__((gnu_inline)) void bar() { foo(); } // expected-warning {{'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10}}
Index: clang/test/CodeGen/inline.c
===
--- clang/test/CodeGen/inline.c
+++ clang/test/CodeGen/inline.c
@@ -52,7 +52,7 @@
 // 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()
@@ -85,6 +85,7 @@
 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() {}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4423,6 +4423,9 @@
 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(AL.getRange(), S.Context,
AL.getAttributeSpellingListIndex()));
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3251,6 +3251,9 @@
   return true;
   }
 
+  if (Context.getLangOpts().CPlusPlus)
+return false;
+
   if (Context.getLangOpts().GNUInline || hasAttr()) {
 // With GNU inlining, a declaration with 'inline' but not 'extern', forces
 // an externally visible definition.
@@ -3279,9 +3282,6 @@
 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,
@@ 

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.



Comment at: lib/AST/Decl.cpp:3283
 
   if (Context.getLangOpts().CPlusPlus)
 return false;

nickdesaulniers wrote:
> 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)?
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.



Comment at: lib/AST/Decl.cpp:3381
   // The rest of this function is C-only.
   assert(!Context.getLangOpts().CPlusPlus &&
  "should not use C inline rules in C++");

nickdesaulniers wrote:
> Ditto.
Hmm, if moved above, I guess we should convert the assert to a plain `if 
(CPlusPlus) return false;` instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/AST/Decl.cpp:3283
 
   if (Context.getLangOpts().CPlusPlus)
 return false;

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



Comment at: lib/AST/Decl.cpp:3381
   // The rest of this function is C-only.
   assert(!Context.getLangOpts().CPlusPlus &&
  "should not use C inline rules in C++");

Ditto.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67414



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

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: dblaikie, pcc, efriedma.
Herald added a project: clang.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D67414

Files:
  lib/AST/Decl.cpp
  test/CodeGen/inline.c


Index: test/CodeGen/inline.c
===
--- test/CodeGen/inline.c
+++ test/CodeGen/inline.c
@@ -52,7 +52,7 @@
 // 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()
@@ -85,6 +85,7 @@
 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() {}
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -3257,7 +3257,8 @@
 //
 // FIXME: What happens if gnu_inline gets added on after the first
 // declaration?
-if (!isInlineSpecified() || getStorageClass() == SC_Extern)
+if (!isInlineSpecified() || getStorageClass() == SC_Extern ||
+Context.getLangOpts().CPlusPlus)
   return false;
 
 const FunctionDecl *Prev = this;
@@ -3360,6 +3361,8 @@
 // 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;
 


Index: test/CodeGen/inline.c
===
--- test/CodeGen/inline.c
+++ test/CodeGen/inline.c
@@ -52,7 +52,7 @@
 // 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()
@@ -85,6 +85,7 @@
 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() {}
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -3257,7 +3257,8 @@
 //
 // FIXME: What happens if gnu_inline gets added on after the first
 // declaration?
-if (!isInlineSpecified() || getStorageClass() == SC_Extern)
+if (!isInlineSpecified() || getStorageClass() == SC_Extern ||
+Context.getLangOpts().CPlusPlus)
   return false;
 
 const FunctionDecl *Prev = this;
@@ -3360,6 +3361,8 @@
 // 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;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits