Prevent type sufix to appear in ROOT normalized name #18373

pcanal · 2025-04-11T22:31:43Z

This fixes #18363 and thus cms-sw/cmssw#47697.

The companion PR is root-project/roottest#1302

This PR actually 2 related parts
a - an update to LLVM type printer and its use within ROOT
b - an improvement in the detection of unwanted auto-parsing

a. LLVM update

The update to LLVM allows to completely prevent the addition of type suffixes when printing names rather than the current default (there for some types - and in addition, at least how we use it in ROOT, even for those types not consistently there).

In practice, in the failing case, we were getting this normalized name:

class reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>

instead of

class reco::PFRecHitSoALayout<128,false>::ViewTemplateFreeParams<128,false,true,true>

Note that in the failing case only one of the two integers is suffixed by "UL". I did not tracked down the cause of this discrepancy.
In addition, we also produce alternate spelling of names to ease lookup (by preventing auto-parsing in some trivial case of alternative spelling).

As a side note, another source of confusion is the following.
In the successful run of the example with get:

70:      instance.AdoptAlternate(::ROOT::AddClassAlternate("reco::PFRecHitSoALayout<128,false>","reco::PFRecHitSoA"));
72:      instance.AdoptAlternate(::ROOT::AddClassAlternate("reco::PFRecHitSoALayout<128,false>","reco::PFRecHitSoALayout<128ul, false>"));
119:      instance.AdoptAlternate(::ROOT::AddClassAlternate("reco::PFRecHitSoALayout<128,false>::ViewTemplateFreeParams<128,false,true,true>","reco::PFRecHitSoA::View"));
121:      instance.AdoptAlternate(::ROOT::AddClassAlternate("reco::PFRecHitSoALayout<128,false>::ViewTemplateFreeParams<128,false,true,true>","reco::PFRecHitSoALayout<128ul, false>::ViewTemplateFreeParams<128ul, false, true, true>"));

where the alternate have the suffix ... but in lower case. The alternates here are produced by abi::__cxa_demangle. (the exact same spelling appears for the alternate in the failing case).

pcanal · 2025-04-11T22:36:25Z

@makortel @Dr15Jones Any opinions on the interface to see the auto-parsed classes and to disable auto-parsing in TClass::GetClass.

github-actions · 2025-04-12T01:10:38Z

Test Results

18 files 18 suites 4d 9h 40m 26s ⏱️
2 740 tests 2 740 ✅ 0 💤 0 ❌
47 647 runs 47 647 ✅ 0 💤 0 ❌

Results for commit d3f4129.

♻️ This comment has been updated with latest results.

vgvassilev · 2025-04-12T05:19:17Z

Can we find which commit broke that in llvm?

interpreter/llvm-project/clang/include/clang/AST/PrettyPrinter.h

pcanal · 2025-04-12T18:06:11Z

Can we find which commit broke that in llvm?

'Technically' it is 8dff24d0b687e3b2781e00697d32983a9d4d7b52 but it was completely extended and reverting it now requires a longer diff:

Details

commit 6e5397bbb20381b93d120bcb8bbd11e985f84102
Author: Philippe Canal <[email protected]>
Date:   Thu Apr 10 14:35:54 2025 -0500

    Revert in Spirit 8dff24d0b687e3b2781e00697d32983a9d4d7b52

