[COFF] Improve correctness of def parsing for GNU features

[mstorsjo]

Summary

The operator == used for exporting a function with a different name in the DLL compared to the name in the import library (which is useful for adding linker level aliases for function in the import library) is a feature distinct and different from the operator = used for exporting a function with a different name (both in import library and DLL) than in the implementation producing the DLL.

When creating an import library using dlltool, from a def file that contains forwards (Func = OtherDll.Func), this shouldn't affect the produced import library, which should still behave just as if it was a normal exported function.

This clears a lot of confusion and subtle misunderstandings, and avoids a parameter that was used to avoid creating weak aliases when invoked from lld. (This parameter was added previously due to the existing conflation of the two features.)

[mstorsjo]

Ping

[mstorsjo]

As a reference to resolve this misconception, GNU ld's manual has got the following grammar for the def files:

EXPORTS
  ( (  ( <name1> [ = <name2> ] )
     | ( <name1> = <module-name> . <external-name>))
  [ @ <integer> ] [NONAME] [DATA] [CONSTANT] [PRIVATE] [== <name3>] ) *

(From https://sourceware.org/binutils/docs-2.29/ld/WIN32.html.) So both = and == do exist in GNU tools, but meaning different things.

[mstorsjo]

Ping @rnk or @ruiu

[rnk]

lgtm

llvm/trunk/include/llvm/Object/COFFImportFile.h

bool isData() const {
     return getCOFFImportHeader()->getType() == COFF::IMPORT_DATA;
   }
 };
 
 struct COFFShortExport {
   std::string Name;
   std::string ExtName;
   std::string SymbolName;
+  std::string AliasTarget;
 
   uint16_t Ordinal = 0;
   bool Noname = false;
   bool Data = false;
   bool Private = false;
   bool Constant = false;
 
-  bool isWeak() {
-    return ExtName.size() && ExtName != Name;
-  }
-
   friend bool operator==(const COFFShortExport &L, const COFFShortExport &R) {
     return L.Name == R.Name && L.ExtName == R.ExtName &&
             L.Ordinal == R.Ordinal && L.Noname == R.Noname &&
             L.Data == R.Data && L.Private == R.Private;
   }
 
   friend bool operator!=(const COFFShortExport &L, const COFFShortExport &R) {
     return !(L == R);
   }
 };
 
 Error writeImportLibrary(StringRef ImportName, StringRef Path,
                          ArrayRef<COFFShortExport> Exports,
-                         COFF::MachineTypes Machine, bool MakeWeakAliases,
-                         bool MinGW);
+                         COFF::MachineTypes Machine, bool MinGW);
 
 } // namespace object
 } // namespace llvm
 
 #endif

llvm/trunk/lib/Object/COFFImportFile.cpp

NewArchiveMember ObjectFactory::createWeakExternal(StringRef Sym,
   // Copied here so we can still use writeStringTable
   char *Buf = Alloc.Allocate<char>(Buffer.size());
   memcpy(Buf, Buffer.data(), Buffer.size());
   return {MemoryBufferRef(StringRef(Buf, Buffer.size()), ImportName)};
 }
 
 Error writeImportLibrary(StringRef ImportName, StringRef Path,
                          ArrayRef<COFFShortExport> Exports,
-                         MachineTypes Machine, bool MakeWeakAliases,
-                         bool MinGW) {
+                         MachineTypes Machine, bool MinGW) {
 
   std::vector<NewArchiveMember> Members;
   ObjectFactory OF(llvm::sys::path::filename(ImportName), Machine);
 
   std::vector<uint8_t> ImportDescriptor;
   Members.push_back(OF.createImportDescriptor(ImportDescriptor));
 
   std::vector<uint8_t> NullImportDescriptor;
   Members.push_back(OF.createNullImportDescriptor(NullImportDescriptor));
 
   std::vector<uint8_t> NullThunk;
   Members.push_back(OF.createNullThunk(NullThunk));
 
   for (COFFShortExport E : Exports) {
     if (E.Private)
       continue;
 
-    if (E.isWeak() && MakeWeakAliases) {
-      Members.push_back(OF.createWeakExternal(E.Name, E.ExtName, false));
-      Members.push_back(OF.createWeakExternal(E.Name, E.ExtName, true));
-      continue;
-    }
-
     ImportType ImportType = IMPORT_CODE;
     if (E.Data)
       ImportType = IMPORT_DATA;
     if (E.Constant)
       ImportType = IMPORT_CONST;
 
     StringRef SymbolName = E.SymbolName.empty() ? E.Name : E.SymbolName;
     ImportNameType NameType = getNameType(SymbolName, E.Name, Machine, MinGW);
     Expected<std::string> Name = E.ExtName.empty()
                                      ? SymbolName
                                      : replace(SymbolName, E.Name, E.ExtName);
 
     if (!Name)
       return Name.takeError();
 
+    if (!E.AliasTarget.empty() && *Name != E.AliasTarget) {
+      Members.push_back(OF.createWeakExternal(E.AliasTarget, *Name, false));
+      Members.push_back(OF.createWeakExternal(E.AliasTarget, *Name, true));
+      continue;
+    }
+
     Members.push_back(
         OF.createShortImport(*Name, E.Ordinal, ImportType, NameType));
   }
 
   return writeArchive(Path, Members, /*WriteSymtab*/ true,
                       object::Archive::K_GNU,
                       /*Deterministic*/ true, /*Thin*/ false);
 }
 
 } // namespace object
 } // namespace llvm

