[Interpreter] Initial generate-bytecode-expectations implementation.

test/cctest/bytecodechecker.cc

+// Copyright 2016 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 <iostream>
+#include "include/libplatform/libplatform.h"
+#include "src/base/logging.h"
+#include "src/compiler.h"
+#include "src/interpreter/bytecode-array-iterator.h"
+#include "src/interpreter/bytecode-generator.h"
+#include "src/interpreter/bytecodes.h"
+#include "src/interpreter/interpreter.h"
+#include "src/v8.h"

[oth, 2016/02/05 11:41:04]

Is there something specific from src/v8.h? Samples use include/v8.h and put it
above libplatform. 

Suggest breaking up sections here based on location, e.g. see style guide and
existing samples.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

No there's nothing specific ATM, I was just following the trend in the other
source files, where src/v8.h is used instead of include/... Anyway, I replaced
that line with include/v8.h
Done.

+
+using namespace i::interpreter;
+
+namespace {
+
+class ArrayBufferAllocator final : public v8::ArrayBuffer::Allocator {
+ public:
+  void *Allocate(size_t length) override {

[oth, 2016/02/05 11:41:05]

The predominant style in v8 is
"void*" rather than "void *" and ditto reference arguments throughout this file.
The Google C++ coding style permits either though.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.

[rmcilroy, 2016/02/08 13:55:31]

Doesn't look like this has been done.

[Stefano Sanfilippo, 2016/02/09 10:09:58]

It was, but cl format reverted to this style before upload (and I didn't
notice). Since both forms are acceptable according to the style guide, we might
just leave it as it is. If you prefer, I can do a cl upload without cl format.  

[rmcilroy, 2016/02/09 11:58:24]

Hmm that's weird (git cl format doesn't do that for me). Did you replace all
instances (including the char* kIndent at the top of the file)? Maybe git cl
format takes the first instance it sees and makes the rest of the file
consistent. I'd still prefer this to be the same as the rest of the V8 style,
but if you can't convince git cl format to play nice then I guess I'll live with
it.§

[Stefano Sanfilippo, 2016/02/09 16:52:50]

I finally did it on the workstation. Maybe it's a Mac issue?

[rmcilroy, 2016/02/10 09:18:00]

Sounds like it - weird. thanks!

+    void *data = AllocateUninitialized(length);
+    if (data != nullptr) memset(data, 0, length);
+    return data;
+  }
+  void *AllocateUninitialized(size_t length) override { return malloc(length); }
+  void Free(void *data, size_t) override { free(data); }
+};
+
+class PlatformWrapper final {
+ public:
+  PlatformWrapper(const char *program_name, const char *wrapper_function_name);
+  ~PlatformWrapper();
+
+  PlatformWrapper(PlatformWrapper &) = delete;

[oth, 2016/02/05 11:41:04]

V8 code generally packages this up with (doesn't deal with the move constructor
for now, but non-issue at present):

DISALLOW_COPY_AND_ASSIGN()

The code might also want to:

DISALLOW_IMPLICIT_CONSTRUCTORS()

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.
I am not too sure this has any use, since we have no ctor which could be
implicit. Should I add it anyway?

+  PlatformWrapper(PlatformWrapper &&) = delete;
+
+  v8::Platform *platform() const { return platform_; }
+  v8::Isolate *isolate() const { return isolate_; }
+  i::Isolate *i_isolate() const {
+    return reinterpret_cast<i::Isolate *>(isolate_);
+  }
+
+ private:
+  v8::Platform *platform_;
+  v8::Isolate *isolate_;

[rmcilroy, 2016/02/05 13:49:51]

use base::SmartPointer<...> for both of these objects (and remove the delete
calls in the destructor).

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done for platform_. base::SmartPointer doesn't seem to handle the Dispose()
method Isolate uses for destruction, so I didn't use a smart pointer for that.

+};
+
+enum ConstantPoolType {
+  kConstantPoolTypeUnknown = 0,
+  kConstantPoolTypeString,
+  kConstantPoolTypeInteger,
+  kConstantPoolTypeFloat,
+};

[rmcilroy, 2016/02/05 13:49:51]

Not used yet - remove for now.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.

