Reland Implement .eh_frame writer and disassembler.

Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/a91dc7cde2fe1f8b9945dc5eba5b9032b98511aa
Cr-Commit-Position: refs/heads/master@{#37754}

[Stefano Sanfilippo, 2016-05-27 19:38:10]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 19:38:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2023503002/20001

[commit-bot: I haz the power, 2016-05-27 19:39:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 19:39:32]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/6432)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/6414)
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/2328)

[Stefano Sanfilippo, 2016-05-31 09:49:02]

Patchset #4 (id:60001) has been deleted

[Stefano Sanfilippo, 2016-05-31 10:46:05]

Patchset #4 (id:80001) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:50:05]

Patchset #2 (id:20001) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:50:13]

Patchset #2 (id:40001) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:50:19]

Patchset #2 (id:100001) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:50:26]

Patchset #2 (id:120001) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:52:05]

Patchset #1 (id:1) has been deleted

[Stefano Sanfilippo, 2016-06-01 14:52:32]

Description was changed from

==========
NOT FOR REVIEW Implement UnwindingInfoWriter and plug into TurboFan.
==========

to

==========

==========

[Stefano Sanfilippo, 2016-06-02 14:26:11]

Patchset #2 (id:160001) has been deleted

[Stefano Sanfilippo, 2016-06-07 19:31:36]

Patchset #2 (id:180001) has been deleted

[Stefano Sanfilippo, 2016-06-21 12:51:06]

Patchset #1 (id:140001) has been deleted

[Stefano Sanfilippo, 2016-06-21 12:51:14]

Patchset #1 (id:200001) has been deleted

[Stefano Sanfilippo, 2016-06-21 12:51:23]

Patchset #1 (id:220001) has been deleted

[Stefano Sanfilippo, 2016-06-21 12:51:31]

Patchset #1 (id:240001) has been deleted

[Stefano Sanfilippo, 2016-06-21 12:51:47]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 12:51:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/260001

[commit-bot: I haz the power, 2016-06-21 12:55:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 12:55:41]

Dry run: Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/17664)

[Stefano Sanfilippo, 2016-06-21 13:53:49]

Description was changed from

==========

==========

to

==========
NOT FOR REVIEW Implement UnwindingInfoWriter and disassembler.
==========

[Stefano Sanfilippo, 2016-06-21 13:53:59]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 13:54:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/280001

[Stefano Sanfilippo, 2016-06-21 13:59:40]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 13:59:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/320001

[commit-bot: I haz the power, 2016-06-21 14:38:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-21 14:38:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/7651)
  v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Stefano Sanfilippo, 2016-06-23 16:00:54]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 16:01:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/340001

[commit-bot: I haz the power, 2016-06-23 16:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 16:04:11]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/7820)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/3721)

[Stefano Sanfilippo, 2016-06-23 16:07:58]

Patchset #5 (id:340001) has been deleted

[Stefano Sanfilippo, 2016-06-23 16:09:30]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 16:09:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/360001

[commit-bot: I haz the power, 2016-06-23 16:20:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 16:20:09]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Stefano Sanfilippo, 2016-06-23 16:35:10]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 16:35:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/380001

[Stefano Sanfilippo, 2016-06-23 16:36:02]

Patchset #5 (id:360001) has been deleted

[commit-bot: I haz the power, 2016-06-23 16:45:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 16:45:28]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[Stefano Sanfilippo, 2016-06-23 16:47:17]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 16:47:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023503002/400001

[commit-bot: I haz the power, 2016-06-23 17:17:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 17:17:47]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-23 17:38:00]

Patchset #5 (id:380001) has been deleted

[Stefano Sanfilippo, 2016-06-24 09:45:32]

Description was changed from

==========
NOT FOR REVIEW Implement UnwindingInfoWriter and disassembler.
==========

to

==========
CFIDirectivesWriter and disassembler
Plug in CodeGenerator::MakeCodeEpilogue
==========

[Stefano Sanfilippo, 2016-06-24 09:51:00]

Description was changed from

==========
CFIDirectivesWriter and disassembler
Plug in CodeGenerator::MakeCodeEpilogue
==========

to

==========
CFIDirectivesWriter and disassembler
Pass optionally to CodeGenerator::MakeCodeEpilogue
==========

[Stefano Sanfilippo, 2016-06-24 14:09:19]

Description was changed from

==========
CFIDirectivesWriter and disassembler
Pass optionally to CodeGenerator::MakeCodeEpilogue
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will add unwinding information to the code
object when present.
==========

[Stefano Sanfilippo, 2016-06-24 14:09:50]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will add unwinding information to the code
object when present.
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the code
object when passed one.
==========

[Stefano Sanfilippo, 2016-06-24 14:10:05]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the code
object when passed one.
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the
code object when passed one.
==========

[Stefano Sanfilippo, 2016-06-24 14:11:00]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the
code object when passed one.
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the
code object when passed one.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-06-24 15:01:19]

Patchset #8 (id:460001) has been deleted

[Stefano Sanfilippo, 2016-06-24 15:01:28]

Patchset #8 (id:480001) has been deleted

[Stefano Sanfilippo, 2016-06-24 15:02:23]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 15:02:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 15:31:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 15:31:06]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-24 16:40:33]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 16:40:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 16:48:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 16:48:33]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/9387)

[Stefano Sanfilippo, 2016-06-24 17:59:30]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 17:59:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-06-24 18:00:08]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a CFIDirectivesWriter and will attach unwinding information to the
code object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the
code object when passed one.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-06-24 18:00:41]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the
code object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-06-24 18:02:10]

Patchset #10 (id:540001) has been deleted

[Stefano Sanfilippo, 2016-06-24 18:03:05]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 18:03:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-06-24 18:04:10]

Patchset #10 (id:560001) has been deleted

[Stefano Sanfilippo, 2016-06-24 18:04:18]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 18:04:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 18:37:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 18:37:28]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-27 14:24:08]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 14:24:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-06-27 14:38:44]

Description was changed from

==========
Implement stack unwinding directives writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

[commit-bot: I haz the power, 2016-06-27 14:50:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 14:50:15]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-27 15:34:56]

ssanfilippo@chromium.org changed reviewers:
+ jarin@chromium.org, rmcilroy@chromium.org

[Stefano Sanfilippo, 2016-06-27 15:34:58]

Here's a follow-up to the freshly landed
https://crrev.com/7d073b03c710b34f001fedd074f0ec9fbbaa5623. PTAL.

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

