Haiyang-Sun / nodeprof.js

Instrumentation framework for Node.js compliant to ECMAScript 2020 based on GraalVM.
Apache License 2.0
53 stars 22 forks source link

Unexpected value for 'dis' in functionEnter hook #35

Closed barslev closed 5 years ago

barslev commented 5 years ago

I have created a small JavaScript program and a Jalangi analysis, which shows a case where the functionEnter hook is triggered with an unexpected value for dis.

// Test program
function f() {
  return this;
}
var y = f();

The JavaScript program simply calls f and writes the return value to y. It is expected that this in f should be the global object. Below is the Jalangi analysis that shows the unexpected behavior:

// DO NOT INSTRUMENT
J$.analysis = {};

(function (sandbox) {
  function AnalysisEngine() {
    this.functionEnter = function functionEnter(iid, f, dis, args) {
      if (f.toString().includes("function (exports, require, module, __filename, __dirname)"))
        return;
      console.log("functionEnter");
      console.log('  dis: ' + dis);
      console.log('  location: ' + sandbox.iidToLocation(iid));
    }

    this.write = function write(iid, name, val, lhs, isGlobal, isScriptLocal) {
      if (name !== 'y')
        return;
      console.log("write to variable: " + name);
      console.log('  val: ' + val);
      console.log('  location: ' + sandbox.iidToLocation(iid));
    }       
  }
  sandbox.analysis = new AnalysisEngine();
})(J$);

Running NodeProf with the above program and analysis, we get the following output:

functionEnter
  dis: undefined
  location: (wrongDisInFunctionEnterTest.js:2:1:4:2)
write to variable: y
  val: [object global]
  location: (wrongDisInFunctionEnterTest.js:5:5:5:12)

It shows correctly that the value written to y is the global object, so f is executed correctly, but when entering function f the value for dis in the functionEnter hook is undefined, instead of being the global object as expected.

Haiyang-Sun commented 5 years ago

@barslev , thanks, l will look into this issue.

alexjordan commented 5 years ago

We are waiting for a Graal.js change to be merged in order to fix this.

alexjordan commented 5 years ago

This has been fixed with 2c91d9bac5538938f02aaf11d34ad69504f4ba97.

I'm still observing cases where the proper this object cannot be resolved by NodeProf (e.g. an empty function or one that does not reference this). We're still looking into that, but to me it sounds like a graal-js optimization we would have to undo.