diff --git a/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp b/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp
index 130bbe8893..80bcaf262d 100644
--- a/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp
+++ b/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp
@@ -108,20 +108,20 @@ static void printIntegral(const TemplateArgument &TemplArg, raw_ostream &Out,
     if (const auto *BT = T->getAs<BuiltinType>()) {
       switch (BT->getKind()) {
       case BuiltinType::ULongLong:
-        Out << Val << "ULL";
-        break;
+//        Out << Val << "ULL";
+//        break;
       case BuiltinType::LongLong:
-        Out << Val << "LL";
-        break;
+//        Out << Val << "LL";
+//        break;
       case BuiltinType::ULong:
-        Out << Val << "UL";
-        break;
+//        Out << Val << "UL";
+//        break;
       case BuiltinType::Long:
-        Out << Val << "L";
-        break;
+//        Out << Val << "L";
+//        break;
       case BuiltinType::UInt:
-        Out << Val << "U";
-        break;
+//        Out << Val << "U";
+//        break;
       case BuiltinType::Int:
         Out << Val;
         break;

However, the improvement in itself is not bad and is actually necessary in other cases when producing code, it just need to be made optional (which is what the commit in this PR does).

hahnjo · 2025-04-12T18:58:21Z

'Technically' it is 8dff24d0b687e3b2781e00697d32983a9d4d7b52

Are you sure this is the right commit? 8dff24d is a downstream commit by Axel.

However, the improvement in itself is not bad and is actually necessary in other cases when producing code, it just need to be made optional (which is what the commit in this PR does).

The code you were touching in your revert is in fact optional, for IncludeType = false. The question is just which code path leads there, see also #17867

Edit: Ah, now I see this is where TemplateParameterList::shouldIncludeTypeForArgument comes into play...

vgvassilev · 2025-04-12T19:10:24Z

Why this cannot go upstream?

pcanal · 2025-04-12T20:57:12Z

Why this cannot go upstream?

I personally do not see any reason why not (It is an opt-in behavior change)

pcanal · 2025-04-12T21:03:46Z

Are you sure this is the right commit? 8dff24d is a downstream commit by Axel.

Yes, it is the one I meant as it 'originally' (or at least that more or what I thought ... I could wrong) started to introduce the suffixes.

The code you were touching in your revert is in fact optional, for IncludeType = false

There is indeed 100% correct ;) and the patch I proposing forces IncludeType to be false; the fact that it is sometimes not false during our calls is the cause of our problems.

Edit: Ah, now I see this is where TemplateParameterList::shouldIncludeTypeForArgument comes into play...

:)

vgvassilev · 2025-04-13T05:59:31Z

Probably the part that we will need to explain is why we need a separate switch and should not be the default behavior.

hahnjo · 2025-04-13T08:02:26Z

Are you sure this is the right commit? 8dff24d is a downstream commit by Axel.

Yes, it is the one I meant as it 'originally' (or at least that more or what I thought ... I could wrong) started to introduce the suffixes.

Our downstream version yes, but not the code that is causing problems / that you were modifying in your "revert": If you look at https://github.com/root-project/root/blame/b41de52bae91bc44a24f132f80a0e973f05b0c35/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp#L107-L135, that code comes from upstream LLVM 13. Our downstream lines are in https://github.com/root-project/root/blame/b41de52bae91bc44a24f132f80a0e973f05b0c35/interpreter/llvm-project/clang/lib/AST/TemplateBase.cpp#L137-L142 and there you can also see that it went through many revisions and doesn't look anymore like the first version in 8dff24d (but only adds it if required).

If you then look at the upstream blame https://github.com/llvm/llvm-project/blame/028429ac452acde227ae0bfafbfe8579c127e1ea/clang/lib/AST/TemplateBase.cpp#L104-L132 you see that it was introduced by llvm/llvm-project@99d63cc (committed by @vgvassilev). That commit also adds TemplateParameterList::shouldIncludeTypeForArgument, which AFAICT was introduced during review in https://reviews.llvm.org/D77598. So what really needs investigating is why the CMS template parameter has dependent type, why upstream thinks this requires printing the type, and whether this is potentially correct and we need to adapt to that behavior in ROOT instead of the other way around.

(And why am I spending so much time explaining to you how git-blame and archeology works...)

hahnjo

Can you please split off the new TClass feature into a separate PR? It should be independent of the template argument printing in Clang.

pcanal · 2025-04-13T17:10:11Z

Our downstream version yes, but not the code that is causing problems / that you were modifying in your "revert":

I have not look at the upstream LLVM and the end result is unchanged:

there is a 'new' behavior in LLVM vaguely relate to Axel's commit
there failure (unwanted suffix) is related to that and to everything you mentioned
adding the option, solve the problem (until we change the name normalization).

So what really needs investigating is why the CMS template parameter has dependent type,

If you want you can explore the details with the standalone reproducer root-project/roottest#1302

and whether this is potentially correct and we need to adapt to that behavior in ROOT instead of the other way around.

Sure but if it is the 'correct' behavior, we still can not adapt ROOT in a short time. We need the name without the suffix (for now). Hence the opt-in proposed behavior which allows to create a consistent no-suffix name.

vgvassilev · 2025-04-13T19:26:03Z

Okay, I think I start understanding where we are going. So in some places we lost the spelling as written in the code and that resulted the suffix to be printed only sometimes. The current approach of dropping the suffix will likely not work for literals that are larger than in and really need the suffix to make it work. We currently have such cases in the dictionaries iirc (mersenne_twister).

Can we find out why the suffix was not printed when the user typed it in the code instead?

hahnjo · 2025-04-14T08:26:56Z

A change that fixes one use case still doesn't make a correct solution. I maintain that more care and deeper investigation is required.

[...] the end result is unchanged:

there is a 'new' behavior in LLVM vaguely relate to Axel's commit

I disagree: Axel's downstream patch adds the type for 64-bit unsigned integers unconditionally if required to represent the literal. llvm/llvm-project@99d63cc adds a suffix for all widths and regardless of the value if IncludeType = true. For me, that's two very different behaviors (admittedly in the same function).

vgvassilev · 2025-04-14T08:39:04Z

I guess this is one of the tricky situations where we have discussed in the past as a theoretical setup.

An experiment needs a fix which ought to be quick. We have a quick fix, which I am pretty sure it is not acceptable upstream. We can't really dedicate a lot of time to turn things upside down in clang due to @pcanal time commitments I suspect.

We have several suboptimal options:

Ask cms to keep a patched version of a patched version of llvm
Merge this as it is, and have more patches of llvm in ROOT
Figure out why we lost the type sugar in that one case, submit a patch upstream, merge it and backport it.

In the past we did not dare with 1); we mostly did 2) and that's why we are where we are now. We rarely tried 3) which arguably suboptimal but a reasonable way forward due to the constrain various people have...

