[PATCH] D107662: [WebAssembly] Implement build-id feature

[Derek Schuff via Phabricator via llvm-commits]

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