[root6] Fix missing declarations in the code interpreted by Cling

[plexoos]

This should have been a part of #448 as the changes are closely related but unfortunately that PR was merged before I had a chance to update it.

A couple of issues seen in the ROOT6 CI tests are taken care of:

1. Include missing definitions when interpreting AgMLGeometry. Unlike rootcint, rootcling needs to see explicit definitions of the types used in an interpreted code.

   LoadTable: .L /star-sw/StarDb/AgMLGeometry/Geometry.y2021a.C
   In file included from input_line_732:1:
   In file included from /star-sw/StarDb/AgMLGeometry/Geometry.y2021a.C:2:
   /star-sw/StarDb/AgMLGeometry/CreateGeometry.h:10:8: error: use of undeclared identifier 'gGeoManager'
     if ( gGeoManager ) {
          ^

2. StBFChain: Declare global pointer to chain in compiled code. It appears that the ROOT6 interpreter prefers to see the declaration of the main StBFChain in the compiled code over the one in the bfc.C macro.

   LoadTable: .L /star-sw/StarDb/AgMLGeometry/Geometry.y2021a.C
   IncrementalExecutor::executeFunction: symbol 'chain' unresolved while linking [cling interface function]!
   root4star: .sl79_gcc485/OBJ/StRoot/St_db_Maker/St_db_Maker.cxx:934: virtual TDataSet*   St_db_Maker::LoadTable(TDataSet*): Assertion `!ee' failed.

   Could it be that global variables in interpreted code are not visible in other interpreted scripts?

   From the C++ point of view, the error actually makes sense because of the extern StBFChain* chain; statement in StarDb/AgMLGeometry/CreateGeometry.h

As suggested during the review, local variables were renamed to avoid confusion with global name chain

[klendathu2k]

[snip]

   Could it be that global variables in interpreted code are not visible in other interpreted scripts?
   From the C++ point of view, the error actually makes sense because of the `extern StBFChain* chain;` statement in `StarDb/AgMLGeometry/CreateGeometry.h`

In principle, the BFC.C macro is compiled by cling. So I would expect that any macro which is loaded at a later point would be able to access a global variable that it declares. But ... this is not the first place where my expectations about cling fail.

StRoot/macros/bfc.C line 32 should be changed to extern StBFChain* chain; so that we only have the single definition in StBFChain.

[plexoos]

In principle, the BFC.C macro is compiled by cling.

If by "compiled" you mean the JIT compilation by clang, it is still should be considered as interpretation of the code although an enhanced one. I would stick to the term because that is how they use it in early Cling presentations as opposed to "normal" external compilation or via ACLiC.

I would expect that any macro which is loaded at a later point would be able to access a global variable

I would think so too. But in this case a macro (Geometry.blah.C) is interpreted in a compiled library (libStBFChain.so) which in turn is loaded from an interpreted macro (bfc.C). Not sure though if an extra layer of indirection plays any role here...

[plexoos]

Could you point to the code where such assignment take place?

[genevb]

Could you point to the code where such assignment take place?

Roughly 15 lines below where you've inserted the global assignment.

[plexoos]

This line in the original file?

https://github.com/star-bnl/star-sw/blob/0c5edf55156c26d5b95ad749e77db1e8f3530c89/StRoot/StBFChain/StBFChain.cxx#L59

I don't think there is a conflict from the language point of view between the global and the local variables in this case, and I also don't see any compiler warning messages... But let's rename the local variable to avoid any confusion. How does that sound?

[genevb]

The compiler may differentiate local vs. global as the local interpretation gets priority at compile time. But it is good to avoid potentially confusing human code readers :-)

I don't have strong feelings about whether the global or local variable should retain the handle chain, but I'm not sure why you would pick changing the old one over the new one. But go for it - I don't care too much which one changes handles.

[plexoos]

but I'm not sure why you would pick changing the old one over the new one

Well because the "new" one is not really new :) It is essentially re-declaration of the global variable defined in the bfc.C macro:

https://github.com/star-bnl/star-sw/blob/827a627f1c7f3de192669786e1fa67283176f9ab/StRoot/macros/bfc.C#L32

We can't rename it because other codes using it (e.g. the Geometry macros) expect this variable to be the global pointer to the current chain. So, to avoid confusing humans renaming the local variable is the easiest option.

Just to reiterate, the reason why we move the declaration from the macro to the library is that Cling cannot see it in another macro loaded/interpreted at runtime. Formally declaring chain in the compiled library appears to help Cling, and CINT seems to be fine with that as well.

[plexoos]

StRoot/macros/bfc.C line 32 should be changed to extern StBFChain* chain; so that we only have the single definition in StBFChain.

This suggestion was tried in 64b449d but the jobs (ROOT5, gcc 4.8.5) fail with the following error:

Error: Symbol chain is not defined in current scope  CreateGeometry.h:26:
*** Interpreter error recovered ***
root4star: .sl79_gcc485/OBJ/StRoot/St_db_Maker/St_db_Maker.cxx:934: virtual TDataSet* St_db_Maker::LoadTable(TDataSet*): Assertion `!ee' failed.
LoadTable: .L /star-sw/StarDb/AgMLGeometry/Geometry.y2022a.C
Error: Process completed with exit code 1.

I am reverting the change. Any more suggestions or shall we merge?

[veprbl]

Since this looks like a cint bug, how about we put extern behind a #ifndef __CINT__?

[plexoos]

Fine by me but I think __CINT__ is also defined in ROOT6 along with __CLING__. Are you proposing to hide extern StBFChain* chain; from ROOT5/Cint completely?

[veprbl]

#if defined(__CLING__) || !defined(__CINT__)
extern
#endif
StBFChain* chain;

[plexoos]

If there are no further comments let's merge this pull request