SIGBUS on ARM32 in utf8statetable.cc:517 #11

ghost · 2015-07-27T23:37:04Z

Originally reported on Google Code with ID 11

I'm trying to get CLD2 working on ARM32 inside of Chromium, cross-compiling from a linux
x64 host to arm32. The library loads properly, but the following crash occurs when
calling DetectLanguageSummary:

Program received signal SIGBUS, Bus error.
#0  CLD2::UTF8GenericScan (st=0x61a82104, str=<optimized out>, bytes_consumed=0x5f00f88c)
    at ../../third_party/cld_2/src/internal/utf8statetable.cc:518

I'll attach the full trace as a file. Well, minus the Chromium bits. Anyhow, the problem
appears to be with this snippet of code in utf8statetable.cc:

  // Do fast for groups of 8 identity bytes.
  // This covers a lot of 7-bit ASCII ~8x faster than the 1-byte loop,
  // including slowing slightly on cr/lf/ht
  //----------------------------
  const uint8* Tbl2 = &st->fast_state[0];
  uint32 losub = st->losub;
  uint32 hiadd = st->hiadd;
  while (src < srclimit8) {
    uint32 s0123 = (reinterpret_cast<const uint32 *>(src))[0];
    uint32 s4567 = (reinterpret_cast<const uint32 *>(src))[1];
    src += 8;


Inspecting the pointers in the debugger during the crash, and looking at the "src"
variable, seems to reveal the problem:
(gdb) p src
$32 = (
    const CLD2::uint8 *) 0x58de4bee "\n\n\n百度一下\n地图贴吧视频图片hao123\n新闻应用音乐文库更多\n小说游戏下载\n把百度放到桌面上，
搜索最方便\n触屏版极速版\nBaidu 京ICP证030173号"

Specifically, src is located at 0x58de4bee. Since this isn't a 4-byte (32-bit) aligned
address, the SIGBUS presumably comes from trying to read it as a uint32*. Many thanks
to aberent@chromium.org and anton@chromium.org for the help in diagnosing this, I was
a bit lost in the weeds looking at my dynamic data changes, which turn out to be completely
unrelated (this happens with and without dynamic data mode).

The suggested workaround for this case is to %4 the address and do a one-off scan of
the first 0-3 bytes (as necessary), and then descend into the fast loop; the concern
is that there may be other places in CLD2 that have similar behavior and might be time
bombs. It might be a good idea to add some memory churning code to the unit tests,
and then start running the unit tests themselves on ARM to further diagnose other problems
like this that might arise.

Reported by andrewhayden@chromium.org on 2014-03-20 15:11:07

- _Attachment: [crashstack.txt](https://storage.googleapis.com/google-code-attachments/cld2/issue-11/comment-0/crashstack.txt)_

The text was updated successfully, but these errors were encountered:

ghost · 2015-07-27T23:37:04Z

Setting priority to high, as Chromium can't use CLD2 on Android or (presumably) iOS
until this is resolved.

Reported by andrewhayden@google.com on 2014-03-20 15:12:01

Labels added: Priority-High
Labels removed: Priority-Medium

ghost · 2015-07-27T23:37:05Z

PS, we should fix this for 64-bit while we're here, so replace "%4" in my previous comment
with "%8".

Reported by andrewhayden@google.com on 2014-03-20 15:12:54

ghost · 2015-07-27T23:37:06Z

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?

Reported by andrewhayden@google.com on 2014-03-20 15:25:52

ghost · 2015-07-27T23:37:06Z

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CIHCGCFD.html
"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."

Reported by anton@chromium.org on 2014-03-20 15:33:13

ghost · 2015-07-27T23:37:07Z

$ grep -R -5 "reinterpret_cast" . | grep -5 int32 > ~/Desktop/unaligned_landmines.txt

Reported by andrewhayden@google.com on 2014-03-20 15:34:52

- _Attachment: [unaligned_landmines.txt](https://storage.googleapis.com/google-code-attachments/cld2/issue-11/comment-5/unaligned_landmines.txt)_

ghost · 2015-07-27T23:37:08Z

."/cldutil_shared.cc-// OVERSHOOTS up to 3 bytes"

What could possibly go wrong?

Reported by anton@chromium.org on 2014-03-20 15:38:56

ghost · 2015-07-27T23:37:09Z

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?

Reported by andrewhayden@google.com on 2014-03-20 15:41:34

ghost · 2015-07-27T23:37:09Z

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.

Reported by andrewhayden@google.com on 2014-03-21 18:09:35

- _Attachment: [cld2_issue11_patch1.diff](https://storage.googleapis.com/google-code-attachments/cld2/issue-11/comment-8/cld2_issue11_patch1.diff)_

ghost · 2015-07-27T23:37:10Z

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?

Reported by andrewhayden@google.com on 2014-03-21 19:36:51

ghost · 2015-07-27T23:37:11Z

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.

Reported by andrewhayden@google.com on 2014-03-27 10:57:41

ghost · 2015-07-27T23:37:11Z

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).

Reported by andrewhayden@google.com on 2014-04-02 15:43:45

ghost · 2015-07-27T23:37:12Z

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.

Reported by andrewhayden@google.com on 2014-04-02 15:58:16

- _Attachment: [port.h_patch_cld2_issue_11](https://storage.googleapis.com/google-code-attachments/cld2/issue-11/comment-12/port.h_patch_cld2_issue_11)_

ghost · 2015-07-27T23:37:13Z

port.h patch submitted as r159 after consultation with Dick.

Reported by andrewhayden@google.com on 2014-04-03 07:17:53

Status changed: Fixed

ghost self-assigned this Jul 27, 2015

ghost added Type-Defect Priority-High labels Jul 27, 2015

ghost closed this as completed Jul 27, 2015

xiaoluqiaoba mentioned this issue Aug 1, 2019

crash SIGSEGV(SEGV_MAPERR) in utf8statetable.cc:384 #62

Open

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIGBUS on ARM32 in utf8statetable.cc:517 #11

SIGBUS on ARM32 in utf8statetable.cc:517 #11

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

SIGBUS on ARM32 in utf8statetable.cc:517 #11

SIGBUS on ARM32 in utf8statetable.cc:517 #11

Comments

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015

ghost commented Jul 27, 2015