[RF] RooUniform in RooArgList in RooProdPdf: segmentation violation during fit

[ynikitenko]

• [google and issues] Checked for duplicates

Describe the bug

When I use RooUniform inside RooProdPdf, this leads to SIGSEGV during execution.

This code raises:

UniformExpZ neutrino_target_z(
    "uniform_exp_z_nt", "uniform (x) exp (z) nt",
    z, nt_h_top, nt_h_bot, l
);
RooProdPdf uxy_nt(
    "uxy_nt", "uxy_nt",
    RooArgList(RooUniform("u_nt_x", "u_nt_x", x),
               RooUniform("u_nt_y", "u_nt_y", y))
);
RooProdPdf nt("nt", "nt", RooArgList(uxy_nt, neutrino_target_z));
// fit
RooFitResult *res = nt.fitTo(data, RooFit::Save(true), RooFit::NumCPU(8));

However, when I write RooUniform definition outside of that RooArgList, everything works fine:

RooUniform u_nt_x("u_nt_x", "u_nt_x", x);
RooUniform u_nt_y("u_nt_y", "u_nt_y", y);
RooProdPdf uxy_nt(
    "uxy_nt", "uxy_nt",
    RooArgList(u_nt_x, u_nt_y)
);

Expected behavior

Should be the same for variables defined inside and outside the constructor.

To Reproduce

3. I compile this code with gcc. Sorry if the format of this bug report is not good :/

Setup

1. ROOT version 6.22/06.
2. Arch Linux.
3. Built myself.

Additional context

