[PATCH] D107662: [WebAssembly] Implement build-id feature
Derek Schuff via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 3 14:11:01 PDT 2022
dschuff added inline comments.
================
Comment at: lld/test/wasm/build-id.s:43
+
+.section .data.foo,"",@
+ .globl foo
----------------
sbc100 wrote:
> Why not just use `lld/test/wasm/Inputs/start.s` for this test .. or does it depend on there being some data?
>
> When you could remove all the `--no-entry` flags above, and all the comment markers, and just call this file `.test`.
no, it doesn't depend on there being data, I basically just started with the x86 version of this same test. But it has some additional assertions that don't apply to us.
So you're saying this would be a .test file because it would only have RUN lines, and the runs would just assemble start.s? That sounds good.
================
Comment at: lld/wasm/Driver.cpp:353
+// synonym for "sha1" because all our hash functions including
+// -build-id=sha1 are actually tree hashes for performance reasons.
+static std::pair<BuildIdKind, std::vector<uint8_t>>
----------------
sbc100 wrote:
> Do we want to support the `tree` alias? I assume this comes from the ELF backend?
yes, this comes from the ELF backend. I think it probably makes sense to support the same options? I would think we'd at least want random UUID, some kind of hash, and user-defined. See also the discussion on the tool-conventions issue though, I'm open to making the implementation different if that makes sense.
================
Comment at: lld/wasm/SyntheticSections.cpp:923
+ // TODO: Remove once we're sure we don't want to encode this as a string.
+ //writeStr(os, std::string(hashSize, ' '), "placeholder");
+}
----------------
sbc100 wrote:
> Do you want to leave these TODOs in here?
no, the idea is to remove them once the discussion on tool-conventions is resolved and before committing this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107662/new/
https://reviews.llvm.org/D107662
More information about the llvm-commits
mailing list