[ELF] Make subsequent opens to auxiliary files append

[abrachet]

Summary

Previously, the same file could be used across diagnostic options but
the file would be silently overwritten by the whichever option gets
handled last.

[MaskRay]

I wonder whether this behavior is conventional? CC some folks to get some opinions.

@arichardson @ikudrin @smeenai @peter.smith

Note, if we use - as the output (stdout), they actually get appended --print-archive-stats=- --why-extract=-. In this case, supporting this seems fine.

lld/ELF/Config.h

468

I wonder whether openAuxiliaryFile is more appropriate. The file type has a wide ranges including non-diagnostic use cases.

lld/test/ELF/diagnostic-options.test

2

This creates a file in the current directory which is not allowed in some cases, e.g. Google's internal lit runner.

3

You can just use /dev/null as the only input file.

[smeenai]

We don't use this ourselves, but in general, appending seems like a much friendlier behavior than overwriting in this case. Do we need to be concerned about any simultaneous attempted writes to the same file though?

[peter.smith]

FWIW Arm's proprietary linker armlink puts out all diagnostics on one stream under append which includes the map file. It works well although we didn't have to contend with multiple threads interleaving the output.

It looks like the current cases are all in the single threaded path so this shouldn't be a problem.

One possibility that could be worth doing if we can detect that files share the same file is to put some extra line breaks or a header to distinguish between diagnostics. That may not be worth the complexity though.

[MaskRay]

lld/ELF/Config.h

458

This variable and the subject haven't been updated.

lld/test/ELF/diagnostic-options.test

5

## From --print-archive-stats

[abrachet]

One possibility that could be worth doing if we can detect that files share the same file is to put some extra line breaks or a header to distinguish between diagnostics. That may not be worth the complexity though.

The --cref output adds a newline, but does it so unconditionally, so --cref without -Map will print a newline followed by the rest of the output to stdout. Regarding the complexity, because raw_fd_ostream is not copyable/movable I think openAuxiliaryFile would need to first create the raw_fd_ostream, write the newline, then create it again to for return elision. I do think that I don't feel strongly either way, so I'll defer to what others think

[abrachet]

Use %t instead of /dev/null which lit emulates on windows by renaming the file

lld/ELF/Config.h

 //===- Config.h -------------------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_ELF_CONFIG_H
 #define LLD_ELF_CONFIG_H
 
 #include "lld/Common/ErrorHandler.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CachePruning.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include <atomic>
 #include <memory>
 #include <optional>
 #include <vector>
 
 namespace lld::elf {
struct Ctx {
   // A tuple of (reference, extractedFile, sym). Used by --why-extract=.
   SmallVector<std::tuple<std::string, const InputFile *, const Symbol &>, 0>
       whyExtractRecords;
   // A mapping from a symbol to an InputFile referencing it backward. Used by
   // --warn-backrefs.
   llvm::DenseMap<const Symbol *,
                  std::pair<const InputFile *, const InputFile *>>
       backwardReferences;
+  llvm::SmallSet<llvm::StringRef, 0> diagnosticFiles;
   // True if SHT_LLVM_SYMPART is used.
   std::atomic<bool> hasSympart{false};
   // True if there are TLS IE relocations. Set DF_STATIC_TLS if -shared.
   std::atomic<bool> hasTlsIe{false};
   // True if we need to reserve two .got entries for local-dynamic TLS model.
   std::atomic<bool> needsTlsLd{false};
 
   void reset();
+
+  llvm::raw_fd_ostream openAuxiliaryFile(llvm::StringRef, std::error_code &);
 };
 
 LLVM_LIBRARY_VISIBILITY extern Ctx ctx;
 
 // The first two elements of versionDefinitions represent VER_NDX_LOCAL and
 // VER_NDX_GLOBAL. This helper returns other elements.
 static inline ArrayRef<VersionDefinition> namedVersionDefs() {
   return llvm::ArrayRef(config->versionDefinitions).slice(2);

lld/ELF/Driver.cpp

void Ctx::reset() {
   duplicates.clear();
   nonPrevailingSyms.clear();
   whyExtractRecords.clear();
   backwardReferences.clear();
   hasSympart.store(false, std::memory_order_relaxed);
   needsTlsLd.store(false, std::memory_order_relaxed);
 }
 
+llvm::raw_fd_ostream Ctx::openAuxiliaryFile(llvm::StringRef filename,
+                                            std::error_code &ec) {
+  using namespace llvm::sys::fs;
+  OpenFlags flags =
+      diagnosticFiles.insert(filename).second ? OF_None : OF_Append;
+  return {filename, ec, flags};
+}
+
 namespace lld {
 namespace elf {
 bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
           llvm::raw_ostream &stderrOS, bool exitEarly, bool disableOutput) {
   // This driver-specific context will be freed later by unsafeLldMain().
   auto *ctx = new CommonLinkerContext;
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
if (isBitcode(mb))
     sym->extract();
 }
 
 static void writeArchiveStats() {
   if (config->printArchiveStats.empty())
     return;
 
   std::error_code ec;
-  raw_fd_ostream os(config->printArchiveStats, ec, sys::fs::OF_None);
+  raw_fd_ostream os = ctx.openAuxiliaryFile(config->printArchiveStats, ec);
   if (ec) {
     error("--print-archive-stats=: cannot open " + config->printArchiveStats +
           ": " + ec.message());
     return;
   }
 
   os << "members\textracted\tarchive\n";
 
static void writeArchiveStats() {
   }
 }
 
 static void writeWhyExtract() {
   if (config->whyExtract.empty())
     return;
 
   std::error_code ec;
-  raw_fd_ostream os(config->whyExtract, ec, sys::fs::OF_None);
+  raw_fd_ostream os = ctx.openAuxiliaryFile(config->whyExtract, ec);
   if (ec) {
     error("cannot open --why-extract= file " + config->whyExtract + ": " +
           ec.message());
     return;
   }
 
   os << "reference\textracted\tsymbol\n";
   for (auto &entry : ctx.whyExtractRecords) {

 // don't make your executable to depend on system libraries (such as libc)
 // because you don't know the exact paths of libraries, even though system
 // libraries that are linked to your executable statically are technically a
 // part of your program. By using --dependency-file option, you can make
 // lld to dump dependency info so that you can maintain exact dependencies
 // easily.
 static void writeDependencyFile() {
   std::error_code ec;
-  raw_fd_ostream os(config->dependencyFile, ec, sys::fs::OF_None);
+  raw_fd_ostream os = ctx.openAuxiliaryFile(config->dependencyFile, ec);
   if (ec) {
     error("cannot open " + config->dependencyFile + ": " + ec.message());
     return;
   }
 
   // We use the same escape rules as Clang/GCC which are accepted by Make/Ninja:
   // * A space is escaped by a backslash which itself must be escaped.
   // * A hash sign is escaped by a single backslash.

lld/ELF/MapFile.cpp

void elf::writeMapAndCref() {
   if (config->mapFile.empty() && !config->cref)
     return;
 
   llvm::TimeTraceScope timeScope("Write map file");
 
   // Open a map file for writing.
   std::error_code ec;
   StringRef mapFile = config->mapFile.empty() ? "-" : config->mapFile;
-  raw_fd_ostream os(mapFile, ec, sys::fs::OF_None);
+  raw_fd_ostream os = ctx.openAuxiliaryFile(mapFile, ec);
   if (ec) {
     error("cannot open " + mapFile + ": " + ec.message());
     return;
   }
 
   if (!config->mapFile.empty())
     writeMapFile(os);
   if (config->cref)
     writeCref(os);
 }

lld/test/ELF/diagnostic-options.test

+# RUN: ld.lld -o /dev/null -shared /dev/null -m elf_x86_64 \
+# RUN:   --print-archive-stats=%t --why-extract=%t --dependency-file=%t -Map=%t
+# RUN: FileCheck --input-file=%t %s
+
+# From --print-archive-stats
+# CHECK: members extracted archive
+
+# From --why-extract
+# CHECK: reference extracted symbol
+
+# From --dependency-file
+# CHECK: /dev/null: \
+
+# From -Map
+# CHECK: VMA LMA Size