Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277443009:
Populate relocation information correctly for RelocatableInt32Constants. (Closed)
DescriptionPopulate relocation information correctly for RelocatableInt32Constants.
BUG=v8:5304
R=ahaas@chromium.org, titzer@chromium.org
Committed: https://crrev.com/53cb7e5ffc58dcc5de0dc9a46a4f38fd4d80b4b6
Cr-Commit-Position: refs/heads/master@{#39112}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add cctest #Patch Set 3 : Rebase + fix serializer test flakiness #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by gdeepti@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.
Description was changed from ========== Populate relocation information correctly for RelocatableInt32Constants. BUG=chromium:5304 R=ahaas@chromium.org, titzer@chromium.org ========== to ========== Populate relocation information correctly for RelocatableInt32Constants. BUG=v8:5304 R=ahaas@chromium.org, titzer@chromium.org ==========
Thanks, lgtm
https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... File src/compiler/x64/code-generator-x64.cc (right): https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... src/compiler/x64/code-generator-x64.cc:2461: if (src.rmode() == RelocInfo::WASM_MEMORY_SIZE_REFERENCE) { Why not if src.rmode() != RelocInfo::kNone?
On 2016/08/26 08:21:10, titzer wrote: > https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... > File src/compiler/x64/code-generator-x64.cc (right): > > https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... > src/compiler/x64/code-generator-x64.cc:2461: if (src.rmode() == > RelocInfo::WASM_MEMORY_SIZE_REFERENCE) { > Why not if src.rmode() != RelocInfo::kNone? Can you add a (cc)test that triggered the bug you fix here?
The CQ bit was checked by gdeepti@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...
Added cctest. https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... File src/compiler/x64/code-generator-x64.cc (right): https://codereview.chromium.org/2277443009/diff/1/src/compiler/x64/code-gener... src/compiler/x64/code-generator-x64.cc:2461: if (src.rmode() == RelocInfo::WASM_MEMORY_SIZE_REFERENCE) { On 2016/08/26 08:21:10, titzer wrote: > Why not if src.rmode() != RelocInfo::kNone? My rationale for this is that it should be a deliberate choice between movl and movq based on rmode. In this case a movl should suffice because memory size will not exceed 32 bits, but for regular memory references there should be no such restriction on x64 and should fall into the if statement above because after patching the memory reference can exceed the space movl allocates. I've found that missing RelocInfo is easier to debug than bugs in generated code because enough space hasn't been allocated. The other option is to have the default be a movq with non zero rmode instead of movl that swallows the rmode - I can change it to that if that is better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
On 2016/08/31 15:54:21, gdeepti wrote: > PTAL lgtm
The CQ bit was checked by gdeepti@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.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2277443009/#ps40001 (title: "Rebase + fix serializer test flakiness")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Populate relocation information correctly for RelocatableInt32Constants. BUG=v8:5304 R=ahaas@chromium.org, titzer@chromium.org ========== to ========== Populate relocation information correctly for RelocatableInt32Constants. BUG=v8:5304 R=ahaas@chromium.org, titzer@chromium.org Committed: https://crrev.com/53cb7e5ffc58dcc5de0dc9a46a4f38fd4d80b4b6 Cr-Commit-Position: refs/heads/master@{#39112} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/53cb7e5ffc58dcc5de0dc9a46a4f38fd4d80b4b6 Cr-Commit-Position: refs/heads/master@{#39112} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