pcanal · 2025-04-14T14:07:51Z

Ask cms to keep a patched version of a patched version of llvm

This is not an option. The problem would affect any code where the class with a dictionary has a template parameter which is a size_t and result is a name normalization that is inconsistent.

Figure out why we lost the type sugar in that one case, submit a patch upstream, merge it and backport it.

I am not sure what you have in mind but the code paths here are the one for 'name normalization' where we don't want the type sugar (except in some cases like Double32_t).

For the record here are 3 stack traces where we need to disallow the priting of the suffix:

Details

    frame #0: 0x0000000110d20990 libCling.so`clang::TemplateArgument::print(clang::PrintingPolicy const&, llvm::raw_ostream&, bool) const [inlined] printIntegral(TemplArg=<unavailable>, Out=0x000000016fdf8fc8, Policy=0x000000016fdf9150, IncludeType=<unavailable>) at TemplateBase.cpp:117:20 [opt]
    frame #1: 0x0000000110d20680 libCling.so`clang::TemplateArgument::print(this=<unavailable>, Policy=0x000000016fdf9150, Out=0x000000016fdf8fc8, IncludeType=<unavailable>) const at TemplateBase.cpp:557:5 [opt]
    frame #2: 0x0000000110d70854 libCling.so`void printTo<clang::TemplateArgument>(llvm::raw_ostream&, llvm::ArrayRef<clang::TemplateArgument>, clang::PrintingPolicy const&, clang::TemplateParameterList const*, bool, unsigned int) [inlined] printArgument(A=0x00000001480096f0, PP=0x000000016fdf9150, OS=0x000000016fdf8fc8, IncludeType=<unavailable>) at TypePrinter.cpp:2029:5 [opt]
    frame #3: 0x0000000110d70844 libCling.so`void printTo<clang::TemplateArgument>(OS=0x000000016fdf92d8, Args=<unavailable>, Policy=0x000000016fdf9150, TPL=0x0000000000000000, IsPack=true, ParmIndex=0) at TypePrinter.cpp:2241:7 [opt]
    frame #4: 0x0000000110d70574 libCling.so`clang::printTemplateArgumentList(OS=<unavailable>, Args=<unavailable>, Policy=<unavailable>, TPL=<unavailable>) at TypePrinter.cpp:2285:3 [opt] [artificial]
    frame #5: 0x0000000110c9b89c libCling.so`clang::NestedNameSpecifier::print(this=<unavailable>, OS=0x000000016fdf92d8, Policy=<unavailable>, ResolveTemplateArguments=<unavailable>) const at NestedNameSpecifier.cpp:0 [opt]
    frame #6: 0x0000000110d74a20 libCling.so`(anonymous namespace)::TypePrinter::printElaboratedBefore(this=0x000000016fdf92c0, T=0x00000001480097d0, OS=0x000000016fdf92d8) at TypePrinter.cpp:1619:18 [opt]
    frame #7: 0x0000000110d72788 libCling.so`(anonymous namespace)::TypePrinter::printBefore(this=0x000000016fdf92c0, T=0x00000001480097d0, Quals=(Mask = 0), OS=0x000000016fdf92d8) at TypeNodes.inc:46:1 [opt]
    frame #8: 0x0000000110d71b40 libCling.so`(anonymous namespace)::TypePrinter::print(this=0x000000016fdf92c0, T=0x00000001480097d0, Quals=<unavailable>, OS=0x000000016fdf92d8, PlaceHolder=(Data = "", Length = 0)) at TypePrinter.cpp:200:3 [opt]
    frame #9: 0x0000000110d71838 libCling.so`clang::QualType::getAsStringInternal(ty=<unavailable>, qs=(Mask = 0), buffer="", policy=0x000000016fdf9540) at TypePrinter.cpp:2477:23 [opt]
  * frame #10: 0x000000010defec28 libCling.so`ROOT::TMetaUtils::GetNormalizedName(norm_name="", type=0x000000016fdf9670, interpreter=0x0000000137007900, normCtxt=0x000000016fdfc5e0) at TClingUtils.cxx:4175:19
    frame #11: 0x000000010deff668 libCling.so`ROOT::TMetaUtils::AnnotatedRecordDecl::AnnotatedRecordDecl(this=0x00006000031f4068, index=1, requestedType=0x00000001480094d0, decl=0x00000001201a6ce0, requestName="reco::PFRecHitSoA::View", rStreamerInfo=true, rNoStreamer=false, rRequestNoInputOperator=false, rRequestOnlyTClass=false, rRequestVersionNumber=-1, rRequestedRNTupleSerializationMode=0, interpreter=0x0000000137007900, normCtxt=0x000000016fdfc5e0) at TClingUtils.cxx:474:4

  * frame #0: 0x0000000110d20990 libCling.so`clang::TemplateArgument::print(clang::PrintingPolicy const&, llvm::raw_ostream&, bool) const [inlined] printIntegral(TemplArg=<unavailable>, Out=0x000000016fdfa468, Policy=0x000000016fdfa5f0, IncludeType=<unavailable>) at TemplateBase.cpp:117:20 [opt]
    frame #1: 0x0000000110d20680 libCling.so`clang::TemplateArgument::print(this=<unavailable>, Policy=0x000000016fdfa5f0, Out=0x000000016fdfa468, IncludeType=<unavailable>) const at TemplateBase.cpp:557:5 [opt]
    frame #2: 0x0000000110d70854 libCling.so`void printTo<clang::TemplateArgument>(llvm::raw_ostream&, llvm::ArrayRef<clang::TemplateArgument>, clang::PrintingPolicy const&, clang::TemplateParameterList const*, bool, unsigned int) [inlined] printArgument(A=0x00000001480098c0, PP=0x000000016fdfa5f0, OS=0x000000016fdfa468, IncludeType=<unavailable>) at TypePrinter.cpp:2029:5 [opt]
    frame #3: 0x0000000110d70844 libCling.so`void printTo<clang::TemplateArgument>(OS=0x000000016fdfa778, Args=<unavailable>, Policy=0x000000016fdfa5f0, TPL=0x0000000000000000, IsPack=true, ParmIndex=0) at TypePrinter.cpp:2241:7 [opt]
    frame #4: 0x0000000110d70574 libCling.so`clang::printTemplateArgumentList(OS=<unavailable>, Args=<unavailable>, Policy=<unavailable>, TPL=<unavailable>) at TypePrinter.cpp:2285:3 [opt] [artificial]
    frame #5: 0x0000000110c9b89c libCling.so`clang::NestedNameSpecifier::print(this=<unavailable>, OS=0x000000016fdfa778, Policy=<unavailable>, ResolveTemplateArguments=<unavailable>) const at NestedNameSpecifier.cpp:0 [opt]
    frame #6: 0x0000000110d74a20 libCling.so`(anonymous namespace)::TypePrinter::printElaboratedBefore(this=0x000000016fdfa760, T=0x00000001480099a0, OS=0x000000016fdfa778) at TypePrinter.cpp:1619:18 [opt]
    frame #7: 0x0000000110d72788 libCling.so`(anonymous namespace)::TypePrinter::printBefore(this=0x000000016fdfa760, T=0x00000001480099a0, Quals=(Mask = 0), OS=0x000000016fdfa778) at TypeNodes.inc:46:1 [opt]
    frame #8: 0x0000000110d71b40 libCling.so`(anonymous namespace)::TypePrinter::print(this=0x000000016fdfa760, T=0x00000001480099a0, Quals=<unavailable>, OS=0x000000016fdfa778, PlaceHolder=(Data = "", Length = 0)) at TypePrinter.cpp:200:3 [opt]
    frame #9: 0x0000000110d71838 libCling.so`clang::QualType::getAsStringInternal(ty=<unavailable>, qs=(Mask = 0), buffer="", policy=0x000000016fdfa9e0) at TypePrinter.cpp:2477:23 [opt]
    frame #10: 0x000000010defec28 libCling.so`ROOT::TMetaUtils::GetNormalizedName(norm_name="", type=0x000000016fdfac60, interpreter=0x0000000137007900, normCtxt=0x000000016fdfc5e0) at TClingUtils.cxx:4175:19

  * frame #0: 0x0000000110d20990 libCling.so`clang::TemplateArgument::print(clang::PrintingPolicy const&, llvm::raw_ostream&, bool) const [inlined] printIntegral(TemplArg=<unavailable>, Out=0x000000016fdf9478, Policy=0x000000016fdf9600, IncludeType=<unavailable>) at TemplateBase.cpp:117:20 [opt]
    frame #1: 0x0000000110d20680 libCling.so`clang::TemplateArgument::print(this=<unavailable>, Policy=0x000000016fdf9600, Out=0x000000016fdf9478, IncludeType=<unavailable>) const at TemplateBase.cpp:557:5 [opt]
    frame #2: 0x0000000110d70854 libCling.so`void printTo<clang::TemplateArgument>(llvm::raw_ostream&, llvm::ArrayRef<clang::TemplateArgument>, clang::PrintingPolicy const&, clang::TemplateParameterList const*, bool, unsigned int) [inlined] printArgument(A=0x0000000158225610, PP=0x000000016fdf9600, OS=0x000000016fdf9478, IncludeType=<unavailable>) at TypePrinter.cpp:2029:5 [opt]
    frame #3: 0x0000000110d70844 libCling.so`void printTo<clang::TemplateArgument>(OS=0x000000016fdf9788, Args=<unavailable>, Policy=0x000000016fdf9600, TPL=0x0000000000000000, IsPack=true, ParmIndex=0) at TypePrinter.cpp:2241:7 [opt]
    frame #4: 0x0000000110d70574 libCling.so`clang::printTemplateArgumentList(OS=<unavailable>, Args=<unavailable>, Policy=<unavailable>, TPL=<unavailable>) at TypePrinter.cpp:2285:3 [opt] [artificial]
    frame #5: 0x0000000110c9b89c libCling.so`clang::NestedNameSpecifier::print(this=<unavailable>, OS=0x000000016fdf9788, Policy=<unavailable>, ResolveTemplateArguments=<unavailable>) const at NestedNameSpecifier.cpp:0 [opt]
    frame #6: 0x0000000110d74a20 libCling.so`(anonymous namespace)::TypePrinter::printElaboratedBefore(this=0x000000016fdf9770, T=0x00000001582256f0, OS=0x000000016fdf9788) at TypePrinter.cpp:1619:18 [opt]
    frame #7: 0x0000000110d72788 libCling.so`(anonymous namespace)::TypePrinter::printBefore(this=0x000000016fdf9770, T=0x00000001582256f0, Quals=(Mask = 0), OS=0x000000016fdf9788) at TypeNodes.inc:46:1 [opt]
    frame #8: 0x0000000110d71b40 libCling.so`(anonymous namespace)::TypePrinter::print(this=0x000000016fdf9770, T=0x00000001582256f0, Quals=<unavailable>, OS=0x000000016fdf9788, PlaceHolder=(Data = "", Length = 0)) at TypePrinter.cpp:200:3 [opt]
    frame #9: 0x0000000110d71838 libCling.so`clang::QualType::getAsStringInternal(ty=<unavailable>, qs=(Mask = 0), buffer="", policy=0x000000016fdf99b0) at TypePrinter.cpp:2477:23 [opt]
    frame #10: 0x000000010df006d4 libCling.so`ROOT::TMetaUtils::TClingLookupHelper::GetPartiallyDesugaredNameWithScopeHandling(this=0x000000016fdfc5a8, tname="reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>", result="", dropstd=true) at TClingUtils.cxx:645:15
    frame #11: 0x0000000101d03e74 libCore.so`TClassEdit::GetNormalizedName(norm_name="reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>", name="reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>") at TClassEdit.cxx:907:34
    frame #12: 0x0000000101d2ce00 libCore.so`TClass::GetClass(name="reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>", load=true, silent=false, hint_pair_offset=0, hint_pair_size=0) at TClass.cxx:3176:10
    frame #13: 0x0000000101d20288 libCore.so`TClass::GetClass(name="reco::PFRecHitSoALayout<128UL,false>::ViewTemplateFreeParams<128,false,true,true>", load=true, silent=false) at TClass.cxx:3071:11

This is necessary to allow ROOT normalized names to never have the type suffix added to integral template arguments.

This solves problems caused by some code paths producing normalized name with unexpectedly added type suffix after on integral non-type template parameters. This fixes root-project#18363

pcanal · 2025-04-14T14:52:04Z

Note: the PR has been trimmed to only the 2 commits related to the name normalization, including 0a0e7a9 which would need to make its way to the mono repo (i.e. 'fix' https://github.com/root-project/root/actions/runs/14448578670/job/40515239847?pr=18373)

The new PR is #18402

vgvassilev · 2025-04-14T17:00:46Z

Ask cms to keep a patched version of a patched version of llvm

This is not an option. The problem would affect any code where the class with a dictionary has a template parameter which is a size_t and result is a name normalization that is inconsistent.

I agree.

Figure out why we lost the type sugar in that one case, submit a patch upstream, merge it and backport it.

I am not sure what you have in mind but the code paths here are the one for 'name normalization' where we don't want the type sugar (except in some cases like Double32_t).

I do not think we understand what this fixes in practice. I want to reproduce the error with clang and normal c++ code first. That would mean taking a look at the patch that @hahnjo sent with his git blame and see how the tests triggered the codepaths in question.

Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite. I think the current patch would break quite a few things.

pcanal · 2025-04-14T17:31:08Z

I think the current patch would break quite a few things.

That's "impossible". The patch is completely opt-in. The code would need to somehow (I assume using UB) set the new data member Policy.NeverIncludeTypeForTemplateArgument ... i.e. If I read my patch correctly and it change the behavior of Clang (beside required a rebuild) then there is something seriously wrong with Clang :) (which as I prefaced is "impossible" :) ).

vgvassilev · 2025-04-14T17:35:16Z

I think the current patch would break quite a few things.

That's "impossible". The patch is completely opt-in. The code would need to somehow (I assume using UB) set the new data member Policy.NeverIncludeTypeForTemplateArgument ... i.e. If I read my patch correctly and it change the behavior of Clang (beside required a rebuild) then there is something seriously wrong with Clang :) (which as I prefaced is "impossible" :) ).

I wrote "Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite."

vgvassilev · 2025-04-14T17:37:25Z

I think the current patch would break quite a few things.

That's "impossible". The patch is completely opt-in. The code would need to somehow (I assume using UB) set the new data member Policy.NeverIncludeTypeForTemplateArgument ... i.e. If I read my patch correctly and it change the behavior of Clang (beside required a rebuild) then there is something seriously wrong with Clang :) (which as I prefaced is "impossible" :) ).

I wrote "Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite."

EDIT: The impossible part is technically right for the current patch and the tests. I can tweak my statement to "I think the current patch would silently break quite a few things" :)

