-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Improved error handling for template instantiation #12449
base: master
Are you sure you want to change the base?
Conversation
Starting build on |
b8e1515
to
1e89113
Compare
The failure to unload broken declarations (@jalopezg-git FYI), does that still happen after this PR, or is this addressed by the PR? I'm not sure I understand how much of the PR description describes this PR vs what's left to be done? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work on this. Can you remind me what typical new failures would look like if instead we were to repeat the lookup without diagnostic suppression, in those cases where cppyy cannot find a suitable function to call?
@@ -1211,6 +1205,9 @@ namespace cling { | |||
S.InstantiateFunctionDefinition(SourceLocation(), TheDecl, | |||
true /*recursive instantiation*/); | |||
} | |||
if (S.getDiagnostics().hasErrorOccurred()) { | |||
TheDecl->setInvalidDecl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't be enough: any instantiation of templates that this template instantiation depends on might have failed, and an unknown subset of the instantiations might need to get unloaded... We might need to listen to the AST operations during lookup and roll everything back. (Not as part of this PR.) @jalopezg-git thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe starting a (nested) transaction and unloading the decl group represented by it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a transaction which gets started in this case. I've modified the logic to catch the error and fail the transaction rather than unloading the decl directly.
All of the code example/output in the PR description corresponds to with this PR included. Repeating the lookup without diagnostic suppression doesn't give the correct error message again ie with superfluous debug output snipped out:
So the relevant error message |
Failure for ubuntu20 build looks possibly unrelated to this PR. |
So in fact there is already a problem with rolling back the transaction even when just using test.h template <typename T>
class Helper {
public:
Helper() {}
std::size_t operator() () const {
const std::size_t res = 0;
res = T{0, 0}.size();
return res;
}
};
template <typename H>
std::size_t call_helper(const H &helper) {
return helper();
} testdeclare.py import ROOT
ret = ROOT.gInterpreter.Declare('#include "test.h"')
print("header include ret", ret)
print("creating helper")
helper = ROOT.Helper[ROOT.std.vector["double"]]()
bad_template = "template std::size_t call_helper<Helper<std::vector<double>>>(const Helper<std::vector<double>>&);"
for i in range(2):
print(f"declare attempt {i}")
ret = ROOT.gInterpreter.Declare(bad_template)
print("ret", ret) output:
So again the error message is different/more obscure on the second attempt at explicit template instantiation. |
I don't know how this PR relates to the unloading issues in cling. What I saw in the past is that |
After modifying the logic to catch the error and fail the transaction rather than unloading the decl directly, repeated attempts at template instantiation from pyroot now behaves similarly to with TInterpreter::Declare. The remaining problems with incomplete rollback are almost certainly related to the issue which @jalopezg-git referred to, and can be fixed by his forthcoming PR. Still TODO for this PR: |
Test Results 14 files 14 suites 3d 16h 17m 51s ⏱️ For more details on these failures, see this check. Results for commit 549109d. ♻️ This comment has been updated with latest results. |
ac63baf
to
59c2de3
Compare
2aa7e7a
to
4e29a95
Compare
The failure in |
PR and description updated addressing also the diagnostic capture and printing. I'm still not totally sure about how the catching of errors and rollback of the transaction is handled in LookupHelper as said in the description. |
Any ideas on the remaining windows failure would also be welcome (it doesn't happen on linux and I don't have a windows setup to test with at the moment) |
@@ -1211,8 +1195,11 @@ namespace cling { | |||
S.InstantiateFunctionDefinition(SourceLocation(), TheDecl, | |||
true /*recursive instantiation*/); | |||
} | |||
if (TheDecl->isInvalidDecl()) { | |||
// if the decl is invalid try to clean up | |||
if (TheDecl->isInvalidDecl() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in particular for this case (invalid decl but no errors ocured) not sure if it's more appropriate to just unload the decl, or propagate an error such that the cleanup is done by failing the transaction as for the case where an error occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be. I am not sure. If I recalled correctly, when that code was written the transaction and transaction unloading was not as mature as it is now.
Related I do not know whether this code path (the related path below) is exercised in the tests (to test it out, what could put an obvious message and a process termination here). If it is not exercise we should probably add a test that does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an example of a declaration being marked as invalid while no diagnostic has been generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution (and sorry for the delay in the review), @bendavid! 🙂 I have attached some comments to further improve the PR.
Regarding the unloading errors that I mentioned in one of my previous comments, I'll open a pull request soon.
/// \brief Uses `clang::TextDiagnosticPrinter` to format diagnostics, which | ||
/// are then passed to a user-provided output stream | ||
/// | ||
class TClingRedirectDiagnosticPrinter : public clang::TextDiagnosticPrinter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether we could somehow reuse TClingDelegateDiagnosticPrinter
; LGTM otherwise 🙂.
virtual ~DiagnosticsRAII(){}; | ||
}; | ||
|
||
class RedirectDiagnostics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, document class.
|
||
public: | ||
TClingDiagnosticsRAII(clang::DiagnosticsEngine &Diags, clang::DiagnosticConsumer *Replace) | ||
: fReplace(Diags, *Replace, false), fConsumer(Replace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with the current TCling::ReportDiagnosticsToErrorHandler()
? This provides users (mainly experiments frameworks) a way to catch clang diagnostics.
(I guess diagnostics generated while this is in effect will not be seen by users; could you confirm?)
@@ -7413,6 +7427,22 @@ Bool_t TCling::LoadText(const char* text) const | |||
return (fInterpreter->declare(text) == cling::Interpreter::kSuccess); | |||
} | |||
|
|||
//////////////////////////////////////////////////////////////////////////////// | |||
/// Make RAII for redirecting diagnostic messages to an ostream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps consider extending the documentation, incl. argument descriptions and/or usage example.
@@ -173,6 +175,7 @@ class TClingClassInfo final : public TClingDeclInfo { | |||
bool IsLoaded() const; | |||
bool IsValidMethod(const char *method, const char *proto, Bool_t objectIsConst, Longptr_t *offset, ROOT::EFunctionMatchMode mode = ROOT::kConversionMatch) const; | |||
int InternalNext(); | |||
cling::LookupHelper::DiagSetting LookupDiagnostics() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the current name not descriptive enough. What about HasLookupDiagnostics()
?
if (fdecl->isInvalidDecl()) { | ||
// if the decl is invalid try to clean up | ||
if (fdecl->isInvalidDecl() && | ||
!S.getDiagnostics().hasErrorOccurred()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
// 0 column version | ||
void Exec(unsigned int) | ||
{ | ||
if (_eventSize) { | ||
throw std::invalid_argument(std::string("RooDataSet can hold ") + std::to_string(_eventSize) + | ||
" variables per event, but RDataFrame passed 0 columns."); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? The if
statement in line 135 seems to be catching also this case. Also, I guess the documentation above should be associated to the templated version of Exec()
below.
@@ -811,6 +811,7 @@ T CallT(Cppyy::TCppMethod_t method, Cppyy::TCppObject_t self, size_t nargs, void | |||
T t{}; | |||
if (WrapperCall(method, nargs, args, (void*)self, &t)) | |||
return t; | |||
throw std::runtime_error("failed to resolve function"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change in behavior documented (i.e., throw
ing instead of return
ing
free((void *)cppresult); | ||
} else { | ||
*length = 0; | ||
free((void *)cppresult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this, and although the code was there before... Given that free()
is called on cppresult
in any case, can't we use instead a std::unique_ptr
that ensures memory is released even when throw
ing?
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation seems to be wrong here (inconsistent use of tabs / spaces?).
Build failed on windows10/cxx14. Failing tests: |
I think this PR stalled for a while; perhaps we can give it a push and finally merge in the coming weeks? |
Yes ok I can come back to this next week. |
FYI, #13565 should fix the issues with unloading that I mentioned before in this PR! I still need to look at two test failures, but it's mostly there 🙂! |
@bendavid #13565 was recently merged into master. Provided that you have the time, you could rebase this PR and see how it works now. |
fce62a9
to
0bf06ad
Compare
Coming back to this finally. I've rebased this one. Before I go through the individual review comments, can someone comment (maybe @vepadulano ) whether what's done in this PR still makes sense or if there are e.g. other error handling hooks that were added from upstream cppyy which would make more sense to use instead? |
…ler errors and warnings during template instantiation
…available on all build platforms
…ed with jitted version of Book
549109d
to
8b22039
Compare
(Updated to fix code formatting) |
Two substantive changes:
This PR fixes #11854
There are still some remaining problems with the transaction rollback, however template instantiation from cppyy now behaves the same as calling
TInterpreter::Declare
in this respect. This is likely related to the issues described by @jalopezg-git in #12449 (comment) and can be fixed in a future PR.Consider the following test case:
test.h:
test.py
The output below is now close to optimal for the first instantiation attempt. On the second instantiation attempt the error message is different/less useful because of the imperfect transaction rollback already noted. (but the same happens instantiating the template through
TInterpreter::Declare
as said)(on the console the output also has the nice highlighting and colors one would normally get from clang diagnostic printing, see screenshot below)
Needless to say, taken together this constitutes a major improvement when trying to use complex templated code with pyroot/cppyy