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

[ntuple] RRecordField creation crashes when TStreamerInfo has insufficient information #14084

Closed
enirolf opened this issue Nov 21, 2023 · 5 comments · Fixed by #14181
Closed
Assignees

Comments

@enirolf
Copy link
Contributor

enirolf commented Nov 21, 2023

N.B: This issue is a continuation of the discussion started in #13904 (comment)

It seems like additional dictionary entries are required for ROOT builds with runtime C++ modules disabled (i.e. builds with -Druntime_cxxmodules=off). This is observed in the ntuple_types.cxx tests for std::map fields. These fields have an inner field of type std::pair representing the key and value of each item. For this specific build configuration, it is required to explicitly add the dictionary entry for this std::pair type. In other words, for a std::map<char, long>, we need to define the following two dictionary entries:

#pragma link C++ class std::map<char, long>+;
#pragma link C++ class std::pair<char, long>+;

instead of only the first one. This does not only pertain to std::map. For example, the same error is observed for a std::set<std::pair<int, CustomStruct>> field (but not forstd::set<std::pair<int, int>>, for example).

Steps to reproduce

  1. Use a ROOT master build with -Druntime_cxxmodules=off.
  2. Remove/comment out the following dictionary entries:
    #pragma link C++ class std::pair<char, long>+;
    #pragma link C++ class std::pair<char, std::int64_t>+;
    #pragma link C++ class std::pair<char, std::string>+;
    #pragma link C++ class std::pair<int, CustomStruct>+;
    #pragma link C++ class std::pair<int, std::vector<CustomStruct>>+;
    #pragma link C++ class std::pair<float, std::map<char, std::int32_t>>+;
    #pragma link C++ class std::pair<char, std::int32_t>+;
  3. Add the following snippet to the constructor for RPairField, after this line:
    fSize = fClass->Size();
    (also make sure to `#include <TVirtualStreamerInfo.h>:
if (fClass->GetDataMember("first") == nullptr || fClass->GetDataMember("second") == nullptr) {
  std::cerr << "The TClass for " << GetType() << " is in state: " << fClass->GetState() << " and has: \n";
  fClass->GetListOfDataMembers()->ls();
  fClass->GetStreamerInfo()->ls();
  throw RException(R__FAIL("Insufficient information for " + GetType()));
}

Observed output

From the snippet added above:

The TClass for std::pair<char,std::int64_t> is in state: 2 and has: 
OBJ: TListOfDataMembers	TListOfDataMembers	List of TDataMembers for a class : 0

StreamerInfo for class: pair<char,long>, checksum=0xb5fb752
  char           first           offset=  0 type= 1 Emulation           
  long           second          offset=  8 type= 4 Emulation           
   i= 0, first           type=  1, offset=  0, len=1, method=0
   i= 1, second          type=  4, offset=  8, len=1, method=0
unknown file: Failure
C++ exception with description "Insufficient information for std::pair<char,std::int64_t>

Stack trace from GDB

(When the above snippet is not included)

#0  0x00007ffff730c6fe in TDataMember::GetOffset (this=0x0) at /home/florine/cern/root/src/core/meta/src/TDataMember.cxx:445
#1  0x00007ffff7d2a7c6 in ROOT::Experimental::RPairField::RPairField (this=0x26dc5d0, fieldName=..., itemFields=...)
    at /home/florine/cern/root/src/tree/ntuple/v7/src/RField.cxx:2936
#2  0x00007ffff7d3181f in std::make_unique<ROOT::Experimental::RPairField, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::array<std::unique_ptr<ROOT::Experimental::Detail::RFieldBase, std::default_delete<ROOT::Experimental::Detail::RFieldBase> >, 2ul>&> () at /usr/include/c++/13/bits/unique_ptr.h:1070
#3  ROOT::Experimental::Detail::RFieldBase::Create (fieldName="_0", canonicalType="std::pair<char,std::int64_t>", 
    typeAlias="std::pair<char,std::int64_t>") at /home/florine/cern/root/src/tree/ntuple/v7/src/RField.cxx:440
#4  0x00007ffff7d32f1c in ROOT::Experimental::Detail::RFieldBase::Create (fieldName="_0", typeName="std::pair<char,std::int64_t>")
    at /home/florine/cern/root/src/tree/ntuple/v7/src/RField.cxx:356
#5  0x00007ffff7d32224 in ROOT::Experimental::Detail::RFieldBase::Create (fieldName="test", canonicalType="std::map<char,int64_t>", 
    typeAlias="std::map<char,int64_t>") at /home/florine/cern/root/src/tree/ntuple/v7/src/RField.cxx:478
#6  0x00007ffff7d32f1c in ROOT::Experimental::Detail::RFieldBase::Create (fieldName="test", typeName="std::map<char, int64_t>")
    at /home/florine/cern/root/src/tree/ntuple/v7/src/RField.cxx:356
@enirolf enirolf changed the title [io] Additional dictionaries are necessary with nortcxxmod enabled [io] Additional dictionaries are necessary without runtime_cxxmodules Nov 21, 2023
@pcanal
Copy link
Member

pcanal commented Nov 21, 2023

From the snippet added above:

So we see that the list of data member is empty but the StreamerInfo is correct. This is the intended state of an auto-generated TClass for std::pair without its information loaded in the interpreter (that loading is more likely to have happened with module enabled).

So technically the issue is that the test is too stringent and need to be changed ... However the code that follow may actually rely on the data member information, if it does then it also needs to be updated to use the TStreamerInfo information instead.

@pcanal
Copy link
Member

pcanal commented Nov 21, 2023

And indeed, it is the code at tree/ntuple/v7/src/RField.cxx:2936 that needs to be tweaked:

   fOffsets[0] = fClass->GetDataMember("first")->GetOffset();
   fOffsets[1] = fClass->GetDataMember("second")->GetOffset();

to (possibly conditionally) use something like:

   auto elem1 = fClass->GetStreamerInfo()->GetStreamerElement("first", fOffsets[0]);
   auto elem2 = fClass->GetStreamerInfo()->GetStreamerElement("second", fOffsets[1]);
   if (!elem1 || !elem2)
       throw RException(R__FAIL("Insufficient information in StreamerInfo for " + GetType()));
}

@pcanal
Copy link
Member

pcanal commented Nov 21, 2023

std::set<std::pair<int, CustomStruct>> field (but not forstd::set<std::pair<int, int>>

The difference is likely due to the fact that we provide the dictionary for a small subset of STL collection of numerical type through core/clingutils/src/*LinkDef.h

@enirolf
Copy link
Contributor Author

enirolf commented Nov 22, 2023

Thanks for the pointers, that makes sense! There are two things that still confuse me, however:

  1. When the std::set fields PR was merged these errors were not yet present in the CI, even though the test includes a std::set<pair<int, CustomStruct>>. In fact, if I go back to the commit right before the maps were added (cddecc1), this test passes without having to have the dictionary entry for the pair. I cannot pinpoint anything that got added alongside the maps that might change this, but perhaps these additions change the internal build machinery just enough for it to also break without modules enabled.
  2. There are no std::pair dictionaries in core/clingutils/src/*LinkDef.h. There are a few defined in other places, but none with the numerical types mentioned here.

In any case, I will add the above suggestion to make sure that at least we can fail more gracefully (I have not checked with RTupleField but I imagine this will also be problematic, so probably this change should be added to RRecordField).

@enirolf enirolf self-assigned this Nov 22, 2023
@enirolf enirolf changed the title [io] Additional dictionaries are necessary without runtime_cxxmodules [ntuple] RRecordField creation crashes when TStreamerInfo has insufficient information Nov 22, 2023
@enirolf enirolf linked a pull request Dec 11, 2023 that will close this issue
Copy link

Hi @jblomer, @enirolf,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants