root icon indicating copy to clipboard operation
root copied to clipboard

GetMethodWithPrototype return the incorrect function ?

Open pcanal opened this issue 4 years ago • 4 comments

As seen by CMS on https://github.com/cms-sw/cmssw/issues/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.

pcanal avatar Apr 21 '21 20:04 pcanal

Caused by d2afe2df1d2ad7c5c0e7f3f71d1090f3d73db534

Axel-Naumann avatar Apr 29 '21 10:04 Axel-Naumann

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 avatar Jun 08 '21 14:06 Axel-Naumann

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?

silverweed avatar Aug 14 '24 11:08 silverweed

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.

srimanob avatar Aug 14 '24 11:08 srimanob

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

dpiparo avatar Sep 03 '24 07:09 dpiparo

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).

srimanob avatar Sep 03 '24 16:09 srimanob

@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!

silverweed avatar Sep 04 '24 07:09 silverweed

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?

srimanob avatar Sep 04 '24 10:09 srimanob

@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 avatar Sep 04 '24 14:09 silverweed