Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hist] prevent nullptr access with copy from default constructor #16773

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

ferdymercury
Copy link
Contributor

Fixes #16771

@ferdymercury ferdymercury requested a review from pcanal October 28, 2024 17:59
@ferdymercury ferdymercury requested a review from lmoneta as a code owner October 28, 2024 17:59
before allocating new memory, make sure we delete potentially existing one previously
Copy link

github-actions bot commented Oct 28, 2024

Test Results

    18 files      18 suites   3d 14h 24m 27s ⏱️
 2 699 tests  2 698 ✅ 0 💤 1 ❌
46 103 runs  46 102 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit ae2a9ab.

♻️ This comment has been updated with latest results.

to avoid segviolation when calling copy-this
@pcanal
Copy link
Member

pcanal commented Oct 29, 2024

Is the failure in roottest-root-hist-h2root at https://github.com/root-project/root/pull/16773/checks?check_run_id=32223807661 understood?

@ferdymercury
Copy link
Contributor Author

Is the failure in roottest-root-hist-h2root at https://github.com/root-project/root/pull/16773/checks?check_run_id=32223807661 understood?

Is it related to #15915 ?
pinging @couet and @hahnjo

@dpiparo
Copy link
Member

dpiparo commented Oct 30, 2024

@ferdymercury à propos roottest-root-hist-h2root: I think this was clarified by the last round of tests. We see a failure on the GPU platform, which is known and unrelated to your changes. If you agree, I would proceed with the merge of this PR

@ferdymercury
Copy link
Contributor Author

Agreed thks!

@dpiparo dpiparo merged commit e21eacf into root-project:master Oct 30, 2024
33 of 41 checks passed
@ferdymercury ferdymercury deleted the th2poly branch October 30, 2024 12:09
@ywkao
Copy link

ywkao commented Nov 4, 2024

Hi @ferdymercury, thank you for the swift fixes!
Would it be possible to backport these changes to versions 6.30 and 6.32?

@guitargeek
Copy link
Contributor

We could, but there will probably be no new patch release anyway, since ROOT 6.34.00 is only a few weeks away. And we're not going to make a new patch release just for this bugfix.

Given this information, could you live without a backport? I guess the only way in which it could help you is if you build your own ROOT from source from the tip of the 6.30 and 6.32 branches, without needing a patch release. But I guess this is not the case?

@guitargeek
Copy link
Contributor

Hi @ywkao, following up on my last comment, what would be motivation for the backports? Are you using these older ROOT version in production and this is a crucial fix? Or you're using ROOT on your laptop and if would be easy to update to 6.34 risk-free? How we proceed depends also on that

@ywkao
Copy link

ywkao commented Nov 6, 2024

Hi @guitargeek, thank you for the information. I found that the fix has been included in the latest CMSSW release so I can continue with the unit test in cms-sw/cmssw#41932 (comment). I will let you know if there is still a need of the backport after confirming with the CMS DQM experts.

@ywkao
Copy link

ywkao commented Nov 7, 2024

Hi @guitargeek, after confirming with CMS people, we would like to have TH2Poly functional in the default CMSSW. A backport to v6.30 and v6.32 will be very helpful.

@guitargeek
Copy link
Contributor

I understand. The backports will be merged tonight or tomorrow morning.

@ywkao
Copy link

ywkao commented Nov 11, 2024

Thank you for the help with the root backports!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug experiment Affects an experiment / reported by its software & computimng experts in:Hist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copying a default constructed TH2Poly fails.
5 participants