llvm/trunk/lib/Object/COFFModuleDefinition.cpp

 namespace object {
 
 enum Kind {
   Unknown,
   Eof,
   Identifier,
   Comma,
   Equal,
+  EqualEqual,
   KwBase,
   KwConstant,
   KwData,
   KwExports,
   KwHeapsize,
   KwLibrary,
   KwName,
   KwNoname,
case '\0':
       return Token(Eof);
     case ';': {
       size_t End = Buf.find('\n');
       Buf = (End == Buf.npos) ? "" : Buf.drop_front(End);
       return lex();
     }
     case '=':
       Buf = Buf.drop_front();
-      // GNU dlltool accepts both = and ==.
-      if (Buf.startswith("="))
+      if (Buf.startswith("=")) {
         Buf = Buf.drop_front();
+        return Token(EqualEqual, "==");
+      }
       return Token(Equal, "=");
     case ',':
       Buf = Buf.drop_front();
       return Token(Comma, ",");
     case '"': {
       StringRef S;
       std::tie(S, Buf) = Buf.substr(1).split('"');
       return Token(Identifier, S);
for (;;) {
       if (Tok.K == KwConstant) {
         E.Constant = true;
         continue;
       }
       if (Tok.K == KwPrivate) {
         E.Private = true;
         continue;
       }
+      if (Tok.K == EqualEqual) {
+        read();
+        E.AliasTarget = Tok.Value;
+        if (Machine == IMAGE_FILE_MACHINE_I386 && !isDecorated(E.AliasTarget, MingwDef))
+          E.AliasTarget = std::string("_").append(E.AliasTarget);
+        continue;
+      }
       unget();
       Info.Exports.push_back(E);
       return Error::success();
     }
   }
 
   // HEAPSIZE/STACKSIZE reserve[,commit]
   Error parseNumbers(uint64_t *Reserve, uint64_t *Commit) {

llvm/trunk/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp

int llvm::dlltoolDriverMain(llvm::ArrayRef<const char *> ArgsArr) {
   }
 
   std::string Path = Args.getLastArgValue(OPT_l);
   if (Path.empty())
     Path = getImplibPath(Def->OutputFile);
 
   if (Machine == IMAGE_FILE_MACHINE_I386 && Args.getLastArg(OPT_k)) {
     for (COFFShortExport& E : Def->Exports) {
-      if (E.isWeak() || (!E.Name.empty() && E.Name[0] == '?'))
+      if (!E.AliasTarget.empty() || (!E.Name.empty() && E.Name[0] == '?'))
         continue;
       E.SymbolName = E.Name;
       // Trim off the trailing decoration. Symbols will always have a
       // starting prefix here (either _ for cdecl/stdcall, @ for fastcall
       // or ? for C++ functions). Vectorcall functions won't have any
       // fixed prefix, but the function base name will still be at least
       // one char.
       E.Name = E.Name.substr(0, E.Name.find('@', 1));
       // By making sure E.SymbolName != E.Name for decorated symbols,
       // writeImportLibrary writes these symbols with the type
       // IMPORT_NAME_UNDECORATE.
     }
   }
 
-  if (writeImportLibrary(Def->OutputFile, Path, Def->Exports, Machine, true, true))
+  if (writeImportLibrary(Def->OutputFile, Path, Def->Exports, Machine, true))
     return 1;
   return 0;
 }

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

 ; RUN: llvm-dlltool -k -m i386 --input-def %s --output-lib %t.a
 ; RUN: llvm-readobj %t.a | FileCheck %s
 ; RUN: llvm-nm %t.a | FileCheck %s -check-prefix=CHECK-NM
 
 LIBRARY test.dll
 EXPORTS
 CdeclFunction
 StdcallFunction@4
 @FastcallFunction@4
-StdcallAlias@4=StdcallFunction@4
+StdcallAlias@4==StdcallFunction@4
 ??_7exception@@6B@
 
 ; CHECK: Name type: noprefix
 ; CHECK: Symbol: __imp__CdeclFunction
 ; CHECK: Symbol: _CdeclFunction
 ; CHECK: Name type: undecorate
 ; 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: U _StdcallFunction@4

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

 ; RUN: llvm-dlltool -m i386:x86-64 --input-def %s --output-lib %t.a
 ; RUN: llvm-nm %t.a | FileCheck %s
 
 LIBRARY test.dll
 EXPORTS
 TestFunction==AltTestFunction
+; When creating an import library, the DLL internal function name of
+; the implementation of a function isn't visible at all.
+ImpLibName = Implementation
+; 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:      U __imp_AltTestFunction
 ; CHECK-NEXT: w __imp_TestFunction
+; CHECK:      T ImpLibName
+; CHECK-NEXT: T __imp_ImpLibName
+; CHECK:      U AltTestFunction2
+; CHECK-NEXT: w ImpLibName2
+; CHECK:      U __imp_AltTestFunction2
+; CHECK-NEXT: w __imp_ImpLibName2
+; CHECK:      T ImpLibName3
+; CHECK-NEXT: T __imp_ImpLibName3