Add --add-ghashes to llvm-objcopy to append a .debug$H to coff objects

[santagada]

Summary

The only problem I have with this change is that apparently link.exe doesn't like the obj files after the section is added... Maybe its related to the missing symbols for this section that clang creates, or maybe link.exe really don't like the .debug$H section or some bug in the generation of the modified obj.

[aganea]

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)

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

93

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.

103

Rename function to something like hashTypes

121

This duplicates behavior in llvm::CodeViewYAML::toDebugH, move both to a common place, like suggested above.

143

Section&

145

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?

149

Indent.

[aganea]

The only problem I have with this change is that apparently link.exe doesn't like the obj files after the section is added...

Could you add more detail? Could you possibly try to pinpoint the issue with a very small use-case please?
LLVM-generated OBJs shall be 100% compatible with MSVC-generated ones. If they are not, we should fix that.

[rupprecht]

General question: why not just use llvm-objcopy --add-section to do this?

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

97

error() should be replaced with "return createStringError(...)" to let the callers handle errors.

[santagada]

In D59025#1419975, @aganea wrote:

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)

We have a distributed compilation system, I changed the c/c++ compiler tool to after the actual compiler run we run llvm-objcopy --add-ghashes <output.obj> so we receive the files already processes from the distributed workers. I didn't even time as I couldn't notice a big slowdown.

Without --add-ghashes:

  Input File Reading:          1331 ms (  1.9%)
  Code Layout:                  446 ms (  0.6%)
  PDB Emission (Cumulative):  68470 ms ( 97.1%)
    Add Objects:              63792 ms ( 90.5%)
      Type Merging:           60617 ms ( 86.0%)
      Symbol Merging:          3077 ms (  4.4%)
    TPI Stream Layout:          785 ms (  1.1%)
    Globals Stream Layout:      526 ms (  0.7%)
    Commit to Disk:            2692 ms (  3.8%)
  Commit Output File:            30 ms (  0.0%)
-------------------------------------------------
Total Link Time:              70505 ms (100.0%)

Agregate job times: 3 hours 2 minutes 17 seconds
Build time: 3 minutes 4 seconds

With --add-ghashes:

  Input File Reading:          1391 ms (  6.9%)
  Code Layout:                  449 ms (  2.2%)
  PDB Emission (Cumulative):  18019 ms ( 89.6%)
    Add Objects:              13046 ms ( 64.8%)
      Type Merging:            9851 ms ( 49.0%)
      Symbol Merging:          3070 ms ( 15.3%)
    TPI Stream Layout:          791 ms (  3.9%)
    Globals Stream Layout:      554 ms (  2.8%)
    Commit to Disk:            2769 ms ( 13.8%)
  Commit Output File:            31 ms (  0.2%)
-------------------------------------------------
Total Link Time:              20121 ms (100.0%)

Agregate job times: 3 hours 3 minutes 12 seconds
Build time: 1 minutes 51 seconds

Where the Agregate is the sum of all the jobs in all the distributed nodes. So although it took a minute more of cpu time in total it took a whole 1 minute less in total for a full rebuild. For users they are usually changing one or two files so for them linking time is the most prevaled unit we use for measuring workflow performance.

[santagada]

In D59025#1419985, @aganea wrote:

The only problem I have with this change is that apparently link.exe doesn't like the obj files after the section is added...

Could you add more detail? Could you possibly try to pinpoint the issue with a very small use-case please?
LLVM-generated OBJs shall be 100% compatible with MSVC-generated ones. If they are not, we should fix that.

Well, link.exe seems to get "confused" it doesn't print any error messages and produce a very big file that is not really an executable. Without any messages or symbols for it its a bit difficult to see what is happening. As I mostly did the same as the gnu-link option does, I guess those obj files should have the same problem.

[santagada]

In D59025#1420024, @rupprecht wrote:

General question: why not just use llvm-objcopy --add-section to do this?

How? I need to compute the ghashes, how would I just add a section to it?

[rupprecht]

In D59025#1420039, @santagada wrote:

In D59025#1420024, @rupprecht wrote:

General question: why not just use llvm-objcopy --add-section to do this?

How? I need to compute the ghashes, how would I just add a section to it?

--add-section adds a given file as a section name, e.g. assuming you have a separate script to compute hashes:
./compute_ghashes input.o > ghash.txt
llvm-objcopy input.o --add-section .debug$H=ghash.txt

[mstorsjo]

In D59025#1420036, @santagada wrote:

Well, link.exe seems to get "confused" it doesn't print any error messages and produce a very big file that is not really an executable. Without any messages or symbols for it its a bit difficult to see what is happening. As I mostly did the same as the gnu-link option does, I guess those obj files should have the same problem.

FWIW, the gnu debuglink feature is only used on executables, not on object files, and only in practice in mingw setups and read by GDB, so there might very well be things in that which confuses link.exe.

[mstorsjo]

llvm/tools/llvm-objcopy/CopyConfig.h

123

When adding a new option here, it might make sense to add an error to the ELF backend saying that this option is unsupported there. (I think the macho backend still doesn't handle any existing options either, without erroring out, so there's probably no need to add any error there yet.)

[santagada]

In D59025#1420534, @mstorsjo wrote:

In D59025#1420036, @santagada wrote:

Well, link.exe seems to get "confused" it doesn't print any error messages and produce a very big file that is not really an executable. Without any messages or symbols for it its a bit difficult to see what is happening. As I mostly did the same as the gnu-link option does, I guess those obj files should have the same problem.

FWIW, the gnu debuglink feature is only used on executables, not on object files, and only in practice in mingw setups and read by GDB, so there might very well be things in that which confuses link.exe.

So adding an option just for lld should be ok as well... I would document on the help of --add-ghashes "(only supported by lld)" at least until someone figures out what is tripping up link.exe because it really shouldn't be a problem. My old approach used old fields in the .debug$T header to point to a blob with the hashes at the end of the .obj file. Altough much more hacky it did not break link.exe at all. I would prefer to leave this as is as this doesn't require any changes to lld.

[santagada]

In D59025#1420056, @rupprecht wrote:

In D59025#1420039, @santagada wrote:

In D59025#1420024, @rupprecht wrote:

General question: why not just use llvm-objcopy --add-section to do this?

How? I need to compute the ghashes, how would I just add a section to it?

--add-section adds a given file as a section name, e.g. assuming you have a separate script to compute hashes:
./compute_ghashes input.o > ghash.txt
llvm-objcopy input.o --add-section .debug$H=ghash.txt

llvm-objcopy doesn't supoprt --add-section for coff, so this wouldn't be possible. Even if it was this is a clean way to add a section that is intended to follow the same format that lld expects and that clang generates.

[santagada]

I will try to do most code changes tomorrow, altough unifying the lld and objcopy generation of the .debug$H section will be complicated as they have completely different apis to obj files.

[rupprecht]

In D59025#1420566, @santagada wrote:

In D59025#1420056, @rupprecht wrote:

In D59025#1420039, @santagada wrote:

In D59025#1420024, @rupprecht wrote:

General question: why not just use llvm-objcopy --add-section to do this?

How? I need to compute the ghashes, how would I just add a section to it?

--add-section adds a given file as a section name, e.g. assuming you have a separate script to compute hashes:
./compute_ghashes input.o > ghash.txt
llvm-objcopy input.o --add-section .debug$H=ghash.txt

llvm-objcopy doesn't supoprt --add-section for coff, so this wouldn't be possible.

I can't think of any reason it shouldn't be implemented -- I assume @mstorsjo just hasn't found time (or a volunteer) to implement that yet :)
i.e. for purposes of design, I think we should pretend that --add-section (and in fact all objcopy methods) are implemented for all object formats.

Even if it was this is a clean way to add a section that is intended to follow the same format that lld expects and that clang generates.

I think that's subjective. It's definitely looks simple here, but it adds complexity to objcopy that I think might not be in the right place. Instead of teaching N llvm tools how to use ghashes, it might be better to just create one, and then tools like objcopy can remain generic.

Anyway, I'm not really that opposed to this change, I just want to figure out what's the threshold for adding flags like this. It's probably totally fine, especially as it seems fairly isolated. I added a few other reviewers who might have an idea. I've never heard of ghashes, so I might not be the best person to comment on this change (other than my existing comments).

Also, if we do add a flag, is it too verbose to call it --add-codeview-ghash or something for consistency? I found three references to ghash CLI options:

1. clang uses --gcodeview-ghash
2. llvm-readobj uses --codeview-ghash
3. lld uses /debug:ghash

[jhenderson]

In D59025#1428431, @rupprecht wrote:

Anyway, I'm not really that opposed to this change, I just want to figure out what's the threshold for adding flags like this. It's probably totally fine, especially as it seems fairly isolated. I added a few other reviewers who might have an idea. I've never heard of ghashes, so I might not be the best person to comment on this change (other than my existing comments).

FWIW, my opinion is that there is nothing wrong with adding new flags that don't otherwise exist in GNU objcopy as long as:

1. They don't have a short alias, or a name that is likely to clash with a new switch in GNU objcopy in the future.
2. There's a real-world use-case (which could be as simple as significantly simplifying our own testing or whatever).
3. The change isn't too invasive, or if it is, there's a massive use-case to make the trade-off worthwhile.

[aganea]

In D59025#1433050, @jhenderson wrote:

2. There's a real-world use-case (which could be as simple as significantly simplifying our own testing or whatever).

There's a strong industrial use-case here, which is to use MSVC for compilation and LLD for linking.
MSVC currently doesn't support (yet?) .debug$H streams, and this patch provides a workaround. This inherently makes linking with LLD faster, by allowing compilation with MSVC + .debug$H computation on remote PCs (when using distributed compilation).
Without this patch, .debug$H computation is done locally by LLD, which significantly increases link time.

In D59025#1428431, @rupprecht wrote:

I've never heard of ghashes

Worth the read: http://blog.llvm.org/2018/01/

[santagada]

Just discovered this doesn't work when using a pch... it gets into an infinite loop trying to hash types, the reason I think that there are types in the .debug$T asking for indexes on the .debug$T of the pch. Somehow I need to fix this... just don't know how yet

[aganea]

In D59025#1436238, @santagada wrote:

Just discovered this doesn't work when using a pch... it gets into an infinite loop trying to hash types, the reason I think that there are types in the .debug$T asking for indexes on the .debug$T of the pch. Somehow I need to fix this... just don't know how yet

Yeah PCH OBJs are a bit tricker. You need to first hash the PCH.OBJ (compiled with /Yc), and make it available in memory, along with the dependent OBJ (compiled with /Yu) that you want to hash.
Just before hashing the /Yu OBJ, you need to replace the first LF_PRECOMP record with the .debug$P stream (which is a .debug$T in disguise). It is only then that you can hash the rest of the /Yu OBJ (you won't need to re-hash the PCH.OBJ stream again).

You can check PDBLinker::mergeInPrecompHeaderObj() to see how things are done in that regards in LLD.
I am currently refactoring all that code to make it suitable for parallelization. I'll see if we can raise it at a higher level into llvm/trunk/llvm/lib/DebugInfo/CodeView

Can you wait a bit until all this goes through?

[santagada]

I can, but looking at the code for PDBLinker::mergeInPrecompHeaderObj and the code that calls it, seems like lld is adding the precompiled header multiple times on the output pdb, there is no checks to make sure it only emits the data once.

Also on the --add-ghashes case seems like I would want to find the file in L_precomp, load it but never save the hashes to the pch types to the obj that reference them.

For a final question, does clang generates a .debug$H to precompiled headers? Because if so I probably want to support adding .debug$H to pch files as well.

[aganea]

In D59025#1436344, @santagada wrote:

I can, but looking at the code for PDBLinker::mergeInPrecompHeaderObj and the code that calls it, seems like lld is adding the precompiled header multiple times on the output pdb, there is no checks to make sure it only emits the data once.

When merging a /Yu OBJ, there's this subtlety which prevents the prepended PCH stream to be merged in again.

Also on the --add-ghashes case seems like I would want to find the file in L_precomp, load it but never save the hashes to the pch types to the obj that reference them.

link.exe requires that the file referenced by LF_PRECOMP is explicty passed on the cmd-line. So the linker only uses the name, not the path. LLD does that too. We should probably retain that behavior in llvm-objcopy as well.
So if a.obj was compiled with /Yu, one would have to say: llvm-objcopy --add-codeview-ghash a.obj precomp.obj
If precomp.obj isn't there on the cmd-line, it should throw pdb::pdb_error_code::external_cmdline_ref like LLD does.
If precomp.obj has a .debug$H stream, it should just skip it (validate maybe if the hash format maches, re-generate hashes otherwise) and only hash a.obj in that case.

For a final question, does clang generates a .debug$H to precompiled headers? Because if so I probably want to support adding .debug$H to pch files as well.

AFAIK, clang does not generate precompiled headers OBJs like Microsoft does. Precompiled headers in clang are only for the compiler front-end. Which means there's no LF_PRECOMP / .debug$P streams generated either.
But you're right, like I mentionned above, we should add a .debug$H stream to Microsoft PCH.OBJ as well.

[aganea]

Hello @santagada ! Do you think your could rebase and address the comments? I think it'd be good to merge this. We could refactor the duplicated code (between this and LLD) afterwards.

llvm/tools/llvm-objcopy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
+  DebugInfoCodeView
   Object
   Option
   Support
   MC
   )
 
 set(LLVM_TARGET_DEFINITIONS ObjcopyOpts.td)
 tablegen(LLVM ObjcopyOpts.inc -gen-opt-parser-defs)

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 //===- COFFObjcopy.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "COFFObjcopy.h"
 #include "Buffer.h"
 #include "CopyConfig.h"
 #include "Object.h"
 #include "Reader.h"
 #include "Writer.h"
 #include "llvm-objcopy.h"
 
