Closed ttencate closed 1 year ago
ttencate
commented
1 year ago When I set these to use clang instead of gcc:
$ export CC=/usr/bin/clang
$ export CXX=/usr/bin/clang++
$ export RUSTFLAGS="-C link-arg=-fuse-ld=lld"... then it links successfully and tests are green! So it's definitely something to do with the C compiler/linker.
ttencate
commented
1 year ago Also tried with gcc11, which is still available in the repositories as a separate package:
$ export CC=/usr/bin/gcc-11
$ export CXX=/usr/bin/g++-11
$ export RUSTFLAGS="-C linker=/usr/bin/gcc-11"This makes it error out with "undefined reference" errors again. Which is weird, because I've been using GCC 11 for quite a while.
ttencate
commented
1 year ago It appears to be a problem with LTO (link-time optimization). HiGHS enables this by default even when producing a static library, and doesn't provide a way to disable it.
As a result of enabling LTO, libhighs.a doesn't contain actual machine code, but only a section with GCC's GIMPLE bytecode (see LTO in GCC internals doc):
$ objdump -x target/debug/build/highs-sys-21b14ce07e65289c/out/lib/libhighs.a | grep -A1 Highs_lpCall
105 .gnu.lto_Highs_lpCall.2217.e0519ffe78402bef 00000fc4 0000000000000000 0000000000000000 0010511d 2**0
CONTENTS, READONLY, EXCLUDEThe symbol Highs_lpCall is defined in the archive, but its address is zero, presumably because the actual machine code is supposed to be generated by the linker plugin:
$ nm target/debug/build/highs-sys-21b14ce07e65289c/out/lib/libhighs.a | grep Highs_lpCall
00000000 T Highs_lpCallThe same goes for the rlib archive, which is not actually linked in any way, it's just an archive of object files containing both the contents of libhighs.a and any compiled Rust code from the highs-sys crate itself (presumably as actual machine code since LTO is not enabled by default on Rust).
This setup is fundamentally broken. Cross-compiler LTO does not exist, because LTO heavily depends on compiler internals. This explains why it worked when I switched to clang: it outputs LLVM bitcode into libhighs.a, which was subsequently picked up and turned into machine code by the clang linker lld.
To turn the GIMPLE byte code produced by gcc into machine code, we would have to use the GNU linker and enable LTO while linking the final program. I didn't get this to work in a quick test, but it's undesirable anyway: it would prevent LTO optimization of Rust code, which is always compiled through LLVM and thus uses its own bitcode format.
One thing we can try is to build libhighs.a using "fat LTO", which contains both bytecode and machine code. That way, it can be linked as normal by a non-GNU linker (but doesn't participate in LTO), but gets the benefit of full cross-language LTO when compiled and linked with clang.
How do we make that happen? Let's first check what the current configuration is actually doing:
$ make -B -C target/debug/build/highs-sys-21b14ce07e65289c/out/build VERBOSE=1This shows -flto=auto -fno-fat-lto-objects being passed to the compiler. This enables LTO (auto is just to determine the number of threads), but explicitly disables fat objects. This is the result of setting CMAKE_INTERPROCEDURAL_OPTIMIZATION to TRUE, as can be seen in /usr/share/cmake/Modules/Compiler/GNU.cmake. There is currently no way to convince CMake to omit this flag when enabling IPO.
I tried adding a flag to build.rs to override -fno-fat-lto-objects:
let dst = Config::new("HiGHS")
...
.define("CMAKE_C_FLAGS", "-ffat-lto-objects")
.define("CMAKE_CXX_FLAGS", "-ffat-lto-objects")
.build();Unfortunately this flag is added before -fno-fat-lto-objects, so it gets overridden and fat LTO gets turned off again.
It looks like HiGHS prefers to compile with clang if it's available, so why is it building with gcc in the first place?
In target/debug/build/highs-sys-21b14ce07e65289c/output we see this line:
running: "cmake" "/tmp/highs-sys/HiGHS" "-DFAST_BUILD=ON" "-DSHARED=OFF" "-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL" "-DCMAKE_C_FLAGS=-ffat-lto-objects" "-DCMAKE_CXX_FLAGS=-ffat-lto-objects" "-DCMAKE_INSTALL_PREFIX=/tmp/highs-sys/target/debug/build/highs-sys-21b14ce07e65289c/out" "-DCMAKE_C_COMPILER=/usr/bin/cc" "-DCMAKE_CXX_COMPILER=/usr/bin/c++" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_ASM_COMPILER=/usr/bin/cc" "-DCMAKE_BUILD_TYPE=Debug"So this is explicitly requesting cc and c++ on the CMake command line, overriding HiGHS's preference. Why? Apparently the cmake crate is setting this and the value is taken from the cc crate, which defaults to cc/c++ but honours the CC and CXX environment variables. I already found yesterday that it works to override these, but I don't think using them in highs-sys is the right approach.
I think HiGHS should just provide a way for the user to disable LTO, instead of forcing it on if the compiler appears to support it. I chimed in on https://github.com/ERGO-Code/HiGHS/issues/1040 to get that addressed.
Whether/how highs-sys should use LTO remains open for debate.
lovasoa
commented
1 year ago Great investigation !
@jajhall , do you think this could be improved directly in highs' build process ?
jajhall
commented
1 year ago I can't say, the opinion that matters is that of @galabovaa
jetuk
commented
1 year ago I appear to be hitting this same issue after upgrading to Fedora 37. I can confirm that using @ttencate patch/branch resolves the issue for me.
jetuk
commented
1 year ago @ttencate re your comments about not compiling highs-sys with clang. If I understand correctly this would prohibit LTO across the highs-sys (Rust) - highs (C++) boundary because they would be compiled with different toolchains?
I have used your patch successfully to compile highs-sys, but now wondering if I am leaving some performance on the table because it can't do full LTO.
ttencate
commented
1 year ago @jetuk That's right; if the C++ code is compiled with gcc, there's no way to have LTO across the Rust/C++ boundary. Note that this also disables LTO between translation units within the highs library. I don't know whether that makes it much slower.
If you build the C++ code with clang (and without my patch of course!) you should be getting the full benefit of LTO. But then success or failure of compilation becomes dependent on having set the right environment variables, which is not great either.
jetuk
commented
1 year ago Thanks for the info. I'll try to do some experiments to see if it makes a significant difference.
lovasoa
commented
1 year ago I'll close this when https://github.com/ERGO-Code/HiGHS/pull/1103 is merged. Additional work will be needed if the root cause is to be addressed, but https://github.com/ERGO-Code/HiGHS/pull/1103 is enough for us to be able to disable LTO on our side.
lovasoa
commented
1 year ago Oh, I see you made a PR with the complete fix; thanks @ttencate ! I'll close this if it is merged upstream too.
ttencate
commented
1 year ago Note that the merge is targeting the latest branch. I don't know what their policy is for getting it merged into master.
jajhall
commented
1 year ago We merge latest into master periodically - but only after doing rather more rigorous performance testing than is possible on Github. It's likely to be a couple of weeks at most
ttencate
commented
1 year ago I think this can now be closed.
On Arch Linux x86_64, Rust 1.66.0 stable, g++ 12.2.0.
This happens on a clean checkout of the latest master (c785240), as well as v1.2.2, as well as v0.2.1 (which I used with great success for months through
good-lp). So it isn't a new problem inhighs-sys, and might very well be a problem with my toolchain.Versions:
Cloning and building:
All fine so far. The problem happens when we try to use the generated library, even with its own integration tests:
Similar output happens for the other integration test,
test_highs_functions, but with undefined references to allHighs_*functions used in that file.Verbose build output:
That
rustccommand, when run manually, also fails with the same error.Running the
cccommand manually fails due to/tmp/rustccbaKBt/symbols.ohaving been removed, but if I remove that from the command line, the same error occurs again.The symbol is mentioned as undefined (
U) in this file:And it gets defined (
T) in this one, which occurs later on the command line, as it should:However, all the symbols appear at address
00000000which seems suspicious to my uninformed eye.I'm honestly not sure if this is a problem with highs, but other libraries that link to C APIs are working fine for me. Any idea what more I could try?