[PATCH] D59025: Add --add-ghashes to llvm-objcopy to append a .debug$H to coff objects

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Leonardo, could you please explain how are you planning on using this feature in production? Are you compiling all OBJs, then running `llvm-objcopy --add-ghashes` on all of them afterwards?
Could you provide some timings? (time to compile your OBJs, time to add the ghashes, linking with and without ghashes)



================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:93
 
+static ArrayRef<uint8_t> consumeDebugMagic(ArrayRef<uint8_t> Data,
+                                           StringRef SecName) {
----------------
This function duplicates `SectionChunk::consumeDebugMagic()`. It would be better maybe to find a suitable place for both of them inside [[ https://github.com/llvm/llvm-project/tree/master/llvm/lib/DebugInfo/CodeView | `llvm\lib\DebugInfo\CodeView\` ]]

Same comment for `mergeDebugT`, `toDebugH` and `addGHashes` below, they should be part of a helper inside the LLVMDebugInfoCodeView library. We might want to incrementally write back `.debug$H` sections from LLD in the future.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:103
+
+const std::vector<GloballyHashedType> mergeDebugT(const Section *Sec) {
+  std::vector<GloballyHashedType> OwnedHashes;
----------------
Rename function to something like `hashTypes`


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:121
+
+ArrayRef<uint8_t> toDebugH(const std::vector<GloballyHashedType> hashes,
+                           BumpPtrAllocator &Alloc) {
----------------
This duplicates behavior in `llvm::CodeViewYAML::toDebugH`, move both to a common place, like suggested above.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:143
+
+  for (Section section: Obj.getSections()) {
+    if (section.Name == ".debug$H")
----------------
`Section&`


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:145
+    if (section.Name == ".debug$H")
+      error("Already has .debug$H");
+
----------------
Do we really want to throw an error here? What if the user wants to run on a bunch of OBJs, some which have `.debug$H`, some which don't?

And what if we change the default algorithm?

Maybe warn instead, and skip the code below?


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:149
+      hashes = mergeDebugT(&section);
+	  found_types = true;
+      break;
----------------
Indent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59025