Skip to content

Conversation

@vgvassilev
Copy link
Member

@vgvassilev vgvassilev commented Dec 19, 2020

Some libraries are layered can depend on other libraries on a private paths. That is, libA can depend on libB which is neither on the LD_LIBRARY_PATH nor on a known system path. The posix linker injects "variables" such as @rpath which is expanded at link time to resolve the libraries on a relative path.

Prior to this patch, cling's Dyld-based symbol resolution could not trace down such cases causing failures in symbol resolution when a symbol is only defined in libB (a private library).

This patch implements the basic posix linker substitutions allowing cling's Dyld-based symbol resolution implementation to follow more closely the linker rules.

Performance

platform test master PRv1 PRv2 mem master mem PRv2
ubu20 roottest-python-pythonizations 6.5 6.55 6.65 342076 347168
ubu20 roottest-cling 112.8 114.53 181.26 411048 416036
ubu20 roottest-python 148.5 136.26 144.72 394292 397120
ubu20 roottest-root-hist 16.78 17.35 15.45 465156 235084
ubu20 roottest-root-math 28.28 27.89 27.40 424072 371256
ubu20 roottest-root-tree 462.84 510.59 465.79 796512 797992
ubu20 roottest-root-treeformula 117.53 129.16 114.44 261184 260168
ubu20 root-io-stdarray 66.90 67.89 65.71 263424 255120
ubu20 roottest-root-treeformula 117.55 129.10
osx 10.15 roottest-python-pythonizations 17.25 16.06
osx 10.15 roottest-cling 346.06 343.46
osx 10.15 roottest-root-hist 36.06 36.22
osx 10.15 roottest-root-math 49.78 52.42
osx 10.15 root-io-stdarray 119.32 118.73

That PR should reduce the number of stat calls improving #7774

Kudos Alexander Penev (@alexander-penev).

@vgvassilev vgvassilev requested a review from oshadura as a code owner December 19, 2020 22:22
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@vgvassilev vgvassilev marked this pull request as draft December 19, 2020 22:22
@phsft-bot
Copy link

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

Failing tests:

@vgvassilev
Copy link
Member Author

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@phsft-bot
Copy link

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

Failing tests:

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

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

Failing tests:

Copy link
Collaborator

@oshadura oshadura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgvassilev wow! changes are so minimal, we always have this feature and never tested it? I afraid I am not sure if CI has R enabled (I see -Dr=Off is everywhere)

@vgvassilev
Copy link
Member Author

The intent was that we only wanted to see what was failing. We are working on a bigger patch which will come in this PR.

@vgvassilev
Copy link
Member Author

@phsft-bot build!

@phsft-bot
Copy link

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

@phsft-bot
Copy link

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

@phsft-bot
Copy link

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

@phsft-bot
Copy link

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

@phsft-bot
Copy link

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-debian10-i386/cxx14.
See console output.

Copy link

@knowall-bot knowall-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply the code formatting changes without bloating the history:
Run:

git checkout fix-root10484"
wget https://raw.githubusercontent.com/llvm/llvm-project/main/clang/tools/clang-format/git-clang-format
chmod +x git-clang-format
git rebase -i -x "git-clang-format master && git commit -a --allow-empty --fixup=HEAD" --strategy-option=theirs origin/master
git log --oneline # to inspect the results
git rebase --autosquash -i master # squash without poluting the history

@phsft-bot
Copy link

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

@phsft-bot
Copy link

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

@vgvassilev vgvassilev marked this pull request as ready for review April 9, 2021 20:21
Some libraries are layered can depend on other libraries on a private paths.
That is, libA can depend on libB which is neither on the LD_LIBRARY_PATH nor
on a known system path. The posix linker injects "variables" such as @rpath
which is expanded at link time to resolve the libraries on a relative path.

Prior to this patch, cling's Dyld-based symbol resolution could not trace down
such cases causing failures in symbol resolution when a symbol is only defined
in libB (a private library).

This patch implements the basic posix linker substitutions allowing cling's
Dyld-based symbol resolution implementation to follow more closely the linker
rules.

Kudos Alexander Penev (@alexander-penev).
@phsft-bot
Copy link

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
Copy link

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

Warnings:

  • [2021-08-05T19:59:31.071Z] /mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp:281:45: warning: ignoring return value of ‘ssize_t readlink(const char*, char*, size_t)’, declared with attribute warn_unused_result [-Wunused-result]

