1782141 - src/mfbt/SIMD.cpp:26:10: runtime error: load of misaligned address 0x30c3486c1d29 for type 'const unsigned short', which requires 2 byte alignment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tyson Smith [:tsmith]]

This was found by enabling the alignment check in UBSan and launching the browser. This type of issue can impact performance and create inconsistencies across platforms, architectures and optimization levels.

Found with m-c 20220728-c067b27a1842

To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="alignment"

src/mfbt/SIMD.cpp:26:10: runtime error: load of misaligned address 0x30c3486c1d29 for type 'const unsigned short', which requires 2 byte alignment
0x30c3486c1d29: note: pointer points here
 00 00 00  61 64 64 6f 6e 73 2e 6d  61 6e 61 67 65 72 00 00  a6 fc e7 60 00 00 00 00  18 02 00 00 1c
              ^ 
    #0 0x55d82b3b1969 in unsigned short mozilla::GetAs<unsigned short>(unsigned long) src/mfbt/SIMD.cpp:26:10
    #1 0x55d82b3a4b13 in unsigned char const* mozilla::TwoByteLoop<unsigned char>(unsigned long, unsigned long, unsigned char, unsigned char) src/mfbt/SIMD.cpp:317:9
    #2 0x55d82b3b275f in unsigned char const* mozilla::FindTwoInBuffer<unsigned char>(unsigned char const*, unsigned char, unsigned char, unsigned long) src/mfbt/SIMD.cpp:368:12
    #3 0x55d82b3a4d1f in mozilla::SIMD::memchr2x8(char const*, char, char, unsigned long) src/mfbt/SIMD.cpp:445:7
    #4 0x7f29d34f82a6 in int Matcher<ManualCmp<unsigned char, unsigned char>, unsigned char, unsigned char>(unsigned char const*, unsigned int, unsigned char const*, unsigned int) src/js/src/builtin/String.cpp:1774:24
    #5 0x7f29d33f7578 in int StringMatch<unsigned char, unsigned char>(unsigned char const*, unsigned int, unsigned char const*, unsigned int) src/js/src/builtin/String.cpp:1856:16
    #6 0x7f29d33f7578 in StringMatch(JSLinearString*, JSLinearString*, unsigned int) src/js/src/builtin/String.cpp:1871:15
    #7 0x7f29d34001f7 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) src/js/src/builtin/String.cpp:2211:24
    #8 0x2742da4bff24  (<unknown module>)

Comment 1

[Mike Hommey [:glandium]]

AFAIK, the caller is supposed to ensure alignment, so this would be in js/

Comment 2

[Jan de Mooij [:jandem]]

NI Doug because likely from bug 1776013.

Comment 3

[Mike Hommey [:glandium]]

Actually, this is a completely new API that was added to mfbt via a bug in js... 2 weeks ago.

Comment 4

[Mike Hommey [:glandium]]

I'm going to have to ask you to move this API out of MFBT. We shouldn't add more .cpp files to MFBT, which is supposed to be headers-only (with exceptions, but we don't add more). Mozglue might be a good place (and SSE.cpp used to be there...). I'll add that both MFBT and mozglue have their own bugzilla components. Please don't add code in there without filing a bug in the appropriate component.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1776013

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1782141 - Move SIMD/SSE files to mozglue r?iain

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1782141 - Replace explicit unaligned load with implicit r?iain

This should result in the same binary. Tested on Godbolt and both Clang
and GCC produced identical code here.

Depends on D153264

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd07c6d3d9fe
Move SIMD/SSE files to mozglue r=iain
https://hg.mozilla.org/integration/autoland/rev/81b72f893608
Replace explicit unaligned load with implicit r=iain

Comment 9

[Cristian Tuns]

Backed out for causing spidermonkey bustages on dependent.js

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/jit-test/tests/latin1/dependent.js:90:13 Error: Assertion failed: got "\u1200\u1200\u1200\u1200abcdefg\u1200", expected "\u1200\u1200\u1200\u1200\u1200"

Failure log for cppunit fail : https://treeherder.mozilla.org/logviewer?job_id=385875044&repo=autoland

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/923fd339d1a3
Move SIMD/SSE files to mozglue r=iain
https://hg.mozilla.org/integration/autoland/rev/61d73e75dc79
Replace explicit unaligned load with implicit r=iain

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/923fd339d1a3
https://hg.mozilla.org/mozilla-central/rev/61d73e75dc79

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:dthayer, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox104 to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Alex Thayer [:alexical] (she/her)]

I think it's quite unlikely that this actually results in a binary difference. This code is only enabled for x86/x64 builds and I don't think the optimizer has a lot of leeway here to mess things up in the name of optimization, and any mess-ups should be well enough covered by tests.