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

JS API: initial implementation (N-API version) #206

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@joyeecheung
Member

joyeecheung commented Jun 24, 2018

Refs: #14

This is a revamp of #147 : now that we have dropped support of Node.js v4.x, we can use N-API (or the C++ wrapper to be exact) to build the binding (which is much easier to use than the V8 API/NAN). It builds on previous work that refactored the build files and restructured how llnode instances are created so this PR is much smaller than what #147 originally was.

The current API is laid down in JSAPI.md, currently the results of the inspection are just strings that we use to print in LLDB instead of structured data - we can implement custom inspector of different types in llv8.h later so that proper JS objects can be returned back to JS.

Note that the current test does not pass on Node v10 core dumps since object inspection of llnode is half-broken on Node v10 (e.g. process.platform is recognized as Smi)

cc @mmarchini @bnoordhuis

JSAPI.md Outdated
* @param {string} executable path to the node executable
* @returns {LLNode} an LLNode instance
*/
static fromCoredump(dump, executable) {}

This comment has been minimized.

@hyj1991

hyj1991 Jun 25, 2018

class LLNode {
  constructor(dump, executable){ }
  
  // LLDB load coredump and executable
  loadCore()
}

Is this form better?

This comment has been minimized.

@joyeecheung

joyeecheung Jun 25, 2018

Member

@hyj1991 I'd like to avoid overloading the constructor, since loading from a core dump should not be the only way to start an llnode instance, we may want to be able to attach to a process or launch a process via the API in the future.

This comment has been minimized.

@hyj1991

hyj1991 Jun 25, 2018

@joyeecheung looking forward to it

@mmarchini

This comment has been minimized.

Member

mmarchini commented Jun 25, 2018

Note that the current test does not pass on Node v10 core dumps since object inspection of llnode is half-broken on Node v10 (e.g. process.platform is recognized as Smi)

Do we have an issue to fix this?

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jun 25, 2018

@mmarchini I don't think so, in general llnode seems pretty broken on v10. Here is what the process object looks like in v10.5.0

See `v8 inspect` output
0x00002974b3103e51:<Object: process properties {
    .title=<unknown field type>,
    .version=<Smi: 1>,
    .versions=<Smi: 1>,
    .arch=<Smi: 1>,
    .platform=<Smi: 1>,
    .release=<Smi: 1>,
    .argv=0x000029740ad2a5e9:<Array: length=2>,
    .execArgv=0x000029740ad2a609:<Array: length=1>,
    .env=0x000029740ad2a629:<Object: (anonymous)>,
    .pid=<Smi: 1>,
    .features=<Smi: 1>,
    .ppid=<unknown field type>,
    .execPath=0x000029740ad2a679:<String: "/Users/joyee/.nv...">,
    .debugPort=<unknown field type>,
    ._startProfilerIdleNotifier=<unknown field type>,
    ._stopProfilerIdleNotifier=<unknown field type>,
    .abort=<unknown field type>,
    .chdir=0x0000297451b5b269:<function: chdir at (external).js:20:33>,
    .umask=0x0000297451b5b2a9:<function: umask at (external).js:24:33>,
    ._getActiveRequests=<unknown field type>,
    ._getActiveHandles=<unknown field type>,
    .reallyExit=<unknown field type>,
    .cwd=<unknown field type>,
    .getuid=<unknown field type>,
    .geteuid=<unknown field type>,
    .getgid=<unknown field type>,
    .getegid=<unknown field type>,
    .getgroups=<unknown field type>,
    ._kill=<unknown field type>,
    .dlopen=<unknown field type>,
    ._debugProcess=<unknown field type>,
    ._debugEnd=<unknown field type>,
    .hrtime=0x0000297451b5c929:<function: hrtime at (external).js:96:35>,
    .cpuUsage=0x0000297451b5cc81:<function: cpuUsage at (external).js:35:39>,
    .uptime=<unknown field type>,
    .memoryUsage=0x0000297451b5cf31:<function: memoryUsage at (external).js:123:45>,
    ._rawDebug=0x0000297451b60759:<function: process._rawDebug at (external).js:250:31>,
    .moduleLoadList=<Smi: 1>,
    .binding=<unknown field type>,
    ._linkedBinding=<unknown field type>,
    ._events=0x000029740ad2a6c1:<Object: Object>,
    ._eventsCount=<Smi: 4>,
    ._maxListeners=0x0000297467d022e1:<undefined>,
    ._fatalException=<unknown field type>,
    .domain=0x0000297467d02201:<null>,
    ._exiting=0x0000297467d023f1:<false>,
    .assert=<unknown field type>,
    .config=0x0000297474f99471:<Object: Object>,
    .setUncaughtExceptionCaptureCallback=<unknown field type>,
    .hasUncaughtExceptionCaptureCallback=<unknown field type>,
    .emitWarning=<unknown field type>,
    .nextTick=<unknown field type>,
    ._tickCallback=<unknown field type>,
    .stdout=<unknown field type>,
    .stderr=<unknown field type>,
    .stdin=<unknown field type>,
    .openStdin=<unknown field type>,
    .initgroups=<unknown field type>,
    .setegid=<unknown field type>,
    .seteuid=<unknown field type>,
    .setgid=<unknown field type>,
    .setuid=<unknown field type>,
    .setgroups=<unknown field type>,
    .exit=<unknown field type>,
    .kill=<unknown field type>,
    .argv0=0x000029740ad30a11:<String: "node">,
    .mainModule=0x000029746a782ae1:<Object: Module>}>

