[WebAssembly] Handle relocations where provisional value != index

[ncw]

Summary

Since rLLD323168, we have relocations where the provisional value is not the same as the symbol index. Thus, we now need to remove the assertion that the provisional value matches the symbol index.

This was in a previous review, but somehow the chunk was dropped in the final commits - it could perfectly well have been my fault during a rebase though.

---

NB. This needs to be applied before D42095. That commit ("#3" in my patch series) did originally work with LLD, but since this chunk was dropped, this review must be merged before D42095 will work again.

[sbc100]

I'm a little sad to loose this extra level to checking.

When you say "we have relocations where the provisional value is not the same as the symbol index", where are you seeing this? Not in any of our tests presumably? Can we write a test case for this?

[ncw]

In D42539#992827, @sbc100 wrote:

I'm a little sad to loose this extra level to checking.

When you say "we have relocations where the provisional value is not the same as the symbol index", where are you seeing this? Not in any of our tests presumably? Can we write a test case for this?

I don't feel too strongly about needing to check the provisional values here: they don't actually do anything - and we aren't checking them for TABLE/MEMORY relocations anyway.

Yes, I'm seeing the problem in our tests. As it says in the description above, the issue is that as soon as D42095 lands, LLC will start to output relocations that cause this check to fail, so this review needs to land before D42095 in order for LLD to keep on working. This chunk was originally part of "LLD patch #3" (and D42095 was originally titled "LLVM patch #3"), so they are a matched pair of changes. This is only coming in its own review because somehow when rLLD323168 was landed this little chunk was dropped.

The code as it stands is just dodgy code full stop, it's reading a relocation "value" and comparing it against the relocation "index". If it was safe to assert that the "value" and "index" are equal, then we wouldn't need both fields on the relocation object! We're only getting away with it here because WasmObjectWriter (until D42095 lands) is currently writing out the "index" as the provisional value.

(Long-term we simply can't write out the index as the value. This is because in general there are more symbols than functions/globals, so the indexes will be larger than the Wasm values. Hence writing out an index as a value is going to cause the Wasm module to fail validation. Your symbol index might be "4", and there might be only three functions, so the Wasm module wouldn't validate if you tried to put a "4" as the operand of the call instruction.)

[sbc100]

Oh I see. I thought we already had such cases, but its only after D42095 that this would actually start breaking.

With respect the provisional value, isn't it the case the every function symbol must point to a valid function index (either an imported or defined function)? So can't we continue to write a reasonable provisional value, and then verify that here?

I know there can be 1 to many relationship between symbols and functions, but that doesn't stop us from writing the provisional index and then also checking it here does it? I guess the check is not a big deal, but it certainly helped during development to spot when my offsets were wrong. I guess I don't mind dropping it now.

[ncw]

Yes, you're right - we could write a check that would verify the correct provisional value is being written out. But that would mean building effectively a symbol table for each input file, and we currently don't have a structure that models the WasmObjectWriter symbol table - since things like aliases and weak symbols etc are globally resolved as soon as we put them in LLD's symbol table. So the symbol lookup table we'd need would have to be per-file, and it just doesn't seem worth it at the moment.

We could do it though - I'd be up for it, if it needs doing. But there's lots of missing input validation in WasmObjectFile, I'm not sure the provisional values are the top priority.

I agree it's good to be careful when removing checks.

[ncw]

D42095 was merged before this - I may be misremembering, but I had in the back of my mind that the LLD tests wouldn't pass without this... have you checked it's OK to leave this unmerged? (Sorry if I'm going mad and got the dependencies between my patches muddled.)

[sbc100]

I found that this is really an essential part of D41955. (If I land D41955 today i'll roll this into it, otherwise can you combine it back into D41955?)

[ncw]

I've updated D41955 to include this chunk.

wasm/InputChunks.cpp

 static void applyRelocation(uint8_t *Buf, const OutputRelocation &Reloc) {
   DEBUG(dbgs() << "write reloc: type=" << Reloc.Reloc.Type
                << " index=" << Reloc.Reloc.Index << " value=" << Reloc.Value
                << " offset=" << Reloc.Reloc.Offset << "\n");
   Buf += Reloc.Reloc.Offset;
   int64_t ExistingValue;
   switch (Reloc.Reloc.Type) {
   case R_WEBASSEMBLY_TYPE_INDEX_LEB:
-  case R_WEBASSEMBLY_FUNCTION_INDEX_LEB:
-  case R_WEBASSEMBLY_GLOBAL_INDEX_LEB:
     ExistingValue = decodeULEB128(Buf);
     // Additional check to verify that the existing value that the location
     // matches our expectations.
     if (ExistingValue != Reloc.Reloc.Index) {
       DEBUG(dbgs() << "existing value: " << decodeULEB128(Buf) << "\n");
       assert(decodeULEB128(Buf) == Reloc.Reloc.Index);
     }
     LLVM_FALLTHROUGH;
+  case R_WEBASSEMBLY_FUNCTION_INDEX_LEB:
+  case R_WEBASSEMBLY_GLOBAL_INDEX_LEB:
   case R_WEBASSEMBLY_MEMORY_ADDR_LEB:
     encodeULEB128(Reloc.Value, Buf, 5);
     break;
   case R_WEBASSEMBLY_TABLE_INDEX_SLEB:
   case R_WEBASSEMBLY_MEMORY_ADDR_SLEB:
     encodeSLEB128(static_cast<int32_t>(Reloc.Value), Buf, 5);
     break;
   case R_WEBASSEMBLY_TABLE_INDEX_I32: