llvm-link: Add support for archive files as inputs.

[jsjodin]

Summary

This patch adds support for archive files as inputs to llvm-link. One of the use-cases is for OpenMP where device specific libraries need to be extracted from libraries containing bundled object files.
The clang-offload-bundler will support extracting these archives, which will be passed into llvm-link, see https://reviews.llvm.org/D80816.

[jdoerfert]

Can you add a test for a broken archive.

[jsjodin]

In D81109#2072178, @jdoerfert wrote:

Can you add a test for a broken archive.

Sure, I can use a non-archive file as an archive. I should also see what happens if .ll files are used inside an archive, I remember something bad happened when I tried it at some point.

[jdoerfert]

In D81109#2072395, @jsjodin wrote:

In D81109#2072178, @jdoerfert wrote:

Can you add a test for a broken archive.

Sure, I can use a non-archive file as an archive. I should also see what happens if .ll files are used inside an archive, I remember something bad happened when I tried it at some point.

A test and, if possible, an error message is probably preferable to "something bad" ;)

[jsjodin]

In D81109#2072453, @jdoerfert wrote:

In D81109#2072395, @jsjodin wrote:

In D81109#2072178, @jdoerfert wrote:

Can you add a test for a broken archive.

Sure, I can use a non-archive file as an archive. I should also see what happens if .ll files are used inside an archive, I remember something bad happened when I tried it at some point.

A test and, if possible, an error message is probably preferable to "something bad" ;)

XXX Yes, I want it to work with .ll files as well, but there is an issue with memory buffers not being null terminated when they are supposed to be. I'm looking into this. XXX

[Edit]. I take that back, if it is not a bitcode file in the archive it is better to error out, otherwise a copy of the buffer will have to be created just to null terminate it. That can be fixed in a separate patch, so this does not grow too large. I will add this as a negative test.

[jdoerfert]

I will add this as a negative test.

Sounds good to me.

[jsjodin]

Added check that archive members are bitcode files, with a negative test. Added a negative test for a broken archive. Note: The error message "file too small to be an archive" is given regardless of the size of the input file. The error is reported if the archive library does not recognize the file.

[jdoerfert]

If you think it's not too much work you could add negative test cases for other broken modules, it seems some errors are not hit so far. I also left a high level comment below.

All in all I think this is fine though. I wait a bit for anyone to chime in before I accept this.

llvm/tools/llvm-link/llvm-link.cpp

209

High level: The code is pretty "unqualified auto" heavy. While I guess this is neither performance critical nor too important, I would recommend to either specify some types where it might be helpful and use qualifiers, const, *, & where applicable.

[jsjodin]

llvm/tools/llvm-link/llvm-link.cpp

209

I had specified some types before submitting the patch, but changed them to auto, since it looked like it was the preferred way of doing things. I can specify some types to make the code easier to follow.

[mehdi_amini]

llvm/tools/llvm-link/llvm-link.cpp

209

The LLVM style is supposed to avoid auto when the type isn't obvious from the context or it helps readability.
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable : "Use auto if and only if it makes the code more readable or easier to maintain."

[jsjodin]

llvm/tools/llvm-link/llvm-link.cpp

209

Okay, thanks for pointing it out. I will update the patch.

[gregrodgers]

LGTM, only one new auto in an existing code with 11 previous autos. Looks like consistent usage.

[JonChesterfield]

I really like this. I've wanted to pass archives of bitcode to llvm-link before and assume others have experienced the same disappointment.

The patch is straightforward, conservatively aborts if the archive has a mix of files in it, uses the existing parsing machinery. Understanding .ll files as well would be a nice future extension.

llvm-link raises reasonable error messages usually, I think this should hit the same handling. Maybe add a test case where the two bitcode files have different triples and check we see the usual error reported?

llvm/tools/llvm-link/llvm-link.cpp

14

Double //, should be llvm/Object

162

double negative - perhaps " failed to read name of archive member"?

354

May be better to read the first few bytes, looking for '!<arch>', instead of going by extension. Don't have strong preferences though.

[ABataev]

llvm/tools/llvm-link/llvm-link.cpp

143

Argv0. Also, better to use StringRef, if possible

144

const std::string &

157

Better to use the real type here rather than auto

158–159

Same here about autos

[jsjodin]

Removed some auto declarations to specific types.

[jsjodin]

Fixed "//" in #include, fixed the parameters to loadArFile, fixed error message.

[jsjodin]

llvm/tools/llvm-link/llvm-link.cpp

14

Not sure how that got there. Fixed.

143

StringRef didn't simplify anything, so I kept the type, but fixed the name.

354

Maybe, but for now I will keep it as-is. If we find this to be a problem in the future it can be fixed then.

