[llvm-objcopy] Cache gnu_debuglink's target CRC

[janisozaur]

Summary

.gnu_debuglink section contains information regarding file with
debugging symbols, identified by its CRC32. This target file is not
intended to ever change or it would invalidate the stored checksum, yet
the checksum is calculated over and over again for each of the objects
inside the archive, usually hundreds of times.

This patch precomputes the CRC32 of the target once and then reuses the
value where required, saving lots of redundant I/O.

The error message reported should stay the same, although now it might
be reported earlier.

[jhenderson]

Whilst I don't necessarily object to this change, I do wonder as to its necessity. Have you seen this as an issue in practice? My understanding of the .gnu_debuglink section is that there should be a single entry in it in an executable, pointing at a single file containing debug information for that executable. Adding the section to relocatable object files (and therefore to archive members) makes no sense, since you don't debug those.

llvm/tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #197450)

Is "no_such_file_or_directory" the only way MemoryBuffer::getFile can file? What about things like "it's a directory" or "access denied"? If it isn't the only way, we shouldn't be using this misleading error code.

[janisozaur]

In D61343#1485793, @jhenderson wrote:

Whilst I don't necessarily object to this change, I do wonder as to its necessity. Have you seen this as an issue in practice?

Yes. For a given foo.a archive and its debug symbols foo.a.debug, try executing llvm-objcopy --add-gnu-debuglink=foo.a.debug foo.a. In my tests with ~130MiB of debug symbols and 300 entries in the archive, it takes about 1 minute 25 seconds to complete without this patch and less than 500ms with this patch, producing SHA1-identical output.

My understanding of the .gnu_debuglink section is that there should be a single entry in it in an executable, pointing at a single file containing debug information for that executable. Adding the section to relocatable object files (and therefore to archive members) makes no sense, since you don't debug those.

I have not investigated why is it being added to each member of the archive, but I can imagine when you create a static archive, there is no section to place this information and only object files exist which are capable of holding such information. Then, when you link said archive to executable, linker is free to only select and use the object files actually in use, hence each of them should carry relevant information.

You can observe very similar behaviour in GNU binutils' objcopy, where I've filed a similar patch.

[janisozaur]

llvm/tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #197450)

This one seemed the most relevant. Originally exit(1) was used explicitly.

All the other option parsing code returns invalid argument (I think it's 22), but it also doesn't verify existence and readability of files.

[jakehehrlich]

This code looks good to me and the use case seems valid. I'll give my LGTM but please wait for others.

llvm/tools/llvm-objcopy/CopyConfig.cpp

494–498 ↗

(On Diff #197450)

This comment reads a bit aggressive to me. Maybe something like "We can avoid recalculating the checksum for every target file inside an archive by precomputing the CRC here. This prevents a significant amount of I/O."

[janisozaur]

Updated comment as per review request

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #197450)

Okay. How about we just use the error code stored in DebugOrErr?

[MaskRay]

llvm/tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #197450)

return errorCodeToError(DebugOrErr.getError());

[MaskRay]

llvm/tools/llvm-objcopy/CopyConfig.cpp

502 ↗

(On Diff #197450)

return createFileError(Config.AddGnuDebugLink, DebugOrErr.getError());

[janisozaur]

Update reported error

[MaskRay]

llvm/tools/llvm-objcopy/CopyConfig.cpp

508 ↗

(On Diff #197735)

Since you are moving the comment, may I ask you to delete It also doesn't negate the initial CRC32 value but it starts by default at 0xFFFFFFFF which is the complement of zero. This sentence if verbose and doesn't appear to help a casual reader understand the rationale.

[janisozaur]

Remove redundant part of comment

[janisozaur]

Simple rebase + removal of unrelated formatting change

[jhenderson]

LGTM.

[janisozaur]

Ping. Is there still anything else to improve here?

[jhenderson]

In D61343#1500501, @janisozaur wrote:

Ping. Is there still anything else to improve here?

I don't think so. You've addressed all the outstanding comments, got two LGTMs, and have given it more than enough time. This can go in when you are ready to commit.

[janisozaur]

Is this an action I can take? This is first time I'm using Phabricator and I can't see any button to submit the change myself. I would expect this is something project maintainers need to take care of.

[jhenderson]

In D61343#1500895, @janisozaur wrote:

Is this an action I can take? This is first time I'm using Phabricator and I can't see any button to submit the change myself. I would expect this is something project maintainers need to take care of.

Do you have commit access for LLVM? If not, I can commit it later today.

[janisozaur]

I doubt I have such access. Can you take over?

[jhenderson]

In D61343#1500916, @janisozaur wrote:

I doubt I have such access. Can you take over?

Sure, no problem.

llvm/trunk/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
 
   // Only applicable for --input-format=binary
   MachineInfo BinaryArch;
   // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
   Optional<MachineInfo> OutputArch;
 
   // Advanced options
   StringRef AddGnuDebugLink;
+  // Cached gnu_debuglink's target CRC
+  uint32_t GnuDebugLinkCRC32;
   StringRef BuildIdLinkDir;
   Optional<StringRef> BuildIdLinkInput;
   Optional<StringRef> BuildIdLinkOutput;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
   StringRef AllocSectionsPrefix;
   DiscardType DiscardMode = DiscardType::None;
 

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/JamCRC.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
 namespace {
if (auto Arg = InputArgs.getLastArg(OBJCOPY_compress_debug_sections,
     }
     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);
+  // The gnu_debuglink's target is expected to not change or else its CRC would
+  // become invalidated and get rejected. We can avoid recalculating the
+  // checksum for every target file inside an archive by precomputing the CRC
+  // here. This prevents a significant amount of I/O.
+  if (!Config.AddGnuDebugLink.empty()) {
+    auto DebugOrErr = MemoryBuffer::getFile(Config.AddGnuDebugLink);
+    if (!DebugOrErr)
+      return createFileError(Config.AddGnuDebugLink, DebugOrErr.getError());
+    auto Debug = std::move(*DebugOrErr);
+    JamCRC CRC;
+    CRC.update(
+        ArrayRef<char>(Debug->getBuffer().data(), Debug->getBuffer().size()));
+    // The CRC32 value needs to be complemented because the JamCRC doesn't
+    // finalize the CRC32 value.
+    Config.GnuDebugLinkCRC32 = ~CRC.getCRC();
+  }
   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/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

for (const auto &Flag : Config.DumpSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = dumpSectionToFile(SecName, File, Obj))
       return createFileError(Config.InputFilename, std::move(E));
   }
 
   if (!Config.AddGnuDebugLink.empty())
-    Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
+    Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
+                                        Config.GnuDebugLinkCRC32);
 
   for (const NewSymbolInfo &SI : Config.SymbolsToAdd) {
     SectionBase *Sec = Obj.findSection(SI.SectionName);
     uint64_t Value = Sec ? Sec->Addr + SI.Value : SI.Value;
     Obj.SymbolTable->addSymbol(
         SI.SymbolName, SI.Bind, SI.Type, Sec, Value, SI.Visibility,
         Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
   }

llvm/trunk/tools/llvm-objcopy/ELF/Object.h

 #include "CopyConfig.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/FileOutputBuffer.h"
-#include "llvm/Support/JamCRC.h"
 #include <cstddef>
 #include <cstdint>
 #include <functional>
 #include <memory>
 #include <set>
 #include <vector>
 
 namespace llvm {

 
 class GnuDebugLinkSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
 private:
   StringRef FileName;
   uint32_t CRC32;
 
-  void init(StringRef File, StringRef Data);
+  void init(StringRef File);
 
 public:
   // If we add this section from an external source we can use this ctor.
-  explicit GnuDebugLinkSection(StringRef File);
+  explicit GnuDebugLinkSection(StringRef File, uint32_t PrecomputedCRC);
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 };
 
 class Reader {
 public:
   virtual ~Reader();
   virtual std::unique_ptr<Object> create() const = 0;

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

LinkSection =
                                       " in section " + Name + " is invalid");
     if (LinkSection->Type == ELF::SHT_SYMTAB)
       LinkSection = nullptr;
   }
 }
 
 void Section::finalize() { this->Link = LinkSection ? LinkSection->Index : 0; }
 
-void GnuDebugLinkSection::init(StringRef File, StringRef Data) {
+void GnuDebugLinkSection::init(StringRef File) {
   FileName = sys::path::filename(File);
   // The format for the .gnu_debuglink starts with the file name and is
   // followed by a null terminator and then the CRC32 of the file. The CRC32
   // should be 4 byte aligned. So we add the FileName size, a 1 for the null
   // byte, and then finally push the size to alignment and add 4.
   Size = alignTo(FileName.size() + 1, 4) + 4;
   // The CRC32 will only be aligned if we align the whole section.
   Align = 4;
   Type = ELF::SHT_PROGBITS;
   Name = ".gnu_debuglink";
   // For sections not found in segments, OriginalOffset is only used to
   // establish the order that sections should go in. By using the maximum
   // possible offset we cause this section to wind up at the end.
   OriginalOffset = std::numeric_limits<uint64_t>::max();
-  JamCRC CRC;
-  CRC.update(ArrayRef<char>(Data.data(), Data.size()));
-  // The CRC32 value needs to be complemented because the JamCRC dosn't
-  // finalize the CRC32 value. It also dosn't negate the initial CRC32 value
-  // but it starts by default at 0xFFFFFFFF which is the complement of zero.
-  CRC32 = ~CRC.getCRC();
 }
 
-GnuDebugLinkSection::GnuDebugLinkSection(StringRef File) : FileName(File) {
-  // Read in the file to compute the CRC of it.
-  auto DebugOrErr = MemoryBuffer::getFile(File);
-  if (!DebugOrErr)
-    error("'" + File + "': " + DebugOrErr.getError().message());
-  auto Debug = std::move(*DebugOrErr);
-  init(File, Debug->getBuffer());
+GnuDebugLinkSection::GnuDebugLinkSection(StringRef File,
+                                         uint32_t PrecomputedCRC)
+    : FileName(File), CRC32(PrecomputedCRC) {
+  init(File);
 }
 
 template <class ELFT>
 void ELFSectionWriter<ELFT>::visit(const GnuDebugLinkSection &Sec) {
   unsigned char *Buf = Out.getBufferStart() + Sec.Offset;
   Elf_Word *CRC =
       reinterpret_cast<Elf_Word *>(Buf + Sec.Size - sizeof(Elf_Word));
   *CRC = Sec.CRC32;