Closed Bug 854602 Opened 7 years ago Closed 7 years ago

improve asm.js validation errors

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

Several of the asm.js validation errors could be much more helpful.  E.g.:
 - X is not a subtype of Y: we should give the X and Y
 - ArrayBuffer byteLength link errors: give the actual byteLength value
iirc, you asked for better asm.js error messages on irc :)
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #744940 - Flags: review?(terrence)
Comment on attachment 744940 [details] [diff] [review]
with many improvements

Review of attachment 744940 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Thanks for doing this: it's going to be an enormous improvement to the usability of asm.js.

As a review general comment, the error messages are really inconsistent about whether they start capitalized and end with a period. I think we should standardize on a Capitalized first letter and a period at the end, but feel free to go with whatever you think best.

My experience using asm.js has been from compiling strings with Function: in this case, the reported error line is always the Function and never the line within the compiled string for some reason. With that in mind, I've tried to point out some places where we can use the new capabilities to remove ambiguity, given that the error message has no context. Feel free take these, or leave them on the floor if you think I'm being absurd.

::: js/src/ion/AsmJS.cpp
@@ +1204,1 @@
>          return false;

It may be worth calling fail(pn, '') if !bytes.ptr(). E.g. attempt to report the error with a short string if the requested message is too large to create under current memory usage.

