-
Notifications
You must be signed in to change notification settings - Fork 65
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
Crash when calling print on the return of an empty UDF #595
Comments
In my debugger it shows that it actually crashes the DAPHNE compiler. So we should maybe catch that already while parsing? 🤔 |
After investigating this further, I found out, that the reason for the crash is an invalid mlir::Value that is returned when a function does not have a return value. I have a local fix for this that returns nullptr instead of that invalid mlir::Value and then later checking on that nullptr to ignore that return value. |
…unction This commit contains two changes more or less related to this issue: 1. There is a check preventing empty function definitions. While I thought the empty function had something to do with the crash at first, this turned out to be not the case. Nevertheless, I kept the change as reminding the user that there's unnecessary code would improve our compiler a tiny little bit (imho). 2. The core issue seems to be that functions without return value internally returned an invalid mlir::Value. Upon doing anything with this value, the program would crash (e.g. accessing any fields or calling any methods). Now we return nullptr and check for that, which gives a somewhat more deterministic behavior. Closes daphne-eu#595
…unction The core issue seems to be that functions without return value internally returned an invalid mlir::Value. Upon doing anything with this value, the program would crash (e.g. accessing any fields or calling any methods). Now we return nullptr and check for that, which gives a somewhat more deterministic behavior. Closes daphne-eu#595
…unction The core issue seems to be that functions without return value internally returned an invalid mlir::Value. Upon doing anything with this value, the program would crash (e.g. accessing any fields or calling any methods). Now we return nullptr and check for that, which gives a somewhat more deterministic behavior. Closes daphne-eu#595
- So far, the code in this PR checked, for each argument to a function call, if it is nullptr ("result" of a UDF without return values), and if so, ignored the argument. - This is not correct, since: - 1) We cannot simply ignore erroneous arguments. For instance, when function foo() has no return values, then "print(foo());" should fail and "print(foo(), 0);" should not print "0", but also fail. - 2) Call arguments are just one out of many places where a UDF with zero results could be used in DaphneDSL. All these places should be handled consistently. In fact, we have valueOrError() for that purpose and it already covers this case. - So this change was undone. - Note that the error message displayed to the user is not informative yet, but that will be improved in a follow-up commit. - Added several script-level test cases. - Check that DAPHNE does not accept UDFs with zero or more than one return value in places where exactly one value is expected. - All of these tests on a UDF with zero return values crash DAPHNE with a segfault on main (so problem in daphne-eu#595) is not specific to call expressions). - These are just a few examples, actually we should have test cases for all uses of "expr" in the DaphneDSL grammar.
Daphne (v0.2 && main branch) crashes when invoking print() on an empty UDF (not return types, no statements in body).
DSL script:
Stacktrace:
I'm assuming that since no return type is known/can be deduced there is a segfault when checking for the type. Imho the daphne compiler should not accept such a script since no return type can be deduced from the function. Options to handle this could be either to enforce the user to have at least one return statement within a UDF or fail the compilation of no return type can be deduced from the UDF.
The text was updated successfully, but these errors were encountered: