[PATCH] D64612: [WebAssembly] i32.const operands should be signed

Fri Jul 12 10:56:48 PDT 2019

tlively marked 3 inline comments as done.
tlively added inline comments.

================
Comment at: lld/test/wasm/data-segments.ll:44
 @b = hidden global [8 x i8] c"goodbye\00", align 1
- at c = hidden global [9 x i8] c"whatever\00", align 1
+ at c = hidden global [10000 x i8] zeroinitializer, align 1
 @d = hidden global i32 42, align 4
----------------
sbc100 wrote:
> How does this change test the logic change?   Seems like 10000 isn't large enough to trigger the binaryen crash?
1000 encoded as unsigned leb in hex is 904E, bu as a signed leb it is 90CE00. The need to leave room for a sign bit bumps the encoding up to use the next byte.

================
Comment at: lld/test/wasm/data-segments.ll:91
 ; PASSIVE-NEXT:        Locals:          []
-; PASSIVE-NEXT:        Body:            4180084100411CFC080000FC0900419C084100410DFC080100FC09010B
+; PASSIVE-NEXT:        Body:            41800841004114FC080000FC090041940841004190CE00FC080100FC090141A4D6004100410DFC080200FC09020B
 ; PASSIVE-NEXT:  - Type:            DATA
----------------
aheejin wrote:
> It'd be better if we can get disassembly, but it seems to be not fully working yet
+1

================
Comment at: lld/wasm/Writer.cpp:661
         writeU8(os, WASM_OPCODE_I32_CONST, "i32.const");
-        writeUleb128(os, s->startVA, "destination address");
+        writeSleb128(os, uint32_t(s->startVA), "destination address");
         // source segment offset
----------------
aheejin wrote:
> Isn't [[ https://github.com/llvm/llvm-project/blob/c46d78d1b7a06aad11f8810279271224fc8466af/lld/wasm/OutputSegment.h#L39 | `OutputSegment::startVA` ]] already a `uint32_t`?
Good catch, this was left over from some previous experimentation.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64612/new/

https://reviews.llvm.org/D64612