First round of comments.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.cc
File src/arm/eh-frame-arm.cc (right):

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.c...
src/arm/eh-frame-arm.cc:10: const char* GetRegisterName(int register_number) {
Just use Register::from_code(i).ToString() and delete this function.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h
File src/arm/eh-frame-arm.h (right):

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:24: 0x01,                    // Code alignment factor: 1
I think we always have code aligned to 4 (or kPointerSize) given the code object
is always pointer aligned. Not sure if this helps or not?

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:25: 0x04,                    // Data alignment factor: 4
Use kDataAlignmentFactor and other constants here where appropriate

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode8
src/eh-frame.cc:8: #include <ostream>
Newline before <iomanip> and after <ostream> (a new block for each header type -
own header, c++ libraries, other headers).

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode56
src/eh-frame.cc:56: static const byte DW_CFA_def_cfa_offset = 0x0e;
Move all these constants to be private constants in the class. They should also
use chromium naming (e.g., kCfaAdvanceLoc1)

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode61
src/eh-frame.cc:61: static const byte DW_EH_PE_sdata4 = 0x0b;
These are all unused - either move to the arch specific headers and use them
there, or remove

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode65
src/eh-frame.cc:65: static const byte kFDEPadding[] = {DW_CFA_nop, DW_CFA_nop,
DW_CFA_nop,
Move into finish function where it's used.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode71
src/eh-frame.cc:71: const int EhFrameWriter::kCIESize = sizeof(kCIE);
I think you need STATIC_CONST_MEMBER_DEFINITION for this.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode81
src/eh-frame.cc:81: static const int kFDEHeaderSize = 4 * kInt32Size + 1;
Move this with the rest of the constants.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode93
src/eh-frame.cc:93: // No way to handle 64bit values, but that's an unrealistic
delta anyway.
No need for this comment, the pc_offset is already int so can never be 64 bits

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode97
src/eh-frame.cc:97: WriteByte((1 << 6) | (delta & 0x3f));  // DW_CFA_advance_loc
use a constant instead of 0x3f

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode98
src/eh-frame.cc:98: } else if (delta <= std::numeric_limits<uint8_t>::max()) {
kMaxUInt8

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:101: } else if (delta <= std::numeric_limits<uint16_t>::max()) {
kMaxUInt16

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:134: // but that name is way too generic.
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

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:140: WriteByte((2 << 6) | (code & 0x3f));  // DW_CFA_offset
constant

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:144: // This method should be SameValue(), but again that name
is too generic.
ditto

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:163: int fde_offset = kCIESize;
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.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:178: unwinding_info_.AddBlock(0x00, EhFrameHdr::kRecordSize);
Could you not just add the actual EhFrameHdr block here instead?

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:181: // Remember to call Finish() before GetUnwindingInfo()!
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.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:208: 
#ifdef ENABLE_DISASSEMBLER

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:215: DCHECK_NOT_NULL(stream);
No need for this, it will already have exploded in stream->flags() by this point
if it was NULL

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:226: static void DumpDWARFDirectives(std::ostream& stream,  //
NOLINT
Put this in the annonymous name space instead of static

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:236: if (((*cur >> 6) & 0xff) == 1) {
Use constants

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:245: if (((*cur >> 6) & 0xff) == 2) {
ditto

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:258: cur += sizeof(byte);
You always cur += sizeof(byte) - just do it once above

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:268: uint16_t value = *reinterpret_cast<const uint16_t*>(cur);
This won't work on machines that don't support unaligned access. Use
ReadUnalignedUInt16 / ReadUnalignedUInt32 etc

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:325: << std::setfill('0') << static_cast<unsigned>(*cur) <<
'\n';
Just the UNREACHABLE here, no need to print the unknown bytecode

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:343: const byte* procedure_offset_address = fde_start + 2 *
kInt32Size;
Constant offsets

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:347: const byte* procedure_size_address = fde_start + 3 *
kInt32Size;
ditto

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:371: << "  .eh_frame_hdr: placeholder\n";
Why bother printing the terminator and placeholder?

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:375: static const int kEncodingSpecifiersSize = 3;
Move with other constants

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.h#newcode23
src/eh-frame.h:23: void DefineCFARegister(int code);
Could you use Register instead of "int code"? (and below)

[Stefano Sanfilippo, 2016-06-28 15:40:28]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 15:40:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 15:42:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 15:42:12]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8121)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8101)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8081)

[Stefano Sanfilippo, 2016-06-28 15:57:57]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 15:58:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 16:24:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 16:24:48]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-29 11:51:09]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 11:51:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-06-29 12:16:03]

Patchset #19 (id:760001) has been deleted

