Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469723002:
Corrected implementation of TwoByteExternalBufferedStream. (Closed)
|
Created:
4 years, 1 month ago by predrag.rudic Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionFix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access'
Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions.
Committed: https://crrev.com/f04a9b49369fd1921fbae38f3783cd639c5fb2eb
Cr-Commit-Position: refs/heads/master@{#40722}
Patch Set 1 #Patch Set 2 : Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access on MIPS' #
Total comments: 6
Patch Set 3 : Corrected implementation of TwoByteExternalBufferedStream #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Corrected implementation of TwoByteExternalBufferedStream. ========== to ========== Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access' Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions. ==========
predrag.rudic@imgtec.com changed reviewers: + nikolaos@chromium.org, vogelheim@chromium.org
PTAL
lgtm
On 2016/11/02 11:07:47, vogelheim wrote:
> lgtm
Additional changes.
PTAL
Description:
Changed test caused TwoByteExternalBufferedStream to fail so additional
fixes were nedded. Test is now improved because chunks now begin on
alternating even and odd addresses. Also TwoByteExternalBufferedStream
is not necessary for ARM architecture.
lgtm Looks good, but please consider the comments below. Thanks. https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... File src/parsing/scanner-character-streams.cc (right): https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... src/parsing/scanner-character-streams.cc:718: bool odd_start = true; Hmm. I note that you quite carefully wrote this to avoid copying when a buffer does *not* start on an odd byte boundary. So, if the intent is that FillBuffer will only be called for odd chunk positions, then please add a DCHECK for this condition. https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... src/parsing/scanner-character-streams.cc:756: 2 * kBufferSize - odd_start * !middle_of_chunk, I'm not so sure this 'odd_start * middle_of_chunk' business (here, and 3x below) is super readable. The reader has to figure out that both are bools, and then map bool->int under multiplication to understand that you're executing... &&. And && is surely the more orthodox operator when operating on two bools. How about you give this expression a name? (size_t start_offset = odd_start && !middle_of_chunk; or something like that) (Or possibly two names, if you need one for middle_of_chunk and one for !middle_of_chunk.) https://codereview.chromium.org/2469723002/diff/20001/test/cctest/parsing/tes... File test/cctest/parsing/test-scanner-streams.cc (right): https://codereview.chromium.org/2469723002/diff/20001/test/cctest/parsing/tes... test/cctest/parsing/test-scanner-streams.cc:31: i += chunk_size, chunk_size = chunk_size * 2 + 1) { Why? I assume you want to have a case where a chunk starts odd but ends even? Since that's a case the remainder of the CL happens to work on... The problem I'm seeing is that most test strings are rather small... 7 or 9 characters. If we progress chunk sizes x*2+1, we'll only have at most three chunks. Maybe we should just do chunk_size++. That'd give us chunks [0..1), [1..3), [3..6), [6..9)...
https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... File src/parsing/scanner-character-streams.cc (right): https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... src/parsing/scanner-character-streams.cc:718: bool odd_start = true; On 2016/11/02 17:29:18, vogelheim wrote: > Hmm. I note that you quite carefully wrote this to avoid copying when a buffer > does *not* start on an odd byte boundary. So, if the intent is that FillBuffer > will only be called for odd chunk positions, then please add a DCHECK for this > condition. > Done. https://codereview.chromium.org/2469723002/diff/20001/src/parsing/scanner-cha... src/parsing/scanner-character-streams.cc:756: 2 * kBufferSize - odd_start * !middle_of_chunk, On 2016/11/02 17:29:18, vogelheim wrote: > I'm not so sure this 'odd_start * middle_of_chunk' business (here, and 3x below) > is super readable. The reader has to figure out that both are bools, and then > map bool->int under multiplication to understand that you're executing... &&. > And && is surely the more orthodox operator when operating on two bools. > > How about you give this expression a name? > (size_t start_offset = odd_start && !middle_of_chunk; or something like that) > (Or possibly two names, if you need one for middle_of_chunk and one for > !middle_of_chunk.) Done. https://codereview.chromium.org/2469723002/diff/20001/test/cctest/parsing/tes... File test/cctest/parsing/test-scanner-streams.cc (right): https://codereview.chromium.org/2469723002/diff/20001/test/cctest/parsing/tes... test/cctest/parsing/test-scanner-streams.cc:31: i += chunk_size, chunk_size = chunk_size * 2 + 1) { On 2016/11/02 17:29:18, vogelheim wrote: > Why? > > I assume you want to have a case where a chunk starts odd but ends even? Since > that's a case the remainder of the CL happens to work on... > > The problem I'm seeing is that most test strings are rather small... 7 or 9 > characters. If we progress chunk sizes x*2+1, we'll only have at most three > chunks. Maybe we should just do chunk_size++. That'd give us chunks [0..1), > [1..3), [3..6), [6..9)... Yes, I wanted to have a chunk which starts on odd address but ends on even and vice versa. chunk_size++ is ok.
The CQ bit was checked by predrag.rudic@imgtec.com 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 predrag.rudic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2469723002/#ps40001 (title: "Corrected implementation of TwoByteExternalBufferedStream")
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 ========== Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access' Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions. ========== to ========== Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access' Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access' Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions. ========== to ========== Fix 'MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access' Removed a wrong condition test in TwoByteExternalBufferedStream. This changed fixes errors that may occur under some conditions. Committed: https://crrev.com/f04a9b49369fd1921fbae38f3783cd639c5fb2eb Cr-Commit-Position: refs/heads/master@{#40722} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f04a9b49369fd1921fbae38f3783cd639c5fb2eb Cr-Commit-Position: refs/heads/master@{#40722} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
