root fwlite interactive plot for PackedCandidates segfaults #42234
Comments
|
A new Issue was created by @slava77 Slava Krutelyov. @Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
|
@slava77 could you perhaps also run valgrind on it? |
|
assign core |
|
New categories assigned: core @Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
running output is available on cmsdev34:/build/slava77/reltest/CMSSW_13_0_9-orig/issue42234/valgrind-29364 ignoring the memleak reports there are a couple of pointers to cmssw libs with There are also a couple of |
|
@Dr15Jones |
|
I am looking at this now. Started about an hour ago. I don't see anything obviously wrong yet, but I'm still looking. As far as I know, no one was working on this problem. Thanks for reporting it. |
|
I'm not sure what I am doing that is different, but I didn't see a seg fault on cmsdev34. For me it ran to the end and I see a plot displayed. I did see multiple cling/clang warnings/errors. I do not know if those are serious issues we should be worried about. @Dr15Jones Do you want me to keep looking tomorrow? Any suggestions at what to look at? I'm done for today. |
|
I managed to reproduce it (on a local system at Cornell), so I tried in a ROOT6 master IB and got something that might be informative to a cling expert. I'll poke at it some more tomorrow: |
... Thank you Dan, But I suppose the other part |
My suspicion is that the and both the crashes in 6.26 and the assertion failure in master went away. This seems like a bug in ROOT? |
|
This is a question because I don't understand how this works. Is it relevant that the enum is defined in a DataFormats package and the usage where the unsigned int type is specified is in a CondFormats package that uses boost serialization? |
Me neither. It looks like when cling processes the CondFormats package it inferred the type for |
|
Maybe not exactly the same issue, but similar from 2018. https://sft.its.cern.ch/jira/si/jira.issueviews:issue-html/ROOT-9660/ROOT-9660.html |
|
root-project/root#5059 looks like it got lost when LLVM was upgraded, as I don't see that code in the current tree. |
|
Just to confirm what we already knew, I can see in my working area the enum declaration modified to explicitly specify the type in _xr.cc file generated for the dictionary: |
|
The workaround is harmless, so might as well implement it, but would be good to get this fixed in ROOT (again?). @vgvassilev @pcanal |
|
Also @hahnjo |
Nice catch! That’s probably it. I know @hahnjo was making some patch cleanup and we might hit a case where we dropped the patch as we don’t have a test case. Now we do. However, on a broader perspective that’s one of the reasons we want C++ modules in cmssw. Cc: @davidlange6 |
|
@vgvassilev @pcanal Is there a plan for getting this fixed? Do we need to do any more from our end? |
|
@hahnjo ping.. |
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234.
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses.
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234.
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234. (cherry picked from commit 09f72c3)
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses. (cherry picked from commit 9d2f761)
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234. (cherry picked from commit 09f72c3)
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses. (cherry picked from commit 9d2f761)
|
Ok, backport PRs for 6.28 and 6.28 are up: root-project/root#13326 root-project/root#13327 @smuzaffar can I run this through CMS testing by opening a PR to https://github.com/cms-sw/root/ ? |
|
@hahnjo , yes PR tests can be run by using the correct cms/v6.2x.... branches. @aandvalenzuela , can you please open cms-sw/root PRs using root-project/root#13326 and root-project/root#13327 for root6.26 and root 6.28 branches of 13.3.X release cycles |
|
I can reproduce the crash in I can not reproduce this issue for CMSSW_13_3 release cycle [c] (which is also using [a] CMSSW_13_2_0_pre2 (with root 6.26 and GCC 11.2.1) [b] CMSSW_13_2_0_pre3 (with root 6.26) [c]
|
Ah right, this was worked around by #42300. Sorry, I missed that there is no easy reproducer in CMSSW
Not for Anyway, I'll take that CMSSW built fine with the patches as a sign that the fix isn't horribly breaking something and merge them into ROOT's release branches. Then it's up to you to decide which route you want to go. Sorry again that your testing was defied by the workaround already put in place... |
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses. (cherry picked from commit 9d2f761)
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses. (cherry picked from commit 9d2f761)
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234. (cherry picked from commit 09f72c3)
This is a heavily reduced case that shows the problem originally reported in cms-sw/cmssw#42234. (cherry picked from commit 09f72c3)
|
I tried an even more complicated So, from my point of view, the issue is resolved. I'm not going to close myself, in case the solution in root still needs to be tracked. |
Just to be clear, this is because of the workaround in #42300 because the proper ROOT fix isn't in that IB yet, right?
We should be good from the ROOT side, I merged the two backports to 6.26 and 6.28. The fixes will appear in the next point releases. |
|
+core Seems like there is nothing more to be done |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This issue is fully signed and ready to be closed. |
|
@cmsbuild, please close |
There can be multiple attributes in the forward declaration, see the added test in roottest/cling/dict/enum (reduced from a case reported by CMS in cms-sw/cmssw#42234), so we have to look for the last closing parentheses.
this was initially noticed in CMSSW_13_0_9, but is also reproduced in CMSSW_13_2_0_pre2
local version on cmsdev34:
crashes
adding more or less variables or reading a remote copy of the file
root://cms-xrd-global.cern.ch//store/data/Run2023D/Muon0/MINIAOD/PromptReco-v1/000/369/956/00000/11189a7c-8a53-4ecf-b1bd-2351a0d1e91b.rootsometimes succeedsThere are clang/cling warnings/errors preceding the crash, but some of them are present also in non-crashing variant.
The crashing variant seems to most consistently have this right before the crash
The stack trace follows:
I know that historically calling the
TTree::DrawonPackedCandidates data that needs unpacking was pretty brittle, but perhaps there can be a resolution.The text was updated successfully, but these errors were encountered: