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

[grimar]

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.

[davide]

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]

In D32907#747211, @davide wrote:

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 ?

[davide]

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]

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

[davide]

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]

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]

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]

• Addressed review comments.

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

[ruiu]

LGTM

lld/trunk/ELF/Target.cpp

if (Val & 1) {
                          ((Val & 2) << 23) |         // H
                          ((Val >> 2) & 0x00ffffff)); // imm24
       break;
     }
     if ((read32le(Loc) & 0xfe000000) == 0xfa000000)
       // BLX (always unconditional) instruction to an ARM Target, select an
       // unconditional BL.
       write32le(Loc, 0xeb000000 | (read32le(Loc) & 0x00ffffff));
     // fall through as BL encoding is shared with B
+    LLVM_FALLTHROUGH;
   case R_ARM_JUMP24:
   case R_ARM_PC24:
   case R_ARM_PLT32:
     checkInt<26>(Loc, Val, Type);
     write32le(Loc, (read32le(Loc) & ~0x00ffffff) | ((Val >> 2) & 0x00ffffff));
     break;
   case R_ARM_THM_JUMP11:
     checkInt<12>(Loc, Val, Type);
case R_ARM_THM_CALL:
     // value of bit 0 of Val, we must select a BL or BLX instruction
     if ((Val & 1) == 0) {
       // Ensure BLX destination is 4-byte aligned. As BLX instruction may
       // only be two byte aligned. This must be done before overflow check
       Val = alignTo(Val, 4);
     }
     // Bit 12 is 0 for BLX, 1 for BL
     write16le(Loc + 2, (read16le(Loc + 2) & ~0x1000) | (Val & 1) << 12);
     // Fall through as rest of encoding is the same as B.W
+    LLVM_FALLTHROUGH;
   case R_ARM_THM_JUMP24:
     // Encoding B  T4, BL T1, BLX T2: Val = S:I1:I2:imm10:imm11:0
     // FIXME: Use of I1 and I2 require v6T2ops
     checkInt<25>(Loc, Val, Type);
     write16le(Loc,
               0xf000 |                     // opcode
                   ((Val >> 14) & 0x0400) | // S
                   ((Val >> 12) & 0x03ff)); // imm10