Supports dumping, adding, and removing of named custom sections, but
nothing else yet.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
- 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. | |
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. 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.) |
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. |
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, |
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) |
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, 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) |
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
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
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? |
- 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.
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. |
- 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.
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.
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.
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. |
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.
@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
Relanded with a fix as rGf2af0607000c
I don't have a BE machine to test on, but I'm *pretty* sure it works :)