[Stefano Sanfilippo, 2016-06-29 14:21:30]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 14:21:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 14:28:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 14:28:35]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Stefano Sanfilippo, 2016-06-29 14:52:49]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 14:52:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 15:15:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 15:15:33]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.cc
File src/arm/eh-frame-arm.cc (right):

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.c...
src/arm/eh-frame-arm.cc:10: const char* GetRegisterName(int register_number) {
On 2016/06/28 10:37:32, rmcilroy wrote:
> Just use Register::from_code(i).ToString() and delete this function.

Register::ToString() appears to show all register as rX or aX, though. I think
having the aliases makes the disassembly more readable.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h
File src/arm/eh-frame-arm.h (right):

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:24: 0x01,                    // Code alignment factor: 1
On 2016/06/28 10:37:32, rmcilroy wrote:
> I think we always have code aligned to 4 (or kPointerSize) given the code
object
> is always pointer aligned. Not sure if this helps or not?

This is just a constant that is factored out from all offsets in AdvanceLocation
directives. Setting it to 1 means that we can directly use masm()->pc_offset(),
instead of having to divide by kPointerSize. Either ways are fine for me though.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:25: 0x04,                    // Data alignment factor: 4
On 2016/06/28 10:37:32, rmcilroy wrote:
> Use kDataAlignmentFactor and other constants here where appropriate

Replaced kInitialCFAOffset where appropriate.

kDataAlignmentFactor is SLEB128 encoded in the CIE, I can't directly use
kDataAlignmentFactor, unfortunately.

Also, since now we use a Register instead of the integer index, we would need a
constant for the initial CFA register, which would be used only here, wdyt?

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode8
src/eh-frame.cc:8: #include <ostream>
On 2016/06/28 10:37:33, rmcilroy wrote:
> Newline before <iomanip> and after <ostream> (a new block for each header type
-
> own header, c++ libraries, other headers).

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode56
src/eh-frame.cc:56: static const byte DW_CFA_def_cfa_offset = 0x0e;
On 2016/06/28 10:37:33, rmcilroy wrote:
> Move all these constants to be private constants in the class. They should
also
> use chromium naming (e.g., kCfaAdvanceLoc1)

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode61
src/eh-frame.cc:61: static const byte DW_EH_PE_sdata4 = 0x0b;
On 2016/06/28 10:37:32, rmcilroy wrote:
> These are all unused - either move to the arch specific headers and use them
> there, or remove

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

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode65
src/eh-frame.cc:65: static const byte kFDEPadding[] = {DW_CFA_nop, DW_CFA_nop,
DW_CFA_nop,
On 2016/06/28 10:37:32, rmcilroy wrote:
> Move into finish function where it's used.

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode71
src/eh-frame.cc:71: const int EhFrameWriter::kCIESize = sizeof(kCIE);
On 2016/06/28 10:37:33, rmcilroy wrote:
> I think you need STATIC_CONST_MEMBER_DEFINITION for this.

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode81
src/eh-frame.cc:81: static const int kFDEHeaderSize = 4 * kInt32Size + 1;
On 2016/06/28 10:37:32, rmcilroy wrote:
> Move this with the rest of the constants.

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode93
src/eh-frame.cc:93: // No way to handle 64bit values, but that's an unrealistic
delta anyway.
On 2016/06/28 10:37:32, rmcilroy wrote:
> No need for this comment, the pc_offset is already int so can never be 64 bits

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode97
src/eh-frame.cc:97: WriteByte((1 << 6) | (delta & 0x3f));  // DW_CFA_advance_loc
On 2016/06/28 10:37:33, rmcilroy wrote:
> use a constant instead of 0x3f

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcode98
src/eh-frame.cc:98: } else if (delta <= std::numeric_limits<uint8_t>::max()) {
On 2016/06/28 10:37:32, rmcilroy wrote:
> kMaxUInt8

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:101: } else if (delta <= std::numeric_limits<uint16_t>::max()) {
On 2016/06/28 10:37:32, rmcilroy wrote:
> kMaxUInt16

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:134: // but that name is way too generic.
On 2016/06/28 10:37:33, rmcilroy wrote:
> 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

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

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:140: WriteByte((2 << 6) | (code & 0x3f));  // DW_CFA_offset
On 2016/06/28 10:37:33, rmcilroy wrote:
> constant

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:144: // This method should be SameValue(), but again that name
is too generic.
On 2016/06/28 10:37:32, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:163: int fde_offset = kCIESize;
On 2016/06/28 10:37:33, rmcilroy wrote:
> 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.

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:178: unwinding_info_.AddBlock(0x00, EhFrameHdr::kRecordSize);
On 2016/06/28 10:37:33, rmcilroy wrote:
> Could you not just add the actual EhFrameHdr block here instead?

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

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:181: // Remember to call Finish() before GetUnwindingInfo()!
On 2016/06/28 10:37:33, rmcilroy wrote:
> 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.

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.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:208: 
On 2016/06/28 10:37:32, rmcilroy wrote:
> #ifdef ENABLE_DISASSEMBLER

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:215: DCHECK_NOT_NULL(stream);
On 2016/06/28 10:37:33, rmcilroy wrote:
> No need for this, it will already have exploded in stream->flags() by this
point
> if it was NULL

Fair enough. Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:226: static void DumpDWARFDirectives(std::ostream& stream,  //
NOLINT
On 2016/06/28 10:37:32, rmcilroy wrote:
> Put this in the annonymous name space instead of static

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:236: if (((*cur >> 6) & 0xff) == 1) {
On 2016/06/28 10:37:32, rmcilroy wrote:
> Use constants

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:245: if (((*cur >> 6) & 0xff) == 2) {
On 2016/06/28 10:37:33, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:258: cur += sizeof(byte);
On 2016/06/28 10:37:32, rmcilroy wrote:
> You always cur += sizeof(byte) - just do it once above

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:268: uint16_t value = *reinterpret_cast<const uint16_t*>(cur);
On 2016/06/28 10:37:33, rmcilroy wrote:
> This won't work on machines that don't support unaligned access. Use
> ReadUnalignedUInt16 / ReadUnalignedUInt32 etc

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:325: << std::setfill('0') << static_cast<unsigned>(*cur) <<
'\n';
On 2016/06/28 10:37:32, rmcilroy wrote:
> Just the UNREACHABLE here, no need to print the unknown bytecode

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:343: const byte* procedure_offset_address = fde_start + 2 *
kInt32Size;
On 2016/06/28 10:37:32, rmcilroy wrote:
> Constant offsets

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:347: const byte* procedure_size_address = fde_start + 3 *
kInt32Size;
On 2016/06/28 10:37:32, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:371: << "  .eh_frame_hdr: placeholder\n";
On 2016/06/28 10:37:32, rmcilroy wrote:
> Why bother printing the terminator and placeholder?

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

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.cc#newcod...
src/eh-frame.cc:375: static const int kEncodingSpecifiersSize = 3;
On 2016/06/28 10:37:32, rmcilroy wrote:
> Move with other constants

Done.

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/620001/src/eh-frame.h#newcode23
src/eh-frame.h:23: void DefineCFARegister(int code);
On 2016/06/28 10:37:33, rmcilroy wrote:
> Could you use Register instead of "int code"? (and below)

Yes, done. I added a RegisterToDwarfCode function used that is used internally
to retrieve the correct code, as indexes in the Register::Code enum do not
necessarily match DWARF register codes as specified by the ABI.

[Stefano Sanfilippo, 2016-06-29 16:44:41]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 16:44:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 17:17:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 17:17:46]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-06-30 10:49:16]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 10:49:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 11:35:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 11:35:30]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-06-30 15:23:11]

Some more comments.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h
File src/arm/eh-frame-arm.h (right):

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:24: 0x01,                    // Code alignment factor: 1
On 2016/06/29 15:16:20, Stefano Sanfilippo wrote:
> On 2016/06/28 10:37:32, rmcilroy wrote:
> > I think we always have code aligned to 4 (or kPointerSize) given the code
> object
> > is always pointer aligned. Not sure if this helps or not?
> 
> This is just a constant that is factored out from all offsets in
AdvanceLocation
> directives. Setting it to 1 means that we can directly use
masm()->pc_offset(),
> instead of having to divide by kPointerSize. Either ways are fine for me
though.

I see, this is fine.

https://codereview.chromium.org/2023503002/diff/620001/src/arm/eh-frame-arm.h...
src/arm/eh-frame-arm.h:25: 0x04,                    // Data alignment factor: 4
On 2016/06/29 15:16:20, Stefano Sanfilippo wrote:
> On 2016/06/28 10:37:32, rmcilroy wrote:
> > Use kDataAlignmentFactor and other constants here where appropriate
> 
> Replaced kInitialCFAOffset where appropriate.
> 
> kDataAlignmentFactor is SLEB128 encoded in the CIE, I can't directly use
> kDataAlignmentFactor, unfortunately.
> 
> Also, since now we use a Register instead of the integer index, we would need
a
> constant for the initial CFA register, which would be used only here, wdyt?

As mentioned in a new comment in eh-frame-x64, I think you should write the
header using the Write... functionality rather than writing it as a blob - that
will allow you to use WriteSLEB to encode this.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode57
src/eh-frame.cc:57: EhFrameWriter::kProcedureAddressOffset = kFDEOffset + 2 *
kInt32Size;
I think it would be better if these were relative to the start of the FDEOffset
(so not taking into account CIE size). That way, you can build the eh_frame, and
keep track of where the FDE starts (in a field), and apply the offsets from
there. That avoids you having to know about sizeof(kCIE) field and make these
STATIC_CONST_MEMBER_DEFINITION (also it will be necessary if you write the CIE
like the FDE as I propose below in eh-frame-x64.h.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode61
src/eh-frame.cc:61: STATIC_CONST_MEMBER_DEFINITION const int
EhFrameWriter::kFDEDirectivesOffset =
This is only used in DisassembleToStream, just define it there

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode66
src/eh-frame.cc:66: int RegisterToDwarfCode(Register name);
these should be methods in EhFrameWriter

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcod...
src/eh-frame.cc:153: 
DCHECK(!eh_frame_finalized_)

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode24
src/eh-frame.h:24: // <base address> = <base register> + <base offset>
What is the base address? Please explain how it relates to the caller's stack
frame.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode30
src/eh-frame.h:30: void DefineBaseAddressRegisterOffset(Register base_register,
int base_offset);
DefineBaseAddressRegisterAndOffset

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode41
src/eh-frame.h:41: // Remember to call Finish() before GetEhFrame()!
Make it clear that the EhWriter must outlive the CodeDesc since the unwind_info
isn't copied

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode90
src/eh-frame.h:90: static const byte kDefCFAOffset = 0x0e;
This could be enums (enum class DwarfOpcode: byte) to keep them grouped.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode91
src/eh-frame.h:91: static const uint32_t kInt32Placeholder = 0xdeadc0de;
newlines between groups

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode94
src/eh-frame.h:94: static const int kSavedRegisterTag = 2;
this should be 1 << 1 to make it clear its a bit field ( and one above should be
1 << 0 too)

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode98
src/eh-frame.h:98: static const int kSavedRegisterMaskSize = 6;
group tags with masks

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode111
src/eh-frame.h:111: bool eh_frame_finalised_;
no need to make this debug only, setting a bool isn't going to make any perf
impact, and it only clutters the code.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode115
src/eh-frame.h:115: List<byte> eh_frame_buffer_;
Why list and not vector?

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode122
src/eh-frame.h:122: static EhFrameHdr MakeDummy();
below constructor. Also, how about Empty instead of Dummy (rename function
CreateEmptyHeader or similar)

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h
File src/x64/eh-frame-x64.h (right):

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:13: static const Register& kInitialBaseRegister = rsp;
Don't use const references. Just use the register code if you can't create a
static const Register.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:15: static const int kDataAlignmentFactor = -8;
Curious, why is this negative, seems pretty strange?

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:17: static const int kInitialStateOffsetInCIE = 19;
This is always 19 on all platforms (the header is always be the same size), just
define directly in the disassemble directives function.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:30: 0x1b,                    // FDE pointers encoding:
kSData4 | kPcRel
The CIE is almost identical for every platform (the only differences AFAIKS is
the size, alignment factor and return address register. Could you split up the
CIE into the header bit and the augmentation data. I also think you should write
these in the same way as you write the FDE, and only define the constants which
are different in this file (some of which, e.g., kDataAlignmentFactor, are
already here).

[Stefano Sanfilippo, 2016-07-04 16:19:26]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 16:19:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-04 17:26:14]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 17:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 17:49:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-04 17:49:29]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-04 18:21:15]

I addressed all the comments and did a bit of additional refactoring. PTAL.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode57
src/eh-frame.cc:57: EhFrameWriter::kProcedureAddressOffset = kFDEOffset + 2 *
kInt32Size;
On 2016/06/30 15:23:10, rmcilroy wrote:
> I think it would be better if these were relative to the start of the
FDEOffset
> (so not taking into account CIE size). That way, you can build the eh_frame,
and
> keep track of where the FDE starts (in a field), and apply the offsets from
> there. That avoids you having to know about sizeof(kCIE) field and make these
> STATIC_CONST_MEMBER_DEFINITION (also it will be necessary if you write the CIE
> like the FDE as I propose below in eh-frame-x64.h.

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode61
src/eh-frame.cc:61: STATIC_CONST_MEMBER_DEFINITION const int
EhFrameWriter::kFDEDirectivesOffset =
On 2016/06/30 15:23:10, rmcilroy wrote:
> This is only used in DisassembleToStream, just define it there

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcode66
src/eh-frame.cc:66: int RegisterToDwarfCode(Register name);
On 2016/06/30 15:23:10, rmcilroy wrote:
> these should be methods in EhFrameWriter

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.cc#newcod...
src/eh-frame.cc:153: 
On 2016/06/30 15:23:10, rmcilroy wrote:
> DCHECK(!eh_frame_finalized_)

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode24
src/eh-frame.h:24: // <base address> = <base register> + <base offset>
On 2016/06/30 15:23:11, rmcilroy wrote:
> What is the base address? Please explain how it relates to the caller's stack
> frame.

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode30
src/eh-frame.h:30: void DefineBaseAddressRegisterOffset(Register base_register,
int base_offset);
On 2016/06/30 15:23:11, rmcilroy wrote:
> DefineBaseAddressRegisterAndOffset

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode41
src/eh-frame.h:41: // Remember to call Finish() before GetEhFrame()!
On 2016/06/30 15:23:11, rmcilroy wrote:
> Make it clear that the EhWriter must outlive the CodeDesc since the
unwind_info
> isn't copied

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode90
src/eh-frame.h:90: static const byte kDefCFAOffset = 0x0e;
On 2016/06/30 15:23:11, rmcilroy wrote:
> This could be enums (enum class DwarfOpcode: byte) to keep them grouped.

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode91
src/eh-frame.h:91: static const uint32_t kInt32Placeholder = 0xdeadc0de;
On 2016/06/30 15:23:11, rmcilroy wrote:
> newlines between groups

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode94
src/eh-frame.h:94: static const int kSavedRegisterTag = 2;
On 2016/06/30 15:23:11, rmcilroy wrote:
> this should be 1 << 1 to make it clear its a bit field ( and one above should
be
> 1 << 0 too)

It's not a bit field, the 2 MSBs are used as opcode, and the other 6 as operand.
Do you think I should use another name in place of "tag"?

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode98
src/eh-frame.h:98: static const int kSavedRegisterMaskSize = 6;
On 2016/06/30 15:23:11, rmcilroy wrote:
> group tags with masks

Done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode111
src/eh-frame.h:111: bool eh_frame_finalised_;
On 2016/06/30 15:23:11, rmcilroy wrote:
> no need to make this debug only, setting a bool isn't going to make any perf
> impact, and it only clutters the code.


Fine, done.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode115
src/eh-frame.h:115: List<byte> eh_frame_buffer_;
On 2016/06/30 15:23:11, rmcilroy wrote:
> Why list and not vector?

I assumed V8 APIs took priority, replaced with std::vector.

https://codereview.chromium.org/2023503002/diff/920001/src/eh-frame.h#newcode122
src/eh-frame.h:122: static EhFrameHdr MakeDummy();
On 2016/06/30 15:23:11, rmcilroy wrote:
> below constructor. Also, how about Empty instead of Dummy (rename function
> CreateEmptyHeader or similar)

Moved. 

*Empty* is fine for me, renamed.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h
File src/x64/eh-frame-x64.h (right):

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:13: static const Register& kInitialBaseRegister = rsp;
On 2016/06/30 15:23:11, rmcilroy wrote:
> Don't use const references. Just use the register code if you can't create a
> static const Register.

I did away completely with this bit in the refactoring, removed.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:15: static const int kDataAlignmentFactor = -8;
On 2016/06/30 15:23:11, rmcilroy wrote:
> Curious, why is this negative, seems pretty strange?

It's negative to reflect the fact that the stack grows downwards. That is, each
slot has an address -8 bytes "larger" than the previous.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:17: static const int kInitialStateOffsetInCIE = 19;
On 2016/06/30 15:23:11, rmcilroy wrote:
> This is always 19 on all platforms (the header is always be the same size),
just
> define directly in the disassemble directives function.

This bit became an arch-independent constant now that we have a WriteCIE()
method, so it has been removed.

https://codereview.chromium.org/2023503002/diff/920001/src/x64/eh-frame-x64.h...
src/x64/eh-frame-x64.h:30: 0x1b,                    // FDE pointers encoding:
kSData4 | kPcRel
On 2016/06/30 15:23:11, rmcilroy wrote:
> The CIE is almost identical for every platform (the only differences AFAIKS is
> the size, alignment factor and return address register. Could you split up the
> CIE into the header bit and the augmentation data. I also think you should
write
> these in the same way as you write the FDE, and only define the constants
which
> are different in this file (some of which, e.g., kDataAlignmentFactor, are
> already here).

Done.

[Stefano Sanfilippo, 2016-07-04 18:36:23]

Patchset #33 (id:1060001) has been deleted

[Stefano Sanfilippo, 2016-07-04 18:37:09]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 18:37:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-04 18:45:13]

Patchset #31 (id:1020001) has been deleted

[Stefano Sanfilippo, 2016-07-04 18:45:31]

Patchset #31 (id:1040001) has been deleted

[commit-bot: I haz the power, 2016-07-04 19:01:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-04 19:01:17]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-05 10:24:27]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 10:24:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-05 10:40:51]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 10:40:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-07-05 10:56:13]

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:44: STATIC_CONST_MEMBER_DEFINITION const uint32_t
EhFrameWriter::kInt32Placeholder;
I don't think you need these do you?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:50: base_offset_(0) {
Set cie_size_ to zero here, to ensure they are properly initialized.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:51: eh_frame_buffer_.reserve(100);
100 seems slightly random. Why not 128?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:54: cie_size_ = eh_frame_offset() - cie_start_offset;
Set this in WriteCIE ?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:82: PatchInt32(cie_size_offset, encoded_cie_size);
Some whitespace would be good to break up this code (e.g., before
WriteInitialState() and cie_end_offset =

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:86: DCHECK_EQ(eh_frame_offset(), fde_offset());
DCHECK_NE(cie_size_, 0)

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:93: WriteByte(0);                   // No augmentation data
Newlines between each of these fields, with comments on their own line

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:157: void EhFrameWriter::RegisterSavedToStack(int register_code,
int offset) {
RecordRegisterSavedToStack

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:173: void EhFrameWriter::RegisterIsValid(Register name) {
RecordRegisterIsValid

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode15
src/eh-frame.h:15: enum DwarfOpcodes : byte {
enum class please

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode27
src/eh-frame.h:27: enum DwarfEncodingSpecifiers : byte {
ditto

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode38
src/eh-frame.h:38: static const int kSavedRegisterTag = 2;
Move SavedRegisterTag below the location mask / mask size and newline between
location and saved register constants

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode46
src/eh-frame.h:46: static const int kTerminatorSize = 4;
kFdeTerminatorSize ?  also put in it's own block (and move
kInitialStateOffsetInCIE to the top of this bloc).

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode50
src/eh-frame.h:50: static const int kDataAlignmentFactor;
This should all be private, none ofthese fields or enums are used outside of
this class or tests.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode94
src/eh-frame.h:94: protected:
This should all be private - you've already made EhFrameWriterInternals a friend
class

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode96
src/eh-frame.h:96: 
nit - remove newline (and group common blocks (e.g., all U/SLEB128 functions -
also make the names complimentary - e.g., Read/Write or Encode/Decode)

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:124: void WriteFDEHeader();
Short comments for these more complex functions

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:128: void WriteInitialState();
WriteInitialStateInCIE ?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:130: void WriteReturnAddressRegisterCode();
This should just be "Register GetReturnAddressRegister()", and then write it in
the common way (using RegisterToDwarfCode and WriteULEB128)

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:132: void Align();
WritePaddingTo8ByteAlign

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:166: int cie_size_;
nit - move cie_size_ to be first field

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:168: friend class EhFrameWriterInternals;  // For unit tests.
Rename this class to EhFrameWriteTest and remove comments
(EhFrameWriterInternals could be a non-test class)

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
File test/unittests/eh-frame-hdr-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-hdr-unittest.cc:10: class EhFrameHdrLayout : public
testing::Test {
I don't think you need this fixture - just create the frames in the tests
themselves

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
File test/unittests/eh-frame-writer-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:20: static const int
kTestRegisterCode = 0;
I would test a more interesting code than '0', that doesn't really check the
encoding is correct

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:48:
EhFrameWriterInternals::kTestRegisterCode;
Don't think you should need this?

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:53: TEST_F(EhFrameWriterInternals,
RecordsAlignment) {
The fixture should be named EhFrameWriterTest

[commit-bot: I haz the power, 2016-07-05 11:03:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-05 11:03:43]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-05 16:02:14]

I addressed all the comments except for those in eh-frame-writer-unittest.cc,
which needs more refactoring (on its way).

I also extracted a EhFrameDisassembler from the *Writer, because that feature is
conceptually separate.

PTAL.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:44: STATIC_CONST_MEMBER_DEFINITION const uint32_t
EhFrameWriter::kInt32Placeholder;
On 2016/07/05 10:56:12, rmcilroy wrote:
> I don't think you need these do you?

Unfortunately I do, otherwise I get a linking error. I am not sure why, though.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:50: base_offset_(0) {
On 2016/07/05 10:56:12, rmcilroy wrote:
> Set cie_size_ to zero here, to ensure they are properly initialized.

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:51: eh_frame_buffer_.reserve(100);
On 2016/07/05 10:56:12, rmcilroy wrote:
> 100 seems slightly random. Why not 128?

I observed that the eh_frame for our code objects usually grows to be around
50-60 bytes, and is almost invariably below 100 bytes. For this reason I thought
100 may make sense for preallocation.

100 is a completely random choice. Either 100 or 128 is fine for me. Wdyt?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:54: cie_size_ = eh_frame_offset() - cie_start_offset;
On 2016/07/05 10:56:12, rmcilroy wrote:
> Set this in WriteCIE ? 

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:82: PatchInt32(cie_size_offset, encoded_cie_size);
On 2016/07/05 10:56:12, rmcilroy wrote:
> Some whitespace would be good to break up this code (e.g., before
> WriteInitialState() and cie_end_offset = 

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:86: DCHECK_EQ(eh_frame_offset(), fde_offset());
On 2016/07/05 10:56:12, rmcilroy wrote:
> DCHECK_NE(cie_size_, 0)

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:93: WriteByte(0);                   // No augmentation data
On 2016/07/05 10:56:12, rmcilroy wrote:
> Newlines between each of these fields, with comments on their own line

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:157: void EhFrameWriter::RegisterSavedToStack(int register_code,
int offset) {
On 2016/07/05 10:56:12, rmcilroy wrote:
> RecordRegisterSavedToStack

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:173: void EhFrameWriter::RegisterIsValid(Register name) {
On 2016/07/05 10:56:12, rmcilroy wrote:
> RecordRegisterIsValid

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode15
src/eh-frame.h:15: enum DwarfOpcodes : byte {
On 2016/07/05 10:56:12, rmcilroy wrote:
> enum class please

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode27
src/eh-frame.h:27: enum DwarfEncodingSpecifiers : byte {
On 2016/07/05 10:56:13, rmcilroy wrote:
> ditto

These need to be | combinable though, static_cast<...>(a) | static_cast<...>(b)
does not seem very elegant to me. Any idea?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode38
src/eh-frame.h:38: static const int kSavedRegisterTag = 2;
On 2016/07/05 10:56:13, rmcilroy wrote:
> Move SavedRegisterTag below the location mask / mask size and newline between
> location and saved register constants

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode46
src/eh-frame.h:46: static const int kTerminatorSize = 4;
On 2016/07/05 10:56:12, rmcilroy wrote:
> kFdeTerminatorSize ?  also put in it's own block (and move
> kInitialStateOffsetInCIE to the top of this bloc).

Done the moving. About the name, the terminator terminates the entire eh_frame,
maybe kEhFrameTerminatorSize?

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode50
src/eh-frame.h:50: static const int kDataAlignmentFactor;
On 2016/07/05 10:56:13, rmcilroy wrote:
> This should all be private, none ofthese fields or enums are used outside of
> this class or tests.

The encoding specifiers are used in the EhFrameHdr, now that I have split
EhFrameDisassembler and EhFrameIterator we also need the others.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode94
src/eh-frame.h:94: protected:
On 2016/07/05 10:56:13, rmcilroy wrote:
> This should all be private - you've already made EhFrameWriterInternals a
friend
> class

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode96
src/eh-frame.h:96: 
On 2016/07/05 10:56:13, rmcilroy wrote:
> nit - remove newline (and group common blocks (e.g., all U/SLEB128 functions -
> also make the names complimentary - e.g., Read/Write or Encode/Decode)

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:124: void WriteFDEHeader();
On 2016/07/05 10:56:13, rmcilroy wrote:
> Short comments for these more complex functions

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:128: void WriteInitialState();
On 2016/07/05 10:56:13, rmcilroy wrote:
> WriteInitialStateInCIE ?

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:130: void WriteReturnAddressRegisterCode();
On 2016/07/05 10:56:13, rmcilroy wrote:
> This should just be "Register GetReturnAddressRegister()", and then write it
in
> the common way (using RegisterToDwarfCode and WriteULEB128)

The problem is that x64 rip is a pseudoregister with no Register instance
associated. If you do not like this approach, we could either have a
kAddressRegisterCode constant or a GetReturnAddressCode() method that give the
DWARF code for the return address register.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:132: void Align();
On 2016/07/05 10:56:13, rmcilroy wrote:
> WritePaddingTo8ByteAlign

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:166: int cie_size_;
On 2016/07/05 10:56:13, rmcilroy wrote:
> nit - move cie_size_ to be first field

Done.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:168: friend class EhFrameWriterInternals;  // For unit tests.
On 2016/07/05 10:56:13, rmcilroy wrote:
> Rename this class to EhFrameWriteTest and remove comments
> (EhFrameWriterInternals could be a non-test class)

Removed the friend altogether, since the internals that needed testing have been
moved to other classes and we agreed to only examine the output .eh_frame

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
File test/unittests/eh-frame-hdr-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-hdr-unittest.cc:10: class EhFrameHdrLayout : public
testing::Test {
On 2016/07/05 10:56:13, rmcilroy wrote:
> I don't think you need this fixture - just create the frames in the tests
> themselves

Done.

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
File test/unittests/eh-frame-writer-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:48:
EhFrameWriterInternals::kTestRegisterCode;
On 2016/07/05 10:56:13, rmcilroy wrote:
> Don't think you should need this?

Also in this case, omitting this bit results in a linking error.

https://codereview.chromium.org/2023503002/diff/1090020/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:53: TEST_F(EhFrameWriterInternals,
RecordsAlignment) {
On 2016/07/05 10:56:13, rmcilroy wrote:
> The fixture should be named EhFrameWriterTest

Done.

[Stefano Sanfilippo, 2016-07-05 16:55:42]

Patchset #40 (id:1250001) has been deleted

[Stefano Sanfilippo, 2016-07-05 19:53:56]

Tests refactored, some of them have been moved to eh-frame-iterator-unittest.cc.
I still have a couple of doubts:

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h#newcod...
src/eh-frame.h:122: void WriteCIE();
I am not sure how to describe these methods in a comment, other than "Write
Common Information Entry." or similar. Any idea?

https://codereview.chromium.org/2023503002/diff/1290001/test/unittests/eh-fra...
File test/unittests/eh-frame-writer-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1290001/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:17: static const int
kTestRegisterCode = 0;
The problem with using different values here is that the register code must be
valid and known to EhFrameWriter::GetDwarfRegisterCode, or the writer will
crash. 

In order to support this case, I am handling the register having code 0 on all
supported platforms, rax for x64, r0 for arm and x0 for arm64. 

We might do the same for another register code, provided that it is valid on all
architectures, or we could add a #ifdef chain to provide a different register on
each arch. Wdyt?

[Stefano Sanfilippo, 2016-07-05 19:55:18]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 19:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 20:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-05 20:19:47]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-06 13:09:40]

Patchset #43 (id:1330001) has been deleted

[rmcilroy, 2016-07-06 13:59:31]

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:51: eh_frame_buffer_.reserve(100);
On 2016/07/05 16:02:13, Stefano Sanfilippo wrote:
> On 2016/07/05 10:56:12, rmcilroy wrote:
> > 100 seems slightly random. Why not 128?
> 
> I observed that the eh_frame for our code objects usually grows to be around
> 50-60 bytes, and is almost invariably below 100 bytes. For this reason I
thought
> 100 may make sense for preallocation.
> 
> 100 is a completely random choice. Either 100 or 128 is fine for me. Wdyt?

64 in that case seems reasonable in that case (seems sensible to be power of
two, and keep memory use low where possible).

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode27
src/eh-frame.h:27: enum DwarfEncodingSpecifiers : byte {
On 2016/07/05 16:02:14, Stefano Sanfilippo wrote:
> On 2016/07/05 10:56:13, rmcilroy wrote:
> > ditto
> 
> These need to be | combinable though, static_cast<...>(a) |
static_cast<...>(b)
> does not seem very elegant to me. Any idea?

OK, enum is fine in that case.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode46
src/eh-frame.h:46: static const int kTerminatorSize = 4;
On 2016/07/05 16:02:13, Stefano Sanfilippo wrote:
> On 2016/07/05 10:56:12, rmcilroy wrote:
> > kFdeTerminatorSize ?  also put in it's own block (and move
> > kInitialStateOffsetInCIE to the top of this bloc).
> 
> Done the moving. About the name, the terminator terminates the entire
eh_frame,
> maybe kEhFrameTerminatorSize?

SGTM

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode50
src/eh-frame.h:50: static const int kDataAlignmentFactor;
On 2016/07/05 16:02:14, Stefano Sanfilippo wrote:
> On 2016/07/05 10:56:13, rmcilroy wrote:
> > This should all be private, none ofthese fields or enums are used outside of
> > this class or tests.
> 
> The encoding specifiers are used in the EhFrameHdr, now that I have split
> EhFrameDisassembler and EhFrameIterator we also need the others.

These shouldn't be part of EhFrameWriter in that case. Perhaps create a separate
class EhFrame for frame specific constants. Maybe this could also replace the
EhFrameHdr class (i.e., move all the EhFrameHeader things into this new EhFrame
class).

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:124: void WriteFDEHeader();
On 2016/07/05 16:02:13, Stefano Sanfilippo wrote:
> On 2016/07/05 10:56:13, rmcilroy wrote:
> > Short comments for these more complex functions
> 
> Done.

Still no comments?

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h#newcod...
src/eh-frame.h:122: void WriteCIE();
On 2016/07/05 19:53:56, Stefano Sanfilippo wrote:
> I am not sure how to describe these methods in a comment, other than "Write
> Common Information Entry." or similar. Any idea?

"Write the Common Information Entry header" is fine - this explains what CIE is.

https://codereview.chromium.org/2023503002/diff/1290001/test/unittests/eh-fra...
File test/unittests/eh-frame-writer-unittest.cc (right):

https://codereview.chromium.org/2023503002/diff/1290001/test/unittests/eh-fra...
test/unittests/eh-frame-writer-unittest.cc:17: static const int
kTestRegisterCode = 0;
On 2016/07/05 19:53:56, Stefano Sanfilippo wrote:
> The problem with using different values here is that the register code must be
> valid and known to EhFrameWriter::GetDwarfRegisterCode, or the writer will
> crash. 
> 
> In order to support this case, I am handling the register having code 0 on all
> supported platforms, rax for x64, r0 for arm and x0 for arm64. 
> 
> We might do the same for another register code, provided that it is valid on
all
> architectures, or we could add a #ifdef chain to provide a different register
on
> each arch. Wdyt?

I guess this is fine then, thanks for the explination.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:151: explicit EhFrameIterator(EhFrameWriter* writer);
Why does this take a frame writer? It should take the output of the  frame
written by Finish, it seems strange to iterate over a writer

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:166: Skip(4 * kInt32Size + 1);
You should have a constant for the size of the FDE header in EhFrame

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:184: bool ReachedEnd() const {
nit - Done() or Finished() is the usual naming

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:192: int GetCurrentOffset() const {
newline

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:203: T GetNextIntegral() {
/s/Integral/Value

[Stefano Sanfilippo, 2016-07-06 16:50:24]

Patchset #46 (id:1410001) has been deleted

[Stefano Sanfilippo, 2016-07-06 16:54:50]

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.cc#newco...
src/eh-frame.cc:51: eh_frame_buffer_.reserve(100);
On 2016/07/06 13:59:30, rmcilroy wrote:
> On 2016/07/05 16:02:13, Stefano Sanfilippo wrote:
> > On 2016/07/05 10:56:12, rmcilroy wrote:
> > > 100 seems slightly random. Why not 128?
> > 
> > I observed that the eh_frame for our code objects usually grows to be around
> > 50-60 bytes, and is almost invariably below 100 bytes. For this reason I
> thought
> > 100 may make sense for preallocation.
> > 
> > 100 is a completely random choice. Either 100 or 128 is fine for me. Wdyt?
> 
> 64 in that case seems reasonable in that case (seems sensible to be power of
> two, and keep memory use low where possible).

Done, I went with 128 so we are covered in almost all cases without
reallocations.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcode50
src/eh-frame.h:50: static const int kDataAlignmentFactor;
On 2016/07/06 13:59:30, rmcilroy wrote:
> On 2016/07/05 16:02:14, Stefano Sanfilippo wrote:
> > On 2016/07/05 10:56:13, rmcilroy wrote:
> > > This should all be private, none ofthese fields or enums are used outside
of
> > > this class or tests.
> > 
> > The encoding specifiers are used in the EhFrameHdr, now that I have split
> > EhFrameDisassembler and EhFrameIterator we also need the others.
> 
> These shouldn't be part of EhFrameWriter in that case. Perhaps create a
separate
> class EhFrame for frame specific constants. Maybe this could also replace the
> EhFrameHdr class (i.e., move all the EhFrameHeader things into this new
EhFrame
> class).

As discussed, I have uploaded a patchset with the EhFrameConstants class, please
let me know your thoughts about it.

https://codereview.chromium.org/2023503002/diff/1090020/src/eh-frame.h#newcod...
src/eh-frame.h:124: void WriteFDEHeader();
On 2016/07/06 13:59:30, rmcilroy wrote:
> On 2016/07/05 16:02:13, Stefano Sanfilippo wrote:
> > On 2016/07/05 10:56:13, rmcilroy wrote:
> > > Short comments for these more complex functions
> > 
> > Done.
> 
> Still no comments?

Sorry, they came in a later patchset.

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1290001/src/eh-frame.h#newcod...
src/eh-frame.h:122: void WriteCIE();
On 2016/07/06 13:59:30, rmcilroy wrote:
> On 2016/07/05 19:53:56, Stefano Sanfilippo wrote:
> > I am not sure how to describe these methods in a comment, other than "Write
> > Common Information Entry." or similar. Any idea?
> 
> "Write the Common Information Entry header" is fine - this explains what CIE
is.

Comments added.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:151: explicit EhFrameIterator(EhFrameWriter* writer);
On 2016/07/06 13:59:30, rmcilroy wrote:
> Why does this take a frame writer? It should take the output of the  frame
> written by Finish, it seems strange to iterate over a writer

In order to extract the buffer from the EhFrameWriter, one must pass through a
CodeDesc, so this seemed a nice shortcut, but I agree it's not immediate what's
going on. Removed and refactored uses.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:166: Skip(4 * kInt32Size + 1);
On 2016/07/06 13:59:30, rmcilroy wrote:
> You should have a constant for the size of the FDE header in EhFrame

Done.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:184: bool ReachedEnd() const {
On 2016/07/06 13:59:30, rmcilroy wrote:
> nit - Done() or Finished() is the usual naming

Done.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:192: int GetCurrentOffset() const {
On 2016/07/06 13:59:30, rmcilroy wrote:
> newline

Done.

https://codereview.chromium.org/2023503002/diff/1350001/src/eh-frame.h#newcod...
src/eh-frame.h:203: T GetNextIntegral() {
On 2016/07/06 13:59:30, rmcilroy wrote:
> /s/Integral/Value

Done.

[Stefano Sanfilippo, 2016-07-06 17:29:53]

Patchset #48 (id:1470001) has been deleted

[Stefano Sanfilippo, 2016-07-06 17:31:05]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 17:31:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-06 18:45:54]

Patchset #50 (id:1530001) has been deleted

[rmcilroy, 2016-07-07 10:11:03]

Looking much better now, thanks. A couple of last comments.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.cc#newco...
src/eh-frame.cc:115: cie_size_ = record_end_offset - size_offset;
Some comments here like WriteFDEHeader

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode13
src/eh-frame.h:13: struct EhFrameConstants final {
class

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode52
src/eh-frame.h:52: static const int kDataAlignmentFactor;
Add a comment that this is defined in eh-writer-<arch>.cc

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode60
src/eh-frame.h:60: static const byte kDummyEhFrame[kEhFrameHdrSize];
Don't make this a static with static initialization. Just build an empty frame
each time in CreateEmptyEhFrame function.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcod...
src/eh-frame.h:135: // This bit is platform specific.
Don't use "bit". If you want group all the architecture dependent functions
together and have a group header saying: 

// Platform specific functions which are implemented in eh-header-<arch>.cc

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcod...
src/eh-frame.h:153: // handling pseudoregisters on some platforms, such as x64.
pseudo-registers and drop the ", such as x64"

[Stefano Sanfilippo, 2016-07-07 10:57:00]

Thanks for the comments, I should have addressed them all. PTAL.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.cc#newco...
src/eh-frame.cc:115: cie_size_ = record_end_offset - size_offset;
On 2016/07/07 10:11:03, rmcilroy wrote:
> Some comments here like WriteFDEHeader

Done.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode13
src/eh-frame.h:13: struct EhFrameConstants final {
On 2016/07/07 10:11:03, rmcilroy wrote:
> class

Done.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode52
src/eh-frame.h:52: static const int kDataAlignmentFactor;
On 2016/07/07 10:11:03, rmcilroy wrote:
> Add a comment that this is defined in eh-writer-<arch>.cc

Done.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcode60
src/eh-frame.h:60: static const byte kDummyEhFrame[kEhFrameHdrSize];
On 2016/07/07 10:11:03, rmcilroy wrote:
> Don't make this a static with static initialization. Just build an empty frame
> each time in CreateEmptyEhFrame function.

Done.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcod...
src/eh-frame.h:135: // This bit is platform specific.
On 2016/07/07 10:11:03, rmcilroy wrote:
> Don't use "bit". If you want group all the architecture dependent functions
> together and have a group header saying: 
> 
> // Platform specific functions which are implemented in eh-header-<arch>.cc

Done.

https://codereview.chromium.org/2023503002/diff/1610001/src/eh-frame.h#newcod...
src/eh-frame.h:153: // handling pseudoregisters on some platforms, such as x64.
On 2016/07/07 10:11:03, rmcilroy wrote:
> pseudo-registers and drop the ", such as x64"

Done.

[Stefano Sanfilippo, 2016-07-07 14:46:26]

Patchset #58 (id:1530002) has been deleted

[Stefano Sanfilippo, 2016-07-07 14:50:35]

In order to support the embedded optional behaviour in UnwindingInfoWriter, I
had to move the code that writes the CIE and FDE header to an explicit Start()
method.

Additionally, the internal state now transitions from undefined to initialised
(after Start() was called) to finalised (after Finish() was called). PTAL.

[Stefano Sanfilippo, 2016-07-07 16:05:28]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 16:05:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 16:28:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 16:28:55]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-07 16:44:01]

Changed to American spelling, could you please take another look?

[rmcilroy, 2016-07-08 09:20:32]

LGTM.

Jaro, do you want to give it a last pass before Stefano lands it?

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc#newco...
src/eh-frame.cc:281: WriteULEB128(base_offset);
Is offset always positive? Could you add a DCHECK if so and make the constraint
clear in the comment in the header file.

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc#newco...
src/eh-frame.cc:299: WriteULEB128(base_offset);
ditto

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.h#newcode67
src/eh-frame.h:67: // The dummy frame is a hack to trigger fp-based unwinding in
Linux perf
/s/dummy/empty

[Stefano Sanfilippo, 2016-07-08 09:32:13]

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc
File src/eh-frame.cc (right):

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc#newco...
src/eh-frame.cc:281: WriteULEB128(base_offset);
On 2016/07/08 09:20:32, rmcilroy wrote:
> Is offset always positive? Could you add a DCHECK if so and make the
constraint
> clear in the comment in the header file.

Yes, positive or zero. Isn't the DCHECK at line 279 already doing what you ask?

Comments added in the .h, thanks.

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.cc#newco...
src/eh-frame.cc:299: WriteULEB128(base_offset);
On 2016/07/08 09:20:32, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.h
File src/eh-frame.h (right):

https://codereview.chromium.org/2023503002/diff/1800001/src/eh-frame.h#newcode67
src/eh-frame.h:67: // The dummy frame is a hack to trigger fp-based unwinding in
Linux perf
On 2016/07/08 09:20:32, rmcilroy wrote:
> /s/dummy/empty 

Done.

[Stefano Sanfilippo, 2016-07-08 09:39:50]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 09:39:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-08 11:15:04]

Patchset #65 (id:1850001) has been deleted

[Stefano Sanfilippo, 2016-07-08 11:16:48]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 11:16:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 11:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 11:22:06]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[Jarin, 2016-07-08 11:24:06]

On 2016/07/08 09:20:32, rmcilroy wrote:
> LGTM.
> 
> Jaro, do you want to give it a last pass before Stefano lands it?

I am busy today. If you guys want to land today, I won't be able to have a look.
I will try to read through the CL by Monday.

[Stefano Sanfilippo, 2016-07-08 11:33:54]

Patchset #65 (id:1870001) has been deleted

[Stefano Sanfilippo, 2016-07-08 11:34:13]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 11:34:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-08 11:36:06]

On 2016/07/08 11:24:06, Jarin wrote:
> On 2016/07/08 09:20:32, rmcilroy wrote:
> > LGTM.
> > 
> > Jaro, do you want to give it a last pass before Stefano lands it?
> 
> I am busy today. If you guys want to land today, I won't be able to have a
look.
> I will try to read through the CL by Monday.

I don't see problems with landing this next week. Thank you Jaro.

[commit-bot: I haz the power, 2016-07-08 13:38:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 13:38:37]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-08 18:48:32]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 18:48:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 20:52:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 20:52:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)

[Stefano Sanfilippo, 2016-07-11 10:05:09]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 10:05:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 10:35:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 10:35:40]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-07-11 18:58:56]

I skimmed through the CL, lgtm.

[Stefano Sanfilippo, 2016-07-11 19:08:56]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 19:09:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 19:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 19:32:37]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-12 11:10:07]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 11:10:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 11:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 11:35:49]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-12 13:26:26]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 13:26:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 13:49:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-12 13:49:41]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-12 13:53:38]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-12 13:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2023503002/#ps2010001
(title: "Factor out code alignment.")

[commit-bot: I haz the power, 2016-07-12 13:53:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-12 14:33:17]

The CQ bit was unchecked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-12 14:37:19]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-12 14:37:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2023503002/#ps2030001
(title: "s/follows initial rule/follows rule in CIE/")

[commit-bot: I haz the power, 2016-07-12 14:37:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 15:01:29]

Description was changed from

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

[commit-bot: I haz the power, 2016-07-12 15:01:34]

Committed patchset #72 (id:2030001)

[commit-bot: I haz the power, 2016-07-12 15:01:46]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 15:04:42]

Description was changed from

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}
==========

[commit-bot: I haz the power, 2016-07-12 15:04:45]

Patchset 72 (id:??) landed as
https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}

[Stefano Sanfilippo, 2016-07-12 16:02:35]

A revert of this CL (patchset #72 id:2030001) has been created in
https://codereview.chromium.org/2143033002/ by ssanfilippo@chromium.org.

The reason for reverting is: The STATIC_CONST_MEMBER_DEFINITION in
eh-frame-writer-unittest.cc causes a compiler error on V8 Win64 - clang
buildbot. Removing that bit should be sufficient..

[Stefano Sanfilippo, 2016-07-12 16:04:52]

Description was changed from

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}
==========

to

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}
==========

[Stefano Sanfilippo, 2016-07-12 16:08:03]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 16:08:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stefano Sanfilippo, 2016-07-12 16:10:03]

Description was changed from

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

BUG=v8:4899
LOG=N
==========

[commit-bot: I haz the power, 2016-07-12 16:10:04]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 16:10:09]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8925)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8895)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/19280)

[Stefano Sanfilippo, 2016-07-12 16:19:27]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 16:19:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 16:42:06]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 16:42:08]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-12 16:42:49]

Description was changed from

==========
Implement .eh_frame writer and disassembler.

Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
to a EhFrameWriter and will attach unwinding information to the code
object when passed one.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/27d810e63b744b5b3d9aa28ff21413247773e6c2
Cr-Commit-Position: refs/heads/master@{#37683}
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-07-12 16:51:48]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

BUG=v8:4899
LOG=N
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot. 

  Removing that bit.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-07-13 10:11:41]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-13 10:11:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2023503002/#ps2070001
(title: "Rebase on master.")

[commit-bot: I haz the power, 2016-07-13 10:11:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 10:16:09]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot. 

  Removing that bit.

BUG=v8:4899
LOG=N
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot. 

  Removing that bit.

BUG=v8:4899
LOG=N
==========

[commit-bot: I haz the power, 2016-07-13 10:16:12]

Committed patchset #74 (id:2070001)

[commit-bot: I haz the power, 2016-07-13 10:16:21]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 10:18:15]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot. 

  Removing that bit.

BUG=v8:4899
LOG=N
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/b413f0ebe1a5dde016bfb94bb80bf872ebc24372
Cr-Commit-Position: refs/heads/master@{#37707}
==========

[commit-bot: I haz the power, 2016-07-13 10:18:18]

Patchset 74 (id:??) landed as
https://crrev.com/b413f0ebe1a5dde016bfb94bb80bf872ebc24372
Cr-Commit-Position: refs/heads/master@{#37707}

[Michael Hablich, 2016-07-13 14:15:13]

A revert of this CL (patchset #74 id:2070001) has been created in
https://codereview.chromium.org/2147883003/ by hablich@chromium.org.

The reason for reverting is: Blocks roll
https://codereview.chromium.org/2148743003/

See
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
for error message.

[Stefano Sanfilippo, 2016-07-13 14:26:08]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/b413f0ebe1a5dde016bfb94bb80bf872ebc24372
Cr-Commit-Position: refs/heads/master@{#37707}
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/b413f0ebe1a5dde016bfb94bb80bf872ebc24372
Cr-Commit-Position: refs/heads/master@{#37707}
==========

[Stefano Sanfilippo, 2016-07-13 14:37:52]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/b413f0ebe1a5dde016bfb94bb80bf872ebc24372
Cr-Commit-Position: refs/heads/master@{#37707}
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N
==========

[Stefano Sanfilippo, 2016-07-13 20:23:33]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 20:23:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 20:49:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-13 20:49:26]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-14 09:22:40]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-14 09:22:41]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2023503002/#ps2130001
(title: "AllStatic on Constants class.")

[commit-bot: I haz the power, 2016-07-14 09:22:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 09:26:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 09:26:05]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/19446)

[Stefano Sanfilippo, 2016-07-14 09:34:49]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 09:34:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 09:59:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 09:59:37]

Dry run: This issue passed the CQ dry run.

[Stefano Sanfilippo, 2016-07-14 10:26:31]

The CQ bit was checked by ssanfilippo@chromium.org

[Stefano Sanfilippo, 2016-07-14 10:26:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2023503002/#ps2150001
(title: "Rebase on master.")

[commit-bot: I haz the power, 2016-07-14 10:26:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 10:31:06]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N
==========

[commit-bot: I haz the power, 2016-07-14 10:31:09]

Committed patchset #78 (id:2150001)

[commit-bot: I haz the power, 2016-07-14 10:31:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 10:33:36]

Description was changed from

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N
==========

to

==========
Reland Implement .eh_frame writer and disassembler.

Original commit message:

  Also, CodeGenerator::MakeCodeEpilogue now accepts an optional pointer
  to a EhFrameWriter and will attach unwinding information to the code
  object when passed one.

Reason for reverting:

  The STATIC_CONST_MEMBER_DEFINITION in eh-frame-writer-unittest.cc
  causes a compiler error on V8 Win64 - clang buildbot.

  Removing that bit.

BUG=v8:4899
LOG=N

Committed: https://crrev.com/a91dc7cde2fe1f8b9945dc5eba5b9032b98511aa
Cr-Commit-Position: refs/heads/master@{#37754}
==========

[commit-bot: I haz the power, 2016-07-14 10:33:39]

Patchset 78 (id:??) landed as
https://crrev.com/a91dc7cde2fe1f8b9945dc5eba5b9032b98511aa
Cr-Commit-Position: refs/heads/master@{#37754}

[Michael Achenbach, 2016-07-14 15:07:29]

FYI: This fails on arm hardware:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/585

[Stefano Sanfilippo, 2016-07-14 15:20:54]

On 2016/07/14 15:07:29, Michael Achenbach wrote:
> FYI: This fails on arm hardware:
> https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/585

Fixed in https://codereview.chromium.org/2153443002, thanks.