This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fix warnings when LLD compiled using gcc 7.1.0
ClosedPublic

Authored by grimar on May 5 2017, 6:44 AM.

Details

Summary

I tried to compile LLD using gcc 7.1.0 and got 2 warnings:

[1/3] Building CXX object tools/lld/ELF/CMakeFiles/lldELF.dir/Target.cpp.o
/home/umb/LLVM/llvm/tools/lld/ELF/Target.cpp: In member function ‘virtual void lld::elf::{anonymous}::ARMTargetInfo::relocateOne(uint8_t*, uint32_t, uint64_t) const’:
/home/umb/LLVM/llvm/tools/lld/ELF/Target.cpp:1897:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
     if ((read32le(Loc) & 0xfe000000) == 0xfa000000)
     ^~
/home/umb/LLVM/llvm/tools/lld/ELF/Target.cpp:1903:3: note: here
   case R_ARM_JUMP24:
   ^~~~
/home/umb/LLVM/llvm/tools/lld/ELF/Target.cpp:1935:14: warning: this statement may fall through [-Wimplicit-fallthrough=]
     write16le(Loc + 2, (read16le(Loc + 2) & ~0x1000) | (Val & 1) << 12);
     ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/umb/LLVM/llvm/tools/lld/ELF/Target.cpp:1938:3: note: here
   case R_ARM_THM_JUMP24:
   ^~~~

Them can be fixed easily. GCC wants to see "fallthrough" word in a comment that has nothing else. People in the web also uses "/* FALLTHRU */" comment for that. Way used in this patch looks to be consistent with rest LLVM code. Patch fixes the warnings for me.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 5 2017, 6:44 AM
davide added a subscriber: davide.May 5 2017, 8:52 AM

This is an annoying warning, but I can see where they come from. Fixing this using a comment is a little fragile. I'd prefer to use an attribute if possible, but I'm not sure MSVC/clang/gcc agree.

grimar added a comment.May 5 2017, 8:54 AM

This is an annoying warning, but I can see where they come from. Fixing this using a comment is a little fragile. I'd prefer to use an attribute if possible, but I'm not sure MSVC/clang/gcc agree.

May be it is reasonable to ignore this warning for whole LLVM then ?

I dont think you should ignore. Use LLVM_FALLTHROUGH instead but make sure this compiles also on Windows/Mac. I'm afrai we need to do similar warning changes in LLVM in the future.

ruiu edited edge metadata.May 5 2017, 11:26 AM

LLVM_FALLTHROUGH is expanded to nothing if a compiler doesn't support [[fallthrough]] or [[clang::fallthrough]]. So it should compile. But I'm not sure if that fixes the issue with a gcc version that doesn't support [[fallthrough]].

In D32907#747429, @ruiu wrote:

LLVM_FALLTHROUGH is expanded to nothing if a compiler doesn't support [[fallthrough]] or [[clang::fallthrough]]. So it should compile. But I'm not sure if that fixes the issue with a gcc version that doesn't support [[fallthrough]].

GCC should have [[gnu::fallthrough]] and had that for a while, IIRC.

ruiu added a comment.May 5 2017, 11:30 AM

Thanks. Then if it doesn't work we can add that to llvm/Support/Compiler.h. But we probably should just add LLVM_FALLTHROUGH to this file and see if it will work.

grimar added a comment.EditedMay 10 2017, 5:56 AM

My investigation about this came to next results.
__cplusplus for GCC 7.1.0 looks defined to 201402L by default:

umb@umb-virtual-machine:~/LLVM/build_gdbindex$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-pc-linux-gnu/7.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --disable-multilib
Thread model: posix
gcc version 7.1.0 (GCC)
umb@umb-virtual-machine:~/LLVM/build_gdbindex$ g++ -dM -E -x c++ /dev/null | grep cplus
#define cplusplus 201402L
umb@umb-virtual-machine:~/LLVM/build_gdbindex$ g++ -std=c++14 -dM -E -x c++ /dev/null | grep cplus
#define
cplusplus 201402L
umb@umb-virtual-machine:~/LLVM/build_gdbindex$ g++ -std=c++17 -dM -E -x c++ /dev/null | grep cplus
#define __cplusplus 201703L

Code we have in Support\Compiler.h checks that __cplusplus is C++17(>201402L) when checking attribute:

/// LLVM_FALLTHROUGH - Mark fallthrough cases in switch statements.
#if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
#define LLVM_FALLTHROUGH [[fallthrough]]
#elif !__cplusplus
// Workaround for llvm.org/PR23435, since clang 3.6 and below emit a spurious
// error when __has_cpp_attribute is given a scoped attribute in C mode.
#define LLVM_FALLTHROUGH
#elif __has_cpp_attribute(clang::fallthrough)
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#else
#define LLVM_FALLTHROUGH
#endif

Initially code above was implemented without C++ standard check in r278868 and updated
to do this check in r278909 because "Clang triggers TONS of warnings about using a C++17 extension".

LLVM_FALLTHROUGH macro so far expands to nothing for me.
If I use [[fallthrough]] or [[gnu::fallthrough]] directly, this fixes the issue.
FWIW, [[fallthrough]] is supported starting from GCC 7 (https://gcc.gnu.org/projects/cxx-status.html).

For now I think we can add following part of code. I checked that it fixed issue for me:

...
#elif __has_cpp_attribute(gnu::fallthrough)
#define LLVM_FALLTHROUGH [[gnu::fallthrough]]
#else
#define LLVM_FALLTHROUGH
#endif

I am going to post it on review.

grimar updated this revision to Diff 98437.May 10 2017, 6:20 AM
  • Addressed review comments.

Together with D33036 this frees LLD from warnings when compiling with GCC 7.

ruiu accepted this revision.May 10 2017, 7:08 AM

LGTM

This revision is now accepted and ready to land.May 10 2017, 7:08 AM
This revision was automatically updated to reflect the committed changes.