Unfortunately (I don't know whether it must be considered a bug) the output about the error appears only when I comment out a lot of code. Usually it only writes

Command terminated by signal 11

Here is the output when I manage to get that:

*** Break *** segmentation violation

===========================================================
There was a crash.
This is the entire stack trace of all threads:
===========================================================
#0 0x00007f21b73d20ca in wait4 () from /usr/lib64/libc.so.6
#1 0x00007f21b734fd6f in do_system () from /usr/lib64/libc.so.6
#2 0x00007f21b9bfc3c6 in TUnixSystem::StackTrace() () from /opt/root/cur/lib/libCore.so
#3 0x00007f21b9bf9705 in TUnixSystem::DispatchSignals(ESignals) () from /opt/root/cur/lib/libCore.so
#4
#5 0x0000000000000000 in ?? ()
#6 0x00007f21b7bf1322 in RooAbsArg::treeNodeServerList(RooAbsCollection*, RooAbsArg const*, bool, bool, bool, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#7 0x00007f21b7bf880d in RooAbsArg::getObservables(RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#8 0x00007f21b7dd0362 in RooProdPdf::factorizeProduct(RooArgSet const&, RooArgSet const&, RooLinkedList&, RooLinkedList&, RooLinkedList&, RooLinkedList&, RooLinkedList&) const () from /opt/root/cur/lib/libRooFitCore.so
#9 0x00007f21b7dd0f20 in RooProdPdf::getPartIntList(RooArgSet const*, RooArgSet const*, char const*) const () from /opt/root/cur/lib/libRooFitCore.so
#10 0x00007f21b7dd5487 in RooProdPdf::getParametersHook(RooArgSet const*, RooArgSet*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#11 0x00007f21b7bf8758 in RooAbsArg::addParameters(RooArgSet&, RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#12 0x00007f21b7bf869d in RooAbsArg::addParameters(RooArgSet&, RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#13 0x00007f21b7bfab85 in RooAbsArg::getParameters(RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#14 0x00007f21b7c3e251 in RooAbsPdf::createNLL(RooAbsData&, RooLinkedList const&) () from /opt/root/cur/lib/libRooFitCore.so
#15 0x00007f21b7c4447a in RooAbsPdf::fitTo(RooAbsData&, RooLinkedList const&) () from /opt/root/cur/lib/libRooFitCore.so
#16 0x00007f21b7c42c78 in RooAbsPdf::fitTo(RooAbsData&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&) () from /opt/root/cur/lib/libRooFitCore.so
#17 0x000055cbe4ef65ad in fit_small_rho(std::vector<double, std::allocator >, std::vector<double, std::allocator >, std::vector<double, std::allocator >, double, char const*) ()
#18 0x000055cbe4ef2964 in main ()
===========================================================

The lines below might hint at the cause of the crash.
You may get help by asking at the ROOT forum https://root.cern.ch/forum
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern.ch/bugs Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
#5 0x0000000000000000 in ?? ()
#6 0x00007f21b7bf1322 in RooAbsArg::treeNodeServerList(RooAbsCollection*, RooAbsArg const*, bool, bool, bool, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#7 0x00007f21b7bf880d in RooAbsArg::getObservables(RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#8 0x00007f21b7dd0362 in RooProdPdf::factorizeProduct(RooArgSet const&, RooArgSet const&, RooLinkedList&, RooLinkedList&, RooLinkedList&, RooLinkedList&, RooLinkedList&) const () from /opt/root/cur/lib/libRooFitCore.so
#9 0x00007f21b7dd0f20 in RooProdPdf::getPartIntList(RooArgSet const*, RooArgSet const*, char const*) const () from /opt/root/cur/lib/libRooFitCore.so
#10 0x00007f21b7dd5487 in RooProdPdf::getParametersHook(RooArgSet const*, RooArgSet*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#11 0x00007f21b7bf8758 in RooAbsArg::addParameters(RooArgSet&, RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#12 0x00007f21b7bf869d in RooAbsArg::addParameters(RooArgSet&, RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#13 0x00007f21b7bfab85 in RooAbsArg::getParameters(RooArgSet const*, bool) const () from /opt/root/cur/lib/libRooFitCore.so
#14 0x00007f21b7c3e251 in RooAbsPdf::createNLL(RooAbsData&, RooLinkedList const&) () from /opt/root/cur/lib/libRooFitCore.so
#15 0x00007f21b7c4447a in RooAbsPdf::fitTo(RooAbsData&, RooLinkedList const&) () from /opt/root/cur/lib/libRooFitCore.so
#16 0x00007f21b7c42c78 in RooAbsPdf::fitTo(RooAbsData&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&, RooCmdArg const&) () from /opt/root/cur/lib/libRooFitCore.so
#17 0x000055cbe4ef65ad in fit_small_rho(std::vector<double, std::allocator >, std::vector<double, std::allocator >, std::vector<double, std::allocator >, double, char const*) ()
#18 0x000055cbe4ef2964 in main ()
===========================================================

Command exited with non-zero status 139

[ynikitenko]

Accidentally it was sent during the writing of this bug, so I had to edit that afterwards, sorry. I tried to search for the bug, but didn't do that extensively, because there might be a host of variants of this bug (I searched only for this title).

I have to say that even if you oblige users to define all variables separately (which would be strange), this is a bit inconsistent with the fact that RooArgList can be defined inside the constructor of RooProdPdf.

[jalopezg-git]

At first sight, RooArgList constructor is taking references to temporaries. I don't know the implementation details of RooArgList, but that might be causing problems.

Assigning to @guitargeek so that he can take a look.

[guitargeek]

Hmm that's not good that you get a crash like this.

The thing is that the RooArgSet and RooArgList created with the constructor where you pass multiple elements are non-owning collections, so they should never passed temporaries. It is also possible to have an owning RooArgList/Set, but in this case it would own all the elements and it would not cover the case where you pass a mix of temporaries and non-temporaries.

A possible solution would be to have a mechanism that detects which arguments need to be owned and which not, but I think that would be too convoluted and not really necessary.

However, we can easily change it such that you get a nice exception with useful information text if the RooArgSet/List constructor detects that one argument is a temporary.

Does that sound like an acceptable solution to you?

[ynikitenko]

"RooArgList created with the constructor where you pass multiple elements are non-owning collections" - it looks like a not very good behaviour. Why can't it be consistent between all constructors?

"It is also possible to have an owning RooArgList/Set, but in this case it would own all the elements" - maybe it would be better than to completely prohibit owning? I think it may be used more often than a mixture of owned/not-owned elements. However, it would still require some code.

I would also add about RooArgList, that it would be great to handle an arbitrary number of elements. Is it possible? I have 10 pdfs, and I have to add the last one after the RooArgList constructor using the add method (because the constructor supports at maximum 9 args).
If one rewrites the constructor, maybe the owning issues might be re-implemented as well.
RooArgList/Set looks like a very general container, so it would benefit from being even more general.

Anyway if you can detect this issue and raise an exception, this would be a good solution. Thanks!

[guitargeek]

Hi! It is consistent for all constructors. By default, the RooAbsCollections are always non-owning. You can think of them as std::vector<RooAbsArg*>, which is what is actually used under the hood.

To make them not too different from STL containers, I think it's good to not implement some mixed ownership model. I also don't think it would be worth it to try to infer if it's a fully-owning collection or a non-owning collection on the fly, because also this will cause further divergence from the behavior of the underlying std::vector<RooAbsArg*>.

I think it would be best if we are more restrictive here, and make it such that the code doesn't even compile when you try to initialize a RooArgList/Set from a temporary. I opened a PR where this is implemented.

And yes, it is possible to construct them with an arbitrary number of elements!

[ynikitenko]

I see. Great speed!

No, when I tried to initialize RooArgList with 10 pdfs, the compiler complained that it can't find a constructor for that. See the code. These constructors are hard-coded for specific numbers of elements.

[guitargeek]

Sorry, I missed that you are using ROOT 6.22. Right, the templated constructor for an arbitrary number of elements only got introduced with ROOT 6.24.