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

GetMethodWithPrototype return the incorrect function ? #7955

Open
pcanal opened this issue Apr 21, 2021 · 9 comments · May be fixed by #16232
Open

GetMethodWithPrototype return the incorrect function ? #7955

pcanal opened this issue Apr 21, 2021 · 9 comments · May be fixed by #16232

Comments

@pcanal
Copy link
Member

pcanal commented Apr 21, 2021

As seen by CMS on cms-sw/cmssw#33466

The following piece of lookup:

{
auto cls = TClass::GetClass("std::vector<int>");
auto meth = cls->GetMethodWithPrototype("operator[]","int",true,ROOT::kConversionMatch);
auto args = meth->GetListOfMethodArgs();
auto methArg = dynamic_cast<TMethodArg*>(args->First());
cout << args->GetEntries() << endl;
cout << methArg->GetTypeName() << endl;
}

print

1
vector<TClass*>::size_type

where one would expect

1
vector<int>::size_type

This has catastrophic consequence when combined with autoloading of library.

In the user's case the search is about std::vector<reco::RecoTauPiZero> but the argument found is vector<ROOT::Experimental::REveTableEntry>::size_type which ends up with a batch job trying to load libREve and thus libGui and thus requiring the reading of $HOME/.root.mimes which fails badly if $HOME is not set (case of somce condor jobs) which issues a Fatal error.

@Axel-Naumann
Copy link
Member

Caused by d2afe2d

Axel-Naumann added a commit to Axel-Naumann/root that referenced this issue May 7, 2021
…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.
Axel-Naumann added a commit to Axel-Naumann/root that referenced this issue May 7, 2021
Axel-Naumann added a commit to Axel-Naumann/root that referenced this issue May 11, 2021
@Axel-Naumann Axel-Naumann added this to the 6.24/02 milestone Jun 4, 2021
@Axel-Naumann Axel-Naumann linked a pull request Jun 8, 2021 that will close this issue
@Axel-Naumann Axel-Naumann modified the milestones: 6.24/02, 6.26/00 Jun 8, 2021
@Axel-Naumann
Copy link
Member

This is critical because we might load who knows what into the frameworks. But because the fix is rather intrusive, and the OP has a simple workaround, I'll not backport the fixes to our existing release branches.

Axel-Naumann added a commit to Axel-Naumann/root that referenced this issue Jan 11, 2022
…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.
Axel-Naumann added a commit to Axel-Naumann/root that referenced this issue Jan 11, 2022
@lmoneta lmoneta removed this from the 6.26/00 milestone Jan 24, 2022
@dpiparo dpiparo assigned dpiparo and unassigned Axel-Naumann Mar 24, 2024
silverweed pushed a commit to silverweed/root that referenced this issue Aug 14, 2024
…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.
silverweed pushed a commit to silverweed/root that referenced this issue Aug 14, 2024
@silverweed
Copy link
Contributor

I rebased @Axel-Naumann 's PR #8124 into #16232, the tests are passing and the reproducer seems to be fixed by it. @dpiparo how do we want to proceed with this?

@srimanob
Copy link

Thanks @silverweed @dpiparo to come back to this.
Just wondering what hold this fix so long, some technical things?
I thought it was fixed already, but never check again.

However, thanks very much.

silverweed pushed a commit to silverweed/root that referenced this issue Aug 14, 2024
…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.
silverweed pushed a commit to silverweed/root that referenced this issue Aug 14, 2024
silverweed added a commit to silverweed/root that referenced this issue Aug 16, 2024
silverweed pushed a commit to silverweed/root that referenced this issue Aug 16, 2024
…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.
silverweed pushed a commit to silverweed/root that referenced this issue Aug 16, 2024
silverweed added a commit to silverweed/root that referenced this issue Aug 16, 2024
silverweed pushed a commit to silverweed/root that referenced this issue Aug 19, 2024
…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.
silverweed pushed a commit to silverweed/root that referenced this issue Aug 19, 2024
silverweed added a commit to silverweed/root that referenced this issue Aug 19, 2024
@dpiparo
Copy link
Member

dpiparo commented Sep 3, 2024

@srimanob we are having issues in reproducing the issue: could the CMS team give us a hand? @silverweed

@dpiparo dpiparo linked a pull request Sep 3, 2024 that will close this issue
@srimanob
Copy link

srimanob commented Sep 3, 2024

Hi @silverweed @dpiparo
FYI @Dr15Jones

I just use the script mentioned in the beginning of this issue and try to run on bare root on lxplus.

If I run barely, I get vector<double>::size_type which maybe OK (not int)

(138) ~/w1/test/temp: root.exe test.C 
   ------------------------------------------------------------------
  | Welcome to ROOT 6.32.04                        https://root.cern |
  | (c) 1995-2024, The ROOT Team; conception: R. Brun, F. Rademakers |
  | Built for linuxx8664gcc on Aug 14 2024, 00:00:00                 |
  | From heads/master@tags/v6-32-04                                  |
  | With g++ (GCC) 11.4.1 20231218 (Red Hat 11.4.1-3)                |
  | Try '.help'/'.?', '.demo', '.license', '.credits', '.quit'/'.q'  |
   ------------------------------------------------------------------

root [0] 
Processing test.C...
1
vector<double>::size_type

But if I source ROOT first (to the same version), and try the script, I got the wrong type.

(139) ~/w1/test/temp: source /cvmfs/sft.cern.ch/lcg/app/releases/ROOT/6.32.04/x86_64-almalinux9.4-gcc114-opt/bin/thisroot.csh 
(140) ~/w1/test/temp: root.exe test.C
   ------------------------------------------------------------------
  | Welcome to ROOT 6.32.04                        https://root.cern |
  | (c) 1995-2024, The ROOT Team; conception: R. Brun, F. Rademakers |
  | Built for linuxx8664gcc on Aug 14 2024, 04:12:34                 |
  | From tags/v6-32-04@v6-32-04                                      |
  | With c++ (GCC) 11.4.1 20231218 (Red Hat 11.4.1-3)                |
  | Try '.help'/'.?', '.demo', '.license', '.credits', '.quit'/'.q'  |
   ------------------------------------------------------------------

root [0] 
Processing test.C...
1
vector<TClass*>::size_type

Additional test on ROOT through CMSSW which is still in 6.30, I get the wrong type (same as original report).

@silverweed
Copy link
Contributor

@srimanob thanks a lot for the details!
I can confirm that #16232 fixes the issue with the reproducers you gave. However, despite it being green on our CI, one test for cmssw has a failure most likely related to the change.
Unfortunately I have not been able to reproduce the crash and it is blocking us from merging the fix; do you think you could help us come up with a reproducer that we can more easily debug? I believe that once we fix that crash we can finally merge this fix.
Thanks again!

@srimanob
Copy link

srimanob commented Sep 4, 2024

Hi @silverweed

I ping our framework and CUDA experts to have a look. By the way, do you understand why if I use ROOT that comes on lxplus, no source of ROOT environment, it shows differently than I source ROOT environment first?

@silverweed
Copy link
Contributor

@srimanob I don't really know why that happens, but it's surely an interesting piece of the puzzle. Unfortunately I'm not familiar enough with cling to draw any conclusion, but perhaps @dpiparo, @vgvassilev or other experts of the subject may have a better understanding of it.

silverweed pushed a commit to silverweed/root that referenced this issue Sep 4, 2024
…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.
silverweed pushed a commit to silverweed/root that referenced this issue Sep 4, 2024
silverweed added a commit to silverweed/root that referenced this issue Sep 4, 2024
@makortel makortel moved this from New to Work in ROOT in ROOT prioritization Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment