Skip to content
Snippets Groups Projects
Commit f2489486 authored by Philippe Canal's avatar Philippe Canal Committed by Enrico Guiraud
Browse files

Fix RDF interpreted snapshot, ROOT-9236

The current failure in snapshot is due to an ABI incompatibility (or so it seems) that we may have encountered before.
The failing line is:
{code:c++}
auto snapshotRDF = std::make_shared<RInterface<RLoopManager>>(std::make_shared<RLoopManager>(nullptr, validCols));
{code}
and one of the component of make_shared is the call to the following shared_ptr constructor
{code:c++}
      // This constructor is non-standard, it is used by allocate_shared.
      template<typename _Alloc, typename... _Args>
        shared_ptr(_Sp_make_shared_tag __tag, const _Alloc& __a,
                   _Args&&... __args)
        : __shared_ptr<_Tp>(__tag, __a, std::forward<_Args>(__args)...)
        { }
{code}
Note the use of std::forward.

The snapshot test has 2 steps:

1. Do the work, including a call to SnapshotImpl which contains the problem line, with only compiled code
2. Do the same work relying on interpreted code, in which case SnapshotImpl is called via the interpreter.

The symptoms of the failure is that **if** the snapshot test is compiled with optimization then

a. The compiled version works fine
b. In the interpreted version the constructor of the RInterface is wrong because the shared_ptr its constructor sees is **not** initialized.

 If the snapshot test is compiled without optimization then both steps succeeds.  The main difference between the two is the amount of fully realized (i.e. non-inlined) functions emitted by the compiler.

In the success fully case we have a stack like

{code}
interp SnapshotImpl calls
  compiled std::make_shared<ROOT::Detail::RDF::RLoopManager …  [in debug mode. this routine and down are used compiled]
    compiled  std::allocate_shared<ROOT::Detail::RDF::RLoopManager
      ... implementation details .. all compiled.
        compiled ROOT::Detail::RDF::RLoopManager::RLoopManager
  compiled  std::make_shared<ROOT::RDF::RInterface …
    compiled  std::allocate_shared<ROOT::RDF::RInterface …
      compiled  std::shared_ptr< ROOT::RDF::RInterface
        compiled std::__shared_ptr<ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager
{code}

{code}
interp SnapshotImpl calls
  interp std::make_shared<ROOT::Detail::RDF::RLoopManager …
    interp  std::allocate_shared<ROOT::Detail::RDF::RLoopManager
      ... implementation details .. all interpreted.
        compiled ROOT::Detail::RDF::RLoopManager::RLoopManager
  interp  std::make_shared<ROOT::RDF::RInterface …
    interp  std::allocate_shared<ROOT::RDF::RInterface …
      interp  std::shared_ptr< ROOT::RDF::RInterface
        compiled  std::__shared_ptr<ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager
{code}

I.e. the major difference is that we have the shared_ptr constructor (which contains std::forward) being interpreted  (rather than compiled) and calling a compiled function (the base class constructor) that takes (is given) an std::forward.   It seems that the parameter and then mangled/placed-wrong in that case.

If I recall correctly this is a know ABI incompatibility between clang and gcc; especially (only?) if it involves temporary.

Splitting the line in two statement, works around the problem:
{code:c++}
      auto rlm_ptr = std::make_shared<RLoopManager>(nullptr, validCols);
      auto snapshotRDF = std::make_shared<RInterface<RLoopManager>>(rlm_ptr);
{code}

In my case, it solves the problem because more of the '2nd' make_shared called are inlined by the compiler (when generating code for the 1st step), including the __shared_ptr constructor ... forcing the interpreter to generate the code and hence side-stepping the issue.  [And if the std::forward issue is limited to temporary it would side-step the issue even if the  __shared_ptr constructor was still outline.
parent 997e1bd6
Loading
...@@ -1679,7 +1679,13 @@ private: ...@@ -1679,7 +1679,13 @@ private:
::TDirectory::TContext ctxt; ::TDirectory::TContext ctxt;
// Now we mimic a constructor for the RDataFrame. We cannot invoke it here // Now we mimic a constructor for the RDataFrame. We cannot invoke it here
// since this would introduce a cyclic headers dependency. // since this would introduce a cyclic headers dependency.
auto snapshotRDF = std::make_shared<RInterface<RLoopManager>>(std::make_shared<RLoopManager>(nullptr, validCols));
// Keep these two statements separated to work-around an ABI incompatibility
// between clang (and thus cling) and gcc in the way std::forward is handled.
// See https://sft.its.cern.ch/jira/browse/ROOT-9236 for more detail.
auto rlm_ptr = std::make_shared<RLoopManager>(nullptr, validCols);
auto snapshotRDF = std::make_shared<RInterface<RLoopManager>>(rlm_ptr);
auto chain = std::make_shared<TChain>(fullTreename.c_str()); auto chain = std::make_shared<TChain>(fullTreename.c_str());
chain->Add(std::string(filename).c_str()); chain->Add(std::string(filename).c_str());
snapshotRDF->fProxiedPtr->SetTree(chain); snapshotRDF->fProxiedPtr->SetTree(chain);
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment