Page MenuHomePhabricator

[WebAssembly] Generalize section ordering constraints
ClosedPublic

Authored by tlively on Feb 15 2019, 3:47 PM.

Details

Summary

Changes from using a total ordering of known sections to using a
dependency graph approach. This allows our tools to accept and process
binaries that are compliant with the spec and tool conventions that
would have been previously rejected. It also means our own tools can
do less work to enforce an artificially imposed ordering. Using a
general mechanism means fewer special cases and exceptions in the
ordering logic.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Feb 15 2019, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 3:47 PM
sbc100 added a subscriber: binji.Feb 15 2019, 4:15 PM

When we discussed this before @binji and others argued that a total ordering made more sense.

llvm/test/MC/WebAssembly/relocs_and_producers.ll
1 ↗(On Diff #187104)

Would it make sense to have the yaml checked in here directly and skip llc? I think you can be explicit about the ordering of the sections in the input if the test is yaml.

tlively updated this revision to Diff 187122.Feb 15 2019, 6:05 PM
tlively marked 2 inline comments as done.
  • Switch .ll test to .yaml and move to test/Object directory

When we discussed this before @binji and others argued that a total ordering made more sense.

What made me think this was a good idea was that I discovered that yaml2obj was not respecting the total ordering in its output by emitting relocations after the producers section. Fixing the total ordering logic to selectively ignore the producers section in some contexts seemed hacky and changing the yaml2obj logic seemed like letting section ordering complexity creep into unrelated code. That being said, I don't want to rehash old conversations pointlessly, so I'm happy to fix yaml2obj instead of making this change if that's what people want.

llvm/test/MC/WebAssembly/relocs_and_producers.ll
1 ↗(On Diff #187104)

Yes, making the test text yaml at least reduces the number of tool invocations. The yaml does not have a separate section for the relocations, though, so I can't be explicit about the ordering.

When we discussed this before @binji and others argued that a total ordering made more sense.

What made me think this was a good idea was that I discovered that yaml2obj was not respecting the total ordering in its output by emitting relocations after the producers section. Fixing the total ordering logic to selectively ignore the producers section in some contexts seemed hacky and changing the yaml2obj logic seemed like letting section ordering complexity creep into unrelated code. That being said, I don't want to rehash old conversations pointlessly, so I'm happy to fix yaml2obj instead of making this change if that's what people want.

I think I argued for doing it this way at the time too, I'm just making sure that we get buy in from those who argued for the total ordering before making this change.

Yeah, I'd still prefer to have a total order if that's not too much effort. The alternative introduces a lot of potential complexity into the consumer, since we'll have a combinatorial explosion of possibilities for where these sections can be located, many of which will likely be untested.

Yeah, I'd still prefer to have a total order if that's not too much effort. The alternative introduces a lot of potential complexity into the consumer, since we'll have a combinatorial explosion of possibilities for where these sections can be located, many of which will likely be untested.

I definitely want to reduce overall complexity, not increase it. My assumption is that if consumer code would be simplified by guaranteeing that section A is before section B, then the DAG ordering should be updated to make that ordering required. Sections should only be unordered with respect to each other if consumers are truly agnostic to their order.

In this case, producer code was able to be simplified by loosening the ordering constraints with no corresponding complications in consumer code that I am aware of. @binji, does this seem reasonable? Are there potential complications that I am missing?

binji added a comment.Feb 19 2019, 2:02 PM

Yeah, I'd still prefer to have a total order if that's not too much effort. The alternative introduces a lot of potential complexity into the consumer, since we'll have a combinatorial explosion of possibilities for where these sections can be located, many of which will likely be untested.

I definitely want to reduce overall complexity, not increase it. My assumption is that if consumer code would be simplified by guaranteeing that section A is before section B, then the DAG ordering should be updated to make that ordering required. Sections should only be unordered with respect to each other if consumers are truly agnostic to their order.

In this case, producer code was able to be simplified by loosening the ordering constraints with no corresponding complications in consumer code that I am aware of. @binji, does this seem reasonable? Are there potential complications that I am missing?

I'm suggesting that we provided an ordering even between sections that otherwise have no constraints, similar to the rest of the wasm format. There's no requirement that the custom sections have an ordering, but I'm worried that we'll end up with consistency issues like we did with the name section (before it was spec'd as being required after the data section).

Yeah, I'd still prefer to have a total order if that's not too much effort. The alternative introduces a lot of potential complexity into the consumer, since we'll have a combinatorial explosion of possibilities for where these sections can be located, many of which will likely be untested.

I definitely want to reduce overall complexity, not increase it. My assumption is that if consumer code would be simplified by guaranteeing that section A is before section B, then the DAG ordering should be updated to make that ordering required. Sections should only be unordered with respect to each other if consumers are truly agnostic to their order.

In this case, producer code was able to be simplified by loosening the ordering constraints with no corresponding complications in consumer code that I am aware of. @binji, does this seem reasonable? Are there potential complications that I am missing?

I'm suggesting that we provided an ordering even between sections that otherwise have no constraints, similar to the rest of the wasm format. There's no requirement that the custom sections have an ordering, but I'm worried that we'll end up with consistency issues like we did with the name section (before it was spec'd as being required after the data section).

I'm totally fine with specifying a mostly-linear ordering between sections, including between custom sections that are meaningful to the toolchain. But I think it also makes sense to allow for exceptions to that when it allows for simpler code, as is the case for the unspecified ordering between the producers section and relocation sections. This CL is about changing the mechanism to allow for this exception, not about changing the ordering of existing sections or changing our norm of specifying such orderings. Do you think we should disallow exceptions entirely?

binji added a comment.Feb 19 2019, 2:29 PM

I'm totally fine with specifying a mostly-linear ordering between sections, including between custom sections that are meaningful to the toolchain. But I think it also makes sense to allow for exceptions to that when it allows for simpler code, as is the case for the unspecified ordering between the producers section and relocation sections. This CL is about changing the mechanism to allow for this exception, not about changing the ordering of existing sections or changing our norm of specifying such orderings. Do you think we should disallow exceptions entirely?

I don't want to push back too hard on this. Since we're talking about tool conventions that are currently used by tools in the LLVM repos, it's probably OK. My point is just that if we don't have a total order, then it's likely that we'll eventually see modules that support all orderings, which will make it harder to reason about. My bias is toward being over-constrained at first, then loosening those constraints, so that's why I'd like to see something more strict here. But if it's especially burdensome to do so, then I think it's fine to go with a simpler solution, as long as it's sufficiently tested.

aheejin accepted this revision.EditedFeb 19 2019, 4:34 PM

I think it'd be OK to relax these constraints, but I don't feel too strongly on either side though. Anyway LGTM to me.

llvm/lib/Object/WasmObjectFile.cpp
1555 ↗(On Diff #187122)

I understand what this does, but maybe add an one-line comment saying this intends to express a DAG ? At first glance, someone might wonder like "Why are there only two disallowed predecessors for WASM_SEC_ORDER_TYPE"?

llvm/test/Object/wasm-relocs-and-producers.yaml
2 ↗(On Diff #187122)

Nit: Can't we merge these into one line?
yaml2obj %s | llvm-objdump -s | FileCheck %s

This revision is now accepted and ready to land.Feb 19 2019, 4:34 PM
This revision was automatically updated to reflect the committed changes.
tlively marked 2 inline comments as done.