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

clean up interfaces for working with functions and strings #35

Closed
davepacheco opened this issue Sep 21, 2015 · 5 comments
Closed

clean up interfaces for working with functions and strings #35

davepacheco opened this issue Sep 21, 2015 · 5 comments
Assignees

Comments

@davepacheco
Copy link
Contributor

As part of the general internal interface clean-up, this ticket covers cleaning up the function-specific interfaces (like getting the name for a function, source code, instructions, and the like).

@davepacheco
Copy link
Contributor Author

This ticket is a first step towards cleaning up various internal mdb_v8 interfaces. There's a lot that could be done here, but I'm trying to focus on cleanup that's pretty clearly helpful in the short term. (I actually started by developing crisper interfaces for new work, as with #17, and this is the first step that involves improving existing interfaces.)

I've polished a decent chunk of this in this branch:
joyent:master...davepacheco:dev-issue-35

The new interfaces are encapsulated in this file:
https://github.com/joyent/mdb_v8/blob/6b8cf0aa0b6f2bc4b10b6c63f5d766bcd239274b/src/mdb_v8_dbg.h

The interesting parts of this interface are implemented in these files:

https://github.com/joyent/mdb_v8/blob/6b8cf0aa0b6f2bc4b10b6c63f5d766bcd239274b/src/mdb_v8_string.c
https://github.com/joyent/mdb_v8/blob/6b8cf0aa0b6f2bc4b10b6c63f5d766bcd239274b/src/mdb_v8_subr.c
https://github.com/joyent/mdb_v8/blob/6b8cf0aa0b6f2bc4b10b6c63f5d766bcd239274b/src/mdb_v8_function.c

You'll find that there are some pretty strong conventions about how these interfaces look. Because I want to do this incrementally rather than trying to rewrite mdb_v8, not everything has a new-style interface, nor have all the consumers been updated. But several consumers have been updated, like dcmd_v8str() and dcmd_v8function(). The latter used to look like this:

https://github.com/joyent/mdb_v8/blob/2b2d9273fb6a636b634ce3102937f84406188182/src/mdb_v8.c#L3725-L3813

That code has direct knowledge of the structure of V8 functions -- knowledge that's duplicated in other dcmds like jsframe. These commands do use some utility functions (like jsfunc_name() and jsfunc_lineno()), but even using these requires that the caller has already picked apart various V8 state. The new function looks like this:

https://github.com/joyent/mdb_v8/blob/6b8cf0aa0b6f2bc4b10b6c63f5d766bcd239274b/src/mdb_v8.c#L3391-L3447

which I think is a lot cleaner.

CC @jclulow @misterdjules @rmustacc @bcantrill

@misterdjules
Copy link
Contributor

It's a big diff and without having used these new interfaces myself, it's difficult to challenge them, but I can see clear improvements over what exists right now, and couldn't think of any red flag. I like where this is going.

@davepacheco
Copy link
Contributor Author

Thanks for taking a look.

@davepacheco
Copy link
Contributor Author

Last call for major feedback on this change. (I'll still be happy to get feedback after this, but I'm going to land the change into master.)

@davepacheco
Copy link
Contributor Author

It took me a long time to do enough regression-testing that I'm satisfied with the change, but I feel it's ready to integrate now. On the plus side, my testing found several bugs that I fixed (mostly not regressions). I'm pretty confident that this is a net step forward, but @misterdjules, please let me know if you want to take another look before I integrate it.

The delta from #master is here:
master...davepacheco:dev-issue-35

Here's the delta from the last review I sent out (on 10/8):
davepacheco/mdb_v8@6b8cf0a...davepacheco:dev-issue-35
(This diff unfortunately includes two recent commits that are already in #master -- the "-g -O" change to GNUMakefile, and the "runtests_node" tool.)

The real work from the last time I sent this out includes:

  • a new set of Developer's Notes (documentation)
  • two new tools that I used to test: mdbv8diff and dumpjsobjects, both described in the new docs
  • fixed an off-by-one bug I found in read_heap_dict() that caused us to report an erroneous extra property on some objects (in a non-deterministic way, to boot)
  • fixed incorrect behavior from jsstr_print() when given garbage strings
  • fixed a crash in dcmd_v8code() when given invalid values
  • fixed a crash in v8function_free() when given a NULL pointer
  • fixed the string truncation behavior to be more precise so that it was more appropriate to allocate a buffer of the appropriate size and then write a string into it. (That still doesn't quite work when characters need to be escaped.) This was surprisingly complicated, and still incomplete because of issue missing ellipsis when printing ConsString with undersized buffer #49 (not a regression).

@davepacheco davepacheco changed the title clean up interfaces for working with functions clean up interfaces for working with functions and strings Nov 12, 2015
davepacheco pushed a commit to davepacheco/mdb_v8 that referenced this issue Nov 13, 2015
@davepacheco davepacheco self-assigned this Nov 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants