Use suitable TmpltSpec for nondep member type names #16232
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results 13 files 13 suites 2d 22h 32m 2s ⏱️ Results for commit aa21d16. ♻️ This comment has been updated with latest results. |
Member
|
The rebase fixes should likely be folded into the respective commits so that each one of them builds. That would allow future bisection in case there are problems and IMHO we don't want to squash-merge a +449 -148 change touching Cling and |
silverweed
force-pushed
the
issue7955-type-first-spec-2
branch
from
d012c8e to
a1310c3
Compare
Contributor
Author
|
I re-rebased the PR, this time making sure every single commit compiles independently (the first time I hoped that git would refuse to continue a rebase if there are still version control markers in the files, but apparently not). |
dpiparo
approved these changes
Aug 16, 2024
Member
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Since the PR is works on all platforms, I propose to add a test for it and then, if everything green, just merge and close the associated item.
silverweed
force-pushed
the
issue7955-type-first-spec-2
branch
from
6e20bb8 to
4e8a43c
Compare
Member
|
"works on all platforms" is not really a review, but sadly I'm not sure who could properly review this... @silverweed the |
silverweed
force-pushed
the
issue7955-type-first-spec-2
branch
from
4e8a43c to
5f3799a
Compare
Member
|
The original PR does not have a lot of details about what this is supposed to be doing... @smuzaffar, can we run this PR on cmssw infrastructure? |
Contributor
|
@vgvassilev , cmssw tests are runing via cms-sw#208 now |
Contributor
|
@vgvassilev , cmssw build failed during dictionaries generation [a]. I think this mostly happens when cuda code is envolved |
|
The case @smuzaffar quoted above is actually CPU-only code (even if it uses Alpaka), so the problem is not specific to CUDA/ROCm. The full build log shows a crash also in DataFormats/Histograms package, that is completely independent from GPUs or Alpaka, and that the dictionary generation succeeds for many CUDA-dependent classes. |
|
Would it be helpful to repeat the test with debug symbols enabled for cling (and ROOT)? |
Contributor
ok, I am re-running cmssw tests with ROOT/cling debug |
Contributor
|
@silverweed , can you please resolve the merge conflicts ? |
…ext: Instead of picking a "random" (the first) specialization when building the name of a non-dependent member type of a template, pick the one that corresponds to the "current" specialization. As CreateNestedNameSpecifierForScopeOf cannot know what's "current", this context needs to be passed down / propagated. Add a couple of asserts that the context is provided when needed, and is a template specialization that matches the type at hand. This fixes root-project#7955.
FIXUP bced2ac. CreateNestedNameSpecifierForScopeOf needs specialization context
silverweed
force-pushed
the
issue7955-type-first-spec-2
branch
from
5f3799a to
aa21d16
Compare
Contributor
Author
|
@makortel yes, it would be quite helpful, thanks :) @smuzaffar I rebased the PR, it should be goood now. |
Member
Why we put cuda in dictionaries? |
We have classes (data products) that use CUDA (or ROCm) runtime API (no kernel calls) e.g. to manage device memory. The CMSSW framework needs some reflection information for all data product classes, and so far this reflection is achieved via ROOT dictionaries. (although we have been discussing on and off whether we'd really want to do something else for non-persistency reflection needs, also for other reasons) |
Contributor
|
stack trace with ROOT/Cling build with debug is below. Full build logs are available here |
Member
|
That looks like a bug in llvm... I wonder on what code it happens. I see there is a debug llvm build already. What's the way nowadays to debug it on the cms infrastructure? |
|
The |
@vgvassilev cms-sw#208 (comment) shows a script that installs the (failed) CMSSW build where you can e.g. run the |
Contributor
|
@vgvassilev , as @makortel mentioned above , you can do something like the following on lxplus to reproduce this build failure |
|
The |
Member
I just got autowarned: |
Contributor
|
@vgvassilev , you can use |
vgvassilev
reviewed
Sep 5, 2024
| std::string typenameStr; | ||
|
|
||
| const clang::ASTContext& astContext = cl.getASTContext(); | ||
| auto CTSD = llvm::dyn_cast<clang::ClassTemplateSpecializationDecl>(&cl); |
Member
There was a problem hiding this comment.
Looks like this example breaks some assumptions in this PR.
#include <vector>
#include <string>
#include <stdint.h>
template <class T>
class MEtoEDM {
public:
typedef std::vector<uint32_t> TagList;
struct MEtoEDMObject {
std::string name;
TagList tags;
T object;
};
};If you can reproduce that crash with a test using the example that'd be a good step forward.
#0 0x00007fffe88f652f in raise () from /lib64/libc.so.6
#1 0x00007fffe88c9e65 in abort () from /lib64/libc.so.6
#2 0x00007fffe88c9d39 in __assert_fail_base.cold.0 () from /lib64/libc.so.6
#3 0x00007fffe88eee86 in __assert_fail () from /lib64/libc.so.6
#4 0x00007fffeaef3d10 in cling::utils::CreateNestedNameSpecifierForScopeOf (Ctx=..., decl=0x3d176f8, FullyQualified=true, Spec=0x0)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/interpreter/cling/lib/Utils/AST.cpp:1561
#5 0x00007fffeaef3f03 in cling::utils::CreateNestedNameSpecifierForScopeOf (Ctx=..., TypePtr=0x3d17a30, FullyQualified=true,
Spec=0x0)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/interpreter/cling/lib/Utils/AST.cpp:1621
#6 0x00007fffeaef4573 in cling::utils::TypeName::GetFullyQualifiedType (QT=..., Ctx=..., Spec=0x0)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/interpreter/cling/lib/Utils/AST.cpp:1760
#7 0x00007fffeaef478e in cling::utils::TypeName::GetFullyQualifiedName[abi:cxx11](clang::QualType, clang::ASTContext const&, clang::ClassTemplateSpecializationDecl const*) (QT=..., Ctx=..., Spec=0x0)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/interpreter/cling/lib/Utils/AST.cpp:1805
#8 0x00007fffea825603 in ROOT::TMetaUtils::GetFullyQualifiedTypeName (typenamestr="", qtype=..., astContext=..., Spec=0x0)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/core/clingutils/src/TClingUtils.cxx:3497
#9 0x00007fffea81c1ef in CreateNameTypeMap (cl=..., nameType=std::map with 1 element = {...})
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7bf90/root-6.33.01/core/clingutils/src/TClingUtils.cxx:1502
#10 0x00007fffea81d7a2 in ROOT::TMetaUtils::WriteClassInit (finalString=..., cl=..., decl=0x3d5f6c8, interp=..., normCtxt=...,
ctorTypes=std::__cxx11::list = {...}, needCollectionProxy=@0x7ffffffef3d7: false)
at /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc12/lcg/root/6.33.01-48fd78c793bc73bd3f87499c5ca7
That is, we introduce a ClassTemplateSpecializationDecl which could be nullptr and yet we will still have a class template. In this line here we get:
(gdb) p cl.getDescribedTemplate()
$15 = (clang::TemplateDecl *) 0x0
(gdb) p cl.getDescribedClassTemplate()
$16 = (clang::ClassTemplateDecl *) 0x0
(gdb) p cl.getTemplateSpecializationKind ()
$17 = clang::TSK_ImplicitInstantiation
p cl.getInstantiatedFromMemberClass () will get us the outer class will still not be the class template specialization decl.
I think we are trying to get the template pattern of the outer class asking its nested class and that's probably not a good idea. I am not entirely sure what this pull request intends to do, looks very hacky anyways, you can probably get away by asking if the class is nested.
I think this PR is quite messy and will require a lot of work to make it consistent and mergeable.
Contributor
Author
|
@vgvassilev should I close this PR or does it make sense to keep it open? |
Contributor
Author
|
@vgvassilev what should we do with this PR? |
Member
|
If it breaks cmssw we cannot merge it if that's the question. |
Contributor
Author
|
Yes, I was wondering if we should keep pursuing this or not. If not I'll just close it rather than leaving it here unattended. |
Member
|
The PR claims to be fixing a bug, maybe understanding what's broken can help. |
Contributor
Author
|
True, so probably it's best to restart fresh from the current state of things rather than rebasing an old fix without really understanding it like I did. That's why I would personally close this - as I'm not really the best person to do this kind of investigation at the moment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebase of #8124
Fixes #7955