From f2489486082cb80533b69b94bfa2d7cb6c0bec49 Mon Sep 17 00:00:00 2001
From: Philippe Canal <pcanal@fnal.gov>
Date: Wed, 6 Jun 2018 12:52:00 -0500
Subject: [PATCH] Fix RDF interpreted snapshot, ROOT-9236
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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.
---
tree/dataframe/inc/ROOT/RDFInterface.hxx | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tree/dataframe/inc/ROOT/RDFInterface.hxx b/tree/dataframe/inc/ROOT/RDFInterface.hxx
index 6a2f8a50140..8c174b64eb5 100644
--- a/tree/dataframe/inc/ROOT/RDFInterface.hxx
+++ b/tree/dataframe/inc/ROOT/RDFInterface.hxx
@@ -1679,7 +1679,13 @@ private:
::TDirectory::TContext ctxt;
// Now we mimic a constructor for the RDataFrame. We cannot invoke it here
// 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());
chain->Add(std::string(filename).c_str());
snapshotRDF->fProxiedPtr->SetTree(chain);
--
GitLab