@phsft-bot
Copy link

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

Errors:

  • [2021-08-05T19:58:33.339Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

Failing tests:

@phsft-bot
Copy link

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
Copy link

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

@phsft-bot
Copy link

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

@phsft-bot
Copy link

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

Errors:

  • [2021-08-06T20:02:38.872Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

@phsft-bot
Copy link

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
Copy link

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

Errors:

  • [2021-08-06T20:34:28.749Z] FAILED: /usr/bin/ccache /usr/bin/c++ -DCLING_VERSION="ROOT_1.0~dev" -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -Iinterpreter/cling/lib/Interpreter -I/mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter -I/mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/include -Iinterpreter/cling/include -isystem interpreter/llvm/src/tools/clang/include -isystem /mnt/build/workspace/root-pullrequests-build/root/interpreter/llvm/src/tools/clang/include -isystem interpreter/llvm/src/include -isystem /mnt/build/workspace/root-pullrequests-build/root/interpreter/llvm/src/include -fdiagnostics-color=always -std=c++14 -pipe -Wshadow -Wall -W -Woverloaded-virtual -fsigned-char -pthread -fno-semantic-interposition -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -fno-strict-aliasing -Wwrite-strings -Wno-shadow -Wno-unused-parameter -Wno-deprecated-declarations -fPIC -fno-rtti -O3 -fvisibility=hidden -fvisibility-inlines-hidden -std=c++14 -MD -MT interpreter/cling/lib/Interpreter/CMakeFiles/obj.clingInterpreter.dir/DynamicLibraryManagerSymbol.cpp.o -MF interpreter/cling/lib/Interpreter/CMakeFiles/obj.clingInterpreter.dir/DynamicLibraryManagerSymbol.cpp.o.d -o interpreter/cling/lib/Interpreter/CMakeFiles/obj.clingInterpreter.dir/DynamicLibraryManagerSymbol.cpp.o -c /mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp
  • [2021-08-06T20:34:28.749Z] /mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp:281:3: error: ‘ssizet_t’ was not declared in this scope
  • [2021-08-06T20:34:28.749Z] /mnt/build/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp:282:8: error: ‘len’ was not declared in this scope

@phsft-bot
Copy link

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-08-06T20:35:25.039Z] /data/sftnight/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp:281:3: error: ‘ssizet_t’ was not declared in this scope
  • [2021-08-06T20:35:25.039Z] /data/sftnight/workspace/root-pullrequests-build/root/interpreter/cling/lib/Interpreter/DynamicLibraryManagerSymbol.cpp:282:8: error: ‘len’ was not declared in this scope

@phsft-bot
Copy link

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
Copy link

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

Errors:

  • [2021-08-06T20:40:03.506Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

@vgvassilev vgvassilev merged commit 68c7ac9 into root-project:master Aug 7, 2021
@vgvassilev vgvassilev deleted the fix-root10484 branch August 7, 2021 07:18
vgvassilev added a commit to vgvassilev/root that referenced this pull request Aug 10, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obsure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasoanble compromise rather than trying to disable the module altoghether or
undefine the fragile macros.

as the global module index is not smart enough to figure it needs to offer
vgvassilev added a commit to vgvassilev/root that referenced this pull request Aug 10, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.

as the global module index is not smart enough to figure it needs to offer
vgvassilev added a commit to vgvassilev/root that referenced this pull request Aug 10, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.
vgvassilev added a commit to vgvassilev/root that referenced this pull request Aug 10, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.
vgvassilev added a commit to vgvassilev/root that referenced this pull request Aug 10, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.
vgvassilev added a commit that referenced this pull request Aug 10, 2021
PR #6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.
pzhristov pushed a commit to alisw/root that referenced this pull request Aug 27, 2021
PR root-project#6969 allowed ROOT to build pcm files for the R package via
the RInterface.pcm. However, various headers of R define macros which are
defined by math.h and others (ERROR in RooFit).

When the global module index is enabled, ROOT loads RInterface on a lookup for
(Range). Then the exported macros "shadow" the ones which are still in the
index and we do not load the correct corresponding module. This obscure problem
seems to be coming from a current deficiency in the implementation/interaction
between clang and the index.

Pre-loading the RInterface, if enabled, seems to resolve the issue at a
reasonable compromise rather than trying to disable the module altogether or
undefine the fragile macros.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants