[llvm-objcopy] [COFF] Update symbol indices in weak externals

[mstorsjo]

Summary

This is, as far as I know, the only other aux symbol whose contents we need to update based on how llvm-objcopy rewrites the files. (We already handle coff_aux_section_definition.)

[jhenderson]

test/tools/llvm-objcopy/COFF/weak-external.test

26–27 ↗

(On Diff #182758)

If these two fields aren't required, it would be nice to get rid of them.

tools/llvm-objcopy/COFF/Reader.cpp

155–156 ↗

(On Diff #182758)

Use createStringError instead of make_error<StringError>

159–160 ↗

(On Diff #182758)

Ditto.

tools/llvm-objcopy/COFF/Writer.cpp

78 ↗

(On Diff #182758)

This seems like a weird condition. Could you explain it please (e.g. with a comment).

83–85 ↗

(On Diff #182758)

Ditto. In this case, you can then take advantage of the printf-style overload to avoid the string concatenation here.

Also putting some quotes around the symbol name might be good, especially if it's theoretically possible to have spaces in your symbol names in COFF.

[mstorsjo]

test/tools/llvm-objcopy/COFF/weak-external.test

26–27 ↗

(On Diff #182758)

Apparently the test works even without them - will remove.

tools/llvm-objcopy/COFF/Reader.cpp

155–156 ↗

(On Diff #182758)

Ok, I can change that.

I've used a lot of make_error<StringError> so far in the COFF backend, so there's a number of other occurrances of it that I could change in a separate patch if you prefer that as well? (And there's one instance of it in the ELF subdir.)

tools/llvm-objcopy/COFF/Writer.cpp

78 ↗

(On Diff #182758)

I can add a comment, sure. The NumberOfAuxSymbols check needs to be at least greater than or equal to one for the code not to write out of bounds, but I've made the check stricter since only exactly equal to one makes sense.

83–85 ↗

(On Diff #182758)

With printf-style formatting, I'd either need to make the StringRef a null terminated string, or use the %.*s formatting syntax with Sym.Name.size(), Sym.Name.data() or something similar, which kinda negates any convenience of printf-style formatting strings.

About quotes around the symbol names - oh, right, yes. It seems I've slipped in a few more of these before as well, I can push an NFC patch that fixes those, the next time I push something.

[mstorsjo]

Applied the suggested changes.

[jhenderson]

LGTM. I'm happy with you updating the last createStringError call if you want, or leaving as is.

tools/llvm-objcopy/COFF/Reader.cpp

155–156 ↗

(On Diff #182758)

That would be good.

tools/llvm-objcopy/COFF/Writer.cpp

83–85 ↗

(On Diff #182758)

You could use StringRef::str() to avoid this.

Updating the other errors later would be good.

[mstorsjo]

tools/llvm-objcopy/COFF/Writer.cpp

83–85 ↗

(On Diff #182758)

Wouldn't that need something like Sym.Name.str().c_str() then? That's also a bit clunky but slightly leaner than the %.*s form.

[jhenderson]

tools/llvm-objcopy/COFF/Writer.cpp

83–85 ↗

(On Diff #182758)

Possibly. I can't remember exactly how the internals work. I think I'd prefer it to the current form, because it keeps the pattern simpler.

[mstorsjo]

Going with %s together with StringRef::str() + .c_str() for formatting. Will use this form for later change to unify on createStringError() then.

llvm/trunk/test/tools/llvm-objcopy/COFF/weak-external.test

+# RUN: yaml2obj %s > %t.in.o
+
+# RUN: llvm-objdump -t %t.in.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-PRE
+
+# RUN: llvm-objcopy -N func %t.in.o %t.out.o
+# RUN: llvm-objdump -t %t.out.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-POST
+
+# RUN: not llvm-objcopy -N .weak.foobar.file1 %t.in.o %t.err.o 2>&1 | FileCheck %s --check-prefix=ERROR
+
+# SYMBOLS: SYMBOL TABLE:
+# SYMBOLS-PRE-NEXT: func
+# SYMBOLS-NEXT: .weak.foobar.file1
+# SYMBOLS-NEXT: foobar
+# SYMBOLS-PRE-NEXT: AUX indx 1
+# SYMBOLS-POST-NEXT: AUX indx 0
+# SYMBOLS-EMPTY:
+
+# ERROR: Symbol 'foobar' is missing its weak target
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            .text
+    Characteristics: [  ]
+symbols:         
+  - Name:            func
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            .weak.foobar.file1
+    Value:           1
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            foobar
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_WEAK_EXTERNAL
+    WeakExternal:    
+      TagIndex:        1
+      Characteristics: IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY
+...

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

 //===- Object.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 LLVM_TOOLS_OBJCOPY_COFF_OBJECT_H
 #define LLVM_TOOLS_OBJCOPY_COFF_OBJECT_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
 #include <cstddef>
 #include <cstdint>
 #include <vector>
 

 };
 
 struct Symbol {
   object::coff_symbol32 Sym;
   StringRef Name;
   std::vector<uint8_t> AuxData;
   ssize_t TargetSectionId;
   ssize_t AssociativeComdatTargetSectionId = 0;
+  Optional<size_t> WeakTargetSymbolId;
   size_t UniqueId;
   size_t RawIndex;
   bool Referenced;
 };
 
 struct Object {
   bool IsPE = false;
 

llvm/trunk/tools/llvm-objcopy/COFF/Reader.h

 using object::COFFObjectFile;
 
 class COFFReader {
   const COFFObjectFile &COFFObj;
 
   Error readExecutableHeaders(Object &Obj) const;
   Error readSections(Object &Obj) const;
   Error readSymbols(Object &Obj, bool IsBigObj) const;
-  Error setRelocTargets(Object &Obj) const;
+  Error setSymbolTargets(Object &Obj) const;
 
 public:
   explicit COFFReader(const COFFObjectFile &O) : COFFObj(O) {}
   Expected<std::unique_ptr<Object>> create() const;
 };
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFF_READER_H

llvm/trunk/tools/llvm-objcopy/COFF/Reader.cpp

else if (static_cast<uint32_t>(SymRef.getSectionNumber() - 1) <
              Sections.size())
       Sym.TargetSectionId = Sections[SymRef.getSectionNumber() - 1].UniqueId;
     else
       return createStringError(object_error::parse_failed,
                                "Section number out of range");
     // For section definitions, check if it is comdat associative, and if
     // it is, find the target section unique id.
     const coff_aux_section_definition *SD = SymRef.getSectionDefinition();
+    const coff_aux_weak_external *WE = SymRef.getWeakExternal();
     if (SD && SD->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
       int32_t Index = SD->getNumber(IsBigObj);
       if (Index <= 0 || static_cast<uint32_t>(Index - 1) >= Sections.size())
         return createStringError(object_error::parse_failed,
                                  "Unexpected associative section index");
       Sym.AssociativeComdatTargetSectionId = Sections[Index - 1].UniqueId;
+    } else if (WE) {
+      // This is a raw symbol index for now, but store it in the Symbol
+      // until we've added them to the Object, which assigns the final
+      // unique ids.
+      Sym.WeakTargetSymbolId = WE->TagIndex;
     }
     I += 1 + SymRef.getNumberOfAuxSymbols();
   }
   Obj.addSymbols(Symbols);
   return Error::success();
 }
 
-Error COFFReader::setRelocTargets(Object &Obj) const {
+Error COFFReader::setSymbolTargets(Object &Obj) const {
   std::vector<const Symbol *> RawSymbolTable;
   for (const Symbol &Sym : Obj.getSymbols()) {
     RawSymbolTable.push_back(&Sym);
     for (size_t I = 0; I < Sym.Sym.NumberOfAuxSymbols; I++)
       RawSymbolTable.push_back(nullptr);
   }
+  for (Symbol &Sym : Obj.getMutableSymbols()) {
+    // Convert WeakTargetSymbolId from the original raw symbol index to
+    // a proper unique id.
+    if (Sym.WeakTargetSymbolId) {
+      if (*Sym.WeakTargetSymbolId >= RawSymbolTable.size())
+        return createStringError(object_error::parse_failed,
+                                 "Weak external reference out of range");
+      const Symbol *Target = RawSymbolTable[*Sym.WeakTargetSymbolId];
+      if (Target == nullptr)
+        return createStringError(object_error::parse_failed,
+                                 "Invalid SymbolTableIndex");
+      Sym.WeakTargetSymbolId = Target->UniqueId;
+    }
+  }
   for (Section &Sec : Obj.getMutableSections()) {
     for (Relocation &R : Sec.Relocs) {
       if (R.Reloc.SymbolTableIndex >= RawSymbolTable.size())
         return createStringError(object_error::parse_failed,
                                  "SymbolTableIndex out of range");
       const Symbol *Sym = RawSymbolTable[R.Reloc.SymbolTableIndex];
       if (Sym == nullptr)
         return createStringError(object_error::parse_failed,
Expected<std::unique_ptr<Object>> COFFReader::create() const {
   }
 
   if (Error E = readExecutableHeaders(*Obj))
     return std::move(E);
   if (Error E = readSections(*Obj))
     return std::move(E);
   if (Error E = readSymbols(*Obj, IsBigObj))
     return std::move(E);
-  if (Error E = setRelocTargets(*Obj))
+  if (Error E = setSymbolTargets(*Obj))
     return std::move(E);
 
   return std::move(Obj);
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

llvm/trunk/tools/llvm-objcopy/COFF/Writer.h

class COFFWriter {
   Buffer &Buf;
 
   size_t FileSize;
   size_t FileAlignment;
   size_t SizeOfInitializedData;
   StringTableBuilder StrTabBuilder;
 
   Error finalizeRelocTargets();
-  Error finalizeSectionNumbers();
+  Error finalizeSymbolContents();
   void layoutSections();
   size_t finalizeStringTable();
   template <class SymbolTy> std::pair<size_t, size_t> finalizeSymbolTable();
 
   Error finalize(bool IsBigObj);
 
   void writeHeaders(bool IsBigObj);
   void writeSections();

llvm/trunk/tools/llvm-objcopy/COFF/Writer.cpp

for (Relocation &R : Sec.Relocs) {
                                  "Relocation target '%s' (%zu) not found",
                                  R.TargetName.str().c_str(), R.Target);
       R.Reloc.SymbolTableIndex = Sym->RawIndex;
     }
   }
   return Error::success();
 }
 
-Error COFFWriter::finalizeSectionNumbers() {
+Error COFFWriter::finalizeSymbolContents() {
   for (Symbol &Sym : Obj.getMutableSymbols()) {
     if (Sym.TargetSectionId <= 0) {
       // Undefined, or a special kind of symbol. These negative values
       // are stored in the SectionNumber field which is unsigned.
       Sym.Sym.SectionNumber = static_cast<uint32_t>(Sym.TargetSectionId);
     } else {
       const Section *Sec = Obj.findSection(Sym.TargetSectionId);
       if (Sec == nullptr)
if (Sym.TargetSectionId <= 0) {
                 Sym.Name.str().c_str());
           SDSectionNumber = Sec->Index;
         }
         // Update the section definition with the new section number.
         SD->NumberLowPart = static_cast<uint16_t>(SDSectionNumber);
         SD->NumberHighPart = static_cast<uint16_t>(SDSectionNumber >> 16);
       }
     }
+    // Check that we actually have got AuxData to match the weak symbol target
+    // we want to set. Only >= 1 would be required, but only == 1 makes sense.
+    if (Sym.WeakTargetSymbolId && Sym.Sym.NumberOfAuxSymbols == 1) {
+      coff_aux_weak_external *WE =
+          reinterpret_cast<coff_aux_weak_external *>(Sym.AuxData.data());
+      const Symbol *Target = Obj.findSymbol(*Sym.WeakTargetSymbolId);
+      if (Target == nullptr)
+        return createStringError(object_error::invalid_symbol_index,
+                                 "Symbol '%s' is missing its weak target",
+                                 Sym.Name.str().c_str());
+      WE->TagIndex = Target->RawIndex;
+    }
   }
   return Error::success();
 }
 
 void COFFWriter::layoutSections() {
   for (auto &S : Obj.getMutableSections()) {
     if (S.Header.SizeOfRawData > 0)
       S.Header.PointerToRawData = FileSize;
std::pair<size_t, size_t> COFFWriter::finalizeSymbolTable() {
   for (const auto &S : Obj.getSymbols())
     SymTabSize += S.AuxData.size();
   return std::make_pair(SymTabSize, sizeof(SymbolTy));
 }
 
 Error COFFWriter::finalize(bool IsBigObj) {
   if (Error E = finalizeRelocTargets())
     return E;
-  if (Error E = finalizeSectionNumbers())
+  if (Error E = finalizeSymbolContents())
     return E;
 
   size_t SizeOfHeaders = 0;
   FileAlignment = 1;
   size_t PeHeaderSize = 0;
   if (Obj.IsPE) {
     Obj.DosHeader.AddressOfNewExeHeader =
         sizeof(Obj.DosHeader) + Obj.DosStub.size();