425569 - Treehydra: Exceptions for read_file and write_file

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

Presently, if the file does not exist (including writing to a nonexistent dir), GCC ICEs. This is annoying. I want Treehydra to generate a JS exception, or at least print an error message and then exit.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

Here's the patch. Hopefully no surprises, I just implemented the most obvious design.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Unit test for proposed patch

Run as the treehydra script with any C++ input file, should pass 8 tests.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 312176 [details] [diff] [review]
Proposed patch

>diff -r a50078160d27 dehydra_builtins.c
>--- a/dehydra_builtins.c	Wed Mar 26 18:47:14 2008 -0700
>+++ b/dehydra_builtins.c	Thu Mar 27 18:34:11 2008 -0700
>@@ -1,3 +1,7 @@
>+#include <string.h>
>+#include <errno.h>
>+#include <stdarg.h>
>+
> #include <config.h>
> #include <system.h>
> #include <coretypes.h>
>@@ -10,6 +14,25 @@
> #include "dehydra.h"
> #include "dehydra_builtins.h"
> #include "xassert.h"
>+
>+/* Use this to throw an exception to JS from a native method. It's fine
>+ * to return it directly, as it returns false. */
>+static JSBool throwException(JSContext *cx, const char *fmt, ...)
>+{
>+  char sbuf[1024];
>+  int size = sizeof(sbuf) / sizeof(sbuf[0]);
>+  va_list argp;
>+
>+  va_start(argp, fmt);
>+  int n = vsnprintf(sbuf, size, fmt, argp);
>+  va_end(argp);
>+  if (n >= size) {
>+    sbuf[size-1] = '\0';
>+  }
>+
>+  JS_SetPendingException(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx, sbuf)));
>+  return JS_FALSE; 

This will make you sad, since you won't have any stack info in it.  Recommend JS_ReportError instead, which gives script and line number.

(We could expose JS_ReportErrorVA, like we do with ConvertArgs, even!)

>+}
> 
> JSBool Print(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>              jsval *rval)
>@@ -119,12 +142,15 @@ ReportError(JSContext *cx, const char *m
> 
> JSBool ReadFile(JSContext *cx, JSObject *obj, uintN argc,
>                 jsval *argv, jsval *rval) {
>-  if (!(argc == 1 && JSVAL_IS_STRING(argv[0]))) return JS_TRUE;
>+  if (!(argc == 1 && JSVAL_IS_STRING(argv[0]))) {
>+    return throwException(cx, "read_file: invalid arguments, requires string");
>+  }
>+  const char *filename = JS_GetStringBytes(JSVAL_TO_STRING(argv[0])); 

Here (and elsewhere) you can use JS_ConvertArguments to save some pain:

if (!JS_ConvertArguments(cx, argc, argv, "s", &filename))
   return JS_FALSE;

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Revised patch

This version uses JS_ReportError and does produce stack traces, so it's better.

I also switched to using JS_ConvertArguments in places where it made sense, which simplifies the code a bit. It does change the behavior--previously, non-string args were an error, now args are converted to strings automatically. This change is a separate patch, so I can easily ditch it or separate it from the other if you don't like it.

Comment 5

[(dormant account)]

Looks good to me. I didn't know about JS_ConvertArguments before, should get that in too. 

One little annoyance that you could fix while at it is that include doesn't change the filename after inclusion. It'd be nice if backtraces showed ../treehydra.js instead of just the filename. But probably better to do this as a separate bug.