[Rivet] Dangerous casting to FinalState

[Chris Pollard]

Hi,

Bringing this back up over here...

Taking what Andy says he's implemented at face value (haven't looked at the
code), Frank: I'm not sure how your original Example1 and Example2c even
compile. You shouldn't be allowed to copy by value anymore. Or did I miss
something?

Chris

On Wed, Jul 1, 2015 at 10:25 PM, Andy Buckley <andy.buckley at cern.ch> wrote:

> A late follow-up on this: on the trunk (i.e. default branch) I just
> added a *tiny* tweak to declare a private virtual operator=(const
> Projection&) method on the Projection base class. It could possibly be
> done better than I have, but I believe this should make it impossible to
> copy-assign any classes derived from Projection.
>
> This should ban accidental copies by value. Everything in the Rivet
> trunk recompiled just fine, but can you test it with your pathological
> examples, Frank?
>
> Andy
>
>
> On 20/05/15 14:19, Frank Siegert wrote:
> > Hi David,
> >
> > any ideas? Do you need more information from me?
> >
> > Cheers,
> > Frank
> >
> >
> >
> > On 7 May 2015 at 14:18, Frank Siegert <frank.siegert at cern.ch> wrote:
> >> Hi David,
> >>
> >> We want to apply the attached patch, which hides the dangerous slicing
> >> constructors for classes derived from FinalState. I then tested
> >> various options of initialising/copying projections, all of which do
> >> the correct thing now (errors during compilation for slicing
> >> constructors, no errors for faithful ones) except for one:
> >>
> >> ZFinder zfinder(fs, cut, PID::ELECTRON, 65*GeV, 115*GeV, 0.2,
> >> ZFinder::CLUSTERNODECAY, ZFinder::TRACK);
> >> VetoedFinalState remainder = zfinder.remainingFinalState();
> >> addProjection(remainder, "RFS");
> >>
> >> I would expect that this should work fine, but at runtime I get:
> >>   Error in MY_RFSTEST2c::init method: No projections registered for
> >> parent 0x7ffe07be8840
> >>
> >> Attached is the analysis code and a HepMC file with one event to
> >> reproduce it quickly after having applied the patch to Rivet:
> >>   $ rivet-buildplugin MY_RFSTEST2c
> >>   $ rivet --pwd -a MY_RFSTEST2c file.hepmc2g
> >>
> >> It would be great if you would have an idea what is going wrong with
> >> the projection cloning/registration.
> >>
> >> Cheers,
> >> Frank
> >>
> >>
> >> On 7 May 2015 at 10:50, David Grellscheid
> >> <david.grellscheid at durham.ac.uk> wrote:
> >>> Hi all,
> >>>
> >>> I'm sorry I haven't followed this from the beginning and I'm confused
> >>> about if we're talking about registered projections or locally created
> >>> ones, as the required copying behaviour is more tricky in the first
> >>> case. Can someone summarise where I can find the current status?
> >>>
> >>>   - what you see as the problem
> >>>   - what you would expect instead
> >>>
> >>> Thanks,
> >>>
> >>>   David
> >>>
> >>> On 04/05/15 17:28, Andy Buckley wrote:
> >>>> On 04/05/15 17:04, Frank Siegert wrote:
> >>>>> Hi Andy,
> >>>>>
> >>>>>> Does that problem disappear if you define remainder as a reference,
> >>>>>> either as VetoedFinalState& or FinalState&?
> >>>>>
> >>>>> Yes, even for FinalState& the problem disappears. That's case 2a in
> my
> >>>>> original examples.
> >>>>>
> >>>>>> It would also be useful if you could use the -l TRACE flag (if I
> >>>>>> remember the syntax correctly) when running, and send the resulting
> log
> >>>>>> so we can see if that parent address corresponds to something.
> Offhand
> >>>>>> it looks like a bit of an odd address value: is this the sort of
> pointer
> >>>>>> value that corresponds to a non-heap-allocated local?
> >>>>>
> >>>>> I have no idea on the pointer value here, but I have put the full
> log into
> >>>>> http://cern.ch/fsiegert/tmp/trace.log
> >>>>
> >>>> Thanks!
> >>>>
> >>>>>> When assigning a Projection like this, a copy will be made. Does
> >>>>>> VetoedFinalState have a copy assignment operator, copy constructor,
> etc.
> >>>>>> defined as per the Rule of Three
> >>>>>> (
> http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29)
> >>>>>> to make this safe? I genuinely can't remember if we do that, since
> >>>>>> copying of projections isn't really the idea. If we don't perhaps we
> >>>>>> could make it a rule in Projection implementation to always hide
> (i.e.
> >>>>>> make private) the copy constructor and copy assignment operators to
> >>>>>> simply ban copy by value and hence force use of references. I
> forget if
> >>>>>> this can be done on the Projection base class or if each new derived
> >>>>>> class would need to explicitly re-hide them. Anyway, food for
> thought
> >>>>>> albeit not quite at black belt level ;-)
> >>>>>
> >>>>> We don't seem to explicitly define any of the three (copy assignment
> >>>>> operator, copy constructor, destructor). I guess the copying that you
> >>>>> mean stems from the clone() function, which probably is using the
> >>>>> constructor from a FinalState instead of a copy constructor? If our
> >>>>> projection registration/cloning can't cope with normal C++ copying
> >>>>> then we should indeed either fix it or hide the copy
> >>>>> constructors/operators. I have to admit that I didn't even realise
> >>>>> that we are internally cloning projections upon registration, and
> >>>>> whatever magic is connected to that. It's probably nothing
> >>>>> super-urgent to fix, since at least it won't give wrong results. But
> >>>>> I'd expect the black-belts to understand this much better than I
> >>>>> (challenge!).
> >>>>
> >>>> Hmm, good point about the cloning. I was just thinking of the local
> copy
> >>>> that occurs if the lvalue isn't a reference type in your example. I'll
> >>>> have to have a look at the current clone implementations to know how
> >>>> safe they are -- in general if we're not defining copy constructor,
> >>>> assignment, and destructor then any projection which holds pointer
> >>>> members could be dodgy in copying. I think we've covered the
> destructor
> >>>> angle, but not necessarily the copying ones... so we might want to ban
> >>>> those, depending on how clone() is currently written. Old rusty
> memories
> >>>> for me...
> >>>>
> >>>> Andy
> >>>>
> > _______________________________________________
> > Rivet mailing list
> > Rivet at projects.hepforge.org
> > https://www.hepforge.org/lists/listinfo/rivet
> >
>
>
> --
> Dr Andy Buckley, Lecturer / Royal Society University Research Fellow
> Particle Physics Expt Group, University of Glasgow
> _______________________________________________
> Rivet mailing list
> Rivet at projects.hepforge.org
> https://www.hepforge.org/lists/listinfo/rivet
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.hepforge.org/lists-archive/rivet/attachments/20150707/76eea5f6/attachment.html>