Fix compilation warnings when compiling with GCC 7.3

[aganea]

Summary

This fixes some warnings when compiling Clang & LLD on a out-of-the-box WSL/Ubuntu 18.04 shell.

[aganea]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

Fixes

[2097/2979] Building CXX object tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
/mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
/mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
     if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
        ^

clang/trunk/unittests/Tooling/LookupTest.cpp

222

Fixes

[2107/2979] Building CXX object tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterTest.cpp.o
/mnt/f/svn/clang/unittests/AST/ASTImporterTest.cpp: In member function ‘void clang::ast_matchers::RedeclChain<TypeParam>::TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition()’:
/mnt/f/svn/clang/unittests/AST/ASTImporterTest.cpp:4051:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
     if (auto *ToT = dyn_cast<TemplateDecl>(ToD))
        ^

lld/trunk/ELF/LinkerScript.cpp

892 ↗

(On Diff #196354)

Fixes

[2238/2979] Building CXX object tools/lld/ELF/CMakeFiles/lldELF.dir/LinkerScript.cpp.o
/mnt/f/svn/lld/ELF/LinkerScript.cpp: In member function ‘void lld::elf::LinkerScript::adjustSectionsBeforeSorting()’:
/mnt/f/svn/lld/ELF/LinkerScript.cpp:891:35: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
           Flags & ((Sec->NonAlloc ? 0 : SHF_ALLOC) | SHF_WRITE | SHF_EXECINSTR);
                     ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~

llvm/trunk/unittests/Support/TypeTraitsTest.cpp

95

Fixes a bunch of

[2828/2979] Building CXX object unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o
/mnt/f/svn/llvm/unittests/Support/TypeTraitsTest.cpp:20:6: warning: ‘void {anonymous}::triviality::TrivialityTester() [with T = {anonymous}::triviality::B&&; bool IsTriviallyCopyConstructible = false; bool IsTriviallyMoveConstructible = true]’ defined but not used [-Wunused-function]
 void TrivialityTester() {
      ^~~~~~~~~~~~~~~~

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

17

Fixes

[2894/2979] Building CXX object unittests/Transforms/Scalar/CMakeFiles/ScalarTests.dir/LoopPassManagerTest.cpp.o
In file included from /mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43:0,
                 from /mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
                 from /mnt/f/svn/llvm/unittests/Transforms/Scalar/LoopPassManagerTest.cpp:29:
/mnt/f/svn/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:896:11: warning: ‘testing::internal::TypedExpectation<F>::~TypedExpectation() noexcept [with F = {anonymous}::MockAnalysisHandleBase<{anonymous}::MockFunctionAnalysisHandle, llvm::Function>::Analysis::Result(llvm::Function&, llvm::AnalysisManager<llvm::Function>&)]’ declared ‘static’ but never defined [-Wunused-function]
   virtual ~TypedExpectation() {
           ^

[tstellar]

llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp

1717–1722

We don't want to have ifdefs for specific compilers, can this be fixed another way or dropped from the patch?

llvm/trunk/unittests/IR/ConstantRangeTest.cpp

462–465

Same here, no compiler specific ifdefs.

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

Same thing here too.

[MaskRay]

llvm/trunk/include/llvm/BinaryFormat/ELF.h

878 ↗

(On Diff #196358)

SHF_NONE is not defined in /usr/binclude/elf.h.

(unsigned)SHF_ALLOC is probably better.

[nikic]

llvm/trunk/unittests/IR/ConstantRangeTest.cpp

462–465

Just doing an unconditional (void)HaveInterrupt3; should be fine here. This warning is likely not specific to GCC 7 anyway.

[shafik]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

You mixed up the error messages but I see what is going on.

So you may want to add a comment since it is not apparent that what is going on is due the EXPECT_TRUE macro eventually expanding to an if..else which is what is triggering the warning. Since someone may come by in the future and just remove the braces since it is not apparent why they are there.

Same below as well.

[nikic]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

EXPECT_* inside if is quite common, I don't think we should add a comment every time it is used.

[aganea]

Adressed some of the comments.

llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp

1717–1722

Removed ifdefs, but I kept the test. This looks like an optimizer issue in GCC, however the regression tests are unaffected by this change. I will c-reduce and report it if it's still there in the latest GCC.

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

Not sure I understand, there're plenty of compiler-specific examples in the .cmake files. Is there an alternate way to fix this?

[tstellar]

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

I know there are some, but I don't think a warning like this is important enough to fix with a compiler specific work-around.

[rnk]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

This is a longstanding issue. gtest even includes this beautiful snippet of code:

// The GNU compiler emits a warning if nested "if" statements are followed by
// an "else" statement and braces are not used to explicitly disambiguate the
// "else" binding.  This leads to problems with code like:
//
//   if (gate)
//     ASSERT_*(condition) << "Some message";
//
// The "switch (0) case 0:" idiom is used to suppress this.
#ifdef __INTEL_COMPILER
# define GTEST_AMBIGUOUS_ELSE_BLOCKER_
#else
# define GTEST_AMBIGUOUS_ELSE_BLOCKER_ switch (0) case 0: default:  // NOLINT
#endif

https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h#L834

The expansion looks something like this:

if (user_cond)
  switch (0) case 0: default:
    if (const TestResult res = ...);
    else fail(res, ...) << "User streaming can follow"

It seems new versions of GCC have gotten "smarter" and this pattern no longer suppresses the warning as desired. It might be worth disabling -Wdangling-else for GCC at this point, since this will be an ongoing problem because LLVM typically elides braces when possible.

Oh, we even reported it back in 2017:
https://github.com/google/googletest/issues/1119
... and it was closed as inactive. Woohoo. :(

[aganea]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

So revert those braces and disable -Wdangling-else if CMAKE_COMPILER_IS_GNUCXX ? (somewhere in HandleLLVMOptions.cmake)

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

You mean more like

# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
if (NOT MSVC)
  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
endif()

Or leave the warning? There's already an #ifdef below in LoopPassManagerTest.cpp but it's not at the right place, this simply corrects it.

[rnk]

clang/trunk/unittests/AST/ASTImporterTest.cpp

4054

On reflection, I'd rather just add the braces and skip the build system complexity. It's what we've done in the past anyway, see rL305507 etc. I'm just annoyed at the lack of upstream gtest maintenance.

[tstellar]

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

Ok, I see in this case you are just moving the ifdef out of the code and into CMake. I really don't like either approach, but I guess this is fine. I would limit the work-around to only known broken compilers though. It looks like this might be fixed in gcc 9.

[aganea]

Please let me know if other changes are needed.

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

14–17

Scoping this issue under GCC version range [6.1, 9.0)

[aganea]

Ping! Is this good to go?

[rnk]

lgtm

The only remaining change I find questionable is the R600 backend change, but on the whole I think it's fine and you don't need to wait for more review. Better to fix the warnings and people with concerns can repaint the bikeshed as they like.

llvm/trunk/unittests/IR/ConstantRangeTest.cpp

462

I don't think a comment here is necessary, casting something to avoid before an assert is a pretty standard idiom for "suppress -Wunused-variable".

[aganea]

In D61046#1486850, @rnk wrote:

The only remaining change I find questionable is the R600 backend change

creduce'd and reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63477#c4 - it is still present with GCC 9.1

clang/trunk/unittests/AST/ASTImporterTest.cpp

TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition() {
 
     Decl *ImportedD = Import(FromD, Lang_CXX);
     Decl *ToTU = ImportedD->getTranslationUnitDecl();
 
     EXPECT_EQ(DeclCounter<DeclTy>().match(ToTU, getPattern()), 1u);
     auto *ToD = LastDeclMatcher<DeclTy>().match(ToTU, getPattern());
     EXPECT_TRUE(ImportedD == ToD);
     EXPECT_FALSE(ToD->isThisDeclarationADefinition());
-    if (auto *ToT = dyn_cast<TemplateDecl>(ToD))
+    if (auto *ToT = dyn_cast<TemplateDecl>(ToD)) {
       EXPECT_TRUE(ToT->getTemplatedDecl());
     }
+  }
 
   void TypedTest_DefinitionShouldBeImportedAsADefinition() {
     Decl *FromTU = getTuDecl(getDefinition(), Lang_CXX);
     auto *FromD = FirstDeclMatcher<DeclTy>().match(FromTU, getPattern());
     ASSERT_TRUE(FromD->isThisDeclarationADefinition());
 
     Decl *ImportedD = Import(FromD, Lang_CXX);
     Decl *ToTU = ImportedD->getTranslationUnitDecl();
 
     EXPECT_EQ(DeclCounter<DeclTy>().match(ToTU, getPattern()), 1u);
     auto *ToD = LastDeclMatcher<DeclTy>().match(ToTU, getPattern());
     EXPECT_TRUE(ToD->isThisDeclarationADefinition());
-    if (auto *ToT = dyn_cast<TemplateDecl>(ToD))
+    if (auto *ToT = dyn_cast<TemplateDecl>(ToD)) {
       EXPECT_TRUE(ToT->getTemplatedDecl());
     }
+  }
 
   void TypedTest_ImportPrototypeAfterImportedPrototype() {
     Decl *FromTU = getTuDecl(
         getPrototype() + getPrototype(), Lang_CXX);
     auto *From0 =
         FirstDeclMatcher<DeclTy>().match(FromTU, getPattern());
     auto *From1 = LastDeclMatcher<DeclTy>().match(FromTU, getPattern());
     ASSERT_FALSE(From0->isThisDeclarationADefinition());
void TypedTest_ImportDefinitions() {
     Decl *Imported1 = Import(From1, Lang_CXX);
     Decl *ToTU = Imported0->getTranslationUnitDecl();
 
     EXPECT_EQ(Imported0, Imported1);
     EXPECT_EQ(DeclCounter<DeclTy>().match(ToTU, getPattern()), 1u);
     auto *To0 = FirstDeclMatcher<DeclTy>().match(ToTU, getPattern());
     EXPECT_TRUE(Imported0 == To0);
     EXPECT_TRUE(To0->isThisDeclarationADefinition());
-    if (auto *ToT0 = dyn_cast<TemplateDecl>(To0))
+    if (auto *ToT0 = dyn_cast<TemplateDecl>(To0)) {
       EXPECT_TRUE(ToT0->getTemplatedDecl());
     }
+  }
 
   void TypedTest_ImportDefinitionThenPrototype() {
     Decl *FromTUDef = getTuDecl(getDefinition(), Lang_CXX, "input0.cc");
     Decl *FromTUProto = getTuDecl(getPrototype(), Lang_CXX, "input1.cc");
     auto *FromDef = FirstDeclMatcher<DeclTy>().match(FromTUDef, getPattern());
     auto *FromProto =
         FirstDeclMatcher<DeclTy>().match(FromTUProto, getPattern());
     ASSERT_TRUE(FromDef->isThisDeclarationADefinition());

clang/trunk/unittests/Tooling/LookupTest.cpp

TEST(LookupTest, replaceNestedClassName) {
   Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
                   "namespace c { using a::b::Foo; Foo f();; }\n");
 
   // Rename TypeLoc `x::y::Old` to new name `x::Foo` at [0] and check that the
   // type is replaced with "Foo" instead of "x::Foo". Although there is a symbol
   // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
   // it's not visible at [0].
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
-    if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+    if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old") {
       EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+    }
   };
   Visitor.runOver(R"(
     // a.h
     namespace x {
      namespace y {
       class Old {};
       class Other {};
      }

llvm/trunk/lib/Target/AMDGPU/R600ISelLowering.cpp

if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(NewBldVec[i])) {
       } else if (C->isExactlyValue(1.0)) {
         RemapSwizzle[i] = 5; // SEL_1
         NewBldVec[i] = DAG.getUNDEF(MVT::f32);
       }
     }
 
     if (NewBldVec[i].isUndef())
       continue;
+    // Fix spurious warning with gcc 7.3 -O3
+    //    warning: array subscript is above array bounds [-Warray-bounds]
+    //    if (NewBldVec[i] == NewBldVec[j]) {
+    //        ~~~~~~~~~~~^
+    if (i >= 4)
+      continue;
     for (unsigned j = 0; j < i; j++) {
       if (NewBldVec[i] == NewBldVec[j]) {
         NewBldVec[i] = DAG.getUNDEF(NewBldVec[i].getValueType());
         RemapSwizzle[i] = j;
         break;
       }
     }
   }

llvm/trunk/unittests/IR/ConstantRangeTest.cpp

EnumerateTwoConstantRanges(Bits,
           } else if (HaveRange1) {
             Upper1 = Num;
           } else {
             HaveRange1 = true;
             Lower1 = Upper1 = Num;
           }
         }
 
+        // Silence warning: variable 'HaveInterrupt3' set but not used
+        (void)HaveInterrupt3;
         assert(!HaveInterrupt3 && "Should have at most three ranges");
 
         ConstantRange SmallestCR = OpFn(CR1, CR2, ConstantRange::Smallest);
         ConstantRange UnsignedCR = OpFn(CR1, CR2, ConstantRange::Unsigned);
         ConstantRange SignedCR = OpFn(CR1, CR2, ConstantRange::Signed);
 
         if (!HaveRange1) {
           EXPECT_TRUE(SmallestCR.isEmptySet());
           EXPECT_TRUE(UnsignedCR.isEmptySet());
           EXPECT_TRUE(SignedCR.isEmptySet());

llvm/trunk/unittests/Support/TypeTraitsTest.cpp

 //===- TypeTraitsTest.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/type_traits.h"
+#include "gtest/gtest.h"
 
 namespace {
 
 // Compile-time tests using static assert.
 namespace triviality {
 
 // Helper for compile time checking trivially copy constructible and trivially
 // move constructible type traits.

 
 template void TrivialityTester<Z &, true, true>();
 template void TrivialityTester<A &, true, true>();
 template void TrivialityTester<B &, true, true>();
 template void TrivialityTester<Z &&, false, true>();
 template void TrivialityTester<A &&, false, true>();
 template void TrivialityTester<B &&, false, true>();
 
+TEST(Triviality, Tester) {
+  TrivialityTester<int, true, true>();
+  TrivialityTester<void *, true, true>();
+  TrivialityTester<int &, true, true>();
+  TrivialityTester<int &&, false, true>();
+
+  TrivialityTester<X, true, true>();
+  TrivialityTester<Y, false, false>();
+  TrivialityTester<Z, false, false>();
+  TrivialityTester<A, true, false>();
+  TrivialityTester<B, false, true>();
+
+  TrivialityTester<Z &, true, true>();
+  TrivialityTester<A &, true, true>();
+  TrivialityTester<B &, true, true>();
+  TrivialityTester<Z &&, false, true>();
+  TrivialityTester<A &&, false, true>();
+  TrivialityTester<B &&, false, true>();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace

llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Analysis
   AsmParser
   Core
   Support
   ScalarOpts
   TransformUtils
   )
 
 add_llvm_unittest(ScalarTests
   LoopPassManagerTest.cpp
   )
+
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()

llvm/trunk/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/SourceMgr.h"
 
-// Workaround for the gcc 6.1 bug PR80916.
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic push
-#  pragma GCC diagnostic ignored "-Wunused-function"
-#endif
-
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-#if defined(__GNUC__) && __GNUC__ > 5
-#  pragma GCC diagnostic pop
-#endif
-
 using namespace llvm;
 
 namespace {
 
 using testing::DoDefault;
 using testing::Return;
 using testing::Expectation;
 using testing::Invoke;