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 ↗

(On Diff #196354)

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 ↗

(On Diff #196354)

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

21–1

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

6

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

7–12

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

2–5

Same here, no compiler specific ifdefs.

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

3–6

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

2–5

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 ↗

(On Diff #196354)

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 ↗

(On Diff #196354)

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

7–12

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

3–6

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

3–6

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 ↗

(On Diff #196354)

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 ↗

(On Diff #196354)

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

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

3–6

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 Diff #196354)

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

3–6

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

3–6

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

2

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

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

 //===-- R600ISelLowering.cpp - R600 DAG Lowering Implementation -----------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// Custom DAG lowering for R600
 //
 //===----------------------------------------------------------------------===//
 
 #include "R600ISelLowering.h"
 #include "AMDGPUFrameLowering.h"
 #include "AMDGPUSubtarget.h"
 #include "R600Defines.h"
 #include "R600FrameLowering.h"
 #include "R600InstrInfo.h"
 #include "R600MachineFunctionInfo.h"
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

 //===- ConstantRangeTest.cpp - ConstantRange tests ------------------------===//
 //
 // 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/IR/ConstantRange.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/KnownBits.h"
 #include "gtest/gtest.h"
EnumerateTwoConstantRanges(Bits,
           } else if (HaveRange1) {
             Upper1 = Num;
           } else {
             HaveRange1 = true;
             Lower1 = Upper1 = Num;
           }
         }
 
+        (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());

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 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.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;