[COFF] Adjust how we flag weak externals

[mstorsjo]

Summary

This fixes PR36096.

Originally based on a patch by Martell Malone.

[martell]

@alexcrichton can you confirm this works as a replacement for your partial revert here https://github.com/rust-lang/llvm/commit/9eb9267 ?

[alexcrichton]

Thanks! This looks like it would do the trick yeah. I think we've since removed the C++ code that caused the error on our end (for entirely unrelated reasons) so we're unlikely to hit this any more (and can probably revert the patch I had), but this'd be good to get into the next version of LLVM!

[pcc]

Probably the ideal test would be an object file with three weak externals with each of the possible characteristics. Then test that the archive symbol table contains only an entry for the symbol with characteristics IMAGE_WEAK_EXTERN_SEARCH_ALIAS.

lib/Object/COFFImportFile.cpp

549 ↗

(On Diff #137931)

Please commit this part as a separate cleanup.

lib/Object/COFFObjectFile.cpp

235 ↗

(On Diff #137931)

I don't think that getCharacteristics does what you are expecting it to do here. It looks like it retrieves the characteristics from the COFF header, and not the symbol: http://llvm-cs.pcc.me.uk/include/llvm/Object/COFF.h#821

Also, I think you will want to compare the characteristics to IMAGE_WEAK_EXTERN_SEARCH_ALIAS with !=, not use it as a bitmask.

[mstorsjo]

It'd be nice to have this regression fixed before 7.0 branches; I'll post an updated version of the patch, taking the previous feedback into account.

[pcc]

lib/Object/COFFObjectFile.cpp

235 ↗

(On Diff #156331)

I'd replace this with isUndefined and move the logic for whether to give a weak external the undefined bit to the isWeakExternal block.

[mstorsjo]

lib/Object/COFFObjectFile.cpp

235 ↗

(On Diff #156331)

Right, that makes it much cleaner, thanks!

[mstorsjo]

Applied @pcc's suggestion.

[pcc]

lib/Object/COFFObjectFile.cpp

223 ↗

(On Diff #156588)

There should always be an auxiliary record for weak externals so you could replace the outer if with if (const coff_aux_weak_external *awe = Symb.getWeakExternal()) and simplify the check here.

[mstorsjo]

Applied the following simplification suggested by @pcc.

[pcc]

LGTM

lib/Object/COFFObjectFile.cpp

220 ↗

(On Diff #156598)

nit: s/awe/AWE/g

llvm/trunk/include/llvm/Object/COFF.h

struct coff_symbol_generic {
   union {
     char ShortName[COFF::NameSize];
     StringTableOffset Offset;
   } Name;
   support::ulittle32_t Value;
 };
 
 struct coff_aux_section_definition;
+struct coff_aux_weak_external;
 
 class COFFSymbolRef {
 public:
   COFFSymbolRef() = default;
   COFFSymbolRef(const coff_symbol16 *CS) : CS16(CS) {}
   COFFSymbolRef(const coff_symbol32 *CS) : CS32(CS) {}
 
   const void *getRawPtr() const {
public:
 
   const coff_aux_section_definition *getSectionDefinition() const {
     if (!getNumberOfAuxSymbols() ||
         getStorageClass() != COFF::IMAGE_SYM_CLASS_STATIC)
       return nullptr;
     return getAux<coff_aux_section_definition>();
   }
 
+  const coff_aux_weak_external *getWeakExternal() const {
+    if (!getNumberOfAuxSymbols() ||
+        getStorageClass() != COFF::IMAGE_SYM_CLASS_WEAK_EXTERNAL)
+      return nullptr;
+    return getAux<coff_aux_weak_external>();
+  }
+
   bool isAbsolute() const {
     return getSectionNumber() == -1;
   }
 
   bool isExternal() const {
     return getStorageClass() == COFF::IMAGE_SYM_CLASS_EXTERNAL;
   }
 

llvm/trunk/lib/Object/ArchiveWriter.cpp

 }
 
 static bool isArchiveSymbol(const object::BasicSymbolRef &S) {
   uint32_t Symflags = S.getFlags();
   if (Symflags & object::SymbolRef::SF_FormatSpecific)
     return false;
   if (!(Symflags & object::SymbolRef::SF_Global))
     return false;
-  if (Symflags & object::SymbolRef::SF_Undefined &&
-      !(Symflags & object::SymbolRef::SF_Indirect))
+  if (Symflags & object::SymbolRef::SF_Undefined)
     return false;
   return true;
 }
 
 static void printNBits(raw_ostream &Out, object::Archive::Kind Kind,
                        uint64_t Val) {
   if (is64BitKind(Kind))
     print<uint64_t>(Out, Kind, Val);

llvm/trunk/lib/Object/COFFObjectFile.cpp

 
 uint32_t COFFObjectFile::getSymbolFlags(DataRefImpl Ref) const {
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   uint32_t Result = SymbolRef::SF_None;
 
   if (Symb.isExternal() || Symb.isWeakExternal())
     Result |= SymbolRef::SF_Global;
 
-  if (Symb.isWeakExternal()) {
+  if (const coff_aux_weak_external *AWE = Symb.getWeakExternal()) {
     Result |= SymbolRef::SF_Weak;
-    // We use indirect to allow the archiver to write weak externs
-    Result |= SymbolRef::SF_Indirect;
+    if (AWE->Characteristics != COFF::IMAGE_WEAK_EXTERN_SEARCH_ALIAS)
+      Result |= SymbolRef::SF_Undefined;
   }
 
   if (Symb.getSectionNumber() == COFF::IMAGE_SYM_ABSOLUTE)
     Result |= SymbolRef::SF_Absolute;
 
   if (Symb.isFileRecord())
     Result |= SymbolRef::SF_FormatSpecific;
 
   if (Symb.isSectionDefinition())
     Result |= SymbolRef::SF_FormatSpecific;
 
   if (Symb.isCommon())
     Result |= SymbolRef::SF_Common;
 
-  if (Symb.isAnyUndefined())
+  if (Symb.isUndefined())
     Result |= SymbolRef::SF_Undefined;
 
   return Result;
 }
 
 uint64_t COFFObjectFile::getCommonSymbolSizeImpl(DataRefImpl Ref) const {
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   return Symb.getValue();

llvm/trunk/test/tools/llvm-ar/coff-weak.yaml

+# RUN: yaml2obj %s -o %t.obj
+#
+# RUN: rm -f %t.ar
+# RUN: llvm-ar crs %t.a %t.obj
+# RUN: llvm-nm -print-armap %t.a | FileCheck %s
+
+# CHECK: Archive map
+# CHECK-NEXT: WeakSearchAlias in coff-weak.yaml.tmp.obj
+# CHECK-EMPTY:
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_UNKNOWN
+  Characteristics: [  ]
+sections:
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
+    SectionData:     ''
+symbols:
+  - Name:            NormalUndefined
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            WeakSearchAlias
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_WEAK_EXTERNAL
+    WeakExternal:
+      TagIndex:        0
+      Characteristics: IMAGE_WEAK_EXTERN_SEARCH_ALIAS
+  - Name:            WeakSearchLibrary
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_WEAK_EXTERNAL
+    WeakExternal:
+      TagIndex:        0
+      Characteristics: IMAGE_WEAK_EXTERN_SEARCH_LIBRARY
+  - Name:            WeakSearchNolibrary
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_WEAK_EXTERNAL
+    WeakExternal:
+      TagIndex:        0
+      Characteristics: IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY
+...

llvm/trunk/test/tools/llvm-dlltool/coff-decorated.def

 ; CHECK: Symbol: __imp__StdcallFunction@4
 ; CHECK: Symbol: _StdcallFunction@4
 ; CHECK: Name type: undecorate
 ; CHECK: Symbol: __imp_@FastcallFunction@4
 ; CHECK: Symbol: @FastcallFunction@4
 ; CHECK: Name type: name
 ; CHECK: Symbol: __imp_??_7exception@@6B@
 ; CHECK: Symbol: ??_7exception@@6B@
-; CHECK-NM: w _StdcallAlias@4
+; CHECK-NM: W _StdcallAlias@4
 ; CHECK-NM: U _StdcallFunction@4

llvm/trunk/test/tools/llvm-dlltool/coff-weak-exports.def

 ; A different import library name and implementation name can be mixed
 ; with exposing it via a different name in the DLL than in code.
 ImpLibName2 = Implementation2 == AltTestFunction2
 ; The fact that a DLL export entry is a forward to a different DLL doesn't
 ; matter for the import library
 ImpLibName3 = kernel32.Sleep
 
 ; CHECK:      U AltTestFunction
-; CHECK-NEXT: w TestFunction
+; CHECK-NEXT: W TestFunction
 ; CHECK:      U __imp_AltTestFunction
-; CHECK-NEXT: w __imp_TestFunction
+; CHECK-NEXT: W __imp_TestFunction
 ; CHECK:      T ImpLibName
 ; CHECK-NEXT: T __imp_ImpLibName
 ; CHECK:      U AltTestFunction2
-; CHECK-NEXT: w ImpLibName2
+; CHECK-NEXT: W ImpLibName2
 ; CHECK:      U __imp_AltTestFunction2
-; CHECK-NEXT: w __imp_ImpLibName2
+; CHECK-NEXT: W __imp_ImpLibName2
 ; CHECK:      T ImpLibName3
 ; CHECK-NEXT: T __imp_ImpLibName3