[LLD] [COFF] Keep the underscore on exported decorated stdcall functions

[mstorsjo]

Summary

This (together with D41631) fixes PR35733.

I'm not very fond of adding yet another flag to signal this particular case, but I'm not sure if it'd be better to overload the existing fields Name/SymbolName/ExtName with extra interpretations for this particular case (I'm afraid that would break some other case where their differences actually matter).

[ruiu]

Hmm, if we have to do this, this patch is okay, but it feels to me we are building a complex theory to describe the behavior of the MSVC linker, but the MSVC linker is likely to follow some simple rules that we do not understand yet. Do you think we can find simpler rules to describe all these behaivors?

[mstorsjo]

In D41632#970546, @ruiu wrote:

Hmm, if we have to do this, this patch is okay, but it feels to me we are building a complex theory to describe the behavior of the MSVC linker, but the MSVC linker is likely to follow some simple rules that we do not understand yet.

Yes, I have the exact same feeling.

Do you think we can find simpler rules to describe all these behaivors?

Maybe, but I think it probably would require adjusting a lot of the logic about how symbol names are decorated/undecorated throughout the linker. A couple months ago when I revisited this topic the last time, I was able to make all the other cases work pretty easily, except this one, and I had hoped nobody actually used/cared about the behaviour in this one.

[ruiu]

Could you add a brief comment to describe the situation (we might not understand the underlying logic correctly, but all these struct members are required to mimic the exact behavior of the observed MSVC link.exe behavior.) It's also nice to add a comment on KeepDecoration.

[mstorsjo]

I figured I could fix this in a much more straightforward fashion, instead of adding a new, very vaguely defined field to the data structures.

[ruiu]

LGTM

This is much much better. Thank you very much for making the improvement over the original patch!

COFF/Config.h

struct Export {
   StringRef Name;       // N in /export:N or /export:E=N
   StringRef ExtName;    // E in /export:E=N
   Symbol *Sym = nullptr;
   uint16_t Ordinal = 0;
   bool Noname = false;
   bool Data = false;
   bool Private = false;
   bool Constant = false;
+  bool KeepDecoration = false;
 
   // If an export is a form of /export:foo=dllname.bar, that means
   // that foo should be exported as an alias to bar in the DLL.
   // ForwardTo is set to "dllname.bar" part. Usually empty.
   StringRef ForwardTo;
   StringChunk *ForwardChunk = nullptr;
 
