Bug #738
prevent direct calls to sdag entry functions
Description
Calls to sdag entries from within "serial" blocks of sdag entries return immediately (or at the first unfulfilled "when" block), like a message send, rather than integrating into the sdag workflow. This is not the expected behavior. Giving the function that implements an sdag entry method a different name than the entry method itself would prevent this, so it would be impossible to accidentally call mysdag() rather than thisProxy->mysdag().
This issue was reported by one of the speakers at the Charm++ workshop.
History
#1 Updated by Phil Miller over 3 years ago
- Assignee set to PPL
- Priority changed from Normal to High
This is a serious issue of ease of use and principle of least surprise that we need to work out. We'll discuss it, and try to find something to do about it.
#2 Updated by Eric Bohm over 3 years ago
Figure out how to handle the case where the user really wants this functionality vs the case where someone stumbles into unexpectedly by nesting.
#3 Updated by Eric Bohm over 3 years ago
- Assignee changed from PPL to Jonathan Lifflander
#4 Updated by Phil Miller over 3 years ago
Per core meeting discussion, this is a semantic issue that needs to be worked out carefully, not just a coding task. Meeting between Phil, Jonathan, & Eric M to work out what to do about this.
#5 Updated by Eric Bohm over 2 years ago
- Assignee changed from Jonathan Lifflander to Nitin Bhat
#6 Updated by Nitin Bhat about 2 years ago
- Status changed from New to Implemented
- Category set to Charmxi
- % Done changed from 0 to 100
- Estimated time set to 2.00 h
Modified every sdag entry method to be implemented in .C as "_sdag_" + entry method name
In a .ci file if there's an entry method called mysdag() that is used with the when construct, it is implemented with a function "_sdag_mysdag". This prevents direct calls made to "mysdag()".
#7 Updated by Nitin Bhat over 1 year ago
- Target version set to 6.8.0
Changed the prefix for sdag entry methods to "_sdag_fnc_" as "_sdag_init" is an already generated xi function.
#8 Updated by Phil Miller over 1 year ago
- Target version changed from 6.8.0 to 6.9.0
Proposed API change, lots of corresponding code to change, not a priority for getting the release out.
#9 Updated by Laxmikant "Sanjay" Kale over 1 year ago
Phil's note 2 years ago said "is a semantic issue that needs to be worked out carefully" and suggested a meeting. Is this a correct, consensus decision? (A minor issue is projections will show unexpected EP name. But that is easy to handle).
#10 Updated by Nitin Bhat over 1 year ago
Fix: Gerrit: https://charm.cs.illinois.edu/gerrit/#/c/2205/
The current implemented fix is where an sdag entry method that contains when statement(s) gets renamed to "_sdag_fnc_"+name and thereby cannot be directly called. Sdag entry methods without when statements can still be directly called.
We did discuss it in core a few months back, but it'll be worthwhile to bring it up again esp because OpenAtom uses direct sdag calls and those will have to change.
Openatom build failure: (http://ppl-jenkins:8080/job/test-openatom/9872/console)
#11 Updated by Laxmikant "Sanjay" Kale over 1 year ago
Ideally, charmxi should also produce an error message (in case the function name fname doesn’t match, but _sdag_fnc_fname matches) telling the programmer not to call sdag. Is that easy/hard/too hard?
#12 Updated by Nitin Bhat over 1 year ago
Didn't entirely understand the example. Do you mean when the user declares an SDAG Entry method in the .ci as "fname" and then calls the xi generated method "_sdag_fnc_fname" directly? (instead of calling proxy.fname();)
#13 Updated by Nitin Bhat about 1 year ago
This is waiting on the changes to be made on Openatom. (https://charm.cs.illinois.edu/gerrit/#/c/2205/)
#14 Updated by Nitin Bhat 11 months ago
I have made changes to Openatom to convert direct sdag calls to proxy calls and have this change on a branch called `nitin/sdag_changes` in the Openatom repository. Need an openatom expert to review and merge this.
#15 Updated by Nitin Bhat 11 months ago
Openatom changes corresponding to this bug fix have been merged: https://charm.cs.illinois.edu/gerrit/#/c/openatom/+/3917/
#16 Updated by Nitin Bhat 10 months ago
- Status changed from Implemented to Merged