[clang-repl] Fix PLT offset too large linker error on ARM

[weliveindetail]

I cross-compile clang-repl with GCC-10 on Ubuntu 20.04 and get this error when linking with gold: PLT offset too large, try linking with --long-plt

[llvmbot]

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

I cross-compile clang-repl with GCC-10 on Ubuntu 20.04 and get this error when linking with gold: PLT offset too large, try linking with --long-plt

---

Full diff: https://github.com/llvm/llvm-project/pull/78959.diff

1 Files Affected:

• (modified) clang/tools/clang-repl/CMakeLists.txt (+4)

diff --git a/clang/tools/clang-repl/CMakeLists.txt b/clang/tools/clang-repl/CMakeLists.txt
index 2ccbe292fd49e09..8589e37418354e7 100644
--- a/clang/tools/clang-repl/CMakeLists.txt
+++ b/clang/tools/clang-repl/CMakeLists.txt
@@ -22,3 +22,7 @@ clang_target_link_libraries(clang-repl PRIVATE
 if(CLANG_PLUGIN_SUPPORT)
   export_executable_symbols_for_plugins(clang-repl)
 endif()
+
+if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "ARM")
+  target_link_options(clang-repl PRIVATE LINKER:--long-plt)
+endif()

[vgvassilev]

LGTM!

[weliveindetail]

Thank for the quick review! Toolchain files seem to set a lowercase string sometimes, so I added a case-conversion.

[shiltian]

This breaks the build on macOS because the linker doesn't support --long-plt. Please fix it or revert the patch.

[weliveindetail]

Oh, that's unfortunate. Let me add a check for LLVM_USE_LINKER=gold. AFAIK it's never used on macOS.

[shiltian]

Oh, that's unfortunate. Let me add a check for LLVM_USE_LINKER=gold. AFAIK it's never used on macOS.

It might be better to check linker flag (https://cmake.org/cmake/help/latest/module/CheckLinkerFlag.html).

[weliveindetail]

@shiltian Does that fix the issue for you?

[shiltian]

@shiltian Does that fix the issue for you?

Yes, thanks!

[mstorsjo]

FYI, this change broke all of my cross compilation setups.

When cross compiling LLVM, I never have set CMAKE_SYSTEM_PROCESSOR so far, since we don't really have anything that uses it (before this), which means that this expands to an empty string. I guess I should set it still though.

Anyway, this change makes it fail with the following error:

CMake Error at /Users/mstorsjo/nightly/llvm-mingw/src/llvm-project/clang/tools/clang-repl/CMakeLists.txt:26 (string):
  string no output variable specified

I can fix it with a trivial change like this though:

diff --git a/clang/tools/clang-repl/CMakeLists.txt b/clang/tools/clang-repl/CMakeLists.txt
index 2a0f617a2c0f..031dcaba5e44 100644
--- a/clang/tools/clang-repl/CMakeLists.txt
+++ b/clang/tools/clang-repl/CMakeLists.txt
@@ -23,7 +23,7 @@ if(CLANG_PLUGIN_SUPPORT)
   export_executable_symbols_for_plugins(clang-repl)
 endif()
 
-string(TOUPPER ${CMAKE_SYSTEM_PROCESSOR} system_processor)
+string(TOUPPER "${CMAKE_SYSTEM_PROCESSOR}" system_processor)
 if(system_processor MATCHES "ARM")
   set(FLAG_LONG_PLT "-Wl,--long-plt")
   llvm_check_linker_flag(CXX ${FLAG_LONG_PLT} LINKER_HAS_FLAG_LONG_PLT)

I'll push this change within a couple hours.

[weliveindetail]

CMake does always have more surprises to offer :) Thanks @mstorsjo for fixing it right away in e3d73ad

When cross compiling LLVM, I never have set CMAKE_SYSTEM_PROCESSOR so far, since we don't really have anything that uses it (before this), which means that this expands to an empty string. I guess I should set it still though.

Yes, I am just getting used to it as well. I think it's worth noting that this should be set in a toolchain file, because CMake seems to have special handling for them. When I pass -DCMAKE_SYSTEM_PROCESSOR=ARM at configuration time, it gets overwritten with my host arch.

[mstorsjo]

When cross compiling LLVM, I never have set CMAKE_SYSTEM_PROCESSOR so far, since we don't really have anything that uses it (before this), which means that this expands to an empty string. I guess I should set it still though.

Yes, I am just getting used to it as well. I think it's worth noting that this should be set in a toolchain file, because CMake seems to have special handling for them. When I pass -DCMAKE_SYSTEM_PROCESSOR=ARM at configuration time, it gets overwritten with my host arch.