pcanal · 2025-04-14T17:37:50Z

I wrote "Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite."

Sorry, I missed that :)

vgvassilev · 2025-04-14T17:41:40Z

I wrote "Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite."

Sorry, I missed that :)

I am not intending to be annoying here and it may not be obvious from outside but we have spent a lot of blood, sweat and tears in trying to reduce patches that were born in the same way as this PR is going...

Let's try to do something else here -- once you get the setup and the commit -- I expect that invested effort won't be significantly larger to fix the issue upstream.

pcanal · 2025-04-14T17:49:48Z

I want to reproduce the error with clang and normal c++ code first.
Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite. I think the current patch would break quite a few things.

That is fair and would be an investigation into whether or not the change @hahnjo mentioned are functioning as intended or not. Can someone volunteer to do this investigation while we merge this maybe temporary patch so that CMSSW can move forward? (Note: As long as TemplateParameterList::shouldIncludeTypeForArgument is anything more that just return Policy.AlwaysIncludeTypeForTemplateArgument then we will still the patch)

pcanal · 2025-04-14T17:52:27Z

Let's try to do something else here -- once you get the setup and the commit -- I expect that invested effort won't be significantly larger to fix the issue upstream.

What is the issue you are suspecting? See the example https://github.com/root-project/roottest/pull/1302/files#diff-9a7c6f13a755a6c9a2b0cef17215a82022ffa407cbe43fe60e6b714f18293ee2.

