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

[dschuff]

Summary

Supports dumping, adding, and removing of named custom sections, but
nothing else yet.

[dschuff]

• 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.

[alexander-shaposhnikov]

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]

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]

• Simplify Section to a struct

[dschuff]

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

7

obj2yaml does not support the -o option.

[alexander-shaposhnikov]

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

[alexander-shaposhnikov]

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]

• Split header generation into separate function

[dschuff]

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.

[alexander-shaposhnikov]

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)

[alexander-shaposhnikov]

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

[dschuff]

• Make Sections a public member of Object; remove getSections/addSections

[dschuff]

• fix clang-format

[dschuff]

OK, made Sections a public member of Object.

[dschuff]

I think all the feedback has now been addressed.

[merge_guards_bot]

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]

• Fix header guard name

[merge_guards_bot]

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

[dschuff]

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

[merge_guards_bot]

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

[jhenderson]

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]

• 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.

[merge_guards_bot]

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]

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]

• 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.

[merge_guards_bot]

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]

• Fix clang-tidy warning

[merge_guards_bot]

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]

• remove stray empty line

[merge_guards_bot]

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]

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]

• remove period, use doxygen comments

[merge_guards_bot]

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]

@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

[dschuff]

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

[dschuff]

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

[kamaub]

In D70930#1847950, @dschuff wrote:

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.