Allow pass-by-value for classes with deleted copy-constructor #14426
Conversation
|
Starting build on |
|
Build failed on windows10/default. Failing tests: |
Test Results 10 files 10 suites 2d 14h 3m 13s ⏱️ For more details on these failures, see this check. Results for commit e910a1c. ♻️ This comment has been updated with latest results. |
| // copy constructor exists, so check for the most common case: the trivial | ||
| // one, but not uniquely available, while there is a move constructor. | ||
| CXXRecordDecl* rtdecl = QT->getAsCXXRecordDecl(); | ||
| if (rtdecl && (rtdecl->hasTrivialCopyConstructor() && !rtdecl->hasSimpleCopyConstructor()) \ |
There was a problem hiding this comment.
Why require rtdecl->hasTrivialCopyConstructor()? AFAICT it should be sufficient to check !rtdecl->hasSimpleCopyConstructor()... (what does rt stand for? There is no t in CXXRecordDecl... IIRC the usual acronym is RD in upstream Clang code)
Also, please remove the final \, the line isn't in a macro...
There was a problem hiding this comment.
I wanted to check if the changes from cppyy worked as-is, it seems like they do (except for Windows for now). Your suggestions are very valid and I will include them in the next changes. But I see no complaint about the overall direction of the PR, so I'm glad about that!
There was a problem hiding this comment.
We will probably need a unittest for callfunc as that’s a critical component for root.
There was a problem hiding this comment.
Ok, I'll try to come up with something that is equivalent to what PyROOT tries to do in this case.
There was a problem hiding this comment.
I have included a first test, based on already available tests for TClingCallFunc. Let me know what you think
There was a problem hiding this comment.
It seems that the currently failing tests (e.g. here) are due to the missing hasTrivialCopyConstructor check, I will reintroduce it in a new commit
There was a problem hiding this comment.
Whatever we do we should do it in sync with upstream otherwise it will be a nightmare to upgrade. If the patch differs we should propose it upstream, if it is only the test we should propose the test. That investment will make our lives easier in near future.
There was a problem hiding this comment.
In principle we can pick what upstream does literally as-is (if we can afford living with the \ character in the if condition). It is not only the test clearly, as this feature was never present before in ROOT's TClingCallFunc. The main issue I see with picking upstream as-is is that there is no way to pinpoint the necessary commits, the whole TClingCallFunc::make_narg_call belongs to a single commit when the fork of ROOT was started https://github.com/wlav/cppyy-backend/blame/25caf988cef1f2f76705c07b7262f076e8ed0e01/cling/src/core/metacling/src/TClingCallFunc.cxx#L478 . I have no problem with copying upstream code verbatim though. Let me know what I should do because this problem is quite visible in experiment code using PyROOT.
There was a problem hiding this comment.
This is one of the situations where we might not be able to. The CallFunc changes are only tested within cppyy context and that might fall short or the changes might cause regressions. We need to re-integrate the TClingCallFunc changes very carefully. We can't really copy verbatim in some cases, especially the lambda support as that I think has a patch in clang.
In an ideal world (with CppInterOp) these changes will be in one place but until then I am not sure if there is a single best go-to strategy.
Let me try to summon @wlav.
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Starting build on |
|
Build failed on windows10/default. Failing tests: |
|
Starting build on |
|
Build failed on windows10/default. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Build failed on windows10/default. Failing tests: |
vepadulano
force-pushed
the
rntuple-fix-pyroot-model-create
branch
from
60e6526 to
2c70368
Compare
|
Starting build on |
|
After a debugging session with @bellenot , we have found out that @vgvassilev The change is taken from cppyy's patches, but as you also suggested it's not trivial to just take all the code verbatim. So I restricted it to the minimum amount of changes that apply for the reported use case. Let me know how to make progress here. |
I believe it would be better to understand what's going on here. From a quick look into the Clang sources, it seems there is some special handling if the (move) constructor is templated. And indeed, this seems to be one difference between Microsoft's STL on the one hand and libc++ and libstdc++ on the other: Microsoft's STL only has templated move constructors while the other two have a "default" version for matching types. If this hypothesis is correct, it would mean that this patch wouldn't work on user-defined types either with only a templated move constructor... |
Have we called |
Interesting! This is something I can test :) |
|
So indeed this other test does not work import ROOT
ROOT.gInterpreter.Declare(r'''
struct A{
int mA{42};
A() {}
A(const A&) = delete;
template<typename T = int>
A(A &&) {}
};
int foo(A a = A{}) { return a.mA; }
''')
print(ROOT.foo())Also, it seems to me that indeed in the case of a templated move constructor, the AST generated by clang simply does not report it in the class definition data, see this example. There, by commenting the template declaration of the move constructor, one can see the corresponding AST definition data being filled, i.e. from to I am surely not expert enough in this area, but I noticed that when the constructor is templated the corresponding |
Yes I tried also that and doesn't change the situation I described in the post above. if (RD) {
clang::Sema &S = fInterp->getSema();
S.ForceDeclarationOfImplicitMembers(RD);
}
if(RD && (RD->hasTrivialCopyConstructor() && !RD->hasSimpleCopyConstructor()) && RD->hasMoveConstructor()) {
// move construction as needed for classes (note that this is implicit)
callbuf << "std::move(*(" << type_name.c_str() << "*)args[" << i << "])";
} else {
// otherwise, and for builtins, use copy construction of temporary*/
callbuf << "*(" << type_name.c_str() << "*)args[" << i << "]";
} |
|
Can you paste the entire callfunc wrapper? |
I suppose that with a template move constructor is not the "canonical form" of a move constructor (or whatever the right standardese is). I was hoping that it's reported once the template is instantiated, but that doesn't seem the case either. In any case, what you really want to ask Clang is "can you copy-construct this thing" and "can you move-construct this thing". That logic is implemented in Final remark: A better heuristic could be to check for |
|
Thanks @hahnjo for the valuable input! I will try to digest Unfortunately So it seems I can't use that method to distinguish this case either... |
In creating the expression to JIT-compile for calling a certain method with arguments, TClingCallFunc takes each argument (type-erased, then casted to the right type) by const-ref. Practically, this pattern prevents pass-by-value semantics on the PyROOT side for classes with a deleted copy-constructor, e.g. std::unique_ptr. This patch aims at adding an automatic std::move when we can detect that the class does not provide a copy constructor. This logic is applied with two different conditions that serve different use cases: * If the class does not have a simple copy constructor but does have a trivial one and also has a move constructor. This is the case for `std::unique_ptr` of libc++ and libstdc++. * If the first condition does not apply, we traverse the available constructors of the class, find the copy constructor, check whether that is deleted. If so, we automatically inject the `std::move`. This is the case of `std::unique_ptr` of the Microsoft STL, which is different since its move constructor is templated. In this case, clang cannot completely identify the move constructor in the AST. By extension this applies to any class with a templated move constructor and a deleted copy constructor. Note that the changes in this commit began from the patched version of TClingCallFunc available in cppyy https://github.com/wlav/cppyy-backend/blob/25caf988cef1f2f76705c07b7262f076e8ed0e01/cling/src/core/metacling/src/TClingCallFunc.cxx#L468-L485 Co-authored-by: Wim Lavrijsen <WLavrijsen@lbl.gov>
vepadulano
force-pushed
the
rntuple-fix-pyroot-model-create
branch
from
2c70368 to
1c7a439
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Failing tests:
And 41 more |
Test that we automatically inject an `std::move` in the TClingCallFunc wrapper call in cases where a copy constructor is not available for the class.
vepadulano
force-pushed
the
rntuple-fix-pyroot-model-create
branch
from
1c7a439 to
e910a1c
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Failing tests:
And 39 more |
|
We probably need to more work to collect the information about the templated ctor. I'd propose to do Sema::LookupConstructors and iterate over the constructor list. If you see a |
|
Superseded by #17030 |
This fixes #14425 (the reproducer is added as test)
Note that the patch comes straight from upstream cppyy https://github.com/wlav/cppyy-backend/blob/25caf988cef1f2f76705c07b7262f076e8ed0e01/cling/src/core/metacling/src/TClingCallFunc.cxx#L468-L485 even though it's a patch in TCling. I open this PR as a draft to start the discussion as to how we can integrate this change, since it's necessary to fix a bug that also affects usage of ROOT classes via PyROOT.