Repository navigation
[dictgen] Register rootcling options in their own llvm::cl subcommand - #23546
guitargeek wants to merge 1 commit into
Conversation
rootcling's options were registered in LLVM's top-level subcommand, next
to all of LLVM's own options. When linking against a shared libLLVM,
both end up in the same registry and names that exist in both abort at
startup, for example rootcling's `-W` and the `-W` alias for `--no-warn`
from MCTargetOptionsCommandFlags:
: CommandLine Error: Option 'W' registered more than once!
llvm::cl only checks for duplicates within a subcommand. Registering the
options in a dedicated `rootcling` subcommand, which RootClingMain
selects by inserting its name as the first argument unless `bare-cling`
was requested, avoids the clash while keeping the command line syntax
unchanged. LLVM's options stay in the top-level subcommand, so cling's
`-mllvm` handling is unaffected.
See root-project#17698.
🤖 Done with the help of AI
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Yes, and that signals that we have two copies of llvm which is something we do not want. |
|
That's something you don't want 🙂 |
I would try to keep out the discussion about |
This comment was marked as outdated.
This comment was marked as outdated.
Since you're asking: one concrete example would be using ROOT together with a library that bundles its own shared LLVM libraries, like PyTorch. Using a ROOT build linked against shared system LLVM in Python together with PyTorch would therefore crash badly. If I understand correctly, @vgvassilev wants to make these situations outright impossible by discouraging linking against shared LLVM, which is honorable. But on the other side you have @ferdymercury or me who are interested in Nix or Debian packaging for example, and we don't like handholding about how we should manage dependencies and like to keep the freedom to choose linking against the shared libraries. And in closed ecosystems like Nix or Debian, you can also strive towards the long-term goal of having ROOT and PyTorch both using the same system LLVM, which is not unrealistic in a few years from now. |
Test Results 23 files 23 suites 3d 19h 4m 10s ⏱️ For more details on these failures, see this check. Results for commit a9e8aa4. |
It sounds like a preference and it is not: I've put several technical arguments consistently and over time about this.
I want that too, but ROOT is the wrong place to fix that. The place to fix it is either LLVM or your favorite distribution or both. |
But that's the whole point -- now we don't know if we have a second (perhaps incompatible) copy of llvm being randomly loaded as part of your ROOT distribution. I'd argue the opposite: this makes the distribution of ROOT fragile because in one case it will have a single copy of llvm and in the other another one. What becomes now also interesting is when somebody decides to include graphics and libmessa which has it's own copy of llvm. Which one would libmessa pick? What about PyTorch or friends? If I were you, I'd ask claude to debug this with -trace-expand of cmake and try to push that change into llvm upstream or packaging because ultimately we mix two concepts: static vs dynamic linking. If ROOT has chosen static (in dual mode distribution) llvm should respect that and not add libLLVM.so into the DT_NEEDED elf section. |
Ok, I understand your point, but being already a If instead one forces the packagers to fix this, it's triplicating the work: the same hard-coded patches have to be maintained for Debian, Nix, conda, ... ?
And that's ok for the moment in my opinion: since it's behind a hidden no-warranty advanced flag, I do not expect it to work perfectly.
In my experimental builds in Debian with system LLVM, I have tried both things:
|
If that's the whole point, then maybe I see where the confusion comes from! Even with this PR, linking against The change that would deserve that scrutiny you have is the future one removing the dylib suppression and supporting What this PR does is fix an So: is there any reason to hold this off, besides it being a prerequisite for a later, more controversial change? |
libCling links LLVM and Clang statically and hides them with -Bsymbolic and libCling.script, so that another LLVM in the process, such as Triton's (PyTorch's compiler), neither binds to libCling's nor has libCling bind to its own. Nothing tests this. When the hiding was incomplete, ALICE saw sporadic crashes with a second libLLVM.so in the process (root-project#19889), and linking libCling against a shared libLLVM, which cannot be hidden, would bring them back unnoticed. Run root.exe and rootcling with a preloaded canary that defines LLVM functions libCling calls, and fail if one of them is reached; a self-test checks that the canary catches a library that relies on the LLVM of its host. With glibc, also fail if the dynamic linker's own log, LD_DEBUG=bindings, shows a binding of an LLVM or Clang symbol across libCling's boundary. Check that the canary's names exist in the LLVM ROOT was built with, so that the test cannot go stale. The tests are ELF only, and skipped for AddressSanitizer builds, which control LD_PRELOAD themselves. Related: the 'Option registered more than once' error with an external LLVM (root-project#12156), and the discussions in root-project#20903 and root-project#23546 about rootcling's llvm::cl options and linking against a shared libLLVM.
|
Side note: when I cherry-picked this into #23372 I saw a build error 'optimize-regalloc' option not found. |
rootcling's options were registered in LLVM's top-level subcommand, next to all of LLVM's own options. When linking against a shared libLLVM, both end up in the same registry and names that exist in both abort at startup, for example rootcling's
-Wand the-Walias for--no-warnfrom MCTargetOptionsCommandFlags:llvm::cl only checks for duplicates within a subcommand. Registering the options in a dedicated
rootclingsubcommand, which RootClingMain selects by inserting its name as the first argument unlessbare-clingwas requested, avoids the clash while keeping the command line syntax unchanged. LLVM's options stay in the top-level subcommand, so cling's-mllvmhandling is unaffected.See #17698.
🤖 Done with the help of AI
Less intrusive alternative to #20903.