[wasm] Throw real Error on wasm trap

src/runtime/runtime-internal.cc

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/runtime/runtime-utils.h"
 
 #include "src/arguments.h"
 #include "src/ast/prettyprinter.h"
 #include "src/bootstrapper.h"
 #include "src/conversions.h"
@@ skipping 78 matching lines @@
   return isolate->ReThrow(args[0]);
 }
 
 
 RUNTIME_FUNCTION(Runtime_ThrowStackOverflow) {
   SealHandleScope shs(isolate);
   DCHECK_LE(0, args.length());
   return isolate->StackOverflow();
 }
 
+RUNTIME_FUNCTION(Runtime_ThrowWasmError) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 1);

[Michael Starzinger, 2016/04/11 18:13:26]

nit: I know we are inconsistent, but lets use "DCHECK_EQ(1, args.length())" for
new code.

[Clemens Hammacher, 2016/04/12 09:33:44]

Sure, didn't see that.

+  CONVERT_SMI_ARG_CHECKED(message_id, 0);
+  // TODO(clemensh): discuss with titzer whether a WasmError class would make
+  // sense here
+  Handle<Object> exception = isolate->factory()->NewError(
+      static_cast<MessageTemplate::Template>(message_id));
+  return isolate->Throw(*exception);

[Michael Starzinger, 2016/04/11 18:13:26]

nit: This can also be written as ...

THROW_NEW_ERROR_RETURN_FAILURE(isolate,
NewError(static_cast<MessageTemplate::Template>(message_id)));

[Clemens Hammacher, 2016/04/12 09:33:44]

and that's considered nicer? ;)
but OK, I changed it :)

[Michael Starzinger, 2016/04/12 10:47:56]

We can discuss which version is nicer and change the existing code to use
Isolate::Throw directly instead of the macro. But having part of the code use
the macro and other parts of the code use Isolate::Throw is not a "nice"
situation. IMHO keeping this call-site in sync with existing code trumps having
this one call-site be "nice".

Also I realize that I am contradicting myself here with the comment above where
I say DCHECK_EQ should be used "at this one call-site". But that DCHECK_EQ above
is one incremental cleanup that some of is collectively agreed to, to eventually
get into a state where the "readability/check" linter check passes. We did not
(yet) agree to cleanup Isolate::Throw in favor of the
THROW_NEW_ERROR_RETURN_FAILURE macro.

Does that make sense or do you think I am completely off here?

+}
 
 RUNTIME_FUNCTION(Runtime_UnwindAndFindExceptionHandler) {
   SealHandleScope shs(isolate);
   DCHECK(args.length() == 0);
   return isolate->UnwindAndFindHandler();
 }
 
 
 RUNTIME_FUNCTION(Runtime_PromoteScheduledException) {
   SealHandleScope shs(isolate);
@@ skipping 375 matching lines @@
   std::stringstream stats_stream;
   isolate->counters()->runtime_call_stats()->Print(stats_stream);
   Handle<String> result =
       isolate->factory()->NewStringFromAsciiChecked(stats_stream.str().c_str());
   isolate->counters()->runtime_call_stats()->Reset();
   return *result;
 }
 
 }  // namespace internal
 }  // namespace v8