C++20 support - Build with patched header file from llvm-11 distrib (STLExtras.h)

[greg7mdp]

Resolves #1121.
This is to address compilation issues with gcc-12 when enabling c++20 compatibility (-std=c++20).
One header from the llvm-11 distribution (STLExtra.h) has redundant qualified template-id references causing the following errors:

/usr/include/llvm-11/llvm/ADT/STLExtras.h:1790:29: error: expected ‘)’ before ‘Index’
 1790 |   result_pair<R>(std::size_t Index, IterOfRange<R> Iter)
      |                 ~           ^~~~~~
      |                             )

This PR adds a patched STLExtra.h file in libraries/chain/llvm_11_patch and updates the CMakeLists.txt to search this directory first.

The change in STLExtras.h just removes the extra <R> in 4 locations. It is purely a syntactic change.

Also, leap now requires exactly llvm-11 instead of any version between 7 and 11 (change in top CMakeLists.txt).

This is a temporary fix, until we update leap to support llvm-13 or above.

[spoonincode]

Since LLVM usage is so isolated, I would prefer us to just compile the problematic files with c++17 until we decide what to do about #692 as that will change the versions we support anyways.

I also don't really like dropping support for 7-10 without going and cleaning up all the cruft that supports those versions.

If going with this approach I would rather see STLExtras.h patched instead of copied in to our repo wholesale. I had to go double check 11.0.0, 11.0.1, and 11.1.0 STLExtras.h were all the same (they are), but there is no guarantee that an llvm in a distro's package hasn't been modified from upstream.

[greg7mdp]

Since LLVM usage is so isolated, I would prefer us to just compile the problematic files with c++17 until we decide what to do about AntelopeIO/spring#578 as that will change the versions we support anyways.

I tried that, but it doesn't work, because we end up mixing libraries compiled with C++17 and C++20 in a link command, and those libraries are incompatible because they use different versions of <span> for example (C++17 uses the libfc emulation of std::span)

I also don't really like dropping support for 7-10 without going and cleaning up all the cruft that supports those versions.

I don't mind cleaning that up.

[greg7mdp]

If going with this approach I would rather see STLExtras.h patched instead of copied in to our repo wholesale. I had to go double check 11.0.0, 11.0.1, and 11.1.0 STLExtras.h were all the same (they are), but there is no guarantee that an llvm in a distro's package hasn't been modified from upstream.

Now that llvm is releasing version 17, it doesn't seem very likely that they would make changes in the STLExtras.h of version 11. And I don't think patching system provided headers is very nice either, so I'm not convinced that it would be preferable to this proposal.

Also we plan on updating the LLVM support before leap 5.0 is released, which would make this whole issue disappear (since STLExtra.h was fixed in LLVM versions 13 and above).

[spoonincode]

This issue will remain as long as we consider Ubuntu 20 a primary target because llvm 12 is the last llvm package in Ubuntu 20's package repo.

Ubuntu 20's llvm-11 package build has 96 patch files included in it (I don't know if they're all used), but that's a good indicator that a distro may be changing upstream and why we should just patch the file instead.

[spoonincode]

This shouldn't be required

[greg7mdp]

OK, removing it.

[greg7mdp]

This shouldn't be required

Only reason I added this is that when you install llvm-dev on ubuntu, it installs llvm-10 by default. So why can we expect llvm-11 to be present?

[spoonincode]

Zach had the same confusion around this script; it really ought to be called install_deps_for_pinned_build.sh, as that's its purpose. Notice how it doesn't install boost either.

[greg7mdp]

I see. llvm is installed in scripts/pinned_build.sh. Unfortunately it installs version 7.0.1.

[greg7mdp]

This issue will remain as long as we consider Ubuntu 20 a primary target because llvm 12 is the last llvm package in Ubuntu 20's package repo.

Ubuntu 20's llvm-11 package build has 96 patch files included in it (I don't know if they're all used), but that's a good indicator that a distro may be changing upstream and why we should just patch the file instead.

Where would you suggest I add a command patching this file?

[spoonincode]

Maybe it ends up looking something like

find_package(Patch REQUIRED)  #good thing is that patch looks like it's included with build-essential
file(COPY ${LLVM_INCLUDE_DIRS}/llvm/ADT/STLExtras.h DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/override/)
add_custom_command(COMMAND ${Patch_EXECUTABLE} ARGS ${CMAKE_CURRENT_BINARY_DIR}/override/STLExtras.h ${CMAKE_CURRENT_SOURCE_DIR}/the.patch)

of course with the proper dependency tracking added.

off hand not sure if there is a better way with cmake or not.

[spoonincode]

I tried that, but it doesn't work, because we end up mixing libraries compiled with C++17 and C++20 in a link command, and those libraries are incompatible because they use different versions of <span> for example (C++17 uses the libfc emulation of std::span)

There is not anything inherently wrong with linking object files compiled with different versions of C++ -- after all, those LLVM libraries we are linking to were compiled with C++14.

LLVMJIT.cpp & LLVMEmitIR.cpp look to be the only two files that use LLVM. LLVMJIT.cpp has a dependency on fc that can likely be fairly easily refactored out. LLVMEmitIR.cpp has dependency on memory.hpp and intrinsic.hpp: memory.hpp might need a wee bit of refactoring to shake off its eosio/chain includes; intrinsic.hpp only pulls in intrinsic_mapping.hpp which looks safe already.

Admittedly this is just from a cursory look, but it seems quite possible that refactoring the LLVM usage to be isolated enough to switch these two files to c++17 isn't too bad.

[greg7mdp]

Matt, since we plan to update llvm to a newer version (>= llvm-13) before the 5.0 release, I don't think there is much benefit to try refactoring out the current implementation which uses older llvm versions.

[spoonincode]

I don't see how we can require llvm >= 13 since llvm 12 is the max that comes with ubuntu 20. Or, are you thinking we drop ubuntu 20 for 5.0?

[greg7mdp]

I don't see how we can require llvm >= 13 since llvm 12 is the max that comes with ubuntu 20. Or, are you thinking we drop ubuntu 20 for 5.0?

I believe it is possible to install llvm-13-dev on ubuntu 20. Actually any version of llvm.

[spoonincode]

It's not in the standard package repo, for example,

$ docker run --rm -it ubuntu:focal
# apt-get update
# apt-get install llvm-13-dev
Reading package lists... Done
Building dependency tree
Reading state information... Done
E: Unable to locate package llvm-13-dev

[greg7mdp]

Yes, I know it is not in the standard package repo. It still can be installed.

$ docker run -ti --rm ubuntu:20.04
# apt-get update
# apt-get install lsb-release wget software-properties-common gnupg
# bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)"
# ls -ld /usr/include/llvm-*
drwxr-xr-x 3 root root 4096 May  1 21:04 /usr/include/llvm-15
drwxr-xr-x 3 root root 4096 May  1 21:04 /usr/include/llvm-c-15

[spoonincode]

Sure, anything can be built and/or installed on anything. But I'd really like to see all dependencies obtainable via the standard package repos or bundled as a git submodule. llvm as a submodule is undesirable due to its size (although eventually one day our hand could be forced due to consensus concerns, TBD if that day comes).

[greg7mdp]

Maybe it ends up looking something like

find_package(Patch REQUIRED)  #good thing is that patch looks like it's included with build-essential
file(COPY ${LLVM_INCLUDE_DIRS}/llvm/ADT/STLExtras.h DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/override/)
add_custom_command(COMMAND ${Patch_EXECUTABLE} ARGS ${CMAKE_CURRENT_BINARY_DIR}/override/STLExtras.h ${CMAKE_CURRENT_SOURCE_DIR}/the.patch)

Thanks @spoonincode, I have added this.

[greg7mdp]

With this last change, the pinned build is successful, even if I edit the top CMakeLists.txt to build in C++20 mode. The tests pass except for:

The following tests FAILED:
	2015 - full-version-label-test (Failed)

For some reason the label ends with -dirty

root@a9d646b4336c:/leap/build# ./tests/full-version-label.sh "v4.1.0-dev" `pwd`/..
##### Nodeos Full Version Label Test #####
Using BUILD_ROOT="/leap/build".
Expecting "v4.1.0-dev-8281b28f28d6fbf2e1e0c33ab992842ec0e46cb1"...
Failed!
"v4.1.0-dev-8281b28f28d6fbf2e1e0c33ab992842ec0e46cb1" != "v4.1.0-dev-8281b28f28d6fbf2e1e0c33ab992842ec0e46cb1-dirty"

[heifner]

Failed!
"v4.1.0-dev-8281b28f28d6fbf2e1e0c33ab992842ec0e46cb1" != "v4.1.0-dev-8281b28f28d6fbf2e1e0c33ab992842ec0e46cb1-dirty"

If you ran that local then that just is an indication that you have uncommitted changes.

[greg7mdp]

If you ran that local then that just is an indication that you have uncommitted changes.

Thanks, Kevin, that explains it. Yes I ran that local and I manually changed the CMAKE_CPP_STANDARD to 20 so I did have uncommitted changes.

[greg7mdp]

Hey @spoonincode , would you mind approving this for now?
If you do find a better fix (by isolating llvm dependencies and compiling those in c++17 mode), we can always revert these changes, they are pretty trivial.

[greg7mdp]

Hey @spoonincode , could we make a decision between this PR and #1192. I totally defer to your jugement.

[greg7mdp]

Closing as we decided to go with PR #1192.