[WebAssembly] Reorder reloc sections to come between symtab and name

[ncw]

Summary

This is required in order to enable relocs to be validated
as they are read in.

Also update tests with new section ordering.

[sbc100]

include/llvm/Object/Wasm.h

264 ↗

(On Diff #136514)

How about "#define INVALID_SECTION UINT32_MAX". Less magical, and less C-style casting seems better.

Actually it looks like these invalid values are not checked anywhere. This part of the change seems unrelated.

lib/Object/WasmObjectFile.cpp

208 ↗

(On Diff #136514)

This seems like it might be overkill to me. My git feeling is that we are only trying to encode a few rules here, but I could be wrong.

257 ↗

(On Diff #136514)

Wouldn't these make more sense as private methods? Aren't you basically passing this there?

329 ↗

(On Diff #136514)

No point in this conditional.

683 ↗

(On Diff #136514)

This seems rather nasty. What can't the check for Name length go here?

778 ↗

(On Diff #136514)

Make this a separate change? [WebAssembly] Validate function signatures in object files

[ncw]

include/llvm/Object/Wasm.h

264 ↗

(On Diff #136514)

It came from a previous version of the diff - where I was using the CodeSection member to test whether the code section had been seen. I left it in because I thought that using an invalid (rather than valid) value as the default was better/safer. And I didn't split it out into another revision because I felt that would be overkill... :(

Happy to change to UINT32_MAX, I just used -1 because that's what the line above already does for StartFunction.

lib/Object/WasmObjectFile.cpp

208 ↗

(On Diff #136514)

Quite a few of the sections have ordering constraints... I started with a couple of hardcoded ones, then when it got to around 4/5 I thought it was worth just encoding the rules from the spec. There's some boilerplate to give each section an "order" number, but the actual logic to check is pretty simple, just a check that the order numbers increase from section to section.

It's cheap to implement and understand, and means you don't need to worry about what might go wrong with home-grown checks. You just get a nice guaranteed section order, and that's safe.

257 ↗

(On Diff #136514)

OK, I can make these methods, it's probably a bit cleaner.

683 ↗

(On Diff #136514)

Because we use the Name in checkSectionOrder, which has to happen before we actually parse the section (since the order check is precisely there to make sure that sections have their prerequisites before being parsed).

You're right it's not lovely to jump to the string end. I did try changing it so that the "Content" of the custom section doesn't include its name, which works fine actually but it seemed not quite "right" in terms of what aligning the section's Contents with the Wasm contents.

778 ↗

(On Diff #136514)

Yup, OK :)

[ncw]

Updated to pull out check on function type indexes into another review, and responded to requests for changes

[sbc100]

lib/MC/WasmObjectWriter.cpp

1310 ↗

(On Diff #136555)

Sorry to be a pain but perhaps we can land this single line change (and the associated test updates) on its own? Then we can bikeshed about the best way to validate ordering at our leisure :)

In also makes sense to me since changing the order, and validating the order can be seen as distinct changes.

[ncw]

Updated to pull out all the validation stuff into another patch, this just reorders the output to be: linking,relocs,name.

[sbc100]

tools/yaml2obj/yaml2wasm.cpp

545 ↗

(On Diff #136767)

I thought we decided this didn't need to change? i.e. we don't care about relative ordering of name vs reloc or linking sections. At least this change relates to reloc vs linking sections, so I'm not sure why the name section moved too.

[sbc100]

Otherwise LGTM

[ncw]

tools/yaml2obj/yaml2wasm.cpp

545 ↗

(On Diff #136767)

I think it would be nice to have "name" come after "linking" at least. (Since we set the name for inputchunks based on the first symbol that references them or the name from the name section, with the name section "winning".)

I'd still personally prefer to nail down the order of the sections, so that compat between tools is guaranteed and no-one needs to try to handle different orderings for the sections; I haven't quite been convinced on the benefits being lenient in section ordering.

This change does exactly what the PR title says: moves reloc to come between linking/symtab and name sections. So in that sense, I've done a matching change in yaml2wasm and WasmObjectWriter.

[sbc100]

tools/yaml2obj/yaml2wasm.cpp

545 ↗

(On Diff #136767)

But we are not the only consumer or producer or wasm files, so we need to as flexible as the spec allows.

I you revert this file this change still does what you want for now, doesn't it? If you don't mind doing that then this LGTM and we can discuss moving the name section separately.

[ncw]

tools/yaml2obj/yaml2wasm.cpp

545 ↗

(On Diff #136767)

I'd be OK with splitting this out - but D43946 changes LLD to output "symtab/relocs/name", and what this change is trying to achieve is to make llc and yaml2wasm match. So one way or another we would need to make the bits agree before we could apply D44024 to validate the section order.

But we are not the only consumer or producer or wasm files, so we need to as flexible as the spec allows.

But we are the producer of the Linking.md spec so we can make it as unflexible as we like :) Currently the "core" spec has no flexibility at all (including the "name" section, which the spec says must come after all other core sections). The only flexibility currently is due to an omission in the Linking.md one that doesn't specify the ordering for the new sections it defines - which would ensure compat with other producers/consumers of wasm, if they agreed on an order.

Sorry to be stubborn - I'll give in and follow your lead on Monday!

[ncw]

Committed without the change to yaml2wasm.

Sam - are you going to do the work to split the relocs into "chunks", ie explicitly associate them with their function/segment? That would be great if you could do that.

And the link to this issue is, it would be great if at the same time you could change the YAML to put the relocations in the "right" place, rather than associating them with the section they apply to.

That's ultimately the "right" fix for the ordering of the YAML I think. I've reverted by hack in yaml2wasm, it's just not good I agree. If the YAML actually printed the relocations in the same place in the YAML that they appear in the WASM file, that would help out.

llvm/trunk/lib/MC/WasmObjectWriter.cpp

void WasmObjectWriter::writeObject(MCAssembler &Asm,
   writeFunctionSection(Functions);
   // Skip the "table" section; we import the table instead.
   // Skip the "memory" section; we import the memory instead.
   writeGlobalSection();
   writeExportSection(Exports);
   writeElemSection(TableElems);
   writeCodeSection(Asm, Layout, Functions);
   writeDataSection();
+  writeLinkingMetaDataSection(SymbolInfos, InitFuncs, Comdats);
   writeCodeRelocSection();
   writeDataRelocSection();
-  writeLinkingMetaDataSection(SymbolInfos, InitFuncs, Comdats);
 
   // TODO: Translate the .comment section to the output.
   // TODO: Translate debug sections to the output.
 }
 
 std::unique_ptr<MCObjectWriter>
 llvm::createWasmObjectWriter(std::unique_ptr<MCWasmObjectTargetWriter> MOTW,
                              raw_pwrite_stream &OS) {
   return llvm::make_unique<WasmObjectWriter>(std::move(MOTW), OS);
 }

llvm/trunk/test/tools/llvm-objdump/wasm.txt

 # RUN: llvm-objdump -h %p/Inputs/trivial.obj.wasm | FileCheck %s
 
 # CHECK:      Sections:
 # CHECK-NEXT: Idx Name          Size      Address          Type
 # CHECK-NEXT:  0 TYPE          0000000e 0000000000000000
 # CHECK-NEXT:  1 IMPORT        0000005d 0000000000000000
 # CHECK-NEXT:  2 FUNCTION      00000002 0000000000000000
 # CHECK-NEXT:  3 CODE          00000019 0000000000000000 TEXT
 # CHECK-NEXT:  4 DATA          0000001c 0000000000000000 DATA
-# CHECK-NEXT:  5 reloc.CODE    00000017 0000000000000000
-# CHECK-NEXT:  6 linking       00000055 0000000000000000
+# CHECK-NEXT:  5 linking       00000055 0000000000000000
+# CHECK-NEXT:  6 reloc.CODE    00000017 0000000000000000
 
 # RUN: llvm-objdump -p %p/Inputs/trivial.obj.wasm | FileCheck %s -check-prefix CHECK-HEADER
 
 # CHECK-HEADER: Program Header:
 # CHECK-HEADER: Version: 0x1
 
 # RUN: llvm-objdump -s --section=CODE %p/Inputs/trivial.obj.wasm | FileCheck %s -check-prefix CHECK-SECTIONS
 
 # CHECK-SECTIONS: Contents of section CODE:
 # CHECK-SECTIONS: 0000 01170041 80808080 00108080 8080001a  ...A............
 # CHECK-SECTIONS: 0010 10818080 80004100 0b                 ......A..

llvm/trunk/test/tools/llvm-readobj/sections.test

 WASM-NEXT:         Name: .rodata..L.str
 WASM-NEXT:         Size: 13
 WASM-NEXT:         Offset: 0
 WASM-NEXT:       }
 WASM-NEXT:     ]
 WASM-NEXT:   }
 WASM-NEXT:   Section {
 WASM-NEXT:     Type: CUSTOM (0x0)
-WASM-NEXT:     Size: 23
+WASM-NEXT:     Size: 65
 WASM-NEXT:     Offset: 191
-WASM-NEXT:     Name: reloc.CODE
+WASM-NEXT:     Name: linking
 WASM-NEXT:   }
 WASM-NEXT:   Section {
 WASM-NEXT:     Type: CUSTOM (0x0)
-WASM-NEXT:     Size: 65
-WASM-NEXT:     Offset: 220
-WASM-NEXT:     Name: linking
+WASM-NEXT:     Size: 23
+WASM-NEXT:     Offset: 262
+WASM-NEXT:     Name: reloc.CODE
 WASM-NEXT:   }
 WASM-NEXT: ]