[dsymutil] Fix handling of common symbols in multiple object files.

[JDevlieghere]

Summary

For common symbols the linker emits only a single symbol entry in the debug map. This caused dsymutil to not relocate common symbols when linking DWARF coming form object files that did not have this entry. This patch fixes that by keeping track of common symbols in the object files and synthesizing a debug map entry for them using the address from the main binary.

[friss]

LGTM

[aleksandr.urakov]

Hi! Could you choose some other names for com1.o and com2.o, please? :) They are reserved names on Windows, so there are problems with checking out this change.

[aprantl]

llvm/tools/dsymutil/MachODebugMapParser.cpp

144

This looks like no-op. I guess we could just ignore the return value?

498–501

is it expected that a symbol with SymbolRef::SF_Absolute | SymbolRef::SF_Common goes into the first if only?

llvm/test/tools/dsymutil/X86/common-sym-multi.test

+RUN: dsymutil -oso-prepend-path %p/../Inputs %p/../Inputs/private/tmp/common/com -f -o - | llvm-dwarfdump -debug-info - | FileCheck %s
+RUN: dsymutil -oso-prepend-path %p/../Inputs %p/../Inputs/private/tmp/common/com -dump-debug-map | FileCheck %s --check-prefix DEBUGMAP
+
+The test was compiled from two source files:
+$ cd /private/tmp/common
+$ cat com1.c
+int i[1000];
+int main() {
+  return i[1];
+}
+$ cat com2.c
+extern int i[1000];
+int bar() {
+  return i[0];
+}
+$ clang -fcommon -g -c com1.c -o com1.o
+$ clang -fcommon -g -c com2.c -o com2.o
+$ clang -fcommon -g com1.o com2.o -o com
+
+CHECK: DW_TAG_compile_unit
+CHECK:   DW_TAG_variable
+CHECK-NOT: {{NULL|DW_TAG}}
+CHECK:     DW_AT_name{{.*}}"i"
+CHECK-NOT: {{NULL|DW_TAG}}
+CHECK:     DW_AT_location{{.*}}DW_OP_addr 0x100001000)
+
+CHECK: DW_TAG_compile_unit
+CHECK:   DW_TAG_variable
+CHECK-NOT: {{NULL|DW_TAG}}
+CHECK:     DW_AT_name{{.*}}"i"
+CHECK-NOT: {{NULL|DW_TAG}}
+CHECK:     DW_AT_location{{.*}}DW_OP_addr 0x100001000)
+
+DEBUGMAP: filename:{{.*}}com1.o
+DEBUGMAP: symbols:
+DEBUGMAP: sym: _i, binAddr: 0x0000000100001000, size: 0x00000000
+DEBUGMAP: filename:{{.*}}com2.o
+DEBUGMAP: symbols:
+DEBUGMAP: sym: _i, binAddr: 0x0000000100001000, size: 0x00000000

llvm/tools/dsymutil/MachODebugMapParser.cpp

 //===- tools/dsymutil/MachODebugMapParser.cpp - Parse STABS debug maps ----===//
 //
 // 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 "BinaryHolder.h"
 #include "DebugMap.h"
 #include "MachOUtils.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
+#include <vector>
 
 namespace {
 using namespace llvm;
 using namespace llvm::dsymutil;
 using namespace llvm::object;
 
 class MachODebugMapParser {
 public:
private:
 
   /// Owns the MemoryBuffer for the main binary.
   BinaryHolder BinHolder;
   /// Map of the binary symbol addresses.
   StringMap<uint64_t> MainBinarySymbolAddresses;
   StringRef MainBinaryStrings;
   /// The constructed DebugMap.
   std::unique_ptr<DebugMap> Result;
+  /// List of common symbols that need to be added to the debug map.
+  std::vector<std::string> CommonSymbols;
 
   /// Map of the currently processed object file symbol addresses.
   StringMap<Optional<uint64_t>> CurrentObjectAddresses;
   /// Element of the debug map corresponding to the current object file.
   DebugMapObject *CurrentDebugMapObject;
 
   /// Holds function info while function scope processing.
   const char *CurrentFunctionName;
void handleStabSymbolTableEntry(uint32_t StringIndex, uint8_t Type,
                                   uint8_t SectionIndex, uint16_t Flags,
                                   uint64_t Value);
 
   template <typename STEType> void handleStabDebugMapEntry(const STEType &STE) {
     handleStabSymbolTableEntry(STE.n_strx, STE.n_type, STE.n_sect, STE.n_desc,
                                STE.n_value);
   }
 
+  void addCommonSymbols();
+
   /// Dump the symbol table output header.
   void dumpSymTabHeader(raw_ostream &OS, StringRef Arch);
 
   /// Dump the contents of nlist entries.
   void dumpSymTabEntry(raw_ostream &OS, uint64_t Index, uint32_t StringIndex,
                        uint8_t Type, uint8_t SectionIndex, uint16_t Flags,
                        uint64_t Value);
 

 /// Reset the parser state corresponding to the current object
 /// file. This is to be called after an object file is finished
 /// processing.
 void MachODebugMapParser::resetParserState() {
   CurrentObjectAddresses.clear();
   CurrentDebugMapObject = nullptr;
 }
 
+/// Commons symbols won't show up in the symbol map but might need to be
+/// relocated. We can add them to the symbol table ourselves by combining the
+/// information in the object file (the symbol name) and the main binary (the
+/// address).
+void MachODebugMapParser::addCommonSymbols() {
+  for (auto &CommonSymbol : CommonSymbols) {
+    uint64_t CommonAddr = getMainBinarySymbolAddress(CommonSymbol);
+    if (CommonAddr == 0) {
+      // The main binary doesn't have an address for the given symbol.
+      continue;
+    }
+    if (!CurrentDebugMapObject->addSymbol(CommonSymbol, None /*ObjectAddress*/,
+                                          CommonAddr, 0 /*size*/)) {
+      // The symbol is already present.
+      continue;
+    }
+  }
+  CommonSymbols.clear();
+}
+
 /// Create a new DebugMapObject. This function resets the state of the
 /// parser that was referring to the last object file and sets
 /// everything up to add symbols to the new one.
 void MachODebugMapParser::switchToNewDebugMapObject(
     StringRef Filename, sys::TimePoint<std::chrono::seconds> Timestamp) {
+  addCommonSymbols();
   resetParserState();
 
   SmallString<80> Path(PathPrefix);
   sys::path::append(Path, Filename);
 
   auto ObjectEntry = BinHolder.getObjectEntry(Path, Timestamp);
   if (!ObjectEntry) {
     auto Err = ObjectEntry.takeError();
for (auto Sym : Obj.symbols()) {
     // The value of some categories of symbols isn't meaningful. For
     // example common symbols store their size in the value field, not
     // their address. Absolute symbols have a fixed address that can
     // conflict with standard symbols. These symbols (especially the
     // common ones), might still be referenced by relocations. These
     // relocations will use the symbol itself, and won't need an
     // object file address. The object file address field is optional
     // in the DebugMap, leave it unassigned for these symbols.
-    if (Sym.getFlags() & (SymbolRef::SF_Absolute | SymbolRef::SF_Common))
+    uint32_t Flags = Sym.getFlags();
+    if (Flags & SymbolRef::SF_Absolute) {
       CurrentObjectAddresses[*Name] = None;
-    else
+    } else if (Flags & SymbolRef::SF_Common) {
+      CurrentObjectAddresses[*Name] = None;
+      CommonSymbols.push_back(*Name);
+    } else {
       CurrentObjectAddresses[*Name] = Addr;
     }
   }
+}
 
 /// Lookup a symbol address in the main binary symbol table. The
 /// parser only needs to query common symbols, thus not every symbol's
 /// address is available through this function.
 uint64_t MachODebugMapParser::getMainBinarySymbolAddress(StringRef Name) {
   auto Sym = MainBinarySymbolAddresses.find(Name);
   if (Sym == MainBinarySymbolAddresses.end())
     return 0;