I updated this PR to fix the compilation error on Windows now that #203 landed. I've split that commit into #208

@mmarchini

Left a few comments, but overall looks great! Thanks!

JSAPI.md Outdated
*
* @returns {Process} Process data
*/
getProcessobject() {}

This comment has been minimized.

@mmarchini

mmarchini Jun 25, 2018

Member

s/getProcessobject/getProcessObject/

JSAPI.md Outdated
* TODO: rematerialize object
* @returns {HeapInstance}
*/
getobjectAtAddress(address) {}

This comment has been minimized.

@mmarchini

mmarchini Jun 25, 2018

Member

s/getobjectAtAddress/getObjectAtAddress/

std::string LLNodeApi::GetProcessState() {
int state = process->GetState();
switch (state) {

This comment has been minimized.

@mmarchini

mmarchini Jun 25, 2018

Member

Could we use SBEvent::GetCStringFromEvent(SBEvent event) -> str const * instead of the switch?

This comment has been minimized.

@joyeecheung

joyeecheung Jun 25, 2018

Member

@mmarchini I dug into the headers a little bit and looks like debugger->StateAsCString returns what we want too

joyeecheung added a commit to joyeecheung/llnode that referenced this pull request Jun 25, 2018

src: move ParseInspectOptions out of CommandBase
It is referenced in both llnode.cc and llscan.cc,
to make sure targets can compile by only including
llscan.h, this is the minimum change required.

PR-URL: nodejs#208
Refs: nodejs#206
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>

joyeecheung added a commit that referenced this pull request Jun 25, 2018

src: move ParseInspectOptions out of CommandBase
It is referenced in both llnode.cc and llscan.cc,
to make sure targets can compile by only including
llscan.h, this is the minimum change required.

PR-URL: #208
Refs: #206
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jun 27, 2018

So, a few things to do before this can land:

  1. Skip the addon build and the test for nightly and canary since node-addon-api isn't there yet
  2. Node.js 6.x does not have objects with the classTickObject and console
  3. Node.js 10.x is broken. We can either skip the test that looks into the process object, or keep it.
@mmarchini

This comment has been minimized.

Member

mmarchini commented Jun 28, 2018

Node.js 10.x is broken. We can either skip the test that looks into the process object, or keep it.

IMO we should keep it since we'll have to fix v10.x sooner or later.

@mmarchini

This comment has been minimized.

Member

mmarchini commented Jun 28, 2018

Here is what the process object looks like in v10.5.0

Ahh, something changed between v10.4.0 and v10.5.0. 10.4.0 seems to be working fine (at least for the process object).

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jul 4, 2018

@mmarchini I've updated the build files and the tests:

  • Split tests to test/addon and test/plugin
  • Skip building addons and running addon tests in nightly and
    canary builds in Travis
  • Do not build the addon by default (so normal users who npm install the release from npm registry will not be affected) for now
  • Add instructions about building and testing the addon in the README (only in the "Develop & Test" section though, we might want to clean up the docs later)
  • Do not expect console and TickObject in the heap since they are not there in 6.x

There are only two failures in Travis now:

  • canary is broken (because llnode is broken on V8 TOT)
  • 10.x is broken on mac OX (because llnode is broken there #209)

This should be safe to land now.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Jul 4, 2018

@mmarchini Does this still look good to you?

@mmarchini

This comment has been minimized.

Member

mmarchini commented Jul 6, 2018

Yes! :)

@joyeecheung joyeecheung closed this Jul 6, 2018

joyeecheung added a commit to joyeecheung/llnode that referenced this pull request Jul 6, 2018

src: implement JavaScript API
Co-authored-by: Richard Chamberlain <richard_chamberlain@uk.ibm.com>
PR-URL: nodejs#206
Refs: nodejs#14
Reviewed-By: Matheus Marchini <matheus@sthima.com>

joyeecheung added a commit to joyeecheung/llnode that referenced this pull request Jul 6, 2018

doc, test: doc API design, add tests
PR-URL: nodejs#206
Refs: nodejs#14
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment