Bad load offset emitted for i64.load on Emscripten 3.1.66+

[Twinklebear]

Summary

I started seeing a crash in our application on Emscripten 3.1.66+ when loading a GLTF file with Magnum and traced it down to what appears to be a bad compile output by Emscripten, where on 3.1.66+ an i64.load instruction is emitted with offset=4294967280 causing the app to crash w/ an out of bounds memory access.

I was able to make an isolated reproducer case repo which you can find here: https://github.com/Twinklebear/repro-gltf-compile-bug , which includes minimal code to reproduce the issue, instructions on building it, and where the bad output assembly can be found. In short, part of the lambda here: https://github.com/mosra/magnum-plugins/blob/master/src/MagnumPlugins/GltfImporter/GltfImporter.cpp#L3349 on Emscripten 3.1.65 compiles to:

local.get $p2
i32.load offset=12
local.get $p2
i32.const 4
i32.sub
i32.load
i32.eq
if $I26
  local.get $l4
  local.get $p2
  i64.load align=4
  local.tee $l8
  i64.store offset=504
  local.get $l4
  local.get $p2
  i32.const 16
  i32.sub
  i64.load align=4
  local.tee $l18
  i64.store offset=880
  local.get $l4
  local.get $l8
  i64.store offset=448
  local.get $l4
  local.get $l18
  i64.store offset=440
  i32.const 0
  local.set $l6
  local.get $l4
  i32.const 448
  i32.add
  local.get $l4
  i32.const 440
  i32.add
  call $Corrade::Containers::operator==_Corrade::Containers::BasicStringView<char_const>__Corrade::Containers::BasicStringView<char_const>_
  br_if $B24
end

However, starting with 3.1.66, the second i64.load here starts being emitted
with an offset of 4294967280 max, causing a memory out of bounds error to occur.

local.get $p2
i32.load offset=12
local.get $p2
i32.const 4
i32.sub
i32.load
i32.eq
if $I26
  local.get $l4
  local.get $p2
  i64.load align=4
  local.tee $l8
  i64.store offset=504
  local.get $l4
  local.get $p2
  i64.load offset=4294967280 align=4
  local.tee $l18
  i64.store offset=880
  local.get $l4
  local.get $l8
  i64.store offset=448
  local.get $l4
  local.get $l18
  i64.store offset=440
  i32.const 0
  local.set $l6
  local.get $l4
  i32.const 448
  i32.add
  local.get $l4
  i32.const 440
  i32.add
  call $Corrade::Containers::operator==_Corrade::Containers::BasicStringView<char_const>__Corrade::Containers::BasicStringView<char_const>_
  br_if $B24
end

The repro case repo should have everything needed to reproduce the good/bad outputs: https://github.com/Twinklebear/repro-gltf-compile-bug but please let me know if you run into issues trying to repro the issue or other questions.

Basic Info

Version of emscripten/emsdk:
Emscripten 3.1.66 and later

Failing command line in full:
See repro repo for build config: https://github.com/Twinklebear/repro-gltf-compile-bug/blob/main/src/CMakeLists.txt , but not sure the flags are relevant as this is a bad ASM output.

Full link command and output with -v appended:
See repro repo for build config: https://github.com/Twinklebear/repro-gltf-compile-bug/blob/main/src/CMakeLists.txt , but not sure the flags are relevant as this is a bad ASM output.

[kripken]

Thanks for the repro case, it does look like enough to see the problem. But it isn't a minimal executable testcase, which ideally would be a single cpp file that can just be run, which is practical for one of the devs to investigate in detail.

Given that, I would suggest that you first bisect here, since that may be the fastest way to find what changed in a regression like this:

https://emscripten.org/docs/contributing/developers_guide.html#bisecting

Bisecting that way can find the exact commit (or range, hopefully short) where the behavior broke for you, and often that is enough to fully understand the problem. Like any bisection, it will take a limited number of steps, so it can be quite fast.

[sbc100]

One useful thing to test would be to see if this is an llvm codegen issue or perhaps a binaryen change. The bisect should tell you this, but you can also try with -sERROR_ON_WASM_CHANGES_AFTER_LINK in order for disable binaryen/wasm-opt from running. That way the final output will be the code that llvm itself emitted.

[Twinklebear]

Sounds good, I'll try bisecting to find the bad commit range this weekend.

Unfortunately it's a bit of a big repro case since it occurred in this library we use which isn't so easy to split out to just make a single file that can compile and repro b/c of the interdependencies in it.

[sbc100]

Sounds good, I'll try bisecting to find the bad commit range this weekend.

Unfortunately it's a bit of a big repro case since it occurred in this library we use which isn't so easy to split out to just make a single file that can compile and repro b/c of the interdependencies in it.

