Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2032753002:
Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest (Closed)
DescriptionReplace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest
On GCC -O2, the comparison operator between realloc's result
and the uintptr_t 0x42 is optimized away.
0x42 is not aligned, so the compiler assumes realloc cannot return this value.
BUG=616860
BUG=internal b/28801531
Committed: https://crrev.com/2dcb3524b6daaa812c2af10baf4b1f4b1a6a25ba
Cr-Commit-Position: refs/heads/master@{#397526}
Patch Set 1 #Patch Set 2 : Switch cast order of EXPECT_EQ #Patch Set 3 : Replace 0x42 with 0x420 #Messages
Total messages: 27 (11 generated)
Description was changed from ========== Add volatile to AllocatorShimTest's ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and an intereger is optimized away. BUG=internal b/28801531 ========== to ========== Add volatile to AllocatorShimTest's ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and an intereger is optimized away. BUG=internal b/28801531 ==========
bcf@chromium.org changed reviewers: + primiano@chromium.org, slan@chromium.org, wzhong@chromium.org
Description was changed from ========== Add volatile to AllocatorShimTest's ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and an intereger is optimized away. BUG=internal b/28801531 ========== to ========== Add volatile to AllocatorShimTest's ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and an integer is optimized away. BUG=internal b/28801531 ==========
If aliasing is the problem, how about using a union without volatile?
On 2016/06/01 22:34:46, wzhong wrote: > If aliasing is the problem, how about using a union without volatile? I tried union without volatile, but it doesn't work in this case.
I am wondering if static_cast is more appropriate than reinterpret_cast here.
On 2016/06/01 22:40:21, wzhong wrote: > I am wondering if static_cast is more appropriate than reinterpret_cast here. You can't static_cast between uintptr_t and ptr.
On 2016/06/01 22:47:58, bcf wrote: > On 2016/06/01 22:40:21, wzhong wrote: > > I am wondering if static_cast is more appropriate than reinterpret_cast here. > > You can't static_cast between uintptr_t and ptr. I'm off corp and cannot see the internal bug Can somebody explain how the compiler is supposed to assume that the return value of the realloc is 0x42 (So the check is going to be true) and not a different value? I wonder if this is just because size==1, so the compiler assumes that a realloc with size=1 is always going to succeed without moving the ptr. I imagine it won't optimize if you pass 1024 as size? (Assuming the problem here is really the eq check)
On 2016/06/01 22:57:08, Primiano Tucci wrote: > On 2016/06/01 22:47:58, bcf wrote: > > On 2016/06/01 22:40:21, wzhong wrote: > > > I am wondering if static_cast is more appropriate than reinterpret_cast > here. > > > > You can't static_cast between uintptr_t and ptr. > > I'm off corp and cannot see the internal bug > Can somebody explain how the compiler is supposed to assume that the return > value of the realloc is 0x42 (So the check is going to be true) and not a > different value? > I wonder if this is just because size==1, so the compiler assumes that a realloc > with size=1 is always going to succeed without moving the ptr. > I imagine it won't optimize if you pass 1024 as size? (Assuming the problem here > is really the eq check) It looks like the compiler assumes the condition is never true, so it skips the equality check and leads to this comical result: ../../base/allocator/allocator_shim_unittest.cc:153: Failure Value of: reinterpret_cast<uintptr_t>(res) Actual: 66 Expected: 0x42u Which is: 66 I found that if we instead reinterpret cast 0x42u to void* the test passes. That's probably a better solution
Description was changed from ========== Add volatile to AllocatorShimTest's ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and an integer is optimized away. BUG=internal b/28801531 ========== to ========== Switch cast order in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. BUG=internal b/28801531 ==========
lgtm
On 2016/06/02 00:38:09, wzhong wrote: > lgtm OK I think I figured this out (thanks to hwennborg@). The issue here is that the compiler expects realloc to never return a non-aligned value. So it assumes the result of comparing the result with 0x42 is always false. Repro case here: http://pastebin.com/WTvCDzzC Please s/0x42/0x420/ :)
Description was changed from ========== Switch cast order in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. BUG=internal b/28801531 ========== to ========== Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. 0x42 is not aligned, so the compiler assumes realloc cannot return this value. BUG=616860 BUG=internal b/28801531 ==========
On 2016/06/02 16:31:50, Primiano Tucci wrote: > On 2016/06/02 00:38:09, wzhong wrote: > > lgtm > > OK I think I figured this out (thanks to hwennborg@). > The issue here is that the compiler expects realloc to never return a > non-aligned value. So it assumes the result of comparing the result with 0x42 is > always false. > Repro case here: > http://pastebin.com/WTvCDzzC > > Please s/0x42/0x420/ :) This is implemented. PTAL.
On 2016/06/02 20:08:18, bcf wrote: > On 2016/06/02 16:31:50, Primiano Tucci wrote: > > On 2016/06/02 00:38:09, wzhong wrote: > > > lgtm > > > > OK I think I figured this out (thanks to hwennborg@). > > The issue here is that the compiler expects realloc to never return a > > non-aligned value. So it assumes the result of comparing the result with 0x42 > is > > always false. > > Repro case here: > > http://pastebin.com/WTvCDzzC > > > > Please s/0x42/0x420/ :) > > This is implemented. PTAL. Shame, I quite liked 0x42. LGTM
The CQ bit was checked by bcf@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/2032753002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2032753002/40001
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 bcf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org Link to the patchset: https://codereview.chromium.org/2032753002/#ps40001 (title: "Replace 0x42 with 0x420")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2032753002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2032753002/40001
Message was sent while issue was closed.
Description was changed from ========== Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. 0x42 is not aligned, so the compiler assumes realloc cannot return this value. BUG=616860 BUG=internal b/28801531 ========== to ========== Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. 0x42 is not aligned, so the compiler assumes realloc cannot return this value. BUG=616860 BUG=internal b/28801531 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. 0x42 is not aligned, so the compiler assumes realloc cannot return this value. BUG=616860 BUG=internal b/28801531 ========== to ========== Replace 0x42 with 0x420 in ThreadDelegateForNewHandlerTest On GCC -O2, the comparison operator between realloc's result and the uintptr_t 0x42 is optimized away. 0x42 is not aligned, so the compiler assumes realloc cannot return this value. BUG=616860 BUG=internal b/28801531 Committed: https://crrev.com/2dcb3524b6daaa812c2af10baf4b1f4b1a6a25ba Cr-Commit-Position: refs/heads/master@{#397526} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2dcb3524b6daaa812c2af10baf4b1f4b1a6a25ba Cr-Commit-Position: refs/heads/master@{#397526} |