I think that's (somewhat?) expected. When you're not cross compiling, cmake explicitly sets CMAKE_SYSTEM_PROCESSOR to CMAKE_HOST_SYSTEM_PROCESSOR, ignoring whatever you set manually. (Dunno if it would make any difference if you'd set it in a toolchain file.) Only if you're cross compiling (which CMake defines as if you're setting CMAKE_SYSTEM_NAME, regardless if cross compiling from Linux to Linux on another arch), it doesn't set it and passes whatever you set originally through.

This actually makes CMAKE_SYSTEM_PROCESSOR a bit problematic; if you're on x86_64 but targeting i386, or on aarch64 but targeting arm, you might not want to consider it a cross build (as you can execute the build products) but CMake then will hide whatever you're really targeting with the info gathered from the host environment.

[weliveindetail]

Oh, I usually don't do that, but it's certainly a valid point. Can you think of a better way to express the condition here? We need -Wl,--long-plt for ARM targets whenever the used linker supports it. Otherwise we have to assume that it emits such PLTs by default.

[mstorsjo]

Oh, I usually don't do that, but it's certainly a valid point. Can you think of a better way to express the condition here? We need -Wl,--long-plt for ARM targets whenever the used linker supports it. Otherwise we have to assume that it emits such PLTs by default.

Not sure; architecture detection in CMake generally is problematic.

It's possible to infer the actual real destination architecture with a compiler test (like compiling and testing if __arm__ is defined). CMake doesn't really provide anything out of the box for that though (it does provide CMAKE_C_COMPILER_ARCHITECTURE_ID on MSVC like compilers, but not elsewhere, see https://gitlab.kitware.com/cmake/cmake/-/issues/17702), and it feels like overkill to add such a compile test here.

Also note that CMAKE_SYSTEM_PROCESSOR is unclear in Darwin universal builds anyway - if I'm compiling with -DCMAKE_OSX_ARCHITECTURES=x86_64;arm64 what will be set in CMAKE_SYSTEM_PROCESSOR? :-)

I think this current form of the check might be ok enough (I don't really know more about the issue that requires --long-plt at the moment and why it's only needed for clang-repl). If you'd want it to hit more universally, perhaps just remove the architecture check - if we test that the linker does support --long-plt without erroring out, we probably can add it, even if we don't really know the exact architecture we're linking for?

[weliveindetail]

I don't really know more about the issue that requires --long-plt at the moment and why it's only needed for clang-repl

clang-repl binary size is ~3.7G in debug mode and this seems to exceed the branch range of default ARM PLT slots. The instruction sequence that's necessary for long-range PLT slots is likely more expensive. I guess this situation is too much of an edge-case to make the effort and detect it upfront, so they just bail out if it happens an ask for the flag.

If you'd want it to hit more universally, perhaps just remove the architecture check - if we test that the linker does support --long-plt without erroring out, we probably can add it, even if we don't really know the exact architecture we're linking for?

Yes, that sounds like the better option. We give it a try later today.

[mstorsjo]

I don't really know more about the issue that requires --long-plt at the moment and why it's only needed for clang-repl

clang-repl binary size is ~3.7G in debug mode and this seems to exceed the branch range of default ARM PLT slots. The instruction sequence that's necessary for long-range PLT slots is likely more expensive. I guess this situation is too much of an edge-case to make the effort and detect it upfront, so they just bail out if it happens an ask for the flag.

Right, I see. Though - why is this only an issue for the clang-repl binary and not the other clang-based ones? Does it happen to hit some maximum when it uses both everything that is in normal clang, plus all the JIT stuff? (Normally I would expect LLDB to be the largest one, as it includes much of Clang, but that's also always split into a separate liblldb.so.)

[weliveindetail]

I was wondering as well and checked the codebase for existing uses, but no findings. Yes, clang has no JIT and may not reach the limit. And yes, LLDB is mostly an .so where the linker's approach might differ and/or it's just not built frequently for ARM. Myself, I usually opt for remote debugging and just build lldb-server.

[weliveindetail]

The above patch landed after release/18.x branched. It drops the check for CMAKE_SYSTEM_PROCESSOR as discussed in this thread and only relies on the linker-flag check.

This seems to be the right thing to do. I checked on 32-bit Raspbian @ RPi4b: It correctly defaults to triple arm-linux-gnueabihf, but CMAKE_SYSTEM_PROCESSOR is AARCH64. Thus, on current release/18.x we skip adding the --long-plt flag in this case. (It doesn't have the resources to link clang-repl, but this is still incorrect.)

It might be a backporting candidate.

[MaskRay]

I just noticed that non-arm architectures have a HAVE_LINKER_FLAG_LONG_PLT check. This is an arm ELF specific option in GNU ld (ignored by lld). You could keep the previous architecture == ARM check so that other machines don't waste time running a link job.