[NFCI] Fix Wattributes warnings from Sema.

[erichkeane]

See: #157834

There is some visibility concerns here, so this patch suppresses the diagnostic. I believe we are doing this intentionally, so unless someone comes up with a good reason we should either remove the visibility of Sema, or make these types visible, this seems like the right way forward.

Fixes: #157834

[llvmbot]

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

See: #157834

There is some visibility concerns here, so this patch suppresses the diagnostic. I believe we are doing this intentionally, so unless someone comes up with a good reason we should either remove the visibility of Sema, or make these types visible, this seems like the right way forward.

Fixes: #157834

---

Full diff: https://github.com/llvm/llvm-project/pull/157906.diff

1 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+21)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 88b67eed5fd37..0df125c9cec9e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3541,6 +3541,10 @@ class Sema final : public SemaBase {
   llvm::SmallSetVector<const TypedefNameDecl *, 4>
       UnusedLocalTypedefNameCandidates;
 
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+#endif
   typedef LazyVector<const DeclaratorDecl *, ExternalSemaSource,
                      &ExternalSemaSource::ReadUnusedFileScopedDecls, 2, 2>
       UnusedFileScopedDeclsType;
@@ -3555,6 +3559,9 @@ class Sema final : public SemaBase {
 
   /// All the tentative definitions encountered in the TU.
   TentativeDefinitionsType TentativeDefinitions;
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
 
   /// All the external declarations encoutered and used in the TU.
   SmallVector<DeclaratorDecl *, 4> ExternalDeclarations;
@@ -4858,6 +4865,10 @@ class Sema final : public SemaBase {
   /// WeakTopLevelDeclDecls - access to \#pragma weak-generated Decls
   SmallVectorImpl<Decl *> &WeakTopLevelDecls() { return WeakTopLevelDecl; }
 
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+#endif
   typedef LazyVector<TypedefNameDecl *, ExternalSemaSource,
                      &ExternalSemaSource::ReadExtVectorDecls, 2, 2>
       ExtVectorDeclsType;
@@ -4866,6 +4877,9 @@ class Sema final : public SemaBase {
   /// us to associate a raw vector type with one of the ext_vector type names.
   /// This is only necessary for issuing pretty diagnostics.
   ExtVectorDeclsType ExtVectorDecls;
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
 
   /// Check if the argument \p E is a ASCII string literal. If not emit an error
   /// and return false, otherwise set \p Str to the value of the string literal
@@ -6466,6 +6480,10 @@ class Sema final : public SemaBase {
   /// same list more than once.
   std::unique_ptr<RecordDeclSetTy> PureVirtualClassDiagSet;
 
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+#endif
   typedef LazyVector<CXXConstructorDecl *, ExternalSemaSource,
                      &ExternalSemaSource::ReadDelegatingConstructors, 2, 2>
       DelegatingCtorDeclsType;
@@ -6473,6 +6491,9 @@ class Sema final : public SemaBase {
   /// All the delegating constructors seen so far in the file, used for
   /// cycle detection at the end of the TU.
   DelegatingCtorDeclsType DelegatingCtorDecls;
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
 
   /// The C++ "std" namespace, where the standard library resides.
   LazyDeclPtr StdNamespace;

[mstorsjo]

I believe we are doing this intentionally, so unless someone comes up with a good reason we should either remove the visibility of Sema

It's not so much that Sema has an explicit visibility - it has the default implicit visibility "default", it's just that the inline methods have visibility "hidden", and the unusual way of using a method as part of a type of a member.

Anyway, this patch doesn't manage to silence the warning - the warnings aren't emitted at this line; all 4 warnings are emitted at the definition of Sema itself.

It does get silenced in this form though:

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 88b67eed5fd3..ab47a6608247 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -847,7 +847,14 @@ void inferNoReturnAttr(Sema &S, const Decl *D);
 
 /// Sema - This implements semantic analysis and AST building for C.
 /// \nosubgrouping
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+#endif
 class Sema final : public SemaBase {
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
   // Table of Contents
   // -----------------
   // 1. Semantic Analysis (Sema.cpp)

[erichkeane]

I believe we are doing this intentionally, so unless someone comes up with a good reason we should either remove the visibility of Sema

It's not so much that Sema has an explicit visibility - it has the default implicit visibility "default", it's just that the inline methods have visibility "hidden", and the unusual way of using a method as part of a type of a member.

Anyway, this patch doesn't manage to silence the warning - the warnings aren't emitted at this line; all 4 warnings are emitted at the definition of Sema itself.

It does get silenced in this form though:

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 88b67eed5fd3..ab47a6608247 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -847,7 +847,14 @@ void inferNoReturnAttr(Sema &S, const Decl *D);
 
 /// Sema - This implements semantic analysis and AST building for C.
 /// \nosubgrouping
+#ifdef __GNUC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+#endif
 class Sema final : public SemaBase {
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif
   // Table of Contents
   // -----------------
   // 1. Semantic Analysis (Sema.cpp)

Thanks! I've done that instead. Its a shame to suppress such a large amount of diagnostics for Sema, but I think thats what we can get away with.

[mstorsjo]

Thanks for the fix!

[shafik]

Now that the original bug report is closed, the problem has not really been "solved" just silenced, right? So should there be a follow-up bug report or maybe this fix should have had a FIXME?

[mstorsjo]

Now that the original bug report is closed, the problem has not really been "solved" just silenced, right? So should there be a follow-up bug report or maybe this fix should have had a FIXME?

An explanation comment may be good in any case... But the original issue, even though it's something that GCC warns about, isn't necessarily something I'd consider an actual issue for us (in this incarnation).