Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043233003:
Prevent sign extension for pointer trace values (Closed)
|
Created:
4 years, 6 months ago by Kunihiko Sakamoto Modified:
4 years, 6 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent sign extension for pointer trace values
This makes 32-bit pointers serialize like "0xdeadbeef", not
"0xffffffffdeadbeef".
BUG=618221
Committed: https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9
Cr-Commit-Position: refs/heads/master@{#399347}
Patch Set 1 : #
Total comments: 2
Messages
Total messages: 21 (10 generated)
Description was changed from ========== WIP BUG= ========== to ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialized like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ==========
Description was changed from ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialized like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ========== to ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialize like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ==========
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043233003/20001
ksakamoto@chromium.org changed reviewers: + caseq@chromium.org, nduca@chromium.org
nduca@ for base/trace_event/ caseq@ for inspector/ What do you think? Please see http://crbug.com/618221 for context.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
primiano@chromium.org changed reviewers: + primiano@chromium.org
LGTM, 32 bit pointers should not be sign extended. Thanks for the fix.
https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:264: static_cast<uint64_t>(reinterpret_cast<uintptr_t>(value.as_pointer))); can we drop the inner one and do a reinterpret_cast<uint64_t>(value.as_pointer) straight away?
Thanks for the review. https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:264: static_cast<uint64_t>(reinterpret_cast<uintptr_t>(value.as_pointer))); On 2016/06/09 16:59:13, caseq wrote: > can we drop the inner one and do a reinterpret_cast<uint64_t>(value.as_pointer) > straight away? Unfortunately, that doesn't prevent sign extension. http://stackoverflow.com/questions/22239752/convert-32-bit-pointer-to-64-bit-... Looks like 2-step conversion is necessary.
On 2016/06/10 05:19:24, Kunihiko Sakamoto wrote: > Thanks for the review. > > https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... > File base/trace_event/trace_event_impl.cc (right): > > https://codereview.chromium.org/2043233003/diff/20001/base/trace_event/trace_... > base/trace_event/trace_event_impl.cc:264: > static_cast<uint64_t>(reinterpret_cast<uintptr_t>(value.as_pointer))); > On 2016/06/09 16:59:13, caseq wrote: > > can we drop the inner one and do a > reinterpret_cast<uint64_t>(value.as_pointer) > > straight away? > > Unfortunately, that doesn't prevent sign extension. > http://stackoverflow.com/questions/22239752/convert-32-bit-pointer-to-64-bit-... > Looks like 2-step conversion is necessary. Ouch! lgtm.
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043233003/20001
Message was sent while issue was closed.
Description was changed from ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialize like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ========== to ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialize like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialize like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 ========== to ========== Prevent sign extension for pointer trace values This makes 32-bit pointers serialize like "0xdeadbeef", not "0xffffffffdeadbeef". BUG=618221 Committed: https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9 Cr-Commit-Position: refs/heads/master@{#399347} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9 Cr-Commit-Position: refs/heads/master@{#399347} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