Is the question whether Clang is correct in tagging the argument's type as 'deduced' for the specific example?
This is a good question but I would have assumed that Clang would have tested that and the answer is likely not 'that' relevant to whether or not we need this patch (for now).

So the most genuine question for this patch is "is there any cases of class template instances where Clang would like to default to add the suffix to any integral number?". If the answer is yes there is at least one case where the suffix will be added, then we need this patch.

The current answer in TemplateParameterList::shouldIncludeTypeForArgument is very clear: there are some cases where it is wanted (current set includes getContainedDeducedType being' true' and for some reason error cases (PTL or index out of range).

to reduce patches that were born in the same way as this PR is going...

I am still not sure why the patch could not be upstreamed ....

Anyways, @devajithvs @aaronj0 Do you have time to look into whether Clang is doing the 'right'/'intended' thing in printing the type in that case. And/or as Vassil suggested, apply this patch to regular clang, turn on the 'Never' flag by default and report the failures?

vgvassilev · 2025-04-14T17:54:28Z

I want to reproduce the error with clang and normal c++ code first.
Another easy way to see the impact of your patch is to apply it on clang, make it on by default and run its test suite. I think the current patch would break quite a few things.

That is fair and would be an investigation into whether or not the change @hahnjo mentioned are functioning as intended or not. Can someone volunteer to do this investigation while we merge this maybe temporary patch so that CMSSW can move forward? (Note: As long as TemplateParameterList::shouldIncludeTypeForArgument is anything more that just return Policy.AlwaysIncludeTypeForTemplateArgument then we will still the patch)

If we merge this pull request we essentially move the responsibility from the author of the PR to somebody else to fix it and that somebody has no incentive to do anything. That's how we got here in a first place. To be constructive, if you feed me a reproducer with standalone clang I can take a look.

pcanal · 2025-04-14T18:24:59Z

I do not think we understand what this fixes in practice.

It fixes that when libCore or rootcling ask for a type to be printed, it can ask *explicitly and locally for the type to be printed without any integer suffix.

hahnjo · 2025-04-15T09:51:05Z

I had a quick look at one of the Clang tests and shouldIncludeTypeForArgument is there for deduced auto template parameters:
https://github.com/llvm/llvm-project/blob/81499edb30665d377a680990ef3c5129f9b54261/clang/test/AST/ast-dump-templates.cpp#L79-L103

In this case, the suffix is absolutely required because all of foo<0,0>, foo<0,0u>, foo<0,0LL> are different types. Yes, ROOT Meta currently doesn't treat them correctly, but for me that means just hiding the type is incorrect. The question is why size_t is treated the same way...

devajithvs · 2025-04-16T13:54:54Z

* adding the option, solve the problem (until we change the name normalization).

What would be required to change the name normalization? (I’m trying to understand)

I assume you mean setting shouldIncludeTypeForArgument to true (always with the suffix), so that the behavior is always consistent and we can also get rid of one of our LLVM patches.

pcanal · 2025-04-16T14:08:06Z

What would be required to change the name normalization? (I’m trying to understand)

The problem is not to change it (albeit due to the many code paths, it might also be not straightforward), it is the consequences of changing it.

The change in name normalization will change the checksum of those classes and all the classes that contain them. We would need to upgrade all the algorithm that are use to match CheckSum values to Class layout schema to handle the transition from the old to the new scheme (to be able to still properly read 'old' file). We may have to adjust older releases (patches) to do the converse to allow forward compatibility. In addition some experiments (CMS for example) have code to make sure that their code is not changed without update class version and so they record the checksum of the current version ... those would need to be updated.
And finally in user written I/O customization rule that depend on CheckSum would need to be reviewed and possibly updated to reflect the new CheckSum.
And we probably also need for backward compatibility to add code to match the old names to the new names.

PS. And it is guaranteed that I forgot one or more additional things to have to handle :(

pcanal requested review from bellenot, vgvassilev and dpiparo as code owners April 11, 2025 22:31

pcanal requested review from hahnjo, devajithvs and aaronj0 and removed request for bellenot April 11, 2025 22:31

pcanal self-assigned this Apr 11, 2025

pcanal added priority:high in:Cling in:Core Libraries in:Dictionaries experiment Affects an experiment / reported by its software & computimng experts labels Apr 11, 2025

pcanal added this to the 6.36.00 milestone Apr 11, 2025

pcanal mentioned this pull request Apr 11, 2025

Segmentation violation when running hltGetConfiguration with option -output full cms-sw/cmssw#47697

Open

ferdymercury reviewed Apr 12, 2025

View reviewed changes

interpreter/llvm-project/clang/include/clang/AST/PrettyPrinter.h Outdated Show resolved Hide resolved

pcanal force-pushed the master-18363 branch from c880b7c to d8d46a9 Compare April 12, 2025 17:38

hahnjo requested changes Apr 13, 2025

View reviewed changes

pcanal mentioned this pull request Apr 13, 2025

Test for name normalization involving size_t template argument root-project/roottest#1302

Open

pcanal mentioned this pull request Apr 14, 2025

meta: Allow global disabling of AutoParsing during TClass::GetClass #18402

Open

pcanal added 2 commits April 14, 2025 09:43

llvm: add Policy::NeverIncludeTypeForTemplateArgument.

0a0e7a9

This is necessary to allow ROOT normalized names to never have the type suffix added to integral template arguments.

meta: Use Policy.NeverIncludeTypeForTemplateArgument.

d3f4129

This solves problems caused by some code paths producing normalized name with unexpectedly added type suffix after on integral non-type template parameters. This fixes root-project#18363

pcanal force-pushed the master-18363 branch from d8d46a9 to d3f4129 Compare April 14, 2025 14:43

pcanal requested a review from hahnjo April 14, 2025 14:52

vgvassilev mentioned this pull request Apr 14, 2025

[ROOT Master, ARM] Multiple RelVals failed - Error parsing payload code for class L2MuonTrajectorySeed cms-sw/cmssw#47774

Open

pcanal closed this Apr 14, 2025

pcanal reopened this Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent type sufix to appear in ROOT normalized name #18373

Prevent type sufix to appear in ROOT normalized name #18373

pcanal commented Apr 11, 2025 •

edited

Loading

pcanal commented Apr 11, 2025

github-actions bot commented Apr 12, 2025 •

edited

Loading

vgvassilev commented Apr 12, 2025

pcanal commented Apr 12, 2025

hahnjo commented Apr 12, 2025 •

edited

Loading

vgvassilev commented Apr 12, 2025

pcanal commented Apr 12, 2025

pcanal commented Apr 12, 2025

vgvassilev commented Apr 13, 2025

hahnjo commented Apr 13, 2025

hahnjo left a comment

pcanal commented Apr 13, 2025

vgvassilev commented Apr 13, 2025

hahnjo commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

pcanal commented Apr 14, 2025 •

edited

Loading

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

vgvassilev commented Apr 14, 2025 •

edited

Loading

pcanal commented Apr 14, 2025

pcanal commented Apr 14, 2025 •

edited

Loading

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

hahnjo commented Apr 15, 2025

devajithvs commented Apr 16, 2025 •

edited

Loading

pcanal commented Apr 16, 2025 •

edited

Loading

Prevent type sufix to appear in ROOT normalized name #18373

Are you sure you want to change the base?

Prevent type sufix to appear in ROOT normalized name #18373

Conversation

pcanal commented Apr 11, 2025 • edited Loading

pcanal commented Apr 11, 2025

github-actions bot commented Apr 12, 2025 • edited Loading

Test Results

vgvassilev commented Apr 12, 2025

pcanal commented Apr 12, 2025

hahnjo commented Apr 12, 2025 • edited Loading

vgvassilev commented Apr 12, 2025

pcanal commented Apr 12, 2025

pcanal commented Apr 12, 2025

vgvassilev commented Apr 13, 2025

hahnjo commented Apr 13, 2025

hahnjo left a comment

Choose a reason for hiding this comment

pcanal commented Apr 13, 2025

vgvassilev commented Apr 13, 2025

hahnjo commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

pcanal commented Apr 14, 2025 • edited Loading

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

vgvassilev commented Apr 14, 2025 • edited Loading

pcanal commented Apr 14, 2025

pcanal commented Apr 14, 2025 • edited Loading

vgvassilev commented Apr 14, 2025

pcanal commented Apr 14, 2025

hahnjo commented Apr 15, 2025

devajithvs commented Apr 16, 2025 • edited Loading

pcanal commented Apr 16, 2025 • edited Loading

pcanal commented Apr 11, 2025 •

edited

Loading

github-actions bot commented Apr 12, 2025 •

edited

Loading

hahnjo commented Apr 12, 2025 •

edited

Loading

pcanal commented Apr 14, 2025 •

edited

Loading

vgvassilev commented Apr 14, 2025 •

edited

Loading

pcanal commented Apr 14, 2025 •

edited

Loading

devajithvs commented Apr 16, 2025 •

edited

Loading

pcanal commented Apr 16, 2025 •

edited

Loading