If the miscompile happens in GltfImporter.cpp it should be possible to verify this by running wasm-validate on the object file (GltfImporter.o). If that is indeed the case the the object file is bad then you should be able to bisect using a single compile+validate step which should be quite fast (and also something for which you can share the input bitcode or preprocessed source).

[Twinklebear]

I bisected between 3.1.65 and 3.1.66, this is the bad commit:

c472a2cca828ad2cdc373cebf24c2d0c8f4b8517 is the first bad commit
commit c472a2cca828ad2cdc373cebf24c2d0c8f4b8517
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date:   Thu Sep 5 17:51:56 2024 +0000

    Roll llvm-project from 0ba78182b975 to bedac64d36dc (39 revisions)

    https://chromium.googlesource.com/external/github.com/llvm/llvm-project.git/+log/0ba78182b975..bedac64d36dc

    2024-09-05 alex@alexrp.com [llvm][SystemZ] Recognize `@GOTENT` modifier in assembler. (#107038)
    2024-09-05 alex@alexrp.com [llvm][SystemZ] Fix parsing of `.cfi_undefined` with percent-less registers. (#107032)
    2024-09-05 me@m-sp.org [mlir][Transforms] Dialect conversion: Make materializations optional (#107109)
    2024-09-05 vporpodas@google.com [SandboxIR][Bench] Initial patch for performance tracking (#107296)
    2024-09-05 Abhina.Sreeskantharajan@ibm.com [NFC][SystemZ][z/OS] Rename autoconversion-related functions to be less generic (#107399)
    2024-09-05 flo@fhahn.com [LV] Remove over-aggressive assert from 3fe6a064f15c.
    2024-09-05 michalt@google.com [RISCV][SiFive7] Change `Latency` of VCIX to the default (#106497)
    2024-09-05 sander.desmalen@arm.com [AArch64] Remove redundant COPY from loadRegFromStackSlot (#107396)
    2024-09-05 alangford@apple.com [lldb] Fix a format string in ClangASTSource (#107325)
    2024-09-05 aheejin@gmail.com [WebAssembly] Simplify a switch-case in CFGStackify (NFC) (#107360)
    2024-09-05 tgymnich@icloud.com [DXIL] Add sign intrinsic part 2 (#101988)
    2024-09-05 ldionne.2@gmail.com [libc++][modules] Get rid of <cstddef> dependency in __datasizeof (#107394)
    2024-09-05 ldionne.2@gmail.com [libc++][NFC] Increase consistency for namespace closing comments
    2024-09-05 jalalonde@fb.com [LLDB][Minidump] Extend the minidump x86_64 registers to include fs_base and gs_base (#106767)
    2024-09-05 michaelrj@google.com [libc] Correct the entrypoints list for ARM/darwin (#107331)
    2024-09-05 luke@igalia.com [RISCV] Update V0Defs after moving Src in peepholes (#107359)
    2024-09-05 thakis@chromium.org [lld/mac] Allow -segprot having stricter initprot than maxprot on mac (#107269)
    2024-09-05 paul.walker@arm.com [LLVM][CodeGen][SVE] Implement nxvbf16 fpextend to nxvf32/nxvf64. (#107253)
    2024-09-05 rjoshi@nvidia.com [NFC][Support] Refactor FormatVariadic code. (#106610)
    2024-09-05 Matthew.Arsenault@amd.com DAG: Handle lowering unordered compare with inf (#100378)
    2024-09-05 jay.foad@amd.com [IR] Check parameters of target extension types on construction (#107268)
    2024-09-05 llvm-dev@redking.me.uk [X86] Fold scalar_to_vector(funnel(x,y,imm)) -> funnel(scalar_to_vector(x),scalar_to_vector(y),imm)
    2024-09-05 craig.topper@sifive.com [RISCV] Don't cost Fmv for Zfinx in isFPImmLegal. (#107361)
    2024-09-05 mital@mitalashok.co.uk [Clang][Parser] Accept P2741R3 (static_assert with user-generated message) in C++11 as an extension (#102044)
    2024-09-05 kazu@google.com [CGOpenMPRuntime] Avoid repeated hash lookups (NFC) (#107358)
    2024-09-05 kazu@google.com [Analysis] Avoid repeated hash lookups (NFC) (#107357)
    2024-09-05 kazu@google.com [ARM] Avoid repeated hash lookups (NFC) (#107356)
    2024-09-05 ekeane@nvidia.com Fix handling of FP-classify where the last arg fails to convert
    2024-09-05 llvm-dev@redking.me.uk [X86] Fold scalar_to_vector(shift(x,imm)) -> vshift(scalar_to_vector(x),imm)
    2024-09-05 hari.limaye@arm.com Reland "[clang] Add nuw attribute to GEPs (#105496)" (#107257)
    2024-09-05 me@antoniofrighetto.com [SCEV] BECount to zero if `((-C + (C smax %x)) /u %x), C > 0` holds
    2024-09-05 tom.eccles@arm.com [flang] Warn when F128 is unsupported (#102147) (#106957)
    2024-09-05 preames@rivosinc.com [SLP] Enable reordering for non-power-of-two vectors (#106638)
    2024-09-05 jonathan_roelofs@apple.com [llvm][AArch64] Improve the cost model for i128 div's (#107306)
    2024-09-05 mtrofin@google.com [mlgo] Fix test post - #106744
    2024-09-05 martin@martin.st [libc++][ci] Add a test configuration with an incomplete sysroot (#107089)
    2024-09-05 npopov@redhat.com [ConstantRangeTest] Set APInt signed flags where needed (NFC)
    2024-09-05 npopov@redhat.com [ConstantRange] Perform increment on APInt (NFC)
    2024-09-05 jacek@codeweavers.com [LLD][COFF][NFC] Use is64Bit in Baserel::getDefaultType. (#107378)

    If this roll has caused a breakage, revert this CL and stop the roller
    using the controls here:
    https://autoroll.skia.org/r/llvm-project-emscripten-releases
    Please CC dschuff@google.com,wasm-waterfall@grotations.appspotmail.com on the revert to ensure that a human
    is aware of the problem.

    To report a problem with the AutoRoller itself, please file a bug:
    https://issues.skia.org/issues/new?component=1389291&template=1850622

    Documentation for the AutoRoller is here:
    https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

    Tbr: wasm-waterfall@grotations.appspotmail.com
    Change-Id: Ib785e291dd38c2ad7292b5bc8ff41915ac2d8aa9
    Reviewed-on: https://chromium-review.googlesource.com/c/emscripten-releases/+/5840113
    Bot-Commit: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
    Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>

 DEPS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I've also added some notes on building a smaller subset of the repo (just the GltfImporter target) and a script that greps through for the bad load offset, which I used during the bisection

[kripken]

@Twinklebear Hmm, interesting. There is one wasm result there,

llvm/llvm-project#107360

@aheejin that PR should be NFC - any chance there is an issue there? Reading the code it looks ok but (1) I am not sure if being Delegate and being a Terminator are disjoint, and (2) I am not very familiar with tablegen.

If there is nothing obvious there, then this might be an LLVM regression in one of the other commits. Bisecting between those might be necessary.

[kripken]

@Twinklebear actually, a quick thing that could rule out llvm/llvm-project#107360 is whether your project uses C++ exceptions - does it?

[sbc100]

Thanks @Twinklebear. The two commits that look like possible suspects from that list are:

2024-09-05 aheejin@gmail.com [WebAssembly] Simplify a switch-case in CFGStackify (NFC) (#107360)
2024-09-05 hari.limaye@arm.com Reland "[clang] Add nuw attribute to GEPs (#105496)" (#107257)

If possible can you attach the preprocessed source code for GltfImporter? You can do this but by using either --save-temps or -E I believe.

[aheejin]

I haven't checked this bug, but we had a similar bug report that turned out to be
#22552 (comment), which was fixed by llvm/llvm-project@940f892.

[Twinklebear]

I think the Magnum library does use exceptions, we don't enable exceptions in WASM (we don't use -fwasm-exceptions) but also don't explicitly pass -fno-exceptions (sometimes one is thrown and we want to debug but it's fine to just crash for debugging). If I build with -fno-exceptions, I still see the bad load on c472a2cca828ad2cdc373cebf24c2d0c8f4b8517.

Here are the outputs of save-temps for GltfImporter.cpp on c472a2cca828ad2cdc373cebf24c2d0c8f4b8517 and 3.1.64:

• c472a2cca828ad2cdc373cebf24c2d0c8f4b8517: c472a2cca828ad2cdc373cebf24c2d0c8f4b8517_temps.tar.gz
• 3.1.64: 3.1.64_temps.tar.gz

[aheejin]

@Twinklebear As I said in #22794 (comment), I don't think this is related to exceptions. The fix has been already landed in LLVM and included in Emscripten 3.1.67. Can you try upgrading Emscripten to 3.1.67+?

[Twinklebear]

I do still see the bad load on 3.1.67, here's the save-temps outputs for that:
3.1.67_temps.tar.gz, I just checked 3.1.71 and it also occurs there, here's the temps files on 3.1.71: 3.1.71_temps.tar.gz

[sbc100]

I was able to reproduce this issue.

$ em++ -std=c++20 -c 3.1.71_temps/GltfImporter.ii
$ wasm-objdump -d GltfImporter.o | grep i64.load.*4294967280
 01c5b1: 29 02 f0 ff ff ff 0f       |                   i64.load 2 4294967280

[sbc100]

The bad load seem to occur in the Magnum::Trade::GltfImporter::doMesh(unsigned int, unsigned int) function. It also only occurs at -O2 and above.

I bisect this down to llvm/llvm-project#107257 which was a reland of llvm/llvm-project#105496