Page MenuHomePhabricator

Fix compilation warnings when compiling with GCC 7.3
ClosedPublic

Authored by aganea on Apr 23 2019, 5:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aganea created this revision.Apr 23 2019, 5:00 PM
aganea marked 5 inline comments as done.Apr 23 2019, 5:05 PM
aganea added inline comments.
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
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() {
           ^
aganea updated this revision to Diff 196358.Apr 23 2019, 5:21 PM
tstellar added inline comments.
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 added inline comments.Apr 23 2019, 10:08 PM
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 added inline comments.Apr 24 2019, 12:30 AM
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 added inline comments.Apr 24 2019, 9:57 AM
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 added inline comments.Apr 24 2019, 10:01 AM
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 updated this revision to Diff 196505.Apr 24 2019, 1:04 PM
aganea marked 7 inline comments as done.

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 added inline comments.Apr 24 2019, 1:13 PM
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 added inline comments.Apr 24 2019, 1:57 PM
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 marked 3 inline comments as done.Apr 24 2019, 2:24 PM
aganea added inline comments.
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
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 added inline comments.Apr 24 2019, 3:07 PM
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 added inline comments.Apr 24 2019, 4:05 PM
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 updated this revision to Diff 196655.Apr 25 2019, 9:26 AM
aganea marked 3 inline comments as done.

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)

Ping! Is this good to go?

rnk accepted this revision.May 1 2019, 2:16 PM

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

This revision is now accepted and ready to land.May 1 2019, 2:16 PM
martong resigned from this revision.May 2 2019, 2:23 AM
aganea marked an inline comment as done.May 6 2019, 6:32 AM
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

This revision was automatically updated to reflect the committed changes.