Closed GoogleCodeExporter closed 9 years ago
Setting priority to high, as Chromium can't use CLD2 on Android or (presumably)
iOS until this is resolved.
Original comment by andrewha...@google.com
on 20 Mar 2014 at 3:12
PS, we should fix this for 64-bit while we're here, so replace "%4" in my
previous comment with "%8".
Original comment by andrewha...@google.com
on 20 Mar 2014 at 3:12
For posterity:
$ cat /proc/cpuinfo | grep ARM
Processor : ARMv7 Processor rev 10 (v7l)
I'm somewhat surprised by this, because the comments in port.h seem to imply
that ARMv7+ can do unaligned reads but they're just slow. Not the case
apparently, as the SIGBUS came from an ARM7 device. Anyhow, I guess we're not
using port.h here, and if we were it would (irritatingly) probably be broken as
well. But theoretically we could do UNALIGNED_LOAD32 for this as a short-term
fix while we get the right thing done. WDYT?
Original comment by andrewha...@google.com
on 20 Mar 2014 at 3:25
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.ht
ml
"To enable unaligned support, you must:
Clear the A bit, bit 1, of CP15 register 1 in your initialization code.
Set the U bit, bit 22, of CP15 register 1 in your initialization code."
Original comment by an...@chromium.org
on 20 Mar 2014 at 3:33
$ grep -R -5 "reinterpret_cast" . | grep -5 int32 >
~/Desktop/unaligned_landmines.txt
Original comment by andrewha...@google.com
on 20 Mar 2014 at 3:34
Attachments:
."/cldutil_shared.cc-// OVERSHOOTS up to 3 bytes"
What could possibly go wrong?
Original comment by an...@chromium.org
on 20 Mar 2014 at 3:38
It looks to me like we need to make repairs in (at least) these spots:
1. UTF8GenericScan in utf8statetable.cc
2. UTF8GenericScanFastAscii in utf8statetable.cc
3. BiHashV2 in cldutil_shared.cc
4. QuadHashV2Mix in cldutil_shared.cc
5. OctaHash40Mix in cldutil_shared.cc
The data loader has the same problem, but it should be immune because the
tables that are being loaded are already 64-bit aligned by the data extractor.
Hooray, forward thinking (thanks, Dick!). Wouldn't hurt to drop a note in
CLD2DynamicDataLoader::loadDataInternal to this effect, though. Dick, do you
see any other spots that need repairs?
Original comment by andrewha...@google.com
on 20 Mar 2014 at 3:41
Here's a proposed patch that fixes #1. I've tested this on ARM, and it works.
The unit tests pass, and I threw in some logging (which I took out before
generating the diff) that shows that the unit tests as-is very thoroughly
execute the range of values 0-7 as initial offsets. Hooray! This seems
reasonable to me, but Dick, PTAL.
Original comment by andrewha...@google.com
on 21 Mar 2014 at 6:09
Attachments:
Checking on the remaining functions:
1. UTF8GenericScan in utf8statetable.cc
Fixed by the patch posted here.
2. UTF8GenericScanFastAscii in utf8statetable.cc
Not called in any code that I can see. Can we just delete this?
3. BiHashV2 in cldutil_shared.cc
Uses the UNALIGNED_LOAD macro, and should therefore not have a problem.
4. QuadHashV2Mix in cldutil_shared.cc
Also uses the UNALIGNED_LOAD macro, and should therefore not have a problem.
5. OctaHash40Mix in cldutil_shared.cc
Also uses the UNALIGNED_LOAD macro, and should therefore not have a problem.
So I think all we actually need is the patch I've posted here, or a better
equivalent. Dick, does this sound good?
Original comment by andrewha...@google.com
on 21 Mar 2014 at 7:36
I had a long discussion offline with several subject matter experts about ARM7+
support for unaligned access, and the takeaway was that under certain
circumstances unaligned access on ARM will cause a SIGBUS or other issues even
if unaligned access is supported and enabled. Their recommendation was that NO
code should rely upon unaligned access if it's going to run on ARM.
The rest of the code here uses the UNALIGNED_LOAD but it's my impression from
the comments in utf8statetable.cc that we need to be fast, so I see no obvious
solution to fix this other than the proposed patch.
Original comment by andrewha...@google.com
on 27 Mar 2014 at 10:57
Dick made a fix to use UNALIGNED_LOAD:
https://code.google.com/p/cld2/source/detail?r=158
Sadly this doens't work. I believe the reason is that port.h incorrectly
assumes that we don't need aligned loads on ARM7:
https://code.google.com/p/cld2/source/browse/trunk/internal/port.h#57
The crash still occurs in exactly the same place with r158. We either need to
fix port.h or apply my patch to this method (and the others using
UNALIGNED_LOAD).
Original comment by andrewha...@google.com
on 2 Apr 2014 at 3:43
Here's a patch that fixes the problem for ARM. I'm dubious that we should be
doing this at all; we should probably just turn off unaligned access for all of
ARM and be done with it. Subsequently we can investigate the possibility of
incrementally killing off the use of UNALIGNED_LOAD in any critical-path code.
Original comment by andrewha...@google.com
on 2 Apr 2014 at 3:58
Attachments:
port.h patch submitted as r159 after consultation with Dick.
Original comment by andrewha...@google.com
on 3 Apr 2014 at 7:17
Original issue reported on code.google.com by
andrewha...@chromium.org
on 20 Mar 2014 at 3:11Attachments: