svaarala / duktape

Duktape - embeddable Javascript engine with a focus on portability and compact footprint
MIT License
5.94k stars 514 forks source link

Incompatible debugger changes for next major release #585

Open svaarala opened 8 years ago

svaarala commented 8 years ago

Some possibilities:

fatcerberus commented 8 years ago

As you note, doing a full heap object inspection can often be an involved process involving multiple requests, state tracking, etc. One possibility for reducing that pain when dealing with Error objects received by the client would be to provide a convenience command that took the Error's heapptr and returned back the typical values used in error handling: filename, line number, etc.

fatcerberus commented 8 years ago

We'll call this hypothetical command 'ParseError'.

svaarala commented 8 years ago

Adding an "eval with arguments" would allow the user to do that parsing relatively conveniently (see #586) without adding an overlapping mechanism so I'd somewhat prefer that.

Further, when new Error properties are added that'd mean extending ParseError with new fields which would be avoided if the user would coerce/parse the error in the most suitable way for them, perhaps including custom error fields, etc.

fatcerberus commented 8 years ago

Just to verify, semantic versioning guarantees apply only to source-level compatibility, not binary, correct? If so one way to improve duk_debugger_attach() would be to have it accept a struct as argument describing all the callbacks.

We'd need to stress in the documentation that the caller must zero the struct first so that when new fields are added in later versions, they don't get garbage values.

svaarala commented 8 years ago

That's one way to approach extensibility - but it prevents literal initializers which is a shame. Further, to be strict, there are no guarantees that zeroing memory initializes pointers to NULL because the NULL bit pattern is not necessarily all zeroes.

svaarala commented 8 years ago

Named struct whose members are not public, with macro initializers would be a source compatible approach. It's used for the duk_def_prop_list() API call and it works quite well for that. The downside is that it's not as direct as intuitive as a plain call with plain arguments.

fatcerberus commented 8 years ago

Huh. Are there really platforms where NULL is not all zero bits? It's a very, very common pattern to initialize struct memory containing pointers using memset so I assumed it was guaranteed by the C standards.

Anyway this kind of thing is why object literals are in my top 3 favorite JS features. :)

svaarala commented 8 years ago

It's not guaranteed; what is guaranteed is that converting an integer 0 to a pointer results in a NULL pointer, but there are no guarantees (AFAIK) for the memory pattern holding a NULL pointer.

svaarala commented 8 years ago

Re: object literals, C99 provides designated initializers ({ .foo=123, .bar=234, ... }) which would be useful for source portability. But there's the same issue with NULL portability, and designated initializers are not part of even C++11 if I've understood correctly (though supported by some compilers) which makes them sometimes an issue even with modern compilers. Needless to say pre-C99 compilers don't have them, and Duktape works with several C89 environments so there's little besides macros to make initializers more easily versionable.

The named "black box struct" + macro initializer approach works pretty well for duk_def_prop_list() so maybe that pattern could be used elsewhere too. It's easily versionable with no footprint cost, and makes very few compiler assumptions.

fatcerberus commented 8 years ago

Yeah, I knew about designated initializers, it's not a replacement for true object literals though since as far as I know you can't construct an object on-demand and pass it to a function, you need to use it to initialize a variable and then pass that. A bit inconvenient, though macros can ease the pain somewhat.

I think you're right about C++ too, I remember reading in MSDN that designated initializers are only supported when compiling C code. :-/

svaarala commented 8 years ago

The testcase failures are for heap destruction finalizer runs and they don't actually indicate a bug as such: the new handling for detach1 runs a forced GC to handle any garbage created during the paused state. This causes a few extra GC rounds for heap destruction, which affects the test results because the finalizers in the tests are sensitive to the "is rescue possible?" flag on purpose.

svaarala commented 8 years ago

The current GC approach in this pull is:

The downside is that the forced GC rounds will also collect garbage accumulated before getting paused, which is a deviation from the normal lifecycle of that garbage. Mark-and-sweep intervals are not guaranteed so nothing is wrong with that as such, but ideally a debugger should have a minimal number of side effects. As things are now, stepping causes forced GC to run every time we step.

Ideally I'd like to just queue objects for refzero handling during the paused state, and then run through the refzero queue on detach/resume. This is not directly possible now because duk_hstring header is stripped of next/prev link pointers (strings live in the string table so they're not needed). So strings must be either freed directly in refzero, or left on the heap for mark-and-sweep to collect.

One compromise now would be to leave strings up to mark-and-sweep but queue objects and buffers using the refzero queue. Since string freeing has no direct side effects (they don't reference other objects and can't have a finalizer) this would have fewer side effects than now.

This situation may change once the string table is reworked to use a single algorithm instead of the two variants now in use (one default, another for low memory targets). Some alternatives include adding link pointers to duk_hstring which would then allow refzero queueing for them too.