Llvm conflict fix by grasph · Pull Request #8717 · root-project/root

grasph · 2021-07-22T10:58:44Z

@vgvassilev, here is the PR so solve the LLVM library conflicts when interfacing ROOT with Julia. I've limited the change to the interpreter package.

I have 25 out of the 2140 tests that fail. It's likely to be due to my local environment. If it can easily be tested with the jenkins system, it will be faster that me trying to investigate the test failures.

Note. The parallel compilation is not working properly, independently of my changes: it fails many times and need to be restarted or build some package in single process mode. Is it a know problem ?

This Pull request:

Changes or fixes:

Adds -Bsymbolic linker option for libCling.so in order get the symbols resolved internally

This is expected to solve conflicts when interfacing with other software using LLVM, like Julia. See https://github.com/JuliaHEP/ROOT.jl/issues/17#issuecomment-882719292

Checklist:

tested changes locally
updated the docs (if necessary)

phsft-bot · 2021-07-22T10:58:46Z

Can one of the admins verify this patch?

vgvassilev · 2021-07-22T11:53:48Z

@phsft-bot build!

phsft-bot · 2021-07-22T11:53:54Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-22T14:45:41Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

grasph · 2021-07-22T15:19:10Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:
* [projectroot.roottest.python.cpp.roottest_python_cpp_cpp](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/123133/testReport/projectroot.roottest.python/cpp/roottest_python_cpp_cpp/)

@vgvassilev , I don't have this test in my checkout:

Test project /home/pgras/git.d/root.grasph/mybuild
No tests were found!!!

Do you have an idea why? I ran cmake with the following options as described in [1]:

cmake -DCMAKE_BUILD_TYPE=Debug -Dtesting=ON -Droottest=ON

Do I need more options? A pyroottest=ON ?

Philippe.

[1] https://root.cern/for_developers/run_the_tests/

vgvassilev · 2021-07-22T15:22:07Z

@grasph, that is a regular failure and it is not caused by your PR.

hahnjo · 2021-07-22T15:29:33Z

We're already adding -fno-semantic-interposition since commit e564c8a / PR #8204 which I think should already set the visibility such that LLVM symbols are hidden from other libraries. Did you recently test a master build or 6.24/02?

vgvassilev · 2021-07-22T15:57:58Z

@hahnjo, this patch is useful for cppyy and it sounds like -Bsymbolic covers more than VISIBILITY_INLINES_HIDDEN while also reducing the PLTs. In particular (ROOT master):

$ nm -CD libCling.so | grep llvm | grep ' [Ww] ' | wc -l
379

grasph · 2021-07-22T16:07:05Z

We're already adding -fno-semantic-interposition since commit e564c8a / PR #8204 which I think should already set the visibility such that LLVM symbols are hidden from other libraries. Did you recently test a master build or 6.24/02?

Interesting. From my understanding of g++ document, this flag with accidentally solves the problem if the compiler optimize the code by in-lining it but it is not guarranted. In addition it can create confusing situations where the problem disappears as soon as you try to debug it using the debugged version of the code.

That said, we are in a confusing situation, as I'm not able to reproduce the problem as soon as I recompile ROOT. Nevertheless, I recompiled many time ROOT last week, and then the problem was systematic. It could be that it is due to the ROOT code version I used.

The problem is there for LCG100 which used ROOT 6.24/00, but not for /cvmfs/sft.cern.ch/lcg/app/releases/ROOT/6.24.02/x86_64-centos7-gcc48-opt/. So it would confirm that -fno-semantic-interposition has solved the problem if it has happened between the two versions. Nevertheless, as I wrote before, in my understanding, it's accidental and won't be as reliable as the -Bsymbolic option.

Philippe.

Axel-Naumann · 2021-07-22T17:18:58Z

How is it accidental? It was totally on purpose, solving other occurrences of this exact same issue. I suppose, because i have not seen any issue / error message / backtrace, so all I can do is guess and assume :-) Can you please provide some details on the issue you try to fix? Can you please rebase this branch onto master? We do not allow merge commits (unless fast forward).

Axel-Naumann · 2021-07-22T17:28:19Z

Iiuc https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic argues that -Bsymbolic should be fine for libCling and is superior to no symbolic interposition

phsft-bot · 2021-07-22T18:33:23Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

grasph · 2021-07-23T07:22:41Z

Iiuc https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic argues that -Bsymbolic should be fine for libCling and is superior to no symbolic interposition

Thanks, Axel, for the link to this interesting article. By "accidental", I meant that, if we assume the gcc manpage description of the flag correct and complete (*), then we rely on a side-effect of compiler optimizations. The article you quoted mention another optimization that should make it work for all calls to functions defined in the same compilation unit (**), while I wrote it will be limited to inline functions.

The -Bsymbolic, which is an option of the linker, instead of compiler, addresses directly what we target. The ld manpage description reads as "When creating a shared library, bind references to global symbols to the definition within the shared library, if any. "

Philippe.

(*) "with -fno-semantic-interposition the compiler assumes that if interposition happens for functions the overwriting function will have precisely the same semantics (and side effects). "

(**) Although the statement on LD_PRELOAD confuses me.

Axel-Naumann

Thanks for your PR!

bellenot · 2021-07-23T09:22:51Z

@grasph please fork also roottest to prevent timeouts on the Windows build nodes. Thanks

grasph · 2021-07-23T09:34:45Z

@grasph please fork also roottest to prevent timeouts on the Windows build nodes. Thanks

Done. Do I need to create a branch with the same name ?

bellenot · 2021-07-23T09:44:21Z

@grasph please fork also roottest to prevent timeouts on the Windows build nodes. Thanks

Done. Do I need to create a branch with the same name ?

Thanks. And no, simply forking is good enough

grasph · 2021-07-26T20:34:33Z

@bellenot I'm not sure why the PR is blocked. Is it an issue with deepcode-ci-bot or is github that did not get that I made the changed @Axel-Naumann requested? I'm leaving for holidays on Wednesday. It would be good to close the PR by then.

pcanal · 2021-07-26T21:18:54Z

@phsft-bot build!

phsft-bot · 2021-07-26T21:19:00Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-26T21:21:14Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-26T21:21:09.750Z] stderr: error: Failed to merge in the changes.
[2021-07-26T21:21:13.231Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-26T21:21:26Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-26T21:21:19.121Z] stderr: error: Failed to merge in the changes.
[2021-07-26T21:21:25.023Z] CMake Error at /mnt/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-26T21:21:27Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-26T21:21:25.515Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-26T21:26:41Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-26T21:26:40.981Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-26T21:28:57Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-07-26T21:28:56.743Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-27T08:39:43Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-27T09:51:37Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

phsft-bot · 2021-07-27T09:58:16Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

weliveindetail · 2021-08-05T16:56:41Z

FYI: You might want to check -Bsymbolic-non-weak-functions in the future as well. It keeps the weird ELF interposition behavior for weak symbols, which can be vital for things like emulated TLS (see last comment in the review). Initial support comes with the current LLD 13 release: https://reviews.llvm.org/D102570#2915489

Axel-Naumann

LGTM - just a tiny comment and we should be good to merge

Axel-Naumann · 2021-08-06T09:16:15Z

@weliveindetail thanks for the info, we will revisit this once -Bsymbolic-non-weak-functions becomes mainstream.

…s resolved internally This is expected to solve conflicts when interfacing with other software using LLVM, like Julia. See JuliaHEP/ROOT.jl#17 (comment)

…akeLists.txt file

…s resolved internally This is expected to solve conflicts when interfacing with other software using LLVM, like Julia. See JuliaHEP/ROOT.jl#17 (comment)

…akeLists.txt file

Co-authored-by: Axel Naumann <Axel.Naumann@cern.ch>

oschulz · 2021-10-01T19:01:39Z

Thanks a lot for this @grasph !

Axel-Naumann · 2021-11-03T18:06:52Z

@phsft-bot build

phsft-bot · 2021-11-03T18:06:58Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

Axel-Naumann · 2021-11-03T18:10:30Z

macOS 12 messes up some system lib's internal (?) llvm symbols with those of cling: CommandLine Error: Option 'h' registered more than once!. Does anyone know why the default namespacing on macOS might fail here? I bet we're missing a similar flag now also for macOS :-/

phsft-bot · 2021-11-03T18:48:34Z

Build failed on mac1014/python3.
Running on macphsft17.dyndns.cern.ch:/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.tree.dataframe.test.gtest_tree_dataframe_test_datasource_csv

phsft-bot · 2021-11-03T20:56:28Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

Axel-Naumann · 2021-11-04T07:08:57Z

Failures are unrelated. Thanks for the contribution!

oschulz · 2021-11-04T12:25:54Z

Which future ROOT release will this (likely) be included in?

