[meta] Fix the crash in tutorial-analysis-dataframe-df014_CSVDataSource-py #18625

hageboeck · 2025-05-06T14:58:35Z

When looking up a TClass via name vs via typeid, the lookup proceeded
without default template arguments in the former, and with default
arguments in the latter case.
This lead to a situation where
tclass->GetActualClass(std::map<x,y>*)
could delete itself while performing the lookup, since a new instance
was created that replaced the existing instance.

In the df014 tutorial, cppyy was essentially calling

   auto first = TClass::GetClass("unordered_map<string,char>", true, true);
   std::unordered_map<std::string,char> map;
   auto second = first->GetActualClass(&map);

Before this PR, first != second, and first was being deleted while first->GetActualClass was still running.

Now, the class lookup in such a case is retried without the default STL arguments, which should lead to first == second in the above example. A test for this behaviour was added.

Thanks to @pcanal for advice.

hageboeck · 2025-05-06T15:08:23Z

Another observation:
When generating dictionaries, the following replacement also leads to a successful TClass lookup:

  vector<pair<const void*,function<void(const string&)> > > instead of
  vector<pair<const void*,function<void (const string&)> > >

It looks like this PR is also speeding up the lookup of vector<pair<>>, because its name wasn't normalised correctly.

pcanal · 2025-05-06T15:26:17Z

core/meta/src/TClass.cxx

