[llvm-objdump][Offload] Use common offload extraction method

[jhuber6]

Summary

A previous patch introduced a common function used to extract offloading
binaries from an image. Therefore we no longer need to duplicate the
functionality in the llvm-objdump implementation. Functionally, this
removes the old warning behaviour when given malformed input. This has
been changed to a hard error, which is effectively the same.

This required a slight tweak in the linker wrapper to filter out the
user passing shared objects directly.

[jhenderson]

As a general rule, dumping tools should avoid hard errors, because usually they prevent the dumping tool from continuing to dump other useful and valid information as part of the same execution. I don't have a strong objection in this particular case, because I presume this error could be avoided by simply not dumping the offloading sections, but I wanted to make the context clear.

llvm/lib/Object/OffloadBinary.cpp

261–264

Is there test code showing the ET_EXEC and ET_DYN objects work here?

[jhuber6]

Adding separate tests for ET_REL, ET_EXEC, and ET_DYN.

[jhenderson]

llvm/test/tools/llvm-objdump/Offloading/elf_dynamic.test

1 ↗

(On Diff #471132)

Rather than duplicating this and the ET_REL cases, you can use yaml2obj's -D option to parameterise the ELF type. Rough code:

# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_DYN -o %t.so
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_EXEC -o %t.bin
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_REL -o %t.o

...
  Data: ELFDATA2LSB
  Type: [[TYPE]]
...

llvm/tools/llvm-objdump/OffloadDump.cpp

63–65

Not sure why this was changed.

76

Should this be "contained in this buffer" rather than "at"?

[jhuber6]

Addressing comments.

[jhuber6]

Is this good to land now?

[jhenderson]

In D136796#3903393, @jhuber6 wrote:

Is this good to land now?

The LLVM community practice is to wait a week between pings unless there's something urgent (and if something is urgent, please say so).

Aside from the clang bit of the patch, this seems good to me, but it would be worth another person involved with offloading to give the final thumbs up.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

1262–1265

This change seems not really part of the patch, since it's touching clang but the patch is for llvm-objdump.

[jhuber6]

In D136796#3904512, @jhenderson wrote:

In D136796#3903393, @jhuber6 wrote:

Is this good to land now?

The LLVM community practice is to wait a week between pings unless there's something urgent (and if something is urgent, please say so).

Aside from the clang bit of the patch, this seems good to me, but it would be worth another person involved with offloading to give the final thumbs up.

I could precommit that part if it would make it cleaner, but since this patch changes the extract routine to handle relocatable objects we need to filter those out of the input since the linker wrapper can't handle them.

[tra]

LGTM, modulo separating clang-related change into a separate patch.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

1262–1265

+1 to landing this separately, possibly with a relevant test.

[jhuber6]

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

1262–1265

I put it in a separate commit but it seems arcanist squashed it in the background.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

for (const opt::Arg *Arg : Args.filtered(OPT_INPUT)) {
     if (!sys::fs::exists(Filename) || sys::fs::is_directory(Filename))
       continue;
 
     ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
         MemoryBuffer::getFileOrSTDIN(Filename);
     if (std::error_code EC = BufferOrErr.getError())
       return createFileError(Filename, EC);
 
+    if (identify_magic((*BufferOrErr)->getBuffer()) ==
+        file_magic::elf_shared_object)
+      continue;
+
     bool IsLazy =
         identify_magic((*BufferOrErr)->getBuffer()) == file_magic::archive;
     if (Error Err = extractOffloadBinaries(
             **BufferOrErr, IsLazy ? LazyInputFiles : InputFiles))
       return std::move(Err);
   }
 
   // Try to extract input from input archive libraries.

llvm/lib/Object/OffloadBinary.cpp

 Error extractOffloadFiles(MemoryBufferRef Contents,
                           SmallVectorImpl<OffloadFile> &Binaries) {
   uint64_t Offset = 0;
   // There could be multiple offloading binaries stored at this section.
   while (Offset < Contents.getBuffer().size()) {
     std::unique_ptr<MemoryBuffer> Buffer =
         MemoryBuffer::getMemBuffer(Contents.getBuffer().drop_front(Offset), "",
                                    /*RequiresNullTerminator*/ false);
+    if (!isAddrAligned(Align(OffloadBinary::getAlignment()),
+                       Buffer->getBufferStart()))
+      Buffer = MemoryBuffer::getMemBufferCopy(Buffer->getBuffer(),
+                                              Buffer->getBufferIdentifier());
     auto BinaryOrErr = OffloadBinary::create(*Buffer);
     if (!BinaryOrErr)
       return BinaryOrErr.takeError();
     OffloadBinary &Binary = **BinaryOrErr;
 
     // Create a new owned binary with a copy of the original memory.
     std::unique_ptr<MemoryBuffer> BufferCopy = MemoryBuffer::getMemBufferCopy(
         Binary.getData().take_front(Binary.getSize()),

 }
 
 Error object::extractOffloadBinaries(MemoryBufferRef Buffer,
                                      SmallVectorImpl<OffloadFile> &Binaries) {
   file_magic Type = identify_magic(Buffer.getBuffer());
   switch (Type) {
   case file_magic::bitcode:
     return extractFromBitcode(Buffer, Binaries);
-  case file_magic::elf_relocatable: {
+  case file_magic::elf_relocatable:
+  case file_magic::elf_executable:
+  case file_magic::elf_shared_object: {
     Expected<std::unique_ptr<ObjectFile>> ObjFile =
         ObjectFile::createObjectFile(Buffer, Type);
     if (!ObjFile)
       return ObjFile.takeError();
     return extractFromBinary(*ObjFile->get(), Binaries);
   }
   case file_magic::archive: {
     Expected<std::unique_ptr<llvm::object::Archive>> LibFile =
         object::Archive::create(Buffer);

llvm/test/tools/llvm-objdump/Offloading/content-failure.test

 Sections:
   - Name:            .llvm.offloading
     Type:            SHT_LLVM_OFFLOADING
     Flags:           [ SHF_EXCLUDE ]
     Address:         0x0
     ShOffset:        0x99999
     AddressAlign:    0x0000000000000008
 
-# CHECK: error: '[[FILENAME]]': The end of the file was unexpectedly encountered
+# CHECK: error: '[[FILENAME]]': while extracting offloading files: The end of the file was unexpectedly encountered

llvm/test/tools/llvm-objdump/Offloading/elf.test

 ## Check that we can dump an offloading binary directly.
 # RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
 # RUN: llvm-objdump --offloading %t.bin | FileCheck %s --match-full-lines --strict-whitespace --implicit-check-not={{.}}
 
 ## Check that we can dump an offloading binary inside of an ELF section.
-# RUN: yaml2obj %s -o %t.elf
-# RUN: llvm-objcopy --update-section .llvm.offloading=%t.bin %t.elf
-# RUN: llvm-objdump --offloading %t.elf | FileCheck %s --check-prefixes=CHECK,ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
+# RUN: yaml2obj %s -o %t -DTYPE=ET_EXEC
+# RUN: yaml2obj %s -o %t.so -DTYPE=ET_DYN
+# RUN: yaml2obj %s -o %t.o -DTYPE=ET_REL
+# RUN: llvm-objcopy --update-section .llvm.offloading=%t.bin %t
+# RUN: llvm-objcopy --update-section .llvm.offloading=%t.bin %t.so
+# RUN: llvm-objcopy --update-section .llvm.offloading=%t.bin %t.o
+# RUN: llvm-objdump --offloading %t | FileCheck %s --check-prefixes=CHECK,ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
+# RUN: llvm-objdump --offloading %t.so | FileCheck %s --check-prefixes=CHECK,ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
+# RUN: llvm-objdump --offloading %t.o | FileCheck %s --check-prefixes=CHECK,ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
-  Type:            ET_EXEC
+  Type:            [[TYPE]]
 Sections:
   - Name:            .llvm.offloading
     Type:            SHT_LLVM_OFFLOADING
     Flags:           [ SHF_EXCLUDE ]
     AddressAlign:    0x0000000000000008
 
 #        ELF:{{.*}}file format elf64-unknown
 #  ELF-EMPTY:

llvm/test/tools/llvm-objdump/Offloading/warning.test

-## Ensure we give a warning on bad input following good input.
-# RUN: yaml2obj %S/Inputs/binary.yaml -o %t-good.bin
-# RUN: yaml2obj %S/Inputs/malformed.yaml -o %t-bad.bin
-# RUN: cat %t-bad.bin >> %t-good.bin
-# RUN: yaml2obj %s -o %t.elf
-# RUN: llvm-objcopy --update-section .llvm.offloading=%t-good.bin %t.elf
-# RUN: llvm-objdump --offloading %t.elf 2>&1 | FileCheck %s -DFILENAME=%t.elf
-
-!ELF
-FileHeader:
-  Class:           ELFCLASS64
-  Data:            ELFDATA2LSB
-  Type:            ET_EXEC
-Sections:
-  - Name:            .llvm.offloading
-    Type:            SHT_LLVM_OFFLOADING
-    Flags:           [ SHF_EXCLUDE ]
-    AddressAlign:    0x0000000000000008
-
-# CHECK: OFFLOADING IMAGE [0]:
-# CHECK: warning: '[[FILENAME]]': while parsing offloading files: The end of the file was unexpectedly encountered

llvm/tools/llvm-objdump/OffloadDump.cpp

 //===-- OffloadDump.cpp - Offloading dumper ---------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file implements the offloading-specific dumper for llvm-objdump.
 ///
 //===----------------------------------------------------------------------===//
+
 #include "OffloadDump.h"
 #include "llvm-objdump.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/Alignment.h"
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::objdump;
static void printBinary(const OffloadBinary &OB, uint64_t Index) {
   outs() << "\nOFFLOADING IMAGE [" << Index << "]:\n";
   outs() << left_justify("kind", 16) << getImageName(OB) << "\n";
   outs() << left_justify("arch", 16) << OB.getArch() << "\n";
   outs() << left_justify("triple", 16) << OB.getTriple() << "\n";
   outs() << left_justify("producer", 16)
          << getOffloadKindName(OB.getOffloadKind()) << "\n";
 }
 
-static Error visitAllBinaries(const OffloadBinary &OB) {
-  uint64_t Offset = 0;
-  uint64_t Index = 0;
-  while (Offset < OB.getMemoryBufferRef().getBufferSize()) {
-    MemoryBufferRef Buffer =
-        MemoryBufferRef(OB.getData().drop_front(Offset), OB.getFileName());
-    auto BinaryOrErr = OffloadBinary::create(Buffer);
-    if (!BinaryOrErr)
-      return BinaryOrErr.takeError();
-
-    OffloadBinary &Binary = **BinaryOrErr;
-    printBinary(Binary, Index++);
-
-    Offset += Binary.getSize();
-  }
-  return Error::success();
-}
-
 /// Print the embedded offloading contents of an ObjectFile \p O.
 void llvm::dumpOffloadBinary(const ObjectFile &O) {
   if (!O.isELF()) {
     reportWarning("--offloading is currently only supported for ELF targets",
                   O.getFileName());
     return;
   }
 
-  for (ELFSectionRef Sec : O.sections()) {
-    if (Sec.getType() != ELF::SHT_LLVM_OFFLOADING)
-      continue;
-
-    Expected<StringRef> Contents = Sec.getContents();
-    if (!Contents)
-      reportError(Contents.takeError(), O.getFileName());
-
-    std::unique_ptr<MemoryBuffer> Buffer =
-        MemoryBuffer::getMemBuffer(*Contents, O.getFileName(), false);
-    if (!isAddrAligned(Align(OffloadBinary::getAlignment()),
-                       Buffer->getBufferStart()))
-      Buffer = MemoryBuffer::getMemBufferCopy(Buffer->getBuffer(),
-                                              Buffer->getBufferIdentifier());
-    auto BinaryOrErr = OffloadBinary::create(*Buffer);
-    if (!BinaryOrErr)
-      reportError(O.getFileName(), "while extracting offloading files: " +
-                                       toString(BinaryOrErr.takeError()));
-    OffloadBinary &Binary = **BinaryOrErr;
+  SmallVector<OffloadFile> Binaries;
+  if (Error Err = extractOffloadBinaries(O.getMemoryBufferRef(), Binaries))
+                                     toString(std::move(Err)));
 
-    // Print out all the binaries that are contained in this buffer. If we fail
-    // to parse a binary before reaching the end of the buffer emit a warning.
-    if (Error Err = visitAllBinaries(Binary))
-      reportWarning("while parsing offloading files: " +
-                        toString(std::move(Err)),
-                    O.getFileName());
-  }
+  // Print out all the binaries that are contained in this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)
+    printBinary(*Binaries[I].getBinary(), I);
 }
 
 /// Print the contents of an offload binary file \p OB. This may contain
 /// multiple binaries stored in the same buffer.
 void llvm::dumpOffloadSections(const OffloadBinary &OB) {
-  // Print out all the binaries that are contained at this buffer. If we fail to
-  // parse a binary before reaching the end of the buffer emit a warning.
-  if (Error Err = visitAllBinaries(OB))
-    reportWarning("while parsing offloading files: " + toString(std::move(Err)),
-                  OB.getFileName());
+  SmallVector<OffloadFile> Binaries;
+  if (Error Err = extractOffloadBinaries(OB.getMemoryBufferRef(), Binaries))
+    reportError(OB.getFileName(), "while extracting offloading files: " +
+                                      toString(std::move(Err)));
+
+  // Print out all the binaries that are contained in this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)
+    printBinary(*Binaries[I].getBinary(), I);
 }