Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1885033005:
Portable Linux perf map formatting. (Closed)
|
Created:
4 years, 8 months ago by Stefano Sanfilippo Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionPortable Linux perf map formatting.
Linux perf expects hex literals without a leading 0x, while some
implementations of printf might prepend one when using the %p format
for pointers, leading to wrongly formatted JIT symbols maps.
Instead, use V8PRIxPTR format string and cast pointer to uintpr_t,
since we have control over the exact output format of integers.
LOG=N
Committed: https://crrev.com/15defcc424935fdde4d4690d19a6045fdac273ca
Cr-Commit-Position: refs/heads/master@{#35571}
Patch Set 1 #Patch Set 2 : Mac OS X fix (cinttypes is in tr1 there). #
Total comments: 2
Patch Set 3 : Use V8PRIxPTR. #Patch Set 4 : Comment added. #Messages
Total messages: 47 (24 generated)
Description was changed from ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some platforms might prepend one when using %p printf format for pointers. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format for integers. BUG=v8:4280 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/1885033005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885033005/1
Description was changed from ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format and cast pointer to uintpr_t, since we have control over the exact output format for integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Fix Linux perf map format. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JITted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JITted symbol maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JITted symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/123)
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/1885033005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885033005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ssanfilippo@chromium.org changed reviewers: + jarin@chromium.org
Since a couple days ago, d8 started emitting wrongly formatted perf JITted symbols maps, in particular, the symbol base address had an extra 0x at the beginning. I didn't dig into the issue any further, as we probably want to print that pointer value as uintptr_t anyway for portability. PTAL.
Description was changed from ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JITted symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact format of integers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. BUG=v8:4280 LOG=N ========== to ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ==========
https://codereview.chromium.org/1885033005/diff/20001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/1885033005/diff/20001/src/log.cc#newcode288 src/log.cc:288: base::OS::FPrint(perf_output_handle_, "%" PRIxPTR " %x %.*s\n", Couldn't you use V8PRIxPTR?
rmcilroy@chromium.org changed reviewers: + jfb@chromium.org, rmcilroy@chromium.org
Adding JFB who made the change to %p in https://codereview.chromium.org/1872203005.
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/1885033005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885033005/40001
https://codereview.chromium.org/1885033005/diff/20001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/1885033005/diff/20001/src/log.cc#newcode288 src/log.cc:288: base::OS::FPrint(perf_output_handle_, "%" PRIxPTR " %x %.*s\n", On 2016/04/14 12:16:53, Jarin wrote: > Couldn't you use V8PRIxPTR? Done.
Description was changed from ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ========== to ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use V8PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ==========
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/14 12:46:03, Jarin wrote: > lgtm Great, thanks. I'll wait until this evening to see if jfb has any comment.
On 2016/04/14 13:33:45, Stefano Sanfilippo wrote: > On 2016/04/14 12:46:03, Jarin wrote: > > lgtm > > Great, thanks. I'll wait until this evening to see if jfb has any comment. I was thinking, maybe I should add a comment to explain why we took time to cast a pointer to uintptr_t and print it as integer, to prevent someone else doing the same change again in the future?
On 2016/04/14 13:40:31, Stefano Sanfilippo wrote: > On 2016/04/14 13:33:45, Stefano Sanfilippo wrote: > > On 2016/04/14 12:46:03, Jarin wrote: > > > lgtm > > > > Great, thanks. I'll wait until this evening to see if jfb has any comment. > > I was thinking, maybe I should add a comment to explain why we took time to cast > a pointer to uintptr_t and print it as integer, to prevent someone else doing > the same change again in the future? Done, could you please take another look?
still lgtm. 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/1885033005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885033005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/14 12:21:40, rmcilroy wrote: > Adding JFB who made the change to %p in > https://codereview.chromium.org/1872203005. Sorry for the breakage. Is there a test for this? If not, could you add one?
On 2016/04/14 20:55:48, JF wrote: > On 2016/04/14 12:21:40, rmcilroy wrote: > > Adding JFB who made the change to %p in > > https://codereview.chromium.org/1872203005. > > Sorry for the breakage. Is there a test for this? If not, could you add one? I think the test would not really pay for itself - it would require some serious refactoring and I am also worried the test would be too brittle. The comment should be enough here.
On 2016/04/18 10:01:00, Jarin wrote: > On 2016/04/14 20:55:48, JF wrote: > > On 2016/04/14 12:21:40, rmcilroy wrote: > > > Adding JFB who made the change to %p in > > > https://codereview.chromium.org/1872203005. > > > > Sorry for the breakage. Is there a test for this? If not, could you add one? > > I think the test would not really pay for itself - it would require some serious > refactoring and I am also worried the test would be too brittle. > > The comment should be enough here. Yep, that was my feeling as well. The issue is about a change in the printf format string, as before this change it wasn't really clear why we were handling a pointer as if it were a uint64. JFB saw a pointer and more than legitimately replaced %llx with %p. I would have done the same, hadn't I known why someone had put a %llx there. Now it should be clear enough that casting to uintptr is the way that line is meant to be :) Cheers.
The CQ bit was checked by ssanfilippo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1885033005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885033005/60001
Message was sent while issue was closed.
Description was changed from ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use V8PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ========== to ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use V8PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use V8PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N ========== to ========== Portable Linux perf map formatting. Linux perf expects hex literals without a leading 0x, while some implementations of printf might prepend one when using the %p format for pointers, leading to wrongly formatted JIT symbols maps. Instead, use V8PRIxPTR format string and cast pointer to uintpr_t, since we have control over the exact output format of integers. LOG=N Committed: https://crrev.com/15defcc424935fdde4d4690d19a6045fdac273ca Cr-Commit-Position: refs/heads/master@{#35571} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/15defcc424935fdde4d4690d19a6045fdac273ca Cr-Commit-Position: refs/heads/master@{#35571} |
