Allow caller to control blaming of __FILE__/__LINE__ in error .fileName and .lineNumber

[fatcerberus]

I sometimes need to throw an error from a Duktape/C function but want it to be augmented with the caller's filename and line number. Currently duk_error() uses __FILE__ and __LINE__, which has forced me to implement a hack using Duktape.act():

https://github.com/fatcerberus/minisphere/blob/master/src/minisphere/api.c#L377-L413

Additionally this forces me to keep the Duktape object visible (I would prefer to hide it) since minisphere needs to access it by name internally.

If there were a variant of duk_error_* that didn't augment, this wouldn't be needed, since Duktape would then default to the first callstack entry with a file/line (in practice, the Ecmascript caller).

[svaarala]

The fix proposed in the original issue title wouldn't be an improvement (IMO) because blaming the __FILE__/__LINE__ is often the approriate measure. Also I think the actual issue is not really about whether or not to augment the error with the C file/line but whether to attribute that to .fileName and .lineNumber. Summary of previous discussion re: blaming is here:

• https://github.com/svaarala/duktape/blob/master/doc/error-objects.rst#choosing-filename-and-linenumber-to-blame-for-an-error

I think the best approach would be to always include the file/line in the trace data so that the C call site always appears in the stack trace. However, it would be good to be able to control the "blaming" for .fileName and .lineNumber. Duktape actually already does that internally, so exposing that via an API would be quite straightforward.

The basic API alternatives would be:

• Add a flags field to duk_error(), duk_push_error_object() etc, which allows the blaming behavior to be controlled.
• Add separate call variants for "no blame" call sites.

[fatcerberus]

Basically I just want a setup where I can throw an error from C code but have it be blamed on the Ecmascript caller when someone accesses the .fileName and .lineNumber of the Error object. This is already the case for errors thrown by e.g. duk_require_xxx, I'm looking for similar functionality for duk_error().

[svaarala]

Right, and you'd get that if (1) the file/line was recorded in the tracedata but (2) marked as "no blame" as is already done for Duktape internal errors. The .fileName and .lineNumber accessors have a check for a blame/noblame flag already:

• https://github.com/svaarala/duktape/blob/master/src/duk_bi_error.c#L247-L258

So the only thing that's needed is some way control whether that flag is set for user created errors. Both settings are needed because sometimes the C call site should be blamed and in other cases it shouldn't.

Even when the C file/line is not blamed for .fileName/.lineNumber it's good to record it in the tracedata so that it shows up in the stack trace. Well, I guess there could be cases where hiding the C code structure would be useful so maybe that'd need a flag of its own.

[svaarala]

@fatcerberus If you want a quick hacky test to see if this works for you, the internal constant controlling the blaming process is:

src/duk_api_internal.h:#define DUK_ERRCODE_FLAG_NOBLAME_FILELINE  (1L << 24)

That is OR'ed with the error code. So, you could try for example:

duk_error(ctx, DUK_ERR_TYPE_ERROR | 0x01000000L, "no blame error");

Then check if the .fileName and .lineNumber come out as you'd expect. The stack trace would stlil identify the C call site.

This is obviously not a solution right now because that constant is internal, but the required change is quite simple (I'll open a pull for discussing the solution in more detail).

[fatcerberus]

Thanks for that hack test, I'll try that out later. For production code I'll probably keep using Duktape.act() (maybe stashing it on startup), but it'll be interesting to see if the hack works.

[fatcerberus]

Regarding Duktape.act(): If I take the heap pointer of that function and then delete the Duktape object from global, that is safe, correct? Because Duktape holds a reference to that object internally.

[svaarala]

Right, Duktape is always reachable for the internals via thr->builtins[DUK_BIDX_DUKTAPE].

[svaarala]

@fatcerberus Yeah, that hack test would simply confirm that we're talking about the same thing :)

[fatcerberus]

Error throw site:

static duk_ret_t
js_sleep(duk_context* ctx)
{
    double seconds = duk_require_number(ctx, 0);

    if (seconds < 0.0)
        duk_error(ctx, DUK_ERR_RANGE_ERROR | 0x01000000, "invalid sleep time");
    delay(seconds);
    return 0;
}

Ecmascript testcase:

var engine = require('engine');
engine.sleep(-8.12);
// RangeError: invalid sleep time (scripts/main.js:2)
// without the hack, the file/line is engine_api.c:140

So yes, adding the bogus internal flag gets me the expected behavior.

[svaarala]

Great, so now it's just a matter of figuring out what the best API change is (preferably something that can be extended more easily than the previous one).

[fatcerberus]

Just to explain a bit why I've been so picky about error blaming behavior: During development it's of course easy to get a backtrace using the debugger (SSJ in my case). In production though, if an unhandled error is encountered it terminates the game with a screen showing the error message along with the file:line where it occurred. There's no room on that screen for a stack dump and even if there were, it would be intimidating to players. :)

So since that single file:line pair is all I will get for errors reported in a released game, it's very important that it be as useful as possible when tracking down a bug. :)

[svaarala]

I've encountered the same situation myself so I understand why the "blamed" file/line should be as relevant as possible. File/line is also available in the same form in several engines, and are programmatic rather than human readable text like the stack trace is.

So, I fully agree Duktape should try to provide as useful values as possible. We've already taken a few steps into that direction in previous changes and I think the issues are now pretty well understood (in that internal document reference I provided).

In essence, there are infrastructure call sites which shouldn't be blamed and user call sites which should so that a single primitive won't work for all cases. The current approach is that all Duktape throw sites are infrastructure and not blamed, and all user throw sites are not infrastructure and are blamed; obviously this won't work perfectly because some application code (outside Duktape) is infrastructure and some isn't, so more control in the API is needed.

All that said, it's entirely possible to replace the .fileName and .lineNumber either as direct properties or by replacing the inherited getter to provide some custom semantics, including some awareness of what tracedata entries to ignore for file/line blaming. But because that's not a trivial thing to implement, and will depend on version specific tracedata format, the default behavior and API should support the common cases.

[fatcerberus]

Anyway, I guess we can continue this discussion in #715 once there's a concrete change to test. For now I'm satisfied to know my issue is understood. :)