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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added inline comments.


================
Comment at: lld/wasm/SyntheticSections.h:444
+    SyntheticSection::writeTo(buf);
+    hashBuf = buf + offset + header.size() + 9 /*name string*/ +
+              1 /* hash size */; // FIXME assert these somewhere
----------------
dschuff wrote:
> sbc100 wrote:
> > This seems a little hacky / fragile, but I can't think of a more elegant way to do it..
> Ah yes I meant to try to fix that; WDYT of this fix?
Seems better, but maybe add a comment here.  What is hashBuf for?  Could a better name be used here?  isHashLocation?  hashPlaceholderPtr?


================
Comment at: lld/wasm/Writer.cpp:1834
   writeSections();
+  writeBuildId();
   if (errorCount())
----------------
dschuff wrote:
> sbc100 wrote:
> > I wonder if this should have a name that lets the reader know its going back an patching an existing location?  Maybe `patchBuildId` ?  But that doesn't convey that this function also calulates the buildid?   "calculateAndPatchBuildId" seems like too much of a mouthful.
> > 
> > Maybe your existing names is best :)
> Yeah, those names are maybe a bit more descriptive but not obviously much better. This is the same name used by the other object formats (and has similar behavior) so consistency seems good in the end.
Makes sense


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107662/new/

https://reviews.llvm.org/D107662