@@ -3181,6 +3181,14 @@ TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hi
      if (normalizedName != name) {
         cl = (TClass*)gROOT->GetListOfClasses()->FindObject(normalizedName.c_str());

+         if (!cl) {
+            // The normalized name above might still contain the default template arguments


That part is odd. Do we know why TClassEdit::GetNormalizedName does not strip the default parameter in this case.

Side note, default-Parameter-related issues:
#7470
#15996
https://its.cern.ch/jira/browse/ROOT-5862
And maybe other loosely related:
#18401
#8470
https://its.cern.ch/jira/browse/ROOT-10311

#7470 Likely different as it is about the forward declaration written in the rootmap file
#15996 Is about the customization of which default argument to drop from the normalization name
#8470 Should not play a role here (it is about handling of unknown types)

its.cern.ch/jira/browse/ROOT-10311 is unrelated (it is about look for an non-existing type) and may have already been solved

its.cern.ch/jira/browse/ROOT-5862
#18401
might or might not be related.

Thanks for the insight!

Side note:
ROOT-10311 is still an issue in master.

TTree::TTree constructor function gets auto-stripped into TTree (it thinks first one might be a namespace) and then GetClass("TTree") returns a valid pointer when it should not

puzzling.

root [0] string s="unordered_map<string,char>" (std::string &) "unordered_map<string,char>" root [1] string n (std::string &) "" root [2] TClassEdit::GetNormalizedName(n, s); root [3] n (std::string &) "unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >"

Digging deeper. There is a difference in interpreted and compiled code. This snippet:

std::string s = "unordered_map<string,char>"; std::string n; TClassEdit::GetNormalizedName(n, s); std::cout << s << " --> " << n << std::endl;

if compiled in an executable prints unordered_map<string,char> --> unordered_map<string,char>.
If interpreted, e.g. at the prompt, prints unordered_map<string,char> --> unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >

Digging deeper. There is a difference in interpreted and compiled code. This snippet:

std::string s = "unordered_map<string,char>"; std::string n; TClassEdit::GetNormalizedName(n, s); std::cout << s << " --> " << n << std::endl;

if compiled in an executable prints unordered_map<string,char> --> unordered_map<string,char>. If interpreted, e.g. at the prompt, prints unordered_map<string,char> --> unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >

I see this, too, in a test, but the difference here is whether the test runs in isolation

./testClassEdit --gtest_filter=*TestName*

or whether it runs after the other tests:

./testClassEdit

I have a fix for when it runs in isolation, but I'm digging now for the case where other queries to the type system have happened before.

That part is odd. Do we know why TClassEdit::GetNormalizedName does not strip the default parameter in this case.

@pcanal Now we know, indeed:

The trailing const in the allocator template was throwing off the type normalisation.

And the __cxx11::basic_string was preventing the normalisation to string.

Those two are solved now, and I get a consistent TClass now in this test (part of the PR now):

auto first = TClass::GetClass("unordered_map<string,char>", true, true); std::unordered_map<std::string, char> map; auto second = first->GetActualClass(&map); EXPECT_EQ(first, second);

Now there seems to be only one remaining issue, and I'm trying to find out if we can live with it. It's about the stripping (or not) of the default template arguments:

Digging deeper. There is a difference in interpreted and compiled code. This snippet:

std::string s = "unordered_map<string,char>"; std::string n; TClassEdit::GetNormalizedName(n, s); std::cout << s << " --> " << n << std::endl;

if compiled in an executable prints unordered_map<string,char> --> unordered_map<string,char>. If interpreted, e.g. at the prompt, prints unordered_map<string,char> --> unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >

The code responsible is this:

root/core/foundation/src/TClassEdit.cxx

Lines 903 to 911 in b30a5f2

if (gInterpreterHelper) {

// See if the expanded name itself is a typedef.

std::string typeresult;

if (gInterpreterHelper->ExistingTypeCheck(norm_name, typeresult)

|| gInterpreterHelper->GetPartiallyDesugaredNameWithScopeHandling(norm_name, typeresult)) {

if (!typeresult.empty()) norm_name = typeresult;

}

}

Before these lines, and with the changes of the PR applied, the unordered_map<very long list> collapses to unordered_map<string,char>, as the documentation of the functions claims:

root/core/foundation/src/TClassEdit.cxx

Lines 853 to 861 in b30a5f2

////////////////////////////////////////////////////////////////////////////////

/// Return the normalized name. See TMetaUtils::GetNormalizedName.

///

/// Return the type name normalized for ROOT,

/// keeping only the ROOT opaque typedef (Double32_t, etc.) and

/// removing the STL collections default parameter if any.

///

/// Compare to TMetaUtils::GetNormalizedName, this routines does not

/// and can not add default template parameters.

However, when gInterpreterHelper != nullptr, the norm_name runs through TCling, and comes back as unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >. This contradicts the docstring, but it explains the difference between interpreted and compiled code that Danilo observed.
This also explains why my test ./testClassEdit --gtest_filter=*TestName* yields the short name, and ./testClassEdit yields the long name with default arguments. Interestingly, the TClass lookup seems to work now in both cases, so the origin of the df104_CSVDataSource.py crash seems to be fixed nevertheless.

That being said, I would like to ask @pcanal and @dpiparo to weigh in what the normalised name should be (see next conversation).

std::string s = "unordered_map<string,char>"; std::string n; TClassEdit::GetNormalizedName(n, s); std::cout << s << " --> " << n << std::endl;

if compiled in an executable prints 1unordered_map<string,char> --> unordered_map<string,char>`.
If interpreted, e.g. at the prompt, prints 1unordered_map<string,char> --> unordered_map<string,char,hash,equal_to,allocator<pair<const string,char> > >1

Since this is GetNormalizedName I would expect that the default template parameter to be dropped for unordered_map.

github-actions · 2025-05-06T19:09:57Z

Test Results

18 files 18 suites 3d 8h 44m 6s ⏱️
2 739 tests 2 560 ✅ 0 💤 179 ❌
47 881 runs 44 867 ✅ 0 💤 3 014 ❌

For more details on these failures, see this check.

Results for commit fbecfcc.

♻️ This comment has been updated with latest results.

When looking up a TClass via name vs via typeid, the lookup proceeded without default template arguments in the former, and with default arguments in the latter case. This lead to a situation where tclass->GetActualClass(std::map<x,y>*) could delete itself while performing the lookup, since a new instance was created that replaced the existing instance. Change in TClassEdit: - Improve the handling of a trailing const in the allocator definition. - Make default allocator check more robust w.r.t. spaces in type definition. - Improve detection of std::basic_string, std::__cxx11::basic_string and similar types with/without const and std::

hageboeck · 2025-05-08T10:35:15Z

core/foundation/test/testClassEdit.cxx

+   const auto target = "unordered_map<string,char>";
+
+   std::string out;
+   TClassEdit::GetNormalizedName(out, in);
+   EXPECT_STREQ(target, out.c_str());
+
+   TClassEdit::GetNormalizedName(out, in_cxx11);
+   EXPECT_STREQ(target, out.c_str());
+}


@pcanal @dpiparo here is the test for the normalised name, which passes when the interpreterHelper isn't instantiated yet, but fails when it is.

In the former case, we get
unordered_map<string,char>
and in the latter:
unordered_map<string,char,hash<string>,equal_to<string>,allocator<pair<const string,char> > >

What should be the correct result?

unordered_map<string,char> is the normalized name.

OK, in this case, after calling the interpreter, we just might have to fixup what it returns, because the type normalisation in TClingUtils explicitly specifies that it doesn't remove the default template parameters:

root/core/clingutils/src/TClingUtils.cxx

Lines 4130 to 4137 in b30a5f2

////////////////////////////////////////////////////////////////////////////////

/// Return the type normalized for ROOT,

/// keeping only the ROOT opaque typedef (Double32_t, etc.) and

/// adding default template argument for all types except those explicitly

/// requested to be drop by the user.

/// Default template for STL collections are not yet removed by this routine.

clang::QualType ROOT::TMetaUtils::GetNormalizedType(const clang::QualType &type, const cling::Interpreter &interpreter, const TNormalizedCtxt &normCtxt)

What do you think @pcanal and @dpiparo?

Right. This does not return the normalized name but the type which indeed might/should still have the default template arguments. If I remember correctly the are meant to be remove lexicographicaly in a higher level routine.

I.e GetNormalizedType can/should return with the default parameter
but in the end GetNormalizedName needs to drop them (for STL collections) both compiled and interpreted code.

This code should be unnecessary once name normalisation is fixed.

- Test that two TClass queries for unordered_map yield the same instance. - Add string type normalisation test both for __cxx11::basic_string and std::basic_string as suggested by Danilo.

ferdymercury · 2025-05-08T16:55:00Z