@@ +2438,5 @@
>  static bool
>  CheckIdentifier(ModuleCompiler &m, PropertyName *name, ParseNode *nameNode)
>  {
>      if (name == m.cx()->names().arguments || name == m.cx()->names().eval)
> +        return m.fail(nameNode, "disallowed asm.js parameter name");

It would be helpful to know the name here:

return m.failf(nameNode, "Parameter name '%s' is disallowed in asm.js.", name->charsZ());

@@ +2454,5 @@
>          name == m.module().importArgumentName() ||
>          name == m.module().bufferArgumentName() ||
>          m.lookupGlobal(name))
>      {
> +        return m.fail(nameNode, "Duplicate names not allowed");

We can put the name in now:

return m.failf(nameNode, "Duplicated name '%s' not allowed at module level.", name->charsZ());

@@ +2476,5 @@
>  static bool
>  CheckArgument(ModuleCompiler &m, ParseNode *arg, PropertyName **name)
>  {
>      if (!IsDefinition(arg))
> +        return m.fail(arg, "overlapping argument names not allowed");

This should probably end with "in asm.js" as with similar errors. I don't see a really smooth way to shoehorn the argument name in here, but this is probably good enough:

return m.fail(arg, "Argument '%s' overlaps another name.", arg->name()->charsZ());

@@ +2481,3 @@
>  
>      if (MaybeDefinitionInitializer(arg))
> +        return m.fail(arg, "default arguments not allowed");

+= " in asm.js."

@@ +2535,5 @@
>  {
>      ParseNode *firstStatement = *stmtIter;
>  
>      if (!IsExpressionStatement(firstStatement))
> +        return m.fail(firstStatement, "No funny stuff before the 'use asm' directive");

"funny stuff" is a surprisingly colloquial turn of phrase -- I'm not sure whether or not that's a bad property to have here though.

@@ +2609,5 @@
>      if (!CheckTypeAnnotation(m, initNode, &coercion, &coercedExpr))
>          return false;
>  
>      if (!coercedExpr->isKind(PNK_DOT))
> +        return m.fail(coercedExpr, "Bad global variable import expression");

"Bad" is a bit generic: the same string as the !IsUseOfName case would work as well here.

@@ +2636,4 @@
>  
>      ParseNode *bufArg = NextNode(ctorExpr);
>      if (!bufArg)
> +        return m.fail(ctorExpr, "Constructor needs an argument");

How about "ArrayView constructor requires at least one argument."?

@@ +2643,4 @@
>  
>      if (!IsUseOfName(bufArg, m.module().bufferArgumentName()))
> +        return m.fail(bufArg, "Argument to typed array constructor must be the name of the module's "
> +                              "ArrayBuffer parameter (the 3rd parameter)");

{} around multi-line if. </excessive-pedantry>

@@ +2683,4 @@
>  
>          AsmJSMathBuiltin mathBuiltin;
>          if (!m.lookupStandardLibraryMathName(field, &mathBuiltin))
> +            return m.fail(initNode, "Does not match a standard Math builtin");

return m.failf(initNode, "'%s' is not a standard Math builtin.", field->charsZ());

@@ +2692,5 @@
>          if (field == m.cx()->names().NaN)
>              return m.addGlobalConstant(varName, js_NaN, field);
>          if (field == m.cx()->names().Infinity)
>              return m.addGlobalConstant(varName, js_PositiveInfinity, field);
> +        return m.fail(initNode, "Does not match a standard global constant");

return m.fail(initNode, "'%s' is not a standard global constant.", field->charsZ());

@@ +2731,1 @@
>  

Remove extra newline.

@@ +2803,5 @@
>          if (!CheckArgument(m, argpn, &argName))
>              return false;
>  
>          if (dupSet.has(argName))
> +            return m.fail(argpn, "asm.js arguments must have distinct names");

return m.failf(argpn, "Argument '%s' does not have a distinct name.", argName->charsZ());

@@ +2892,5 @@
>              return false;
>      }
>  
>      if (fn && fn->isKind(PNK_NOP))
> +        return m.fail(fn, "duplicate function names are not allowed");

return m.failf(fn, "Function '%s' does not have a distinct name.", fn->name()->charsZ());

@@ +2929,5 @@
>      unsigned length = ListLength(arrayLiteral);
>  
>      if (!IsPowerOfTwo(length))
> +        return m.failf(arrayLiteral, "Function-pointer table's length must be a power of 2 "
> +                       "(current size is %u)", length);

{} </excessive-pedentry>

@@ +2983,5 @@
>      PropertyName *funcName = returnExpr->name();
>  
>      const ModuleCompiler::Func *func = m.lookupFunction(funcName);
>      if (!func)
> +        return m.fail(returnExpr, "exported function name not found");

return m.failf(returnExpr, "Exported function name '%s' not found.", func->charsZ());

@@ +3007,5 @@
>          PropertyName *funcName = initNode->name();
>  
>          const ModuleCompiler::Func *func = m.lookupFunction(funcName);
>          if (!func)
> +            return m.fail(initNode, "exported function name not found");

return m.fail(initNode, "Exported function '%s' not found.", func->charsZ());

@@ +3145,3 @@
>      } else {
>          if (TypedArrayShift(*viewType) != 0)
> +            return f.fail(indexExpr, "Index expression isn't shifted, so this must be an Int8/Uint8 access");

s/this/it/

@@ +3516,5 @@
>      if (!CheckCallArgs(f, callNode, Use::ToNumber, &args))
>          return false;
>  
>      if (args.length() != arity)
> +        return f.fail(callNode, "Math builtin call passed wrong number of argument");

return f.failf(callNode, "Math builtin call passed %d arguments, expected %d", args.length(), arity);

@@ +4384,5 @@
>      ParseNode *expr = UnaryKid(returnStmt);
>  
>      if (!expr) {
>          if (f.func().returnType().which() != RetType::Void)
> +            return f.fail(returnStmt, "All return statements must return void");

If you can get the name of the function from |f|, it would be nice to have it in this error.

@@ +4463,5 @@
>          return false;
>  
>      ParseNode *initNode = MaybeDefinitionInitializer(var);
>      if (!initNode)
> +        return m.fail(var, "Variable needs explicit type declaration via an initial value");

return m.failf(var, "Variable '%s' needs explicit type declaration via an initial value.", name->charsZ());

@@ +4468,3 @@
>  
>      if (!IsNumericLiteral(initNode))
> +        return m.fail(initNode, "Variable initialization value needs to be a numeric literal");

return m.failf(initNode, "Variable initialization value for local name '%s' needs to be a numeric literal.", name->charsZ());

@@ +4480,5 @@
>        case NumLit::Double:
>          type = VarType::Double;
>          break;
>        case NumLit::OutOfRangeInt:
> +        return m.fail(initNode, "Variable initializer is out of representable integer range");

return m.failf(initNode, "Variable initializer for local name '%s' is out of representable integer range.", name->charsZ());

@@ +4485,5 @@
>      }
>  
>      FunctionCompiler::LocalMap::AddPtr p = locals->lookupForAdd(name);
>      if (p)
> +        return m.fail(initNode, "Local names must be unique");

return m.fail(initNode, "Local name '%s' is not unique.", name->charsZ());
Attachment #744940 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
Thanks for the thoughtful review comments!

> As a review general comment, the error messages are really inconsistent
> about whether they start capitalized and end with a period. I think we
> should standardize on a Capitalized first letter and a period at the end,
> but feel free to go with whatever you think best.

Ah, good catch, you're right.  I think I should go with lowercase, no ., because these error strings are embedded in the bigger error message "asm.js type error: %s:".

> >          return false;
> 
> It may be worth calling fail(pn, '') if !bytes.ptr().

The problem is that we'll try to free() that string literal in the destructor.  Note: these strings are pretty small so we'll only OOM if we're right at the razor's edge in which case we're about dead anyway.

>  With that in mind, I've tried to point out some places where we can use
>  the new capabilities to remove ambiguity, given that the error message has no context.

Good point!

> @@ +2476,5 @@
> >  static bool
> >  CheckArgument(ModuleCompiler &m, ParseNode *arg, PropertyName **name)
> >  {
> >      if (!IsDefinition(arg))
> > +        return m.fail(arg, "overlapping argument names not allowed");
> 
> This should probably end with "in asm.js" as with similar errors.

Recall that this whole string is prefixed with "asm.js type error: ".  This points out to me that I've actually included "in asm.js" in quite a few places where it should be removed.
(In reply to Luke Wagner [:luke] from comment #3)
> (In reply to Terrence Cole [:terrence] from comment #2)
> Thanks for the thoughtful review comments!

You are very welcome! It took longer than I would have liked, but I managed to convince myself that I'm actually saving time: I'll probably see most of these from the user perspective at some point.

> > >          return false;
> > 
> > It may be worth calling fail(pn, '') if !bytes.ptr().
> 
> The problem is that we'll try to free() that string literal in the
> destructor.  Note: these strings are pretty small so we'll only OOM if we're
> right at the razor's edge in which case we're about dead anyway.

|fail| strdups internally; since the |free| is going to fail regardless, the hope is that we can get get past the destructor and make epsilon more progress by duping a zero-length string instead of a small string. As you said, it's extremely unlikely to be helpful in practice.
 
> >  With that in mind, I've tried to point out some places where we can use
> >  the new capabilities to remove ambiguity, given that the error message has no context.
> 
> Good point!

Now that I know where to look, I'll file patches if I run into more cases like this when hacking with asm.js.
https://hg.mozilla.org/mozilla-central/rev/b7c57f0f0427
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 882037
You need to log in before you can comment on or make changes to this bug.