[WebAssembly] Properly calculate function body offset, and write I32 values.

[yurydelendik]

Summary

• Gets function size field from right location
• Writes I32 values during compression

[sbc100]

Does these fix the issues you had with you large binary?

[yurydelendik]

Yes, is does. Large binary demo I have now runs without issue.

[ruiu]

I think a patch like this needs a test to demonstrate that a patch actually fixes an issue.

[yurydelendik]

The test at D46416 tests this -- it was a luck that the unset buffer (Buf) contains mostly right values.

[ruiu]

Was the test in that change was failing on your machine? If it passed (if it happened to work), you still need to change the test, so that it fails without this patch and passes with this patch.

[sbc100]

In D47204#1108297, @ruiu wrote:

Was the test in that change was failing on your machine? If it passed (if it happened to work), you still need to change the test, so that it fails without this patch and passes with this patch.

There are two parts to this change. The write32le() is there for correctness but really could be replaces with llvm_unreachable since this relocation type should never occur in the code section today.

The second fix which was to pass the correct argument to decodeULEB128() is hard to write a test for. It a used of uninitialized memory which should really have been caught be msan (do we run that on any bots?, perhaps not with wasm enabled?). Its hard to write a failing test because of this and I'm not sure unit tests are the best way to check these types of failures.

[ruiu]

I'm not sure if I fully understand the problem, but don't you have a binary that didn't link correctly without this patch, no? I thought that you could reduce it to create a test.

As to the code that could be llvm_unreacahble, I think it should be llvm_unreacahble. If some path is not expected to run, we shouldn't write code for it.

[sbc100]

Dead code removal: https://reviews.llvm.org/D47226

[sbc100]

In term of the uninitialized memory issue, let me see if I can restate so that we can come to the same understanding.

The code is calling decodeULEB128(Buf, &Count); to decode at LEB stored in Buf but its ignoring the result. All it cares about is the number of bytes consumed (Count). As it happens in the original code Buf is pointing to uninitialized memory. It also happens that in the test cases we have Count is always expected to be 1 (since the size of the functions we are testing with are < 127 so it only take a one byte LEB to store their length). So as long as the uninitialized data bytes is between 1 and 127 then the tests will pass. We could construct a test with larger function body which would probably fail.. but it really depends on the uninitialized data so its only likely to fail :) But we should probably as that test anyway I guess?

[ruiu]

Yes, I think you should write a test with a large function to exercise this code. Also, I don't think the buffer is an initialized memory. It is pointing to an output mmap'ed file, and the mmap'ed file is filled with 0.

wasm/InputChunks.cpp

static unsigned writeCompressedReloc(uint8_t *Buf, const WasmRelocation &Rel,
   case R_WEBASSEMBLY_GLOBAL_INDEX_LEB:
   case R_WEBASSEMBLY_MEMORY_ADDR_LEB:
     return encodeULEB128(Value, Buf);
   case R_WEBASSEMBLY_TABLE_INDEX_SLEB:
   case R_WEBASSEMBLY_MEMORY_ADDR_SLEB:
     return encodeSLEB128(static_cast<int32_t>(Value), Buf);
   case R_WEBASSEMBLY_TABLE_INDEX_I32:
   case R_WEBASSEMBLY_MEMORY_ADDR_I32:
+    write32le(Buf, Value);
     return 4;
   default:
     llvm_unreachable("unknown relocation type");
   }
 }
 
 static unsigned getRelocWidthPadded(const WasmRelocation &Rel) {
   switch (Rel.Type) {
void InputFunction::writeTo(uint8_t *Buf) const {
 
   Buf += OutputOffset;
   uint8_t *Orig = Buf; (void)Orig;
 
   const uint8_t *SecStart = File->CodeSection->Content.data();
   const uint8_t *FuncStart = SecStart + getInputSectionOffset();
   const uint8_t *End = FuncStart + Function->Size;
   uint32_t Count;
-  decodeULEB128(Buf, &Count);
+  decodeULEB128(FuncStart, &Count);
   FuncStart += Count;
 
   DEBUG(dbgs() << "write func: " << getName() << "\n");
   Buf += encodeULEB128(CompressedFuncSize, Buf);
   const uint8_t *LastRelocEnd = FuncStart;
   for (const WasmRelocation &Rel : Relocations) {
     unsigned ChunkSize = (SecStart + Rel.Offset) - LastRelocEnd;
     DEBUG(dbgs() << "  write chunk: " << ChunkSize << "\n");