[ABataev]

llvm/tools/llvm-link/llvm-link.cpp

143

StringRef is not about simplification, this is common practice in LLVM to use it instead of const char *.

202–203

Just

if (L.linkModules(*Result, std::move(M), ApplicableFlags))
  return nullptr;

207

Does it really worth using std::move here and in other places?

[jsjodin]

Removed local Err variable.

[jsjodin]

llvm/tools/llvm-link/llvm-link.cpp

207

Yes, otherwise you get a compilation error.

[jsjodin]

If Mehdi or someone else outside AMD could approve (or suggest more changes) that would be helpful. Thanks!

[ABataev]

llvm/tools/llvm-link/llvm-link.cpp

167

Why not StringRef?

[mehdi_amini]

In D81109#2089969, @gregrodgers wrote:

LGTM, only one new auto in an existing code with 11 previous autos. Looks like consistent usage.

Some previous reviewers not catching deviation from the coding standard does not seem like a justification to continue this route, but rather an indication that the rest of the code could be updated.
Also the sheer number of uses of auto does not say anything: it is all about *how* it is used.

[mehdi_amini]

LGTM (after remaining minor inline comments)

llvm/tools/llvm-link/llvm-link.cpp

14

Includes are sorted alphabetically, clang format should do this for you (run git clang-format HEAD~...HEAD to format only your diff)

176

Nit: you're missing a space between "failed" and '

187

Nit: in other places the error message include the Archive name, I don't know if it was intentional to omit it here.

196

(Same missing space here)

[jdoerfert]

FWIW, LGTM too.

[JonChesterfield]

Where's the right place to document this new feature?

It's occurred to me that people may want to use llvm-link as a drop in replacement for a partial link, and this patch implements the equivalent to --whole-archive. That is, we extract all the objects, not only the ones that match an unresolved symbol earlier in the link order.

That'll surprise some users so I think we should write down that this is the behaviour they'll get, and I'm not sure where.

[jdoerfert]

In D81109#2097795, @JonChesterfield wrote:

Where's the right place to document this new feature?

It's occurred to me that people may want to use llvm-link as a drop in replacement for a partial link, and this patch implements the equivalent to --whole-archive. That is, we extract all the objects, not only the ones that match an unresolved symbol earlier in the link order.

That is how llvm-link always worked, I mean the "whole-archive" way, isn't it?

That'll surprise some users so I think we should write down that this is the behaviour they'll get, and I'm not sure where.

LLVM-link documentation + Change Logs
@jsjodin ^

llvm/test/tools/llvm-link/Inputs/f.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @f() {
+entry:
+  ret void
+}

llvm/test/tools/llvm-link/Inputs/g.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @g() {
+entry:
+  ret void
+}

llvm/test/tools/llvm-link/Inputs/h.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @h() {
+entry:
+  ret void
+}

llvm/test/tools/llvm-link/archive-bad.ll

+# RUN: cp %S/Inputs/f.ll %t.fg.a
+# RUN: not llvm-link %S/Inputs/h.ll %t.fg.a -o %t.linked.bc 2>&1 | FileCheck %s
+
+# RUN: rm -f %t.fg.a
+# RUN: rm -f %t.linked.bc
+
+# CHECK: file too small to be an archive

llvm/test/tools/llvm-link/archive.ll

+# RUN: llvm-as %S/Inputs/f.ll -o %t.f.bc
+# RUN: llvm-as %S/Inputs/g.ll -o %t.g.bc
+# RUN: llvm-ar cr %t.fg.a %t.f.bc %t.g.bc
+# RUN: llvm-ar cr %t.empty.a
+# RUN: llvm-link %S/Inputs/h.ll %t.fg.a %t.empty.a -o %t.linked.bc
+
+# RUN: llvm-nm %t.linked.bc | FileCheck %s
+
+# RUN: rm -f %t.f.bc
+# RUN: rm -f %t.g.bc
+# RUN: rm -f %t.fg.a
+# RUN: rm -f %t.empty.a
+# RUN: rm -f %t.linked.bc
+
+# CHECK: -------- T f
+# CHECK: -------- T g
+# CHECK: -------- T h

llvm/test/tools/llvm-link/archivell.ll

+# RUN: llvm-ar cr %t.fg.a %S/Inputs/f.ll llvm-as %S/Inputs/g.ll
+# RUN: not llvm-link %S/Inputs/h.ll %t.fg.a -o %t.linked.bc 2>&1 | FileCheck %s
+
+# RUN: rm -f %t.fg.a
+# RUN: rm -f %t.linked.bc
+
+# CHECK: error: member of archive is not a bitcode file

llvm/tools/llvm-link/llvm-link.cpp

 //===- llvm-link.cpp - Low-level LLVM linker ------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This utility may be invoked in the following manner:
 //  llvm-link a.bc b.bc c.bc -o x.bc
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/Object/Archive.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/IR/AutoUpgrade.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
static std::unique_ptr<Module> loadFile(const char *argv0,
   if (MaterializeMetadata) {
     ExitOnErr(Result->materializeMetadata());
     UpgradeDebugInfo(*Result);
   }
 
   return Result;
 }
 
+static std::unique_ptr<Module> loadArFile(const char *Argv0,
+                                          const std::string &ArchiveName,
+                                          LLVMContext &Context, Linker &L,
+                                          unsigned OrigFlags,
+                                          unsigned ApplicableFlags) {
+  std::unique_ptr<Module> Result(new Module("ArchiveModule", Context));
+  if (Verbose)
+    errs() << "Reading library archive file '" << ArchiveName
+           << "' to memory\n";
+  ErrorOr<std::unique_ptr<MemoryBuffer>> Buf =
+    MemoryBuffer::getFile(ArchiveName, -1, false);
+  ExitOnErr(errorCodeToError(Buf.getError()));
+  Error Err = Error::success();
+  object::Archive Archive(Buf.get()->getMemBufferRef(), Err);
+  ExitOnErr(std::move(Err));
+  for (const object::Archive::Child &C : Archive.children(Err)) {
+    Expected<StringRef> Ename = C.getName();
+    if (Error E = Ename.takeError()) {
+      errs() << Argv0 << ": ";
+      WithColor::error()
+          << " failed to read name of archive member"
+          << ArchiveName << "'\n";
+      return nullptr;
+    };
+    std::string ChildName = Ename.get().str();
+    if (Verbose)
+      errs() << "Parsing member '" << ChildName
+             << "' of archive library to module.\n";
+    SMDiagnostic ParseErr;
+    Expected<MemoryBufferRef> MemBuf = C.getMemoryBufferRef();
+    if (Error E = MemBuf.takeError()) {
+      errs() << Argv0 << ": ";
+      WithColor::error() << " loading memory for member '" << ChildName
+                         << "' of archive library failed'" << ArchiveName
+                         << "'\n";
+      return nullptr;
+    };
+
+    if (!isBitcode(reinterpret_cast<const unsigned char *>
+                   (MemBuf.get().getBufferStart()),
+                   reinterpret_cast<const unsigned char *>
+                   (MemBuf.get().getBufferEnd()))) {
+      errs() << Argv0 << ": ";
+      WithColor::error() << "  member of archive is not a bitcode file: '"
+                         << ChildName << "'\n";
+      return nullptr;
+    }
+
+    std::unique_ptr<Module> M = parseIR(MemBuf.get(), ParseErr, Context);
+
+    if (!M.get()) {
+      errs() << Argv0 << ": ";
+      WithColor::error() << " parsing member '" << ChildName
+                         << "' of archive library failed'" << ArchiveName
+                         << "'\n";
+      return nullptr;
+    }
+    if (Verbose)
+      errs() << "Linking member '" << ChildName << "' of archive library.\n";
+    bool Err = L.linkModules(*Result, std::move(M), ApplicableFlags);
+    if (Err)
+      return nullptr;
+    ApplicableFlags = OrigFlags;
+  } // end for each child
+  ExitOnErr(std::move(Err));
+  return Result;
+}
+
 namespace {
 
 /// Helper to load on demand a Module from file and cache it for subsequent
 /// queries during function importing.
 class ModuleLazyLoaderCache {
   /// Cache of lazily loaded module for import.
   StringMap<std::unique_ptr<Module>> ModuleMap;
 

 static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
                       const cl::list<std::string> &Files,
                       unsigned Flags) {
   // Filter out flags that don't apply to the first file we load.
   unsigned ApplicableFlags = Flags & Linker::Flags::OverrideFromSrc;
   // Similar to some flags, internalization doesn't apply to the first file.
   bool InternalizeLinkedSymbols = false;
   for (const auto &File : Files) {
-    std::unique_ptr<Module> M = loadFile(argv0, File, Context);
+    std::unique_ptr<Module> M =
+      (llvm::sys::path::extension(File) == ".a")
+          ? loadArFile(argv0, File, Context, L, Flags, ApplicableFlags)
+          : loadFile(argv0, File, Context);
     if (!M.get()) {
       errs() << argv0 << ": ";
       WithColor::error() << " loading file '" << File << "'\n";
       return false;
     }
 
     // Note that when ODR merging types cannot verify input files in here When
     // doing that debug metadata in the src module might already be pointing to