   // True if this /export option was in .drectves section.

COFF/Driver.cpp

for (auto *Arg : Args) {
     case OPT_export: {
       Export E = parseExport(Arg->getValue());
       if (Config->Machine == I386 && Config->MinGW) {
         if (!isDecorated(E.Name))
           E.Name = Saver.save("_" + E.Name);
         if (!E.ExtName.empty() && !isDecorated(E.ExtName))
           E.ExtName = Saver.save("_" + E.ExtName);
       }
+      if (Config->Machine == I386 && !Config->MinGW && E.Name.startswith("_") &&
+          E.Name.contains('@'))
+        E.KeepDecoration = true;
       E.Directives = true;
       Config->Exports.push_back(E);
       break;
     }
     case OPT_failifmismatch:
       checkFailIfMismatch(Arg->getValue());
       break;
     case OPT_incl:
for (Export &E1 : Config->Exports) {
     E2.Name = E1.Name;
     E2.SymbolName = E1.SymbolName;
     E2.ExtName = E1.ExtName;
     E2.Ordinal = E1.Ordinal;
     E2.Noname = E1.Noname;
     E2.Data = E1.Data;
     E2.Private = E1.Private;
     E2.Constant = E1.Constant;
+    E2.KeepDecoration = E1.KeepDecoration;
     Exports.push_back(E2);
   }
 
   auto E = writeImportLibrary(getImportName(AsLib), getImplibPath(), Exports,
                               Config->Machine, false);
   handleAllErrors(std::move(E),
                   [&](ErrorInfoBase &EIB) { error(EIB.message()); });
 }
for (COFFShortExport E1 : M.Exports) {
     E2.Name = Saver.save(E1.Name);
     if (E1.isWeak())
       E2.ExtName = Saver.save(E1.ExtName);
     E2.Ordinal = E1.Ordinal;
     E2.Noname = E1.Noname;
     E2.Data = E1.Data;
     E2.Private = E1.Private;
     E2.Constant = E1.Constant;
+    E2.KeepDecoration = E1.KeepDecoration;
     Config->Exports.push_back(E2);
   }
 }
 
 // A helper function for filterBitcodeFiles.
 static bool needsRebuilding(MemoryBufferRef MB) {
   // The MSVC linker doesn't support thin archives, so if it's a thin
   // archive, we always need to rebuild it.
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Handle /export
   for (auto *Arg : Args.filtered(OPT_export)) {
     Export E = parseExport(Arg->getValue());
     if (Config->Machine == I386) {
       if (!isDecorated(E.Name))
         E.Name = Saver.save("_" + E.Name);
       if (!E.ExtName.empty() && !isDecorated(E.ExtName))
         E.ExtName = Saver.save("_" + E.ExtName);
+      if (Config->Machine == I386 && !Config->MinGW && E.Name.startswith("_") &&
+          E.Name.contains('@'))
+        E.KeepDecoration = true;
     }
     Config->Exports.push_back(E);
   }
 
   // Handle /def
   if (auto *Arg = Args.getLastArg(OPT_deffile)) {
     // parseModuleDefs mutates Config object.
     parseModuleDefs(Arg->getValue());

COFF/DriverUtils.cpp

while (!Rest.empty()) {
     goto err;
   }
   return E;
 
 err:
   fatal("invalid /export: " + Arg);
 }
 
-static StringRef undecorate(StringRef Sym) {
-  if (Config->Machine != I386)
+static StringRef undecorate(StringRef Sym, bool KeepDecoration = false) {
+  if (Config->Machine != I386 || KeepDecoration)
     return Sym;
   return Sym.startswith("_") ? Sym.substr(1) : Sym;
 }
 
 // Performs error checking on all /export arguments.
 // It also sets ordinals.
 void fixupExports() {
   // Symbol ordinals must be unique.
if (!E.ForwardTo.empty() || !Sym) {
       E.SymbolName = Sym->getName();
     }
   }
 
   for (Export &E : Config->Exports) {
     if (!E.ForwardTo.empty()) {
       E.ExportName = undecorate(E.Name);
     } else {
-      E.ExportName = undecorate(E.ExtName.empty() ? E.Name : E.ExtName);
+      E.ExportName =
+          undecorate(E.ExtName.empty() ? E.Name : E.ExtName, E.KeepDecoration);
     }
   }
 
   // Uniquefy by name.
   DenseMap<StringRef, Export *> Map(Config->Exports.size());
   std::vector<Export> V;
   for (Export &E : Config->Exports) {
     auto Pair = Map.insert(std::make_pair(E.ExportName, &E));

test/COFF/def-export-stdcall.s

 # RUN: echo -e "LIBRARY foo\nEXPORTS\n  _stdcall@8\n  @fastcall@8\n  vectorcall@@8" > %t.deco.def
 # RUN: lld-link -entry:dllmain -dll -def:%t.deco.def %t.obj -out:%t.dll -implib:%t.lib
 # RUN: llvm-readobj %t.lib | FileCheck -check-prefix DECORATED-IMPLIB %s
 # RUN: llvm-readobj -coff-exports %t.dll | FileCheck -check-prefix DECORATED-EXPORTS %s
 
 # DECORATED-IMPLIB: Name type: name
 # DECORATED-IMPLIB-NEXT: __imp_@fastcall@8
 # DECORATED-IMPLIB-NEXT: @fastcall@8
-# TODO: To match link.exe, this one should also be Name type: name.
-# DECORATED-IMPLIB: Name type: noprefix
+# DECORATED-IMPLIB: Name type: name
 # DECORATED-IMPLIB-NEXT: __imp__stdcall@8
 # DECORATED-IMPLIB-NEXT: _stdcall@8
 # DECORATED-IMPLIB: Name type: name
 # DECORATED-IMPLIB-NEXT: __imp_vectorcall@@8
 # DECORATED-IMPLIB-NEXT: vectorcall@@8
 
 # DECORATED-EXPORTS: Name: @fastcall@8
-# TODO: To match link.exe, this one should actually be _stdcall@8
-# DECORATED-EXPORTS: Name: stdcall@8
+# DECORATED-EXPORTS: Name: _stdcall@8
 # DECORATED-EXPORTS: Name: vectorcall@@8
 
 
 # RUN: echo -e "LIBRARY foo\nEXPORTS\n  stdcall@8\n  @fastcall@8" > %t.deco-mingw.def
 # RUN: lld-link -lldmingw -entry:dllmain -dll -def:%t.deco-mingw.def %t.obj -out:%t.dll -implib:%t.lib
 # RUN: llvm-readobj %t.lib | FileCheck -check-prefix DECORATED-MINGW-IMPLIB %s
 # RUN: llvm-readobj -coff-exports %t.dll | FileCheck -check-prefix DECORATED-MINGW-EXPORTS %s
 

test/COFF/dllexport.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=i686-windows-msvc %s -o %t.obj
 
 # RUN: lld-link -entry:dllmain -dll %t.obj -out:%t.dll -implib:%t.lib
 # RUN: llvm-readobj %t.lib | FileCheck -check-prefix DECORATED-IMPLIB %s
 # RUN: llvm-readobj -coff-exports %t.dll | FileCheck -check-prefix DECORATED-EXPORTS %s
 
 # DECORATED-IMPLIB: Name type: name
 # DECORATED-IMPLIB-NEXT: __imp_@fastcall@8
 # DECORATED-IMPLIB-NEXT: @fastcall@8
+# DECORATED-IMPLIB: Name type: name
+# DECORATED-IMPLIB-NEXT: __imp__stdcall@8
+# DECORATED-IMPLIB-NEXT: _stdcall@8
 # DECORATED-IMPLIB: Name type: noprefix
 # DECORATED-IMPLIB-NEXT: __imp___underscored
 # DECORATED-IMPLIB-NEXT: __underscored
-# TODO: To match link.exe, this one should also be Name type: name.
-# DECORATED-IMPLIB: Name type: noprefix
-# DECORATED-IMPLIB-NEXT: __imp__stdcall@8
-# DECORATED-IMPLIB-NEXT: _stdcall@8
 # DECORATED-IMPLIB: Name type: name
 # DECORATED-IMPLIB-NEXT: __imp_vectorcall@@8
 # DECORATED-IMPLIB-NEXT: vectorcall@@8
 
 # DECORATED-EXPORTS: Name: @fastcall@8
+# DECORATED-EXPORTS: Name: _stdcall@8
 # DECORATED-EXPORTS: Name: _underscored
-# TODO: To match link.exe, this one should actually be _stdcall@8
-# DECORATED-EXPORTS: Name: stdcall@8
 # DECORATED-EXPORTS: Name: vectorcall@@8
 
         .def     _stdcall@8;
         .scl    2;
         .type   32;
         .endef
         .globl  _stdcall@8
         .globl  @fastcall@8