Skip to content
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

return/throw callback support #37

Open
mwaldrich opened this issue Jul 2, 2019 · 10 comments
Open

return/throw callback support #37

mwaldrich opened this issue Jul 2, 2019 · 10 comments

Comments

@mwaldrich
Copy link
Contributor

Is there an ETA for implementation of these callbacks? My team would like to use these in our analysis.

@mwaldrich
Copy link
Contributor Author

I think it's also important to note that functionExit/invokeFun aren't sufficient for knowing what line of code returns. This is because try/finally blocks can execute code after an expression is returned.

For example, the given analysis used on the given code makes it impossible to tell which line of code triggers the return.

analysis.js:

// DO NOT INSTRUMENT
(function (sandbox) {
    function MyAnalysis() {

        /**
         * These callbacks are called before and after a function, method, or constructor invocation.
         **/
        this.invokeFunPre = function (iid, f, base, args, isConstructor, isMethod, functionIid, functionSid) {
            console.log("invokeFunPre");
        };
        this.invokeFun = function (iid, f, base, args, result, isConstructor, isMethod, functionIid, functionSid) {
            console.log("invokeFun");
        };

        /**
         * These callbacks are called before the execution of a function body starts and after it completes.
         **/
        this.functionEnter = function (iid, f, dis, args) {
            console.log("functionEnter");
        };
        this.functionExit = function (iid, returnVal, wrappedExceptionVal) {
            console.log("functionExit");
        };

        /**
         * These callbacks are called after a variable is read or written.
         **/
        this.read = function (iid, name, val, isGlobal, isScriptLocal) {
            console.log("read " + val);
        };
        this.write = function (iid, name, val, lhs, isGlobal, isScriptLocal) {
            console.log("write " + name + " = " + val);
        };
    }

    sandbox.analysis = new MyAnalysis();
})(J$);

function.js:

let a = 0;

function example(arg1, arg2) {
    try {
        return arg1;
    } finally {
        a = 1;
        arg2;
    }
}

example(10, 20);

mx jalangi --analysis analysis.js function.js:

functionEnter
functionExit
functionEnter
write example = function example(arg1, arg2) {
    try {
        return arg1;
    } finally {
        a = 1;
        arg2;
    }
}
write a = 0
read function example(arg1, arg2) {
    try {
        return arg1;
    } finally {
        a = 1;
        arg2;
    }
}
invokeFunPre
functionEnter
read 10
write a = 1
read 20
functionExit
invokeFun
functionExit

Ideally, listening to the return callback would give us:

functionEnter
functionExit
functionEnter
write example = function example(arg1, arg2) {
    try {
        return arg1;
    } finally {
        a = 1;
        arg2;
    }
}
write a = 0
read function example(arg1, arg2) {
    try {
        return arg1;
    } finally {
        a = 1;
        arg2;
    }
}
invokeFunPre
functionEnter
read 10
return 10 // NEW CALLBACK
write a = 1
read 20
functionExit
invokeFun
functionExit

@mwaldrich
Copy link
Contributor Author

Any update on this feature?

@Haiyang-Sun
Copy link
Owner

The ThrowNode has a branch tag, which I can hook in NodeProf. However there is not yet any specific tag for the ReturnNode.

@eleinadani is it possible to add a new return tag for ReturnNode?

@eleinadani
Copy link
Collaborator

couldn't you simply intercept all throw/return values via onReturnExceptional and by instrumenting the function roots? One problem with ReturnNode is that it's not used everywhere, as we sometimes store return values in a frame slot for opt

@Haiyang-Sun
Copy link
Owner

It is about the comment from @mwaldrich above

I think it's also important to note that functionExit/invokeFun aren't sufficient for knowing what line of code returns. This is because try/finally blocks can execute code after an expression is returned.

Existing value returned or thrown do not reveal the line of code which returns or throws.

@alexjordan
Copy link
Collaborator

Instrumentation support for return has been merged upstream in oracle/graaljs@e4bd649

@mwaldrich
Copy link
Contributor Author

Thanks for the implementation! I'm starting to use this in my analysis now.

I noticed this callback doesn't provide the function that the code is returning from. While it is possible for me to maintain a call stack in my analysis, it would be much more efficient to re-use the call stack in Graal.

In addition, this change would make the _return callback consistent with the other function-related callbacks (invokeFun, functionEnter), since they pass the function as an argument as well.

@Haiyang-Sun
Copy link
Owner

@mwaldrich this is doable, I can add support for this.

@alexjordan
Copy link
Collaborator

I would advise against this: executing a return statement does not imply that the function indeed exits (functionExit does). _return is more closely related to control-flow than function, so adding the containing function is misleading IMO.

Consider this example:

 try {
   throw Error("with finally");
   return 'nope';
 } catch(e) {
   return 42;
 } finally {
   return 43;
 }

The _return callback will trigger like this:

> return (src/ch.usi.inf.nodeprof.test/js/minitests/exitException.js:53:4:53:14) val returns 42
> return (src/ch.usi.inf.nodeprof.test/js/minitests/exitException.js:55:4:55:14) val returns 43

@mwaldrich
Copy link
Contributor Author

Yes, it wouldn't make sense to model a callstack using the _return callback, because it does not signal the exiting of a function. But since I am trying to avoid modeling the callstack altogether, I wasn't trying to use _return for this.

I wanted the function in the _return callback just so I could associate function invocations with the values they returned.

If I wanted to do this with the current callback, I would need to model a callstack in my analysis (using functionEnter and functionExit), and associate the _return callback with the function on the top of the stack. If the _return callback just gave the surrounding function as an argument, it would eliminate the need for me to model the callstack.

However, I could see how people might assume that _return does always signal the exit of a function. Maybe we could specify this in the documentation of the _return callback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants