Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993653003:
Initial support for emitting unwinding information in perf jitdump. (Closed)
|
Created:
4 years, 7 months ago by Stefano Sanfilippo Modified:
4 years, 5 months ago CC:
v8-reviews_googlegroups.com, v8-mips-ports_googlegroups.com, v8-x87-ports_googlegroups.com, v8-ppc-ports_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionThis commit is the first step towards emitting unwinding information in
the .eh_frame format as part of the jitdump generated when
FLAG_perf_prof is enabled. The final goal is allowing precise unwinding
of callchains that include JITted code when profiling V8 using perf.
Unwinding information is stored in the body of code objects after the
code itself, prefixed with its length and aligned to a 8-byte boundary.
A boolean flag in the header signals its presence, resulting in zero
memory overhead when the generation of unwinding info is disabled or
no such information was attached to the code object.
A new jitdump record type (with id 4) is introduced for specifying
optional unwinding information for code load records. The EhFrameHdr
struct is also introduced, together with a constructor to initialise it
from the associated code object.
At this stage no unwinding information is written to the jitdump, but
the infrastructure for doing so is ready in place.
BUG=v8:4899
LOG=N
Committed: https://crrev.com/7d073b03c710b34f001fedd074f0ec9fbbaa5623
Cr-Commit-Position: refs/heads/master@{#37296}
Patch Set 1 : #Patch Set 2 : Rebase. #Patch Set 3 : Cleanup. #Patch Set 4 : Rebase. #
Total comments: 38
Patch Set 5 : Review. #Patch Set 6 : Fix missed header rename. #Patch Set 7 : Rebase. #Patch Set 8 : Turn kSizeOfCIE into EhFrameHdr::kCIESize. #Patch Set 9 : Format fixes. #Patch Set 10 : Fix imprecision in diagram . #
Total comments: 14
Patch Set 11 : Second review. #Patch Set 12 : Reword comment under diagram. #Patch Set 13 : Third review. #Patch Set 14 : Enable with --perf-prof-unwinding-info. #
Dependent Patchsets: Messages
Total messages: 162 (104 generated)
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
Some high-level drive-by comments. https://codereview.chromium.org/1993653003/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1993653003/diff/1/src/objects.cc#newcode13076 src/objects.cc:13076: // Pad to 2**3 boundary This should be behind the FLAG_perf_prof https://codereview.chromium.org/1993653003/diff/1/src/objects.h File src/objects.h (right): https://codereview.chromium.org/1993653003/diff/1/src/objects.h#newcode5251 src/objects.h:5251: static const int kUnwindingInfoSizeOffset = kInstructionSizeOffset + kIntSize; We should try and avoid having to add this if possible. Maybe we want to make profiling a compile time option so that we only incur this overhead for builds we want to profile.
Patchset #1 (id:1) has been deleted
On 2016/05/18 11:26:58, rmcilroy wrote: > Some high-level drive-by comments. > > https://codereview.chromium.org/1993653003/diff/20001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/1993653003/diff/20001/src/objects.cc#newcode1... > src/objects.cc:13076: // Pad to 2**3 boundary > This should be behind the FLAG_perf_prof Done. > https://codereview.chromium.org/1993653003/diff/20001/src/objects.h > File src/objects.h (right): > > https://codereview.chromium.org/1993653003/diff/20001/src/objects.h#newcode5251 > src/objects.h:5251: static const int kUnwindingInfoSizeOffset = > kInstructionSizeOffset + kIntSize; > We should try and avoid having to add this if possible. Maybe we want to make > profiling a compile time option so that we only incur this overhead for builds > we want to profile. That might work, but might also make profiling too much of a hassle, if you had to recompile or have a separate build for it. As discussed, I will try and see if we can avoid explicitly storing the size of the unwinding data. Thanks.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/7333)
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/80001
The CQ bit was unchecked by commit-bot@chromium.org
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/7336)
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...) v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/120001
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/160001
The CQ bit was unchecked by commit-bot@chromium.org
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/5979) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/5967) v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/1874)
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:180002) has been deleted
Patchset #2 (id:230001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/250001
The CQ bit was unchecked by commit-bot@chromium.org
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/5993) v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/5981)
Patchset #2 (id:250001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/290001
The CQ bit was unchecked by commit-bot@chromium.org
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/6002) v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/6017) v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/1897)
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/310001
Patchset #5 (id:350001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/370001
The CQ bit was unchecked by commit-bot@chromium.org
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/7598)
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/390001
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/450001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #10 (id:470001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/530001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/6347) v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/2259)
Patchset #12 (id:530001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/550001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
Patchset #12 (id:550001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/570001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/1...) v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/7796)
Patchset #1 (id:270001) has been deleted
Patchset #1 (id:290001) has been deleted
Patchset #1 (id:310001) has been deleted
Patchset #1 (id:330001) has been deleted
Patchset #1 (id:370001) has been deleted
Patchset #1 (id:390001) has been deleted
Patchset #1 (id:410001) has been deleted
Patchset #1 (id:430001) has been deleted
Patchset #1 (id:450001) has been deleted
Patchset #1 (id:490001) has been deleted
Patchset #1 (id:510001) has been deleted
Patchset #1 (id:570001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/610001
The CQ bit was unchecked by commit-bot@chromium.org
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/6367)
Patchset #1 (id:590001) has been deleted
Patchset #1 (id:610001) has been deleted
Patchset #2 (id:650001) has been deleted
Patchset #2 (id:670001) has been deleted
Description was changed from ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame, generation of DWARF directives will be introduced at a later point. ========== to ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame, generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ==========
Description was changed from ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame, generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ========== to ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame, generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ==========
Description was changed from ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame, generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ========== to ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame. Generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ==========
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993653003/630001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
Description was changed from ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame. Generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2010133005/. ========== to ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame. Generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2023503002/. ==========
Patchset #1 (id:630001) has been deleted
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/740001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PREVIEW ONLY Add .eh_frame records to jittudmp. For now, all code objects get the dummy eh_frame. Generation of DWARF directives is introduced in the followup https://codereview.chromium.org/2023503002/. ========== to ========== ------------------------------------------------------------------------ This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is emitted, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + jarin@chromium.org, titzer@chromium.org
Description was changed from ========== ------------------------------------------------------------------------ This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is emitted, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is emitted, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ==========
jarin@ PTAL to the jitdump part, especially to confirm whether it fits with what
is already in place.
rmcilroy@ PTAL to the integration in code objects, possibly to the assembler
boilerplate in arch-specific files too.
jarin@, rmcilroy@ also PTAL to unwinding-info.{cc,h}. I am not terribly sure
about the filename, so I would be grateful if you could comment in this regard
as well. I considered maybe renaming it to eh-frame.{cc,h}, but the same file
will also contain the DWARF directives writer at a certain point, which is
certainly related but not exclusive to a .eh_frame.
titzer@ PTAL to wasm-module.cc, hopefully just a rubber stamp.
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is emitted, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ==========
First pass of comments. https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1430: int unpadded_body_size = desc.unwinding_info_size > 0 Create a local variable for has_unwinding_info and use it here and below (and I think the check should be desc.unwinding_info != nullptr, with a corresponding DCHECK that desc.unwinding_info_size is zero if not. https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1430: int unpadded_body_size = desc.unwinding_info_size > 0 nit - just body_size (the RoundUp at the end isn't really padding, just alignment). https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1432: desc.unwinding_info_size + kInt64Size Do this addition in an if block (setting body_size to desc.instr_size outside the if, and then rounding up and adding in the if has_unwinding_info block. https://codereview.chromium.org/1993653003/diff/740001/src/globals.h File src/globals.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/globals.h#newcode558 src/globals.h:558: int unwinding_info_size; nit - move above Assembler* origin https://codereview.chromium.org/1993653003/diff/740001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects-inl.h#newc... src/objects-inl.h:6430: int field_offset = RoundUp(kHeaderSize + instruction_size(), kInt64Size); Factor out the code to get the offset of the unwinding info size offset into a helper function, and use it here and in unwinding_info_start() https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13471: void Code::CopyFrom(const CodeDesc& desc) { Please add a cctest which tests this function both with and without passing unwinding_info, checking that the code object is structured properly (no need to create valid assembly code, just pass some easy to check bytes in the CodeDesc and check they all go into the correct place. Also add a cctest that NewCode sizes code objects appropriately. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13478: // pad to 2**3 boundary please write number of bytes instead of 2**3. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13480: int padding_size = ((desc.instr_size + 7) & (~7)) - desc.instr_size; I don't think it's necessary to copy zero's into the padding here, just leave it as it is since it should never be read. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13484: CopyBytes(RoundUp(instruction_end(), kInt64Size), Just use set_unwinding_size https://codereview.chromium.org/1993653003/diff/740001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode4994 src/objects.h:4994: DECL_BOOLEAN_ACCESSORS(has_unwinding_info) nit - move down to near unwinding_info_start. Also, please add a simple diagram showing the layout of the unwinding info (including that the first address is the length). https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5136 src/objects.h:5136: // Returns the size of the instructions, padding, and relocation information. update comment. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5146 src/objects.h:5146: // Returns the address of the unwinding information. information, if any. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5149 src/objects.h:5149: // Returns the address past the end of the unwinding information. /s/past/right after/ https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc File src/perf-jit.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:331: int unwinding_info_size; nit - just use the field rather than having an extra local. https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:342: int padding_size = ((content_size + 7) & (~7)) - content_size; Don't use literals for '7'. Can you use RoundUp here instead? https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:349: PatchProcedureBoundariesInEhFrame(code); You don't have any code which actually writes to the unwinding_info here. It's impossible for me to tell what this is doing given I don't know how the unwinding info is structured yet. Could you remove this function (and anything relating to reading from unwind_info) from this CL and move it to one where it actually does something (and can be tested). Let's just make this CL about writing the dummy header and having the plumbing to pass unwind_info on code objects. https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc File src/unwinding-info.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc#... src/unwinding-info.cc:19: EhFrameHdr::EhFrameHdr(Code* code) { Can we have a test (cctest or unittest) for this class. https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc#... src/unwinding-info.cc:45: void PatchProcedureBoundariesInEhFrame(Code* code) { As mentioned above, please remove this from this CL for now, however when adding it back, I'm not convinced it should live here. It should be part of a class (maybe the Code class if it is messing with the code object? ) What is this actually doing, and why can't it be done as part of generating the unwinding info? https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.h File src/unwinding-info.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.h#n... src/unwinding-info.h:15: class EhFrameHdr final { This file should be eh_frame_header (and the the class named similarly, i.e., EhFrameHeader).
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/760001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #8 (id:820001) has been deleted
Patchset #5 (id:760001) has been deleted
Patchset #5 (id:780001) has been deleted
Patchset #5 (id:800001) has been deleted
Patchset #5 (id:840001) has been deleted
Patchset #5 (id:860001) has been deleted
Patchset #5 (id:880001) has been deleted
Patchset #5 (id:900001) has been deleted
Patchset #5 (id:920001) has been deleted
https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1430: int unpadded_body_size = desc.unwinding_info_size > 0 On 2016/06/21 13:47:43, rmcilroy wrote: > nit - just body_size (the RoundUp at the end isn't really padding, just > alignment). Done. https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1430: int unpadded_body_size = desc.unwinding_info_size > 0 On 2016/06/21 13:47:43, rmcilroy wrote: > nit - just body_size (the RoundUp at the end isn't really padding, just > alignment). Done. https://codereview.chromium.org/1993653003/diff/740001/src/factory.cc#newcode... src/factory.cc:1432: desc.unwinding_info_size + kInt64Size On 2016/06/21 13:47:43, rmcilroy wrote: > Do this addition in an if block (setting body_size to desc.instr_size outside > the if, and then rounding up and adding in the if has_unwinding_info block. Done. https://codereview.chromium.org/1993653003/diff/740001/src/globals.h File src/globals.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/globals.h#newcode558 src/globals.h:558: int unwinding_info_size; On 2016/06/21 13:47:43, rmcilroy wrote: > nit - move above Assembler* origin Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects-inl.h#newc... src/objects-inl.h:6430: int field_offset = RoundUp(kHeaderSize + instruction_size(), kInt64Size); On 2016/06/21 13:47:43, rmcilroy wrote: > Factor out the code to get the offset of the unwinding info size offset into a > helper function, and use it here and in unwinding_info_start() Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13471: void Code::CopyFrom(const CodeDesc& desc) { On 2016/06/21 13:47:43, rmcilroy wrote: > Please add a cctest which tests this function both with and without passing > unwinding_info, checking that the code object is structured properly (no need to > create valid assembly code, just pass some easy to check bytes in the CodeDesc > and check they all go into the correct place. Also add a cctest that NewCode > sizes code objects appropriately. Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13478: // pad to 2**3 boundary On 2016/06/21 13:47:43, rmcilroy wrote: > please write number of bytes instead of 2**3. Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13480: int padding_size = ((desc.instr_size + 7) & (~7)) - desc.instr_size; On 2016/06/21 13:47:43, rmcilroy wrote: > I don't think it's necessary to copy zero's into the padding here, just leave it > as it is since it should never be read. Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.cc#newcode... src/objects.cc:13484: CopyBytes(RoundUp(instruction_end(), kInt64Size), On 2016/06/21 13:47:43, rmcilroy wrote: > Just use set_unwinding_size Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode4994 src/objects.h:4994: DECL_BOOLEAN_ACCESSORS(has_unwinding_info) On 2016/06/21 13:47:44, rmcilroy wrote: > nit - move down to near unwinding_info_start. > > Also, please add a simple diagram showing the layout of the unwinding info > (including that the first address is the length). Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5136 src/objects.h:5136: // Returns the size of the instructions, padding, and relocation information. On 2016/06/21 13:47:44, rmcilroy wrote: > update comment. Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5146 src/objects.h:5146: // Returns the address of the unwinding information. On 2016/06/21 13:47:44, rmcilroy wrote: > information, if any. Done. https://codereview.chromium.org/1993653003/diff/740001/src/objects.h#newcode5149 src/objects.h:5149: // Returns the address past the end of the unwinding information. On 2016/06/21 13:47:44, rmcilroy wrote: > /s/past/right after/ Done. https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc File src/perf-jit.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:331: int unwinding_info_size; On 2016/06/21 13:47:44, rmcilroy wrote: > nit - just use the field rather than having an extra local. Done. https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:342: int padding_size = ((content_size + 7) & (~7)) - content_size; On 2016/06/21 13:47:44, rmcilroy wrote: > Don't use literals for '7'. Can you use RoundUp here instead? Done. https://codereview.chromium.org/1993653003/diff/740001/src/perf-jit.cc#newcod... src/perf-jit.cc:349: PatchProcedureBoundariesInEhFrame(code); On 2016/06/21 13:47:44, rmcilroy wrote: > You don't have any code which actually writes to the unwinding_info here. It's > impossible for me to tell what this is doing given I don't know how the > unwinding info is structured yet. Could you remove this function (and anything > relating to reading from unwind_info) from this CL and move it to one where it > actually does something (and can be tested). Let's just make this CL about > writing the dummy header and having the plumbing to pass unwind_info on code > objects. Agreed. Done. https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc File src/unwinding-info.cc (right): https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc#... src/unwinding-info.cc:19: EhFrameHdr::EhFrameHdr(Code* code) { On 2016/06/21 13:47:44, rmcilroy wrote: > Can we have a test (cctest or unittest) for this class. Done. I wrote a cctest because we need to pass through the factory to instantiate a Code object. https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.cc#... src/unwinding-info.cc:45: void PatchProcedureBoundariesInEhFrame(Code* code) { On 2016/06/21 13:47:44, rmcilroy wrote: > As mentioned above, please remove this from this CL for now, however when adding > it back, I'm not convinced it should live here. It should be part of a class > (maybe the Code class if it is messing with the code object? ) What is this > actually doing, and why can't it be done as part of generating the unwinding > info? Removed. The issue was that in some circumstances the reported size of code after compilation would not match the one seen at the moment of emitting the jitdump. This function calculates the code size and writes it to the placeholder in the CIE just before dumping it, at the latest moment possible. I am not sure where else this bit of code could go of if it should stay at all, let me first investigate the root cause of the issue. https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.h File src/unwinding-info.h (right): https://codereview.chromium.org/1993653003/diff/740001/src/unwinding-info.h#n... src/unwinding-info.h:15: class EhFrameHdr final { On 2016/06/21 13:47:44, rmcilroy wrote: > This file should be eh_frame_header (and the the class named similarly, i.e., > EhFrameHeader). .eh_frame_hdr is the name of the section and how it is referred to in all documentation, so I'd rather not expand the Hdr bit in the name. I agree on the first part, and I have renamed this file to eh-frame.h, instead of eh-frame-hdr.h, because other things related to the eh-frame might end up here. Same for the .cc file.
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object, and a function for patching the position and size of the JITted code before writing the record in the jitdump. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ==========
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993653003/960001
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is laid in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is put in place. BUG=v8:4899 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:980001) has been deleted
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is put in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is ready in place. BUG=v8:4899 LOG=N ==========
LGTM, thanks Stefano. Jaro, could you also take a look please? https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc File src/eh-frame.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:61: // which means we don't need to know the absolute value of E. I'm not sure what the "Instead..." sentence really means. Do we need to know that in this file? It doesn't look like you need to know the location of (E) here, is that right? https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:65: : code->instruction_size(); Looks like this code is in a couple of places (e.g., perf-jit). As a followup CL maybe we could add a helper function for this onto the code object. https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:76: offset_to_eh_frame_ = -(eh_frame_size + 4); // A -> D Where does the "+ 4" come from? Could you make the 4 a constant so that it is clear. https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc#newcod... src/factory.cc:1437: RoundUp(body_size, kInt64Size) + desc.unwinding_info_size + kInt64Size; What's the extra kInt64Size here for? Is that the size of the unwinding_info_size field? If so, please add a comment. https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... File test/cctest/test-eh-frame-hdr.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... test/cctest/test-eh-frame-hdr.cc:68: int N = EhFrameHdr::kCIESize; No need for this local, just use directly below
https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc File src/eh-frame.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:61: // which means we don't need to know the absolute value of E. On 2016/06/24 09:45:25, rmcilroy wrote: > I'm not sure what the "Instead..." sentence really means. Do we need to know > that in this file? It doesn't look like you need to know the location of (E) > here, is that right? The point is that we are guaranteed that there will never be padding between the .eh_frame and the .eh_frame_hdr, because (B) already has the correct alignment. Instead, E does not have any alignment requirement, so there might be padding between E and D. The next aligned address after E (i.e. the location of D) is RoundUp(E, 8) = RoundUp(F + <.text size>, 8), which means the distance F -> D is RoundUp(F + <.text size>, 8) - F. So we would need to know either E or F. However, since F is 8-byte aligned, we can simplify the calculation above as F + RoundUp(<.text size>, 8) - F = RoundUp(<.text size>, 8). Yes, we don't need to know the absolute values of E or F, but only because we are in a particular case, and I felt I needed to make that explicit. https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:65: : code->instruction_size(); On 2016/06/24 09:45:26, rmcilroy wrote: > Looks like this code is in a couple of places (e.g., perf-jit). As a followup CL > maybe we could add a helper function for this onto the code object. Sure thing, maybe let's coordinate with jarin@. https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:76: offset_to_eh_frame_ = -(eh_frame_size + 4); // A -> D On 2016/06/24 09:45:25, rmcilroy wrote: > Where does the "+ 4" come from? Could you make the 4 a constant so that it is > clear. + 4 is the size of <version> (1 byte) and <encoding specifiers> (3 byte) in the diagram above. Maybe I could write "4 byte" next to the A->B interval? https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc#newcod... src/factory.cc:1437: RoundUp(body_size, kInt64Size) + desc.unwinding_info_size + kInt64Size; On 2016/06/24 09:45:26, rmcilroy wrote: > What's the extra kInt64Size here for? Is that the size of the > unwinding_info_size field? If so, please add a comment. Done. https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... File test/cctest/test-eh-frame-hdr.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... test/cctest/test-eh-frame-hdr.cc:68: int N = EhFrameHdr::kCIESize; On 2016/06/24 09:45:26, rmcilroy wrote: > No need for this local, just use directly below Done.
On 2016/06/24 10:36:37, Stefano Sanfilippo wrote: > https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc > File src/eh-frame.cc (right): > > https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... > src/eh-frame.cc:61: // which means we don't need to know the absolute value of > E. > On 2016/06/24 09:45:25, rmcilroy wrote: > > I'm not sure what the "Instead..." sentence really means. Do we need to know > > that in this file? It doesn't look like you need to know the location of (E) > > here, is that right? > > The point is that we are guaranteed that there will never be padding between the > .eh_frame and the .eh_frame_hdr, because (B) already has the correct alignment. > Instead, E does not have any alignment requirement, so there might be padding > between E and D. The next aligned address after E (i.e. the location of D) is > RoundUp(E, 8) = RoundUp(F + <.text size>, 8), which means the distance F -> D is > RoundUp(F + <.text size>, 8) - F. So we would need to know either E or F. > However, since F is 8-byte aligned, we can simplify the calculation above as F + > RoundUp(<.text size>, 8) - F = RoundUp(<.text size>, 8). > > Yes, we don't need to know the absolute values of E or F, but only because we > are in a particular case, and I felt I needed to make that explicit. > > https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... > src/eh-frame.cc:65: : code->instruction_size(); > On 2016/06/24 09:45:26, rmcilroy wrote: > > Looks like this code is in a couple of places (e.g., perf-jit). As a followup > CL > > maybe we could add a helper function for this onto the code object. > > Sure thing, maybe let's coordinate with jarin@. > > https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... > src/eh-frame.cc:76: offset_to_eh_frame_ = -(eh_frame_size + 4); // A -> D > On 2016/06/24 09:45:25, rmcilroy wrote: > > Where does the "+ 4" come from? Could you make the 4 a constant so that it is > > clear. > > + 4 is the size of <version> (1 byte) and <encoding specifiers> (3 byte) in the > diagram above. Maybe I could write "4 byte" next to the A->B interval? > > https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc > File src/factory.cc (right): > > https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc#newcod... > src/factory.cc:1437: RoundUp(body_size, kInt64Size) + desc.unwinding_info_size + > kInt64Size; > On 2016/06/24 09:45:26, rmcilroy wrote: > > What's the extra kInt64Size here for? Is that the size of the > > unwinding_info_size field? If so, please add a comment. > > Done. > > https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... > File test/cctest/test-eh-frame-hdr.cc (right): > > https://codereview.chromium.org/1993653003/diff/1060001/test/cctest/test-eh-f... > test/cctest/test-eh-frame-hdr.cc:68: int N = EhFrameHdr::kCIESize; > On 2016/06/24 09:45:26, rmcilroy wrote: > > No need for this local, just use directly below > > Done. lgtm. (Since this is not under a flag, could you make sure it does not break --perf-prof with unpatched perf?)
Patchset #12 (id:1090001) has been deleted
https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc File src/eh-frame.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:76: offset_to_eh_frame_ = -(eh_frame_size + 4); // A -> D On 2016/06/24 10:36:36, Stefano Sanfilippo wrote: > On 2016/06/24 09:45:25, rmcilroy wrote: > > Where does the "+ 4" come from? Could you make the 4 a constant so that it is > > clear. > > + 4 is the size of <version> (1 byte) and <encoding specifiers> (3 byte) in the > diagram above. Maybe I could write "4 byte" next to the A->B interval? Why not just have two constants: kVersionSize and kEncodingSpecifiersSize, that would make this clear without additional comments. https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc#newcod... src/factory.cc:1437: RoundUp(body_size, kInt64Size) + desc.unwinding_info_size + kInt64Size; On 2016/06/24 10:36:37, Stefano Sanfilippo wrote: > On 2016/06/24 09:45:26, rmcilroy wrote: > > What's the extra kInt64Size here for? Is that the size of the > > unwinding_info_size field? If so, please add a comment. > > Done. There are multiple kInt64Size here so the comment doesn't make it clear it is the addition not the RoundUp. Could you just add a local unwinding_info_size_field_size which should make it clear.
https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc File src/eh-frame.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/eh-frame.cc#newco... src/eh-frame.cc:76: offset_to_eh_frame_ = -(eh_frame_size + 4); // A -> D On 2016/06/24 12:55:25, rmcilroy wrote: > On 2016/06/24 10:36:36, Stefano Sanfilippo wrote: > > On 2016/06/24 09:45:25, rmcilroy wrote: > > > Where does the "+ 4" come from? Could you make the 4 a constant so that it > is > > > clear. > > > > + 4 is the size of <version> (1 byte) and <encoding specifiers> (3 byte) in > the > > diagram above. Maybe I could write "4 byte" next to the A->B interval? > > Why not just have two constants: kVersionSize and kEncodingSpecifiersSize, that > would make this clear without additional comments. Done. https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/1993653003/diff/1060001/src/factory.cc#newcod... src/factory.cc:1437: RoundUp(body_size, kInt64Size) + desc.unwinding_info_size + kInt64Size; On 2016/06/24 12:55:25, rmcilroy wrote: > On 2016/06/24 10:36:37, Stefano Sanfilippo wrote: > > On 2016/06/24 09:45:26, rmcilroy wrote: > > > What's the extra kInt64Size here for? Is that the size of the > > > unwinding_info_size field? If so, please add a comment. > > > > Done. > > There are multiple kInt64Size here so the comment doesn't make it clear it is > the addition not the RoundUp. Could you just add a local > unwinding_info_size_field_size which should make it clear. Done.
On 2016/06/24 12:02:14, Jarin wrote: > lgtm. (Since this is not under a flag, could you make sure it does not break > --perf-prof with unpatched perf?) I don't know if it is the intended behaviour of perf inject or a bug, but in presence of records with an unknown id it skips the whole jitdump, not just the record. So at least for now we do need a separate flag for the --perf-prof feature to keep working. I used --perf-prof-unwinding-info, wdyt?
ssanfilippo@chromium.org changed reviewers: + ahaas@chromium.org
@ahaas could you please take a look at src/wasm/wasm-module.cc?
On 2016/06/24 13:30:31, Stefano Sanfilippo wrote: > On 2016/06/24 12:02:14, Jarin wrote: > > lgtm. (Since this is not under a flag, could you make sure it does not break > > --perf-prof with unpatched perf?) > > I don't know if it is the intended behaviour of perf inject or a bug, but in > presence of records with an unknown id it skips the whole jitdump, not just the > record. So at least for now we do need a separate flag for the --perf-prof > feature to keep working. I used --perf-prof-unwinding-info, wdyt? Sounds good, thanks.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ssanfilippo@chromium.org changed reviewers: + bradnelson@google.com
On 2016/06/24 at 15:34:24, commit-bot wrote: > Dry run: This issue passed the CQ dry run. wasm lgtm
The CQ bit was checked by ssanfilippo@chromium.org
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/1993653003/#ps1150001 (title: "Enable with --perf-prof-unwinding-info.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is ready in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is ready in place. BUG=v8:4899 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #14 (id:1150001)
Message was sent while issue was closed.
Description was changed from ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is ready in place. BUG=v8:4899 LOG=N ========== to ========== This commit is the first step towards emitting unwinding information in the .eh_frame format as part of the jitdump generated when FLAG_perf_prof is enabled. The final goal is allowing precise unwinding of callchains that include JITted code when profiling V8 using perf. Unwinding information is stored in the body of code objects after the code itself, prefixed with its length and aligned to a 8-byte boundary. A boolean flag in the header signals its presence, resulting in zero memory overhead when the generation of unwinding info is disabled or no such information was attached to the code object. A new jitdump record type (with id 4) is introduced for specifying optional unwinding information for code load records. The EhFrameHdr struct is also introduced, together with a constructor to initialise it from the associated code object. At this stage no unwinding information is written to the jitdump, but the infrastructure for doing so is ready in place. BUG=v8:4899 LOG=N Committed: https://crrev.com/7d073b03c710b34f001fedd074f0ec9fbbaa5623 Cr-Commit-Position: refs/heads/master@{#37296} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7d073b03c710b34f001fedd074f0ec9fbbaa5623 Cr-Commit-Position: refs/heads/master@{#37296} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