+#include "llvm/DebugInfo/CodeView/TypeHashing.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/COFF.h"
+#include "llvm/Support/BinaryStreamWriter.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/JamCRC.h"
 #include "llvm/Support/Path.h"
 #include <cassert>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
+using namespace codeview;
 
 static bool isDebugSection(const Section &Sec) {
   return Sec.Name.startswith(".debug");
 }
 
 static uint64_t getNextRVA(const Object &Obj) {
   if (Obj.getSections().empty())
     return 0;
static void addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
   // Sec.Header.NumberOfRelocations is filled in by the writer.
   Sec.Header.NumberOfLinenumbers = 0;
   Sec.Header.Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
                                IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_DISCARDABLE;
   Sections.push_back(Sec);
   Obj.addSections(Sections);
 }
 
+static ArrayRef<uint8_t> consumeDebugMagic(ArrayRef<uint8_t> Data,
+                                           StringRef SecName) {
+  // First 4 bytes are section magic.
+  if (Data.size() < 4)
+    error(SecName + " too short");
+  if (support::endian::read32le(Data.data()) != COFF::DEBUG_SECTION_MAGIC)
+    error(SecName + " has an invalid magic");
+  return Data.slice(4);
+}
+
+const std::vector<GloballyHashedType> mergeDebugT(const Section *Sec) {
+  std::vector<GloballyHashedType> OwnedHashes;
+
+  ArrayRef<uint8_t> Data = Sec->getContents();
+  Data = consumeDebugMagic(Data, ".debug$T");
+  if (Data.empty())
+    return OwnedHashes;
+
+  BinaryByteStream Stream(Data, support::little);
+  CVTypeArray Types;
+  BinaryStreamReader Reader(Stream);
+  if (auto EC = Reader.readArray(Types, Reader.getLength()))
+    error("Reader::readArray failed: " + toString(std::move(EC)));
+
+  OwnedHashes = GloballyHashedType::hashTypes(Types);
+  return OwnedHashes;
+}
+
+ArrayRef<uint8_t> toDebugH(const std::vector<GloballyHashedType> hashes,
+                           BumpPtrAllocator &Alloc) {
+  uint32_t Size = 8 + 8 * hashes.size();
+  uint8_t *Data = Alloc.Allocate<uint8_t>(Size);
+  MutableArrayRef<uint8_t> Buffer(Data, Size);
+  BinaryStreamWriter Writer(Buffer, llvm::support::little);
+  uint32_t magic = COFF::DEBUG_HASHES_SECTION_MAGIC;
+  cantFail(Writer.writeInteger(magic));
+  cantFail(Writer.writeInteger(uint16_t(0)));
+  cantFail(Writer.writeInteger(uint16_t(GlobalTypeHashAlg::SHA1_8)));
+  for (const auto &H : hashes) {
+    cantFail(Writer.writeBytes(H.Hash));
+  }
+  assert(Writer.bytesRemaining() == 0);
+  return Buffer;
+}
+
+static void addGHashes(Object &Obj) {
+  std::vector<GloballyHashedType> hashes;
+
+  bool found_types = false;
+
+  for (Section section: Obj.getSections()) {
+    if (section.Name == ".debug$H")
+      error("Already has .debug$H");
+
+    if (section.Name == ".debug$T") {
+      hashes = mergeDebugT(&section);
+    found_types = true;
+      break;
+    }
+  }
+
+  if (!found_types)
+    return;
+
+  std::vector<Section> Sections;
+  Section Sec;
+  BumpPtrAllocator Allocator;
+  Sec.setOwnedContents(toDebugH(hashes, Allocator));
+  Sec.Name = ".debug$H";
+  Sec.Header.VirtualSize = 0;
+  Sec.Header.VirtualAddress = 0;
+  Sec.Header.SizeOfRawData = Sec.getContents().size();
+  // Sec.Header.PointerToRawData is filled in by the writer.
+  Sec.Header.PointerToRelocations = 0;
+  Sec.Header.PointerToLinenumbers = 0;
+  // Sec.Header.NumberOfRelocations is filled in by the writer.
+  Sec.Header.NumberOfLinenumbers = 0;
+  Sec.Header.Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
+                               IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_DISCARDABLE | IMAGE_SCN_ALIGN_4BYTES;
+  std::vector<Symbol> Symbols;
+
+  Sections.push_back(Sec);
+  Obj.addSections(Sections);
+}
+
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
     // Contrary to --only-keep-debug, --only-section fully removes sections that
     // aren't mentioned.
     if (!Config.OnlySection.empty() &&
         !is_contained(Config.OnlySection, Sec.Name))
       return true;
Obj.removeSymbols([&](const Symbol &Sym) {
     }
 
     return false;
   });
 
   if (!Config.AddGnuDebugLink.empty())
     addGnuDebugLink(Obj, Config.AddGnuDebugLink);
 
+  if (Config.AddGHashes)
+    addGHashes(Obj);
+
   if (!Config.BuildIdLinkDir.empty() || Config.BuildIdLinkInput ||
       Config.BuildIdLinkOutput || !Config.SplitDWO.empty() ||
       !Config.SymbolsPrefix.empty() || !Config.AddSection.empty() ||
       !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
       !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
       !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
       !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
       !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty() ||

llvm/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
 
   // ELF entry point address expression. The input parameter is an entry point
   // address in the input ELF file. The entry address in the output file is
   // calculated with EntryExpr(input_address), when either --set-start or
   // --change-start is used.
   std::function<uint64_t(uint64_t)> EntryExpr;
 
   // Boolean options
+  bool AddGHashes = false;
   bool DeterministicArchives = true;
   bool ExtractDWO = false;
   bool KeepFileSymbols = false;
   bool LocalizeHidden = false;
   bool OnlyKeepDebug = false;
   bool PreserveDates = false;
   bool StripAll = false;
   bool StripAllGNU = false;

llvm/tools/llvm-objcopy/CopyConfig.cpp

if (Arg->getOption().getID() == OBJCOPY_compress_debug_sections_eq) {
       if (!zlib::isAvailable())
         return createStringError(
             errc::invalid_argument,
             "LLVM was not compiled with LLVM_ENABLE_ZLIB: can not compress");
     }
   }
 
   Config.AddGnuDebugLink = InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
+  Config.AddGHashes = InputArgs.hasArg(OBJCOPY_add_ghashes);
   Config.BuildIdLinkDir = InputArgs.getLastArgValue(OBJCOPY_build_id_link_dir);
   if (InputArgs.hasArg(OBJCOPY_build_id_link_input))
     Config.BuildIdLinkInput =
         InputArgs.getLastArgValue(OBJCOPY_build_id_link_input);
   if (InputArgs.hasArg(OBJCOPY_build_id_link_output))
     Config.BuildIdLinkOutput =
         InputArgs.getLastArgValue(OBJCOPY_build_id_link_output);
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);

llvm/tools/llvm-objcopy/ObjcopyOpts.td

 def preserve_dates : Flag<["-", "--"], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 def p : Flag<["-"], "p">, Alias<preserve_dates>;
 
 defm add_gnu_debuglink
     : Eq<"add-gnu-debuglink", "Add a .gnu_debuglink for <debug-file>">,
       MetaVarName<"debug-file">;
 
+def add_ghashes : Flag<["-", "--"], "add-ghashes">,
+                  HelpText<"Add .h global hashes">;
+
 defm remove_section : Eq<"remove-section", "Remove <section>">,
                       MetaVarName<"section">;
 def R : JoinedOrSeparate<["-"], "R">, Alias<remove_section>;
 
 defm rename_section
     : Eq<"rename-section",
          "Renames a section from old to new, optionally with specified flags. "
          "Flags supported for GNU compatibility: alloc, load, noload, "