Reland Implement .eh_frame writer and disassembler.

src/eh-frame.cc

Index: src/eh-frame.cc
diff --git a/src/eh-frame.cc b/src/eh-frame.cc
index af85e0b8d5f7143beb3c5243513a69fef0f5f919..d29e8d44720295e0abb383a13b8af728a4b19937 100644
--- a/src/eh-frame.cc
+++ b/src/eh-frame.cc
@@ -3,18 +3,373 @@
 // found in the LICENSE file.
 
 #include "src/eh-frame.h"
+#include <iomanip>
+#include <limits>
+#include <ostream>

[rmcilroy, 2016/06/28 10:37:33]

Newline before <iomanip> and after <ostream> (a new block for each header type -
own header, c++ libraries, other headers).

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

 #include "src/objects-inl.h"
 #include "src/objects.h"
 
+#if V8_TARGET_ARCH_X64
+#include "src/x64/eh-frame-x64.h"
+#elif V8_TARGET_ARCH_ARM
+#include "src/arm/eh-frame-arm.h"
+#elif V8_TARGET_ARCH_ARM64
+#include "src/arm64/eh-frame-arm64.h"
+#else
+
+// Dummy placeholders
+
 namespace v8 {
 namespace internal {
 
-static const int DW_EH_PE_pcrel = 0x10;
-static const int DW_EH_PE_datarel = 0x30;
-static const int DW_EH_PE_udata4 = 0x03;
-static const int DW_EH_PE_sdata4 = 0x0b;
+static const int kInitialCFARegister = 0;
+static const int kInitialCFAOffset = 0;
+static const int kDataAlignmentFactor = 1;
+static const int kInitialStateOffsetInCIE = 0;
+
+static const byte kCIE[8] = {0};
+
+const char* GetRegisterName(int register_number) {
+  UNIMPLEMENTED();
+  return nullptr;
+}
+
+}  // namespace internal
+}  // namespace v8
+
+#endif
+
+namespace v8 {
+namespace internal {
+
+static_assert(sizeof(kCIE) % kPointerSize == 0,
+              "CIE must be aligned to pointer size");
+
+static const char DW_CFA_nop = 0x00;
+
+static const byte DW_CFA_advance_loc1 = 0x02;
+static const byte DW_CFA_advance_loc2 = 0x03;
+static const byte DW_CFA_advance_loc4 = 0x04;
+static const byte DW_CFA_same_value = 0x08;
+static const byte DW_CFA_def_cfa = 0x0c;
+static const byte DW_CFA_def_cfa_register = 0x0d;
+static const byte DW_CFA_def_cfa_offset = 0x0e;

[rmcilroy, 2016/06/28 10:37:33]

Move all these constants to be private constants in the class. They should also
use chromium naming (e.g., kCfaAdvanceLoc1)

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+
+static const byte DW_EH_PE_pcrel = 0x10;
+static const byte DW_EH_PE_datarel = 0x30;
+static const byte DW_EH_PE_udata4 = 0x03;
+static const byte DW_EH_PE_sdata4 = 0x0b;

[rmcilroy, 2016/06/28 10:37:32]

These are all unused - either move to the arch specific headers and use them
there, or remove

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Actually, these are used in the EhFrameHdr ctor below. I moved them as private
constants inside that class.

+
+static const byte kEhFrameTerminator[] = {0x00, 0x00, 0x00, 0x00};
+
+static const byte kFDEPadding[] = {DW_CFA_nop, DW_CFA_nop, DW_CFA_nop,

[rmcilroy, 2016/06/28 10:37:32]

Move into finish function where it's used.

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+                                   DW_CFA_nop, DW_CFA_nop, DW_CFA_nop,
+                                   DW_CFA_nop, DW_CFA_nop};
+
+const char* GetRegisterName(int register_number);
+
+const int EhFrameWriter::kCIESize = sizeof(kCIE);

[rmcilroy, 2016/06/28 10:37:33]

I think you need STATIC_CONST_MEMBER_DEFINITION for this.

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+
+EhFrameWriter::EhFrameWriter()
+    : last_pc_offset_(0),
+      cfa_register_(kInitialCFARegister),
+      cfa_offset_(kInitialCFAOffset) {
+  WriteBytes(&kCIE[0], sizeof(kCIE));
+  WriteFDEHeader();
+}
+
+static const int kFDEHeaderSize = 4 * kInt32Size + 1;

[rmcilroy, 2016/06/28 10:37:32]

Move this with the rest of the constants.

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+
+void EhFrameWriter::WriteFDEHeader() {
+  WriteInt32(0);                          // Placeholder for size of the FDE
+  WriteInt32(sizeof(kCIE) + kInt32Size);  // Backwards offset to the CIE
+  WriteInt32(0);  // Placeholder for pointer to procedure
+  WriteInt32(0);  // Placeholder for size of the procedure
+  WriteByte(0);   // No augmentation data
+}
+
+void EhFrameWriter::AdvanceLocation(int pc_offset) {
+  DCHECK_GE(pc_offset, last_pc_offset_);
+  // No way to handle 64bit values, but that's an unrealistic delta anyway.

[rmcilroy, 2016/06/28 10:37:32]

No need for this comment, the pc_offset is already int so can never be 64 bits

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+  uint32_t delta = pc_offset - last_pc_offset_;
+
+  if (delta <= 0x3f) {
+    WriteByte((1 << 6) | (delta & 0x3f));  // DW_CFA_advance_loc

[rmcilroy, 2016/06/28 10:37:33]

use a constant instead of 0x3f

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+  } else if (delta <= std::numeric_limits<uint8_t>::max()) {

[rmcilroy, 2016/06/28 10:37:32]

kMaxUInt8

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+    WriteByte(DW_CFA_advance_loc1);
+    WriteByte(delta);
+  } else if (delta <= std::numeric_limits<uint16_t>::max()) {

[rmcilroy, 2016/06/28 10:37:32]

kMaxUInt16

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+    WriteByte(DW_CFA_advance_loc2);
+    WriteInt16(delta);
+  } else {
+    WriteByte(DW_CFA_advance_loc4);
+    WriteInt32(delta);
+  }
+
+  last_pc_offset_ = pc_offset;
+}
+
+void EhFrameWriter::DefineCFAOffset(int cfa_offset) {
+  DCHECK_GE(cfa_offset, 0);
+  WriteByte(DW_CFA_def_cfa_offset);
+  WriteULEB128(cfa_offset);
+  cfa_offset_ = cfa_offset;
+}
+
+void EhFrameWriter::DefineCFARegister(int code) {
+  WriteByte(DW_CFA_def_cfa_register);
+  WriteULEB128(code);
+  cfa_register_ = code;
+}
+
+void EhFrameWriter::DefineCFARegisterOffset(int code, int cfa_offset) {
+  WriteByte(DW_CFA_def_cfa);
+  WriteULEB128(code);
+  WriteULEB128(cfa_offset);
+  cfa_offset_ = cfa_offset;
+  cfa_register_ = code;
+}
+
+// Following the DWARF terminology, this method should be called Offset(),
+// but that name is way too generic.

[rmcilroy, 2016/06/28 10:37:33]

Don't use the DWARF terminology - could you rename the interface of other
functions in this writer to be more understandable to V8 specifics rather than
trying to follow the DWARF naming - this class should abstrac DWARFness from the
rest of V8 as much as possible. Remove this comment too

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Comment removed. Please let me know if you think the name of the methods is
still obscure.

+void EhFrameWriter::SaveRegisterToStack(int code, int offset) {
+  DCHECK_GE(code, 0);
+  DCHECK_LE(code, 0x3f);
+  DCHECK_EQ(offset % kDataAlignmentFactor, 0);
+
+  WriteByte((2 << 6) | (code & 0x3f));  // DW_CFA_offset

[rmcilroy, 2016/06/28 10:37:33]

constant

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+  WriteULEB128(offset / std::abs(kDataAlignmentFactor));
+}
+
+// This method should be SameValue(), but again that name is too generic.

[rmcilroy, 2016/06/28 10:37:32]

ditto

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+void EhFrameWriter::RegisterHasInitialValue(int code) {
+  DCHECK_GE(code, 0);
+  WriteByte(DW_CFA_same_value);
+  WriteULEB128(code);
+}
+
+void EhFrameWriter::Finish(int code_size) {
+  DCHECK_GE(unwinding_info_.length(), kCIESize);
+
+  // Add padding
+  int unpadded_fde_size = unwinding_info_.length() - kCIESize;
+  int padded_fde_size = RoundUp(unpadded_fde_size, 8);
+  int fde_padding_size = padded_fde_size - unpadded_fde_size;
+
+  DCHECK_LT(fde_padding_size, static_cast<int>(sizeof(kFDEPadding)));
+  WriteBytes(&kFDEPadding[0], fde_padding_size);
+
+  // Write the size of the FDE now that we know it.
+  int fde_offset = kCIESize;

[rmcilroy, 2016/06/28 10:37:33]

Please have these as static consts, and DCHECK that the writer is at the correct
location when writing the placeholder, and DCHECK here (or in PatchInt32) that
the value is still the placeholder.

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+  PatchInt32(fde_offset, padded_fde_size);
+
+  // Write the size and offset to procedure.
+  int function_boundaries_offset = fde_offset + 2 * kInt32Size;
+  PatchInt32(function_boundaries_offset,
+             -(RoundUp(code_size, 8) + function_boundaries_offset));
+
+  int code_size_offset = function_boundaries_offset + kInt32Size;
+  PatchInt32(code_size_offset, code_size);
+
+  // Terminate the .eh_frame.
+  WriteBytes(&kEhFrameTerminator[0], sizeof(kEhFrameTerminator));
+
+  // Add .eh_frame_hdr placeholder.
+  unwinding_info_.AddBlock(0x00, EhFrameHdr::kRecordSize);

[rmcilroy, 2016/06/28 10:37:33]

Could you not just add the actual EhFrameHdr block here instead?

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Yes. This change resulted in a bit of refactoring in EhFrameHdr and related
tests.

+}
+
+// Remember to call Finish() before GetUnwindingInfo()!

[rmcilroy, 2016/06/28 10:37:33]

This comment here is useless since it's in the cc file not the header (so anyone
calling GetEhFrame won't necessarily see it). Add the comment in the header and
add a DCHECK here that finish was called.

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done. The only way to know if Finish() was called is to keep a boolean flag, so
now we have a field for this purpose when DEBUG is defined. 

+void EhFrameWriter::GetEhFrame(CodeDesc* desc) {
+  desc->unwinding_info_size = unwinding_info_.length();
+  desc->unwinding_info = unwinding_info_.begin();
+}
+
+void EhFrameWriter::WriteULEB128(uint32_t value) {
+  do {
+    byte chunk = value & 0x7f;
+    value >>= 7;
+    if (value != 0) chunk |= 0x80;
+    unwinding_info_.Add(chunk);
+  } while (value != 0);
+}
+
+static uint32_t DecodeULEB128(const byte* encoded, int* encoded_size) {
+  const byte* cur = encoded;
+  uint32_t decoded_value = 0;
+
+  do {
+    decoded_value <<= 7;
+    decoded_value += static_cast<uint32_t>(static_cast<unsigned>(*cur & 0x7f));
+  } while (*cur++ >= 0x80);
+
+  *encoded_size = static_cast<int>(cur - encoded);
+  return decoded_value;
+}
+

[rmcilroy, 2016/06/28 10:37:32]

#ifdef ENABLE_DISASSEMBLER

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+namespace {
+
+class StreamModifiersScope final {
+ public:
+  explicit StreamModifiersScope(std::ostream* stream)
+      : stream_(stream), flags_(stream->flags()) {
+    DCHECK_NOT_NULL(stream);

[rmcilroy, 2016/06/28 10:37:33]

No need for this, it will already have exploded in stream->flags() by this point
if it was NULL

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Fair enough. Done.

+  }
+  ~StreamModifiersScope() { stream_->flags(flags_); }
 
-const int EhFrameHdr::kCIESize = 0;
+ private:
+  std::ostream* stream_;
+  std::ios::fmtflags flags_;
+};
+
+}  // namespace
+
+static void DumpDWARFDirectives(std::ostream& stream,  // NOLINT

[rmcilroy, 2016/06/28 10:37:32]

Put this in the annonymous name space instead of static

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+                                const byte* begin, const byte* end) {
+  StreamModifiersScope modifiers_scope(&stream);
+
+  const byte* cur = begin;
+  uint32_t offset_in_procedure = 0;
+
+  while (cur != end) {
+    stream << reinterpret_cast<const void*>(cur) << "  ";
+
+    if (((*cur >> 6) & 0xff) == 1) {

[rmcilroy, 2016/06/28 10:37:32]

Use constants

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+      int value = *cur & 0x3f;
+      cur += sizeof(byte);
+      offset_in_procedure += value;
+      stream << "| pc_offset=" << std::dec << offset_in_procedure
+             << " (delta=0x" << std::hex << value << ")\n";
+      continue;
+    }
+
+    if (((*cur >> 6) & 0xff) == 2) {

[rmcilroy, 2016/06/28 10:37:33]

ditto

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+      stream << "| " << GetRegisterName(*cur & 0x3f);
+      cur += sizeof(byte);
+      int decoded_size = 0;
+      int decoded_offset = static_cast<int>(DecodeULEB128(cur, &decoded_size));
+      cur += decoded_size;
+      stream << " saved at cfa" << std::showpos << std::dec
+             << decoded_offset * kDataAlignmentFactor << '\n';
+      continue;
+    }
+
+    switch (*cur) {
+      case DW_CFA_advance_loc1: {
+        cur += sizeof(byte);

[rmcilroy, 2016/06/28 10:37:32]

You always cur += sizeof(byte) - just do it once above

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+        unsigned value = *reinterpret_cast<const uint8_t*>(cur);
+        cur += sizeof(uint8_t);
+        offset_in_procedure += value;
+        stream << "| pc_offset=" << std::dec << offset_in_procedure
+               << " (delta=0x" << std::hex << value << ")\n";
+        break;
+      }
+      case DW_CFA_advance_loc2: {
+        cur += sizeof(byte);
+        uint16_t value = *reinterpret_cast<const uint16_t*>(cur);

[rmcilroy, 2016/06/28 10:37:33]

This won't work on machines that don't support unaligned access. Use
ReadUnalignedUInt16 / ReadUnalignedUInt32 etc

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+        cur += sizeof(uint16_t);
+        offset_in_procedure += value;
+        stream << "| pc_offset=" << std::dec << offset_in_procedure
+               << " (delta=0x" << std::hex << value << ")\n";
+        break;
+      }
+      case DW_CFA_advance_loc4: {
+        cur += sizeof(byte);
+        uint32_t value = *reinterpret_cast<const uint32_t*>(cur);
+        offset_in_procedure += value;
+        cur += sizeof(uint32_t);
+        stream << "| pc_offset=" << std::dec << offset_in_procedure
+               << " (delta=0x" << std::hex << value << ")\n";
+        break;
+      }
+      case DW_CFA_def_cfa: {
+        cur += sizeof(byte);
+        int decoded_size = 0;
+        int cfa_register = DecodeULEB128(cur, &decoded_size);
+        cur += decoded_size;
+        int cfa_initial_offset = DecodeULEB128(cur, &decoded_size);
+        cur += decoded_size;
+        stream << "| cfa_register=" << GetRegisterName(cfa_register)
+               << ", cfa_offset=0x" << std::hex << cfa_initial_offset << '\n';
+        break;
+      }
+      case DW_CFA_def_cfa_offset: {
+        cur += sizeof(byte);
+        int decoded_size = 0;
+        stream << "| cfa_offset=0x" << std::hex
+               << DecodeULEB128(cur, &decoded_size) << '\n';
+        cur += decoded_size;
+        break;
+      }
+      case DW_CFA_def_cfa_register: {
+        cur += sizeof(byte);
+        int decoded_size = 0;
+        stream << "| cfa_register="
+               << GetRegisterName(DecodeULEB128(cur, &decoded_size)) << '\n';
+        cur += decoded_size;
+        break;
+      }
+      case DW_CFA_same_value: {
+        cur += sizeof(byte);
+        int decoded_size = 0;
+        stream << "| " << GetRegisterName(DecodeULEB128(cur, &decoded_size))
+               << " to initial value\n";
+        cur += decoded_size;
+        break;
+      }
+      case DW_CFA_nop:
+        cur += sizeof(byte);
+        stream << "| nop\n";
+        break;
+      default:
+        stream << "| UNKNOWN BYTECODE 0x" << std::hex << std::setw(2)
+               << std::setfill('0') << static_cast<unsigned>(*cur) << '\n';

[rmcilroy, 2016/06/28 10:37:32]

Just the UNREACHABLE here, no need to print the unknown bytecode

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+        UNREACHABLE();
+        return;
+    }
+  }
+}
+
+void EhFrameWriter::DisassembleToStream(std::ostream& stream,  // NOLINT
+                                        const byte* start, const byte* end) {
+  const byte* cie_directives_start = start + kInitialStateOffsetInCIE;
+  const byte* cie_directives_end = start + sizeof(kCIE);
+  DCHECK_LE(cie_directives_start, cie_directives_end);
+
+  const byte* fde_start = start + sizeof(kCIE);
+
+  stream << reinterpret_cast<const void*>(start) << "  .eh_frame: CIE\n";
+  DumpDWARFDirectives(stream, cie_directives_start, cie_directives_end);
+
+  const byte* procedure_offset_address = fde_start + 2 * kInt32Size;

[rmcilroy, 2016/06/28 10:37:32]

Constant offsets

[Stefano Sanfilippo, 2016/06/29 15:16:20]

Done.

+  int32_t procedure_offset =
+      *reinterpret_cast<const int32_t*>(procedure_offset_address);
+
+  const byte* procedure_size_address = fde_start + 3 * kInt32Size;

[rmcilroy, 2016/06/28 10:37:32]

ditto

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

+  uint32_t procedure_size =
+      *reinterpret_cast<const uint32_t*>(procedure_size_address);
+
+  stream << reinterpret_cast<const void*>(fde_start) << "  .eh_frame: FDE\n"
+         << reinterpret_cast<const void*>(procedure_offset_address)
+         << "  | procedure offset=" << procedure_offset << '\n'
+         << reinterpret_cast<const void*>(procedure_size_address)
+         << "  | procedure size=" << procedure_size << '\n';
+
+  const byte* fde_directives_start = fde_start + kFDEHeaderSize;
+  const byte* fde_directives_end =
+      end - EhFrameHdr::kRecordSize - sizeof(kEhFrameTerminator);
+  DCHECK_LE(fde_directives_start, fde_directives_end);
+
+  DumpDWARFDirectives(stream, fde_directives_start, fde_directives_end);
+
+  const byte* fde_terminator_start = fde_directives_end;
+  stream << reinterpret_cast<const void*>(fde_terminator_start)
+         << "  .eh_frame: terminator\n";
+
+  const byte* eh_frame_hdr_start =
+      fde_terminator_start + sizeof(kEhFrameTerminator);
+  stream << reinterpret_cast<const void*>(eh_frame_hdr_start)
+         << "  .eh_frame_hdr: placeholder\n";

[rmcilroy, 2016/06/28 10:37:32]

Why bother printing the terminator and placeholder?

[Stefano Sanfilippo, 2016/06/29 15:16:20]

I believe it might be useful to check the exact layout of the .eh_frame in the
disassembly.

+}
 
 static const int kVersionSize = 1;
 static const int kEncodingSpecifiersSize = 3;

[rmcilroy, 2016/06/28 10:37:32]

Move with other constants

[Stefano Sanfilippo, 2016/06/29 15:16:21]

Done.

@@ -82,7 +437,7 @@ EhFrameHdr::EhFrameHdr(Code* code) {
         -(eh_frame_size + kVersionSize + kEncodingSpecifiersSize);  // A -> D
     lut_entries_number_ = 1;
     offset_to_procedure_ = -(RoundUp(code_size, 8) + eh_frame_size);  // B -> F
-    offset_to_fde_ = -(eh_frame_size - kCIESize);                     // B -> C
+    offset_to_fde_ = -(eh_frame_size - EhFrameWriter::kCIESize);      // B -> C
   } else {
     // Create a dummy table
     offset_to_eh_frame_ = 0;