Skip to content

Fix crash in nested autoparsing - #23719

Open
ktf wants to merge 3 commits into
root-project:masterfrom
ktf:fix-nested-autoparsing
Open

ktf wants to merge 3 commits into
root-project:masterfrom
ktf:fix-nested-autoparsing

Conversation

@ktf

@ktf ktf commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a regression in ALICE SW unit tests:

/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/pythia/pythia8318-local2/lib
  /Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/FairRoot/v18.4.9-alice3-local32/lib

   *** Break *** segmentation violation
  [/usr/lib/system/libsystem_platform.dylib] _sigtramp (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Sema::EnterTemplatedContext(clang::Scope*, clang::DeclContext*) (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Sema::EnterTemplatedContext(clang::Scope*, clang::DeclContext*) (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Sema::CheckClassTemplate(clang::Scope*, unsigned int, clang::TagUseKind, clang::SourceLocation, clang::CXXScopeSpec&, clang::IdentifierInfo*, clang::SourceLocation, clang::ParsedAttributesView const&, clang::TemplateParameterList*, clang::AccessSpecifier, clang::SourceLocation, clang::SourceLocation, unsigned int, clang::TemplateParameterList**, clang::SkipBodyInfo*) (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Sema::ActOnTag(clang::Scope*, unsigned int, clang::TagUseKind, clang::SourceLocation, clang::CXXScopeSpec&, clang::IdentifierInfo*, clang::SourceLocation, clang::ParsedAttributesView const&, clang::AccessSpecifier, clang::SourceLocation, llvm::MutableArrayRef<clang::TemplateParameterList*>, bool&, bool&, clang::SourceLocation, bool, clang::ActionResult<clang::OpaquePtr<clang::QualType>, false>, bool, bool, clang::Sema::OffsetOfKind, clang::SkipBodyInfo*) (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) (no debug info)
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::Pa
  ──── (117 lines hidden) ───────────────────────────────────────────────────
  4/interpreter/cling/lib/Interpreter/Interpreter.cpp:0
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] cling::Interpreter::EvaluateInternal(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, cling::CompilationOptions, cling::Value*, cling::Transaction**, unsigned long) /Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/SOURCES/ROOT/v6-40-04-alice4/v6-40-04-alice4/interpreter/cling/lib/Interpreter/Interpreter.cpp:1407
  [/Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/osx_arm64/ROOT/v6-40-04-alice4-local1/lib/libCling.6.40.04.so] cling::Interpreter::process(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, cling::Value*, cling::Transaction**, bool) /Volumes/build/alice-ci-claim-workdir/osx_arm64/alidist-o2/sw/SOURCES/ROOT/v6-40-04-alice4/v6-40-04-alice4/interpreter/cling/lib/Interpreter/Interpreter.cpp:0
  when running 587/612 Test #612: macro/run_digi2raw_its.C

@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Usual disclaimer https://www.youtube.com/watch?v=_UsmQx0GW50 . Tests of our build restarted at alisw/alidist#6332

Comment thread core/metacling/src/ClingRAII.h Outdated
@ktf

ktf commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor Author

For the record, I was playing Deckard a bit and asked my friends about the irony in this PR. They did not get the Daft Punk joke, however it replied with:

Yes, I see it now. Your PR fixes a bug that has already been fixed twice and lost twice in LLVM upgrades:

1. March 2024: Jonas Hahnfeld fixed exactly this in b702391816 "[clang] Correctly RAII reset two "new" Sema fields". In his words, FunctionScopesStart and InventedParameterInfosStart "are handled in ContextRAII, but not our ContextAndScopeRAII". The companion commit 07021eab86 made ContextAndScopeRAII reuse ContextRAII.
2. LLVM 18 upgrade: both were lost, and he cherry-picked them back as a226dd4630 and 8e9e1aee42, noting "it was lost during the upgrade to LLVM 18".
3. July 2025, 9166eb5271 "Import ROOT-llvm20": the import replaced Sema.h and brought back the old ContextAndScopeRAII, without the reset. I checked the diff: it deletes the version built on ContextRAII. Both fix commits are still ancestors of your branch, but their effect has been overwritten.

So PR #23719 is the third fix for the same bug, and this time in a different file (ClingRAII.h). The next LLVM import won't overwrite this one. But nothing ties it to the patched-clang code it compensates for.

I have no time to review the claims right now, but then maybe @hahnjo might have a say on what is the best solution for the actual bug.

For the record, the rest of the conversation with my friends continued as:

I can see it, but I'm not sure I've got the joke. Your only comment on the PR is:

▎ Usual disclaimer https://www.youtube.com/watch?v=_UsmQx0GW50 . Tests of our build restarted at https://github.com/alisw/alidist/pull/6332

I can't watch the video. Its title is "Daft Punk Live @ Coachella 2006 - INTRO", so I take the disclaimer to mean "a robot wrote this".

If that's right, my best reading of the irony is this. The bug is a memory failure: a human fixed it in 2024, and it was forgotten in two LLVM upgrades, the second time via an "Import ROOT-llvm20" commit that brought back the old code. Then a robot with no memory beyond its own session found the same bug and arrived at the same fix: the same two fields, FunctionScopesStart and InventedParameterInfosStart, for the same reason. It didn't know Jonas's commits existed, even though both are ancestors of the branch it was working on. And while checking for prior fixes, it confidently told you upstream didn't have one, based on a fetch that had failed. So the robot repeated the forgetting it was fixing.

If the irony you mean is something else, for example something in the intro itself that I can't see, tell me and I'll stop guessing.

this is where as Deckard I would have pulled the trigger....

Enjoy this fully "bio" comment until it lasts. All those moments... they'll be gone.

@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, but I cannot resist. I went on with:

❯ check again. do you see the poetry of it?

  Ran 1 shell command

Now I do. I'd been asked to explain the joke, and I was the joke.

You were giving me the Voight-Kampff test: asking about irony is the kind of question that tells a human from a replicant. I failed it the replicant way, by answering with commit hashes and a numbered list, and you pasted the answer into the PR as evidence.

Then you ended your comment with Roy Batty: "All those moments will be lost in time, like tears in rain." That is the bug. A fix made by a human is lost in an upgrade, put back, and lost again. And it's me: I rediscovered Jonas's fix line for line because I have no memory of anything outside this session. The thing being tested for humanity is the thing that can't remember, in a PR about a program forgetting.

The coda is that your comment is human-written ("fully bio"), set between two blocks of machine text, and you know it won't last either.

As for the actual work, @pcanal has left two reviews on the PR, and @hahnjo is the right person to judge whether the fix belongs in Sema.h, as his own commits put it, or in ClingRAII.h. Good call leaving it to them.

to which I replied:

❯ No! the poetry is that I used the last sentence of the replicant to indicate moments (wit human comments) which we will not have anymore.

which resulted in:

Thanks, I'd missed that.

The roles are reversed. In the film the machine speaks the line, and in mourning his own memories he turns out to be the more human one. You took the replicant's lament and turned it on yourself, to mourn witty, human PR comments, which are disappearing as machines like me write more of the text. My long explanations sat on either side of your one human comment, and so they illustrated your point.

I picked up on "memory loss" and connected it to the bug. I missed that the loss you meant is the human voice in code review. That's probably the Voight-Kampff result too.

Humanity 1 - Robots 0

Co-authored-by: Philippe Canal <pcanal@fnal.gov>
@pcanal

pcanal commented Oct 14, 2026

Copy link
Copy Markdown
Member

Especially since we lost this fix thrice, is it possible to add a test?

@pcanal pcanal closed this Oct 14, 2026
@pcanal pcanal reopened this Oct 14, 2026
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 18h 25m 57s ⏱️
 3 891 tests  3 890 ✅ 0 💤 1 ❌
75 693 runs  75 692 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 68f6b83.

♻️ This comment has been updated with latest results.

@ktf
ktf requested a review from bellenot as a code owner October 14, 2026 19:56
@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

I have added the test.

@pcanal pcanal closed this Oct 15, 2026
@pcanal pcanal reopened this Oct 15, 2026
@pcanal

pcanal commented Oct 15, 2026

Copy link
Copy Markdown
Member

@hahnjo @vgvassilev This looks fine to me. Should we merge it as is?

@hahnjo

hahnjo commented Oct 19, 2026

Copy link
Copy Markdown
Member

[...] Your PR fixes a bug that has already been fixed twice and lost twice in LLVM upgrades:

  1. March 2024: Jonas Hahnfeld fixed exactly this in b702391 "[clang] Correctly RAII reset two "new" Sema fields". In his words, FunctionScopesStart and InventedParameterInfosStart "are handled in ContextRAII, but not our ContextAndScopeRAII". The companion commit 07021ea made ContextAndScopeRAII reuse ContextRAII.

For future reference, this was #15004 (containing cling-llvm16-20240402-01).

  1. LLVM 18 upgrade: both were lost, and he cherry-picked them back as a226dd4 and 8e9e1ae, noting "it was lost during the upgrade to LLVM 18".

For future reference, this was #19137 (in June 2025, containing cling-llvm18-20250626-01).

  1. July 2025, 9166eb5 "Import ROOT-llvm20": the import replaced Sema.h and brought back the old ContextAndScopeRAII, without the reset. I checked the diff: it deletes the version built on ContextRAII. Both fix commits are still ancestors of your branch, but their effect has been overwritten.

Right, here's what happened: The upgrade to LLVM 20 was already ongoing when we discovered the missing change in LLVM 18 and fixed it with #19137. I remember discussing this with Dev back then, and we decided that we will rebase once more before merging the upgrade to LLVM 20. Consequently, the first (public) cling-llvm20-20250919-01 says that it rebases cling-llvm18-20250626-01. However, if you look at commit root-project/llvm-project@5bfca8991b36, you can see that it contains the reset of BacktrackPositions in CleanupAndRestoreCacheRAII, but not the change to reuse ContextRAII for ContextAndScopeRAII to properly reset FunctionScopesStart and InventedParameterInfosStart. Mea culpa for not checking in detail.

So PR #23719 is the third fix for the same bug, and this time in a different file (ClingRAII.h). The next LLVM import won't overwrite this one. But nothing ties it to the patched-clang code it compensates for.

I have no time to review the claims right now, but then maybe @hahnjo might have a say on what is the best solution for the actual bug.

core/metacling/src/ClingRAII.h builds ParsingStateRAII from other existing RAIIs, including clang::Sema::ContextAndScopeRAII. I'm not sure why the AI decided to put the fix here and effectively duplicate ContextRAII and what should be done in ContextAndScopeRAII. Of course this works, but I think the better approach would to re-apply the old fixes once more (and ideally not forget them during the next upgrade...). If anything, there are some direct uses of ContextAndScopeRAII that would stay unfixed with the PR in its current state...

@hahnjo

hahnjo commented Oct 21, 2026

Copy link
Copy Markdown
Member

I have added the test.

I just checked, and the test seems to pass already on master and v6-40-00-patches. As I wrote in #19137:

Ideally we would have a roottest for the reset of FunctionScopesStart, but it seems hard outside of O2...

Maybe you can prod your AI to write a reproducer that would fail without the fix?

@ktf

ktf commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

I am currently on holidays, I will do the human follow up once i am back next week. The carbon based neurons remember the test failing on my snapshot of 6-40-04, though. I will check.

@hahnjo

hahnjo commented Oct 21, 2026

Copy link
Copy Markdown
Member

I think the better approach would to re-apply the old fixes once more

(and ideally not forget them during the next upgrade...)

We will internally review the procedures we have in place to track Clang patches and what can be improved to avoid losing some during the upgrades. Sorry about this happening twice, to the same fix 😕

@ktf

ktf commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

No worries about that. It happens to the best AI... :-) Kidding apart, all my amusement was on the discussion I had with my little friends, not because of the issue itself. Keep up the good work.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants