Page MenuHomePhabricator

[llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy
ClosedPublic

Authored by dschuff on Dec 2 2019, 3:54 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dschuff marked 3 inline comments as done.Dec 16 2019, 10:17 AM
  • Remove OwnedContents for now, and give Writer::SectionHeader its correct name
llvm/tools/llvm-objcopy/wasm/Object.h
22

This interface (owning the contents) is actually used in D70970, but not here; I'll move it to the next review.
For now I kept the naming ("contentsRef") the same. in the next review we can discuss the current approach (which is meant for moving the contents) vs. your suggested approach which would copy them.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
31

You're right, it's a single byte in the spec; it doesn't make any difference if it's less than 128 and I think we are not 100% consistent in the tools code. But I fixed it here.

llvm/tools/llvm-objcopy/wasm/Writer.h
33

Actually the root problem here is that SectionData is totally misnamed. It's not actually the section payload at all, it's just the header, which does need to be computed from the final layout. (that's why it's a SmallVector). I think I originally had both headers and data and I must have renamed/eliminated the wrong name when I change it.
I just renamed it SectionHeader and it all hopefully makes more sense now.

In light of that, finalize does more than just compute the size, it generates the header as well, and as @sbc100 says, matches the way we use the term in the wasm backend and lld.

Thanks for your patience, a few more comments.

llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
7

-o ?

llvm/tools/llvm-objcopy/wasm/Object.h
22

well, for now it seems like it should be a struct (e.g. as in the comment above), because otherwise this looks a bit strange imo. (+ it looks like this "using" and the setter method would not be necessary).

42

since WasmObjectHeader is public i would convert this class into a struct as well / get rid of boilerplate code (getSections/addSections etc.)
So basically I'd keep it as simple & clear as possible, and if we need to add complexity we'll do it when/where it's required (and there it'll be easier to understand / judge what exactly is necessary).

dschuff marked an inline comment as done.Dec 16 2019, 5:35 PM
dschuff added inline comments.
llvm/tools/llvm-objcopy/wasm/Object.h
42

I understand the concern here (and above) but I'd rather not completely refactor this class now, only to completely rewrite it back tomorrow (since I intend to finish up D70970 as soon as this patch lands). I don't think it makes sense to get rid of addSections given that we'll have removeSections soon, and removeSections is slightly nontrivial. I think the more useful question though is how to handle owned and non-owned section data, since in the next change we'll need to have both. I went ahead and rewrote Section for now, and I made the Object own the data (instead of the section) in the next patch so you can see what you think of that.

dschuff updated this revision to Diff 234203.Dec 16 2019, 5:36 PM
  • Simplify Section to a struct
dschuff marked an inline comment as done.Dec 16 2019, 5:38 PM
dschuff added inline comments.
llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
7

obj2yaml does not support the -o option.

llvm/tools/llvm-objcopy/wasm/Object.cpp
22 ↗(On Diff #234203)

Sections.insert(Sections.end(), NewSections.begin(), NewSections.end());

llvm/tools/llvm-objcopy/wasm/Object.h
23

"public:" is unnecessary

llvm/tools/llvm-objcopy/wasm/Reader.cpp
25

Sections.push_back({WS.Type(), WS.Name, WS.Content});

llvm/tools/llvm-objcopy/wasm/Writer.h
33

I see, thank you for the additional context, the new name makes more sense,
but I'm still kinda curious - what are the advantages of keeping this around vs "writing on the fly" ?
the latter looks simpler + a little bit less code

llvm/tools/llvm-objcopy/wasm/Writer.h
33

+ regardless of the matter, i think the code which generates a SectionHeader for a given Section should be put into a helper function + it would be good to add a brief comment explaining the format + a link to the detailed specification (for those who will read this code in the future)

dschuff updated this revision to Diff 234406.Dec 17 2019, 3:32 PM
dschuff marked 3 inline comments as done.
  • Split header generation into separate function
dschuff marked an inline comment as done.Dec 17 2019, 3:36 PM
dschuff added inline comments.
llvm/tools/llvm-objcopy/wasm/Writer.h
33

I think the reason it's split into a separate step is that because of the LEB128 encodings (ubiquitous in wasm), the size of the section isn't known until the encodings are done. Here it only shows up in header generation where the size includes the encoded length of the section name, but there are other cases (especially when dealing with relocations and instructions) where there are more LEBs that we might prefer to either leave encoded with extra bytes (for rewriting later) or compress to reduce binary size.

okay, thank you again for the patience, overall this looks cleaner to me

llvm/tools/llvm-objcopy/wasm/Reader.cpp
27

sorry, I didn't notice this before - but this is one reason why I wasn't a big fan of the proposed interface,
adding a section (or sections) always involves a copy.
For example, if vector<Section> Sections was just a public field of Object
(or Object was a struct (it still can have a method removeSections if necessary))
you would simply do the following:

Obj.Sections.reserve(WasmObj.sections().size());
for (SectionRef Sec : WasobObj.sections()) {
     Sections.push_back(... );
}

Frankly speaking it still feels like addSections(...) doesn't buy us much. (and the same is true for getSections)

(+ please, apply clang-format --style=llvm to the diff (if not yet))

dschuff updated this revision to Diff 239707.Jan 22 2020, 2:47 PM
  • Make Sections a public member of Object; remove getSections/addSections
dschuff updated this revision to Diff 239708.Jan 22 2020, 2:48 PM
  • fix clang-format

OK, made Sections a public member of Object.

I think all the feedback has now been addressed.

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dschuff updated this revision to Diff 239721.Jan 22 2020, 3:23 PM
  • Fix header guard name

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Hm, the bot seems to be using the wrong version of the patch.

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I've given this another quick once over, and made a few small comments. I'll try to take a more in-depth look tomorrow or early next week.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
25–26

Missing trailing full stops on these two lines.

32

// static?

llvm/tools/llvm-objcopy/wasm/Writer.h
33–34

Not a big deal, but any particular reason you've called this generateSectionHeader instead of the shorter createSectionHender?

dschuff updated this revision to Diff 239921.Jan 23 2020, 9:04 AM
dschuff marked 3 inline comments as done.
  • Rename generateSectionHeader, clarify comments
llvm/tools/llvm-objcopy/wasm/Writer.cpp
25–26

done (but these are intentionally not complete sentences).

32

This is just the common annotation indicating that generateSectionHeader is a static method. I added a blank line to make it more clear that it goes with the method definition rather than the block comment above.

llvm/tools/llvm-objcopy/wasm/Writer.h
33–34

No particular reason; habit from working on code generators I suppose. I changed it.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson added inline comments.Jan 27 2020, 6:54 AM
llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
24

Is this using needed? You're already inside the wasm namespace.

44

Strictly speaking, basic copying isn't a flag! Perhaps simply "no flags are supported yet" or possibly "no flags are supported yet. Only basic copying is allowed".

llvm/tools/llvm-objcopy/wasm/Writer.cpp
23–31

I wonder whether this comment would make more sense in the header?

32

If it's common practice within other areas of WASM code, then that's fine, but I wasn't aware that it's a wider thing. To me, it initially looked like you'd just accidentally commented it out or something similar.

77

A blank line before this and the similar comment above would nicely break this function up, I think.

dschuff updated this revision to Diff 240702.Jan 27 2020, 3:27 PM
dschuff marked 5 inline comments as done.
  • Rename generateSectionHeader, clarify comments
llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
44

Done as you suggested, (but I added a full stop at the end ;-).

llvm/tools/llvm-objcopy/wasm/Writer.cpp
23–31

Yeah that makes sense; moved the comment.

32

Looking again, it doesn't seem to be as common in LLVM as in some other codebases I've used; I'll just remove it here.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dschuff updated this revision to Diff 240713.Jan 27 2020, 4:04 PM
  • Fix clang-tidy warning

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dschuff updated this revision to Diff 240736.Jan 27 2020, 5:07 PM
  • remove stray empty line

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson accepted this revision.Jan 28 2020, 1:13 AM

LGTM, with two nits.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
44

Actually, for error messages, we usually don't end with a full stop :-)

The style we try to canonicalise on in the other binutils is lower-case first letter of first sentence, and no trailing full stops.

Don't ask me why that is the style, mind you...

llvm/tools/llvm-objcopy/wasm/Writer.h
33–41

We're a bit hit-and-miss on this, but it's probably good practice to use doxygen-style comments (i.e. '///') for things like this in headers.

dschuff updated this revision to Diff 240911.Jan 28 2020, 9:38 AM
dschuff marked 2 inline comments as done.
  • remove period, use doxygen comments
This revision was automatically updated to reflect the committed changes.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

kamaub added a subscriber: kamaub.EditedJan 29 2020, 7:18 AM

@dschuff please note this patch causes build failures for PowerPC big endian buildbots with commit rGa928d127a52a
If the solution isn't a simple fix please revert the commit so we may continue testing the rest of the repository

D72723 may show up with some failures in between but here but this has since been resolved
i've attached the builds that broke and the lastest build from those bots:

clang-ppc64be-linux:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43577
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43643

clang-ppc64be-linux-multistage:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23316
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23336

clang-ppc64be-linux-lnt:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34687
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34719

I won't have time to look at it today; i'll just revert and take a look tomorrow.

Relanded with a fix as rGf2af0607000c
I don't have a BE machine to test on, but I'm *pretty* sure it works :)

I won't have time to look at it today; i'll just revert and take a look tomorrow.

Thank you, the buildbots are green again, best of luck addressing the failure.