Page MenuHomePhabricator

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

Authored by ncw on Mar 1 2018, 7:01 AM.

Details

Summary

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

Also update tests with new section ordering.

Diff Detail

Repository
rL LLVM

Event Timeline

ncw created this revision.Mar 1 2018, 7:01 AM
sbc100 added inline comments.Mar 1 2018, 9:08 AM
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 added inline comments.Mar 1 2018, 9:21 AM
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 this revision to Diff 136555.Mar 1 2018, 9:53 AM

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

sbc100 added inline comments.Mar 1 2018, 10:06 AM
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 this revision to Diff 136767.Mar 2 2018, 9:03 AM
ncw retitled this revision from [WebAssembly] Reorder reloc sections to come after symtab and validate section order to [WebAssembly] Reorder reloc sections to come between symtab and name.
ncw edited the summary of this revision. (Show Details)

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

sbc100 added inline comments.Mar 2 2018, 9:40 AM
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 added a comment.Mar 2 2018, 9:40 AM

Otherwise LGTM

ncw added inline comments.Mar 2 2018, 10:07 AM
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 added inline comments.Mar 2 2018, 10:25 AM
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 added inline comments.Mar 2 2018, 11:03 AM
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!

This revision was not accepted when it landed; it landed in state Needs Review.Mar 5 2018, 5:01 AM
This revision was automatically updated to reflect the committed changes.
ncw added a comment.Mar 5 2018, 5:04 AM

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.