Skip to content

Deal with multi schema layout for a pair (caused enum underlying type)#10132

Merged
pcanal merged 12 commits intoroot-project:masterfrom
pcanal:master-issue-10131
Mar 21, 2022
Merged

Deal with multi schema layout for a pair (caused enum underlying type)#10132
pcanal merged 12 commits intoroot-project:masterfrom
pcanal:master-issue-10131

Conversation

@pcanal
Copy link
Copy Markdown
Member

@pcanal pcanal commented Mar 15, 2022

This fixes #10131.

The core issue is that TDataMember::Init and TStreamerInfo::GenerateInfoForPair were not consistent. TDataMember::Init was ignoring the underlying type of an enum while the newer TStreamerInfo::GenerateInfoForPair was taking it in consideration. In the reported case, it meant that some of the pair's TStreamerInfo recorded the type as being 'signed intwhile other was recording the type asunsigned int`.

In addition the whole infrastructure assumed (but only in "some/most" places) that the TStreamerInfo for a std::pair could never change and the infrastructure was also inconsistent on knowing whether the schema layout for std::pair is version of non-versioned.

NOTE: The last commit is might cause the user classes to require a version incrementing when using enums ... (i.e. this commit might be removed) ... it was indeed removing ... in addition changing the stored type of the enum changes the schema layout but does not (sometimes?) change (yet?) the checksum so it leads to incorrect reading of data ....

@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

pcanal added 6 commits March 16, 2022 15:33
In TGenCollectionProxy's init, hen needing to replace the
TClass for std::pair by a 'better' version, make sure to
import all the existing StreamerInfo.

Also, make TClass::ForceReload public so that TGenCollectionProxy can use it.
If we don't have a dictionary for a std::pair and do not have (in the call to TClass::Init) the hints
necessary to build it correctly (offset and size), then look for a related collection (STL associated
container) that would induce the generation of the std::pair's TClass (it's collection proxy has
the necessary information).
Because enums can change underlying type (especially since gathering
the enum underlying info was inconsistent between TStreamerInfo::GenerateInfoForPair
and TDataMember::Init, the schema layout for a std::pair *can* change ....
@pcanal pcanal force-pushed the master-issue-10131 branch from 1635627 to b556649 Compare March 17, 2022 03:37
@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Build failed on mac11/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

@phsft-bot
Copy link
Copy Markdown

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2022-03-17T22:59:22.613Z] /data/sftnight/workspace/root-pullrequests-build/root/io/io/src/TStreamerInfo.cxx:2138:23: warning: declaration of ‘offset’ shadows a previous local [-Wshadow]

Failing tests:

@phsft-bot
Copy link
Copy Markdown

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@pcanal
Copy link
Copy Markdown
Member Author

pcanal commented Mar 18, 2022

@phsft-bot build

@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2022-03-18T04:18:16.393Z] /home/sftnight/build/workspace/root-pullrequests-build/root/io/io/src/TStreamerInfo.cxx:2138:23: warning: declaration of ‘offset’ shadows a previous local [-Wshadow]

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2022-03-18T04:23:52.746Z] /mnt/build/workspace/root-pullrequests-build/root/io/io/src/TStreamerInfo.cxx:2138:23: warning: declaration of ‘offset’ shadows a previous local [-Wshadow]

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2022-03-18T04:47:30.980Z] /data/sftnight/workspace/root-pullrequests-build/root/io/io/src/TStreamerInfo.cxx:2138:23: warning: declaration of ‘offset’ shadows a previous local [-Wshadow]

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2022-03-18T04:31:11.804Z] /home/sftnight/build/workspace/root-pullrequests-build/root/io/io/src/TStreamerInfo.cxx:2138:23: warning: declaration of ‘offset’ shadows a previous local [-Wshadow]

@phsft-bot
Copy link
Copy Markdown

Build failed on mac1015/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

Copy link
Copy Markdown
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this? And requesting my review you get a couple of questions for free :-)

// In this case we initialised this TClass instance starting from the fwd declared state
// and we know we have no dictionary: no need to warn
::Warning("TClass::Init", "no dictionary for class %s is available", fName.Data());
const bool ispairbase = TClassEdit::IsStdPairBase(fName.Data()) && !IsFromRootCling();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not immediately obvious to me why this needs && !IsFromRootCling(). Sorry. Maybe add a comment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is explained on line 3079+. Basically in rootcling we should not interfere with explicit dictionary request.

// We are in the case of an 'emulated' class.

if (fOnFileClassVersion >= 2) {
if (fOnFileClassVersion >= 2 && !isStdPair) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is fOnFileClassVersion for an existing pair?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the failing case it was 2.

Make sure that an old StreamerInfo transfered from an emulated TClass into
a compiled or synthetic TClass is properly reset (correct TStreamerElement
offfset and correct size).
@pcanal pcanal force-pushed the master-issue-10131 branch from cbaeedf to 9678405 Compare March 18, 2022 13:35
@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@pcanal pcanal merged commit 4dc5dd9 into root-project:master Mar 21, 2022
@pcanal pcanal deleted the master-issue-10131 branch March 21, 2022 16:51
pcanal added a commit to root-project/roottest that referenced this pull request Mar 21, 2022
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.

Open too many different non-versioned layouts for pair

3 participants