Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415093002:
MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access (Closed)
|
Created:
4 years, 2 months ago by predrag.rudic Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com, v8-mips-ports_googlegrpups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS: Fix Utf16CharacterStream scanner crash due to missaligned access
TEST=ctest/test-scanner-streams/CharacterStreams
BUG=
Committed: https://crrev.com/bc43d6fe78975b9cecb19721c0cc235125bd0319
Cr-Commit-Position: refs/heads/master@{#40637}
Patch Set 1 #Patch Set 2 : Implement TwoByteExternalBufferedStream for unaligned access #
Total comments: 1
Patch Set 3 : Added comments and conditional compilation for TwoByteExternalBufferedStream. #Messages
Total messages: 36 (26 generated)
predrag.rudic@imgtec.com changed reviewers: + nikolaos@chromium.org, vogelheim@chromium.org
PTAL
On 2016/10/13 14:48:34, predrag.rudic wrote: > PTAL This is most probably correct. However, I don't understand why ReadUnalignedValue should be used here. In particular, I don't understand: (1) Why the pointer is not aligned in the first place. If it's something related to the way that some particular test in cctest/parsing/test-scanner-streams.cc is written, which provides an unaligned pointer, I suggest that the test be fixed instead. (2) What the macro does --- presumably it has some performance benefit for some architectures? Furthermore, I think that the commit log is a bit misleading. This patch does not fix cctest/(parsing/test-scanner-streams/)CharacterStream. It patches the scanner implementation.
Description was changed from ========== MIPS: Fix cctest/CharacterStream due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= ========== to ========== MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= ==========
On 2016/10/13 18:18:55, nickie wrote: > On 2016/10/13 14:48:34, predrag.rudic wrote: > > PTAL > > This is most probably correct. However, I don't understand why > ReadUnalignedValue should be used here. In particular, I don't understand: > > (1) Why the pointer is not aligned in the first place. If it's something > related to the way that some particular test in > cctest/parsing/test-scanner-streams.cc is written, which provides an unaligned > pointer, I suggest that the test be fixed instead. > > (2) What the macro does --- presumably it has some performance benefit for some > architectures? > > Furthermore, I think that the commit log is a bit misleading. This patch does > not fix cctest/(parsing/test-scanner-streams/)CharacterStream. It patches the > scanner implementation. Hello, Pointer is not aligned because in file scanner-character-streams.cc line 625 there is adding one to aligned uint8 pointer in case there is odd start, and this value is assigned to 16-bit pointer. In MIPS architecture, it is not allowed to access 16-bit data on unaligned address.So when this pointer is dereferenced in test-scanner-streams.cc, it causes bus error. In order to remedy that problem I added inline function that, for MIPS architecture, copies 16 bits to aligned address.
On 2016/10/14 07:23:55, predrag.rudic wrote: > On 2016/10/13 18:18:55, nickie wrote: > > On 2016/10/13 14:48:34, predrag.rudic wrote: > > > PTAL > > > > This is most probably correct. However, I don't understand why > > ReadUnalignedValue should be used here. In particular, I don't understand: > > > > (1) Why the pointer is not aligned in the first place. If it's something > > related to the way that some particular test in > > cctest/parsing/test-scanner-streams.cc is written, which provides an unaligned > > pointer, I suggest that the test be fixed instead. > > > > (2) What the macro does --- presumably it has some performance benefit for > some > > architectures? > > > > Furthermore, I think that the commit log is a bit misleading. This patch does > > not fix cctest/(parsing/test-scanner-streams/)CharacterStream. It patches the > > scanner implementation. > > Hello, > > Pointer is not aligned because in file scanner-character-streams.cc line 625 > there > is adding one to aligned uint8 pointer in case there is odd start, and this > value > is assigned to 16-bit pointer. In MIPS architecture, it is not > allowed to access 16-bit data on unaligned address.So when this pointer is > dereferenced in test-scanner-streams.cc, it causes bus error. > > In order to remedy that problem I added inline function that, for MIPS > architecture, > copies 16 bits to aligned address. Ah, I understand the problem now, but I'm not very happy with the solution. We had very carefully refactored that part so that the fast path would be just a memory access and a pointer increment... changing this to be a function call which, depending on platform, calls another function, and then rely on the fact that all of these would usually be inlined and just do a memory access is... a bit obtuse. So... as I understand this: The real problem is that in TwoByteExternalStreamingStream we're receiving a byte buffer and we're re-interpreting it as a ucs-2 buffer, possibly at odd offsets (if the sum of the previous buffers had odd length). If so, my preference would be to not do that. That is: Do not use that implementation for those platforms, and instead have an additional, simpler TwoByteExternalStreamingStream implementation much like GenericStringUtf16CharacterStream, which simply copies the data. The copy target would then always be aligned. The implementation of ScannerStream::For would then have an #ifdef to select between either impl. That's unfortunately a bit more work on your side; but I'd really prefer to keep the rather critical Scanner::Advance method as it currently us.
PTAL
lgtm (w/ the one nitpick) Many thanks for taking the extra time! https://codereview.chromium.org/2415093002/diff/20001/src/parsing/scanner-cha... File src/parsing/scanner-character-streams.cc (right): https://codereview.chromium.org/2415093002/diff/20001/src/parsing/scanner-cha... src/parsing/scanner-character-streams.cc:811: #if !(V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_ARM) Please add the same #if around the two implementations, since we'll only ever use one (and the other is dead code).
The CQ bit was checked by vogelheim@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: The author predrag.rudic@imgtec.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
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: The author predrag.rudic@imgtec.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
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: The author predrag.rudic@imgtec.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
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: The author predrag.rudic@imgtec.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
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/2415093002/#ps40001 (title: "Added comments and conditional compilation for 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 ========== MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= ========== to ========== MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= ========== to ========== MIPS: Fix Utf16CharacterStream scanner crash due to missaligned access TEST=ctest/test-scanner-streams/CharacterStreams BUG= Committed: https://crrev.com/bc43d6fe78975b9cecb19721c0cc235125bd0319 Cr-Commit-Position: refs/heads/master@{#40637} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bc43d6fe78975b9cecb19721c0cc235125bd0319 Cr-Commit-Position: refs/heads/master@{#40637} |