grasph · 2021-11-04T12:57:19Z

Which future ROOT release will this (likely) be included in?

Hello @oschulz , I let Axel answer to your release question.

I expect you are interested by the interfacing with Julia. This is already working starting from version 6.24/02 (explained in a previous comment of the PR discussion).

This pull request is only consolidating a code correction which was included in the release between 6.24/00 and 6.24/02).

Philippe.

oschulz · 2021-11-04T14:28:43Z

I expect you are interested by the interfacing with Julia.

Yes - thanks @grasph !

grasph requested a review from bellenot as a code owner July 22, 2021 10:58

jblomer assigned vgvassilev Jul 22, 2021

vgvassilev requested review from Axel-Naumann and hahnjo July 22, 2021 15:19

Axel-Naumann requested changes Jul 23, 2021

View reviewed changes

Comment thread interpreter/CMakeLists.txt Outdated

Comment thread interpreter/CMakeLists.txt Outdated

grasph requested a review from Axel-Naumann July 26, 2021 09:32

hahnjo removed request for couet, guitargeek, hageboeck, lmoneta and pcanal July 27, 2021 08:40

Axel-Naumann requested changes Aug 6, 2021

View reviewed changes

Comment thread interpreter/CMakeLists.txt

grasph and others added 6 commits August 24, 2021 11:44

Adds -Bsymbolic linker option for libCling.so in order get the symbol…

3ec490b

…s resolved internally This is expected to solve conflicts when interfacing with other software using LLVM, like Julia. See JuliaHEP/ROOT.jl#17 (comment)

Grouped flags related to link premption togeher in the interpreter/CM…

a80e598

…akeLists.txt file

Adds -Bsymbolic linker option for libCling.so in order get the symbol…

42f590e

…s resolved internally This is expected to solve conflicts when interfacing with other software using LLVM, like Julia. See JuliaHEP/ROOT.jl#17 (comment)

Grouped flags related to link premption togeher in the interpreter/CM…

bd1b7b4

…akeLists.txt file

Update interpreter/CMakeLists.txt

5c94911

Co-authored-by: Axel Naumann <Axel.Naumann@cern.ch>

cosmetics

2ee441c

grasph force-pushed the llvm-conflict-fix branch from 50483c3 to 2ee441c Compare August 24, 2021 09:49

Axel-Naumann approved these changes Nov 4, 2021

View reviewed changes

Axel-Naumann merged commit 1dd4734 into root-project:master Nov 4, 2021

Uh oh!

Conversation

grasph commented Jul 22, 2021

This Pull request:

Changes or fixes:

Checklist:

Uh oh!

phsft-bot commented Jul 22, 2021

Uh oh!

vgvassilev commented Jul 22, 2021

Uh oh!

phsft-bot commented Jul 22, 2021

Uh oh!

phsft-bot commented Jul 22, 2021

Failing tests:

Uh oh!

grasph commented Jul 22, 2021

Failing tests:

Uh oh!

vgvassilev commented Jul 22, 2021

Uh oh!

hahnjo commented Jul 22, 2021

Uh oh!

vgvassilev commented Jul 22, 2021

Uh oh!

grasph commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Axel-Naumann commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Axel-Naumann commented Jul 22, 2021

Uh oh!

phsft-bot commented Jul 22, 2021

Uh oh!

grasph commented Jul 23, 2021

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bellenot commented Jul 23, 2021

Uh oh!

grasph commented Jul 23, 2021

Uh oh!

bellenot commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grasph commented Jul 26, 2021

Uh oh!

pcanal commented Jul 26, 2021

Uh oh!

phsft-bot commented Jul 26, 2021

Uh oh!

phsft-bot commented Jul 26, 2021

Errors:

Uh oh!

phsft-bot commented Jul 26, 2021

Errors:

Uh oh!

phsft-bot commented Jul 26, 2021

Errors:

Uh oh!

phsft-bot commented Jul 26, 2021

Errors:

Uh oh!

phsft-bot commented Jul 26, 2021

Errors:

Uh oh!

phsft-bot commented Jul 27, 2021

Uh oh!

phsft-bot commented Jul 27, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 27, 2021

Failing tests:

Uh oh!

weliveindetail commented Aug 5, 2021

Uh oh!

grasph commented Jul 22, 2021 •

edited

Loading

Axel-Naumann commented Jul 22, 2021 •

edited

Loading

bellenot commented Jul 23, 2021 •

edited

Loading