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

Add virtual dtors for MakeProvenanceReader & ExpressionElementFinderBase #18190

Merged
merged 1 commit into from Apr 4, 2017

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Apr 4, 2017

CMSSW GCC 7.0.1 builds now follows jemalloc (dev branch) and also has a
new TCMalloc. Both of these have C++14 sized deallocation feature
enabled. This means that if possible operator delete will be called with
size argument, which will help allocator to reduce the time to free the
object. E.g. this is not possible for incomplete types which size is
unknown at a time.

GCC 7.0.1 builds contain an additional build of jemalloc
(jemalloc-debug) which contains extra asserts and checks for developers.
It helped to find two places were operate delete is called using base
class type instead of actual type. E.g., this means that wrong size (8
bytes) were passed to operator delete instead of actual type (16 bytes).

This seems to be undefined behavior in C++14. From [expr.delete] (5.3.5
Delete):

In the first alternative (delete object), if the static type of the
object to be deleted is different from its dynamic type, the static type
shall be a base class of the dynamic type of the object to be deleted
and the static type shall have a virtual destructor or the behavior is
undefined.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

CMSSW GCC 7.0.1 builds now follows jemalloc (dev branch) and also has a
new TCMalloc. Both of these have C++14 sized deallocation feature
enabled. This means that if possible operator delete will be called with
size argument, which will help allocator to reduce the time to free the
object. E.g. this is not possible for incomplete types which size is
unknown at a time.

GCC 7.0.1 builds contain an additional build of jemalloc
(jemalloc-debug) which contains extra asserts and checks for developers.
It helped to find two places were operate delete is called using base
class type instead of actual type. E.g., this means that wrong size (8
bytes) were passed to operator delete instead of actual type (16 bytes).

This seems to be undefined behavior in C++14. From [expr.delete] (5.3.5
Delete):

    In the first alternative (delete object), if the static type of the
    object to be deleted is different from its dynamic type, the static type
    shall be a base class of the dynamic type of the object to be deleted
    and the static type shall have a virtual destructor or the behavior is
    undefined.

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2017

A new Pull Request was created by @davidlt for master.

It involves the following packages:

CommonTools/Utils
IOPool/Input

@perrotta, @smuzaffar, @Dr15Jones, @monttj, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @wddgit this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@davidlt
Copy link
Contributor Author

davidlt commented Apr 4, 2017

More details are available in commit notes, which are not showed on GitHub.

It was debugged using TopQuarkAnalysis/TopKinFitter/test/ttFullLepKinSolutionProducer_cfg.py

@Dr15Jones
Copy link
Contributor

Please test

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18893/console Started: 2017/04/04 07:55

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2017

@perrotta
Copy link
Contributor

perrotta commented Apr 4, 2017

+1
Virtual destructors added in these two classes

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.

None yet

5 participants