+
+PlatformWrapper::PlatformWrapper(const char *program_name,

[rmcilroy, 2016/02/05 13:49:51]

/s/program_name/exec_path

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.

+                                 const char *wrapper_function_name) {

[rmcilroy, 2016/02/05 13:49:51]

no need to pass this to the PlatformWrapper, just move the setting of
i::FLAG_ignition_filter before you compile and run the function itself. In fact,
given that we pretty much support all code now, just removing the
ignition_filter flag entirely would probably be fine.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Ok, for the moment I replaced the filter with a "*" catchall, I'll just drop the
line when we remove the flag.

+  v8::V8::InitializeICU();
+  v8::V8::InitializeExternalStartupData(program_name);
+  platform_ = v8::platform::CreateDefaultPlatform();
+  v8::V8::InitializePlatform(platform_);
+  v8::V8::Initialize();
+
+  ArrayBufferAllocator allocator;
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator = &allocator;
+
+  isolate_ = v8::Isolate::New(create_params);
+  i::FLAG_ignition = true;
+  i::FLAG_ignition_filter = i::StrDup(wrapper_function_name);
+  i::FLAG_always_opt = false;
+  i::FLAG_allow_natives_syntax = true;
+  i::FLAG_legacy_const = true;

[rmcilroy, 2016/02/05 13:49:51]

just move these to the top of the function, then there shouldn't be any need to
call interpreter()->Initialize() (it should happen during V8 initialization if 

Also, remove the i::FLAG_legacy_const = true; (Mythri has removed this one from
test-bytecode-generator in https://codereview.chromium.org/1634153002/).

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.
Actually, not calling Initialize() on the interpreter results in a segfault the
first time the Isolate is used. I don't know whether this is the intended
behaviour or a bug.
Done.

+  i_isolate()->interpreter()->Initialize();
+}
+
+PlatformWrapper::~PlatformWrapper() {
+  isolate_->Dispose();
+  v8::V8::Dispose();
+  v8::V8::ShutdownPlatform();
+  delete platform_;
+  platform_ = nullptr;
+}
+
+v8::Local<v8::String> MakeV8LocalStringFromUTF8(const PlatformWrapper &platform,
+                                                const char *data) {
+  return v8::String::NewFromUtf8(platform.isolate(), data,
+                                 v8::NewStringType::kNormal)
+      .ToLocalChecked();
+}
+
+static std::string WrapCodeInFunction(const char *function_name,
+                                      const std::string &body) {
+  std::ostringstream program_stream;
+  program_stream << "function " << function_name << "() {" << body << "}\n"
+                 << function_name << "();";
+
+  return program_stream.str();
+}
+
+v8::Local<v8::Value> CompileAndRun(const PlatformWrapper &platform,
+                                   const v8::Local<v8::Context> &context,
+                                   const char *program) {
+  v8::Local<v8::String> source = MakeV8LocalStringFromUTF8(platform, program);
+  v8::Local<v8::Script> script =
+      v8::Script::Compile(context, source).ToLocalChecked();
+
+  v8::Local<v8::Value> result;
+  bool success = script->Run(context).ToLocal(&result);
+  CHECK(success);
+
+  return result;
+}
+
+i::Handle<v8::internal::BytecodeArray> GetBytecodeArrayForGlobal(
+    const PlatformWrapper &platform, const v8::Local<v8::Context> &context,
+    const char *global_name) {
+  v8::Local<v8::String> v8_global_name =
+      MakeV8LocalStringFromUTF8(platform, global_name);
+  v8::Local<v8::Function> function = v8::Local<v8::Function>::Cast(
+      context->Global()->Get(context, v8_global_name).ToLocalChecked());
+  i::Handle<i::JSFunction> js_function =
+      i::Handle<i::JSFunction>::cast(v8::Utils::OpenHandle(*function));
+
+  i::Handle<i::BytecodeArray> bytecodes =
+      i::handle(js_function->shared()->bytecode_array(), platform.i_isolate());
+
+  return bytecodes;
+}
+
+void PrintBytecodeOperand(std::ostream &stream,
+                          const BytecodeArrayIterator &bytecode_iter,
+                          const Bytecode &bytecode, int op_index) {
+  OperandType op_type = Bytecodes::GetOperandType(bytecode, op_index);
+
+  switch (op_type) {
+    case OperandType::kMaybeReg8:
+    case OperandType::kReg8:
+    case OperandType::kRegPair8:
+    case OperandType::kRegOut8:
+    case OperandType::kRegOutTriple8:
+    case OperandType::kRegOutPair8: {
+      Register op_value_register = bytecode_iter.GetRegisterOperand(op_index);
+      stream << "R(" << op_value_register.index() << ')';
+      break;
+    }
+    case OperandType::kMaybeReg16:
+    case OperandType::kReg16:
+    case OperandType::kRegPair16:
+    case OperandType::kRegOut16:
+    case OperandType::kRegOutTriple16:
+    case OperandType::kRegOutPair16: {
+      Register op_value_register = bytecode_iter.GetRegisterOperand(op_index);
+      stream << "R16(" << op_value_register.index() << ')';
+      break;
+    }
+    case OperandType::kImm8: {
+      int op_value_immediate = bytecode_iter.GetImmediateOperand(op_index);
+      stream << "U8(" << op_value_immediate << ')';
+      break;
+    }
+    case OperandType::kIdx8: {
+      int op_value_index = bytecode_iter.GetIndexOperand(op_index);
+      stream << "U8(" << op_value_index << ')';
+      break;
+    }
+    case OperandType::kRegCount8: {
+      int op_value_regcount = bytecode_iter.GetCountOperand(op_index);
+      stream << "U8(" << op_value_regcount << ')';
+      break;
+    }
+    case OperandType::kIdx16: {
+      int op_value_index = bytecode_iter.GetIndexOperand(op_index);
+      stream << "U16(" << op_value_index << ')';
+      break;
+    }
+    case OperandType::kRegCount16: {
+      int op_value_regcount = bytecode_iter.GetCountOperand(op_index);
+      stream << "U8(" << op_value_regcount << ')';

[rmcilroy, 2016/02/05 13:49:51]

This should be U16

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Silly copy and paste mistake is silly. Fixed.

+      break;
+    }

[rmcilroy, 2016/02/05 13:49:51]

Personally I would prefer that this is written using the helpers in the bytecode
array iterator. I think you could write it as follows:

OperandType op_type = Bytecodes::GetOperandType(bytecode, op_index);
OperandSize op_size = Bytecodes::GetOperandSize(bytecode, op_index);

const char* size;
switch (op_size):
  case OperandSize::kNone:
    UNREACHABLE();
    break;
  case OperandSize::kByte:
    size = "8";
    break;
  case OperandSize::kShort:
    size = "16";
    break;
}

if (IsRegisterOperandType(op_type) {
  Register register = bytecode_iter.GetRegisterOperand(op_index);
  stream << "R" << size << "(" << register.index() << ")";
} else {
  uint32_t raw_value = bytecode_iter.GetRawOperand(op_index, op_type);
  stream << "U" << size << "(" << raw_value << ")";
}

WDYT?

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Seems OK to me, as long as GetRawOperand() is guaranteed to return a
ready-to-print representation of the operand and no further processing is
performed by the Get* methods on BytecodeArrayIterator.

[rmcilroy, 2016/02/08 13:55:31]

Yup, I think this should always be safe.

+    default:
+      UNREACHABLE();
+      return;
+  }
+}
+
+void PrintBytecode(std::ostream &stream,
+                   const BytecodeArrayIterator &bytecode_iter) {
+  Bytecode bytecode = bytecode_iter.current_bytecode();
+
+  stream << "B(" << Bytecodes::ToString(bytecode) << ')';

[rmcilroy, 2016/02/05 13:49:51]

You will need a comma after the B(..) or the final operand. It's OK to have
trailing commas in this sequence so just always emit the comma after B(..) and
after every PrintBytecodeOperand should be fine.

+
+  int operands_count = Bytecodes::NumberOfOperands(bytecode);
+  for (int op_index = 0; op_index < operands_count; ++op_index) {
+    stream << ", ";
+    PrintBytecodeOperand(stream, bytecode_iter, bytecode, op_index);
+  }
+}
+
+void PrintBytecodeArray(std::ostream &stream,

[oth, 2016/02/05 11:41:04]

Something for you and Ross, but consider emitting a comment banner that says how
this file is generated and have the diff portion ignore those lines.

[rmcilroy, 2016/02/05 13:49:51]

+1

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done, I have added a boolean flag to print a banner over the generated snippet.
Let me know if the message needs to be amended.

+                        i::Handle<i::BytecodeArray> bytecode_array) {
+  const int kPointerSize = sizeof(void *);
+
+  int frame_size = bytecode_array->frame_size();
+  if (frame_size > kPointerSize) {
+    stream << ' ' << frame_size / kPointerSize << " * kPointerSize,\n";

[rmcilroy, 2016/02/05 13:49:51]

Could you make these indented by the same amount as the test file (7 spaces I
think).

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done. For now the indent amount is hardcoded because it was the easiest option.
Let me know if it needs to be configurable.

+  } else if (frame_size == kPointerSize) {
+    stream << " kPointerSize,\n";
+  } else {
+    stream << " 0,\n";
+  }
+
+  stream << ' ' << bytecode_array->parameter_count() << ",\n"
+         << ' ' << bytecode_array->length() << ",\n {\n     ";
+
+  BytecodeArrayIterator bytecode_iter{bytecode_array};
+
+  bool emitting_first_instruction = true;
+  for (; !bytecode_iter.done(); bytecode_iter.Advance()) {
+    // Print separator before each instruction, except the first one
+    if (!emitting_first_instruction) {

[oth, 2016/02/05 11:41:04]

The code can be simplified here using BytecodeIterator::current_offset() rather
than having to toggle a flag.

[rmcilroy, 2016/02/05 13:49:51]

Also, again trailing commas are OK I think, so you could simplify this by just
emitting the commas in PrintBytecode.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

I ended up following oth suggestion, which simplifies the code, but still avoids
the trailing comma. I know it's OK, but this way is slightly nicer and it
doesn't add a lot of complications (just one extra line).

WDYT?

[rmcilroy, 2016/02/08 13:55:31]

SGTM, thanks.

+      stream << ",\n     ";
+    } else {
+      emitting_first_instruction = false;
+    }
+
+    PrintBytecode(stream, bytecode_iter);
+  }
+
+  stream << "\n },\n";
+}
+
+}  // namespace
+
+int main(int argc, char **argv) {
+  if (argc < 2) {
+    std::cerr << "Usage: " << argv[0] << " \"return null;\"\n";

[oth, 2016/02/05 11:41:05]

This usage message is cryptic.

Going forward, it might be better to take a js file name as an argument and/or
read from stdin.

[rmcilroy, 2016/02/05 13:49:51]

+1, I would have it read from stdin in this CL since future CLs will be reading
this from a file anyways. 

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done. 0 arguments or "-" means stdin, anything else and we try to open the
filename.
Also, clarified the help message (now available with --help as well).

+    return 1;
+  }
+
+  const char *body = argv[1];
+  const char *wrapper_function_name = "__bytecodechecker_wrapper__";
+
+  PlatformWrapper platform(argv[0], wrapper_function_name);

[rmcilroy, 2016/02/05 13:49:51]

I'd rename PlatformWrapper as V8InitializationScope or similar.

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.

+  {
+    v8::Isolate::Scope isolate_scope(platform.isolate());
+    v8::HandleScope handle_scope(platform.isolate());
+    v8::Local<v8::Context> context = v8::Context::New(platform.isolate());
+    v8::Context::Scope context_scope(context);
+
+    std::string source_code = WrapCodeInFunction(wrapper_function_name, body);
+    CompileAndRun(platform, context, source_code.c_str());
+
+    i::Handle<i::BytecodeArray> bytecode_array =
+        GetBytecodeArrayForGlobal(platform, context, wrapper_function_name);
+
+    std::cout << "{" << '"' << body << "\",\n";

[rmcilroy, 2016/02/05 13:49:51]

nit - move this into PrintBytecodeArray (and the close brackets below)

[Stefano Sanfilippo, 2016/02/08 13:18:34]

Done.

+
+    PrintBytecodeArray(std::cout, bytecode_array);
+
+    std::cout << "}\n";
+  }
+}