[LLD] [COFF] Disallow replacing DefinedImport* symbols with other symbols

[mstorsjo]

Summary

When DefinedImportThunk and DefinedImportData symbols are created, a pointer to them is stored in the ImportFile class, stored as a pointer to the specific subclass. If these symbols are replaced, these pointers no longer point to this particular base class.

This fixes crashes that were possible before, by touching these symbols that now are populated as e.g. a DefinedRegular, via the old pointers with DefinedImportThunk type.

This is one way of solving the issue, the alternative would be to not store pointers to the subclass but actually check them on use.

[mstorsjo]

Committed the alternative, D48953, instead.

COFF/SymbolTable.cpp

 Symbol *SymbolTable::addRegular(InputFile *F, StringRef N,
                                 const coff_symbol_generic *Sym,
                                 SectionChunk *C) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedRegular>(S))
+  if (WasInserted || (!isa<DefinedRegular>(S) && !isa<DefinedImportThunk>(S) &&
+                      !isa<DefinedImportData>(S)))
     replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ false,
                                   /*IsExternal*/ true, Sym, C);
   else
     reportDuplicate(S, F);
   return S;
 }
 
 std::pair<Symbol *, bool>
 SymbolTable::addComdat(InputFile *F, StringRef N,
                        const coff_symbol_generic *Sym) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedRegular>(S)) {
+  if (WasInserted || (!isa<DefinedRegular>(S) && !isa<DefinedImportThunk>(S) &&
+                      !isa<DefinedImportData>(S))) {
     replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ true,
                                   /*IsExternal*/ true, Sym, nullptr);
     return {S, true};
   }
   if (!cast<DefinedRegular>(S)->isCOMDAT())
     reportDuplicate(S, F);
   return {S, false};
 }
 
 Symbol *SymbolTable::addCommon(InputFile *F, StringRef N, uint64_t Size,
                                const coff_symbol_generic *Sym, CommonChunk *C) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  if (WasInserted || !isa<DefinedCOFF>(S))
+  if (!WasInserted && (isa<DefinedImportThunk>(S) || isa<DefinedImportData>(S)))
+    reportDuplicate(S, F);
+  else if (WasInserted || !isa<DefinedCOFF>(S))
     replaceSymbol<DefinedCommon>(S, F, N, Size, Sym, C);
   else if (auto *DC = dyn_cast<DefinedCommon>(S))
     if (Size > DC->getSize())
       replaceSymbol<DefinedCommon>(S, F, N, Size, Sym, C);
   return S;
 }
 
 DefinedImportData *SymbolTable::addImportData(StringRef N, ImportFile *F) {

test/COFF/Inputs/otherFunc.s

+.global otherFunc
+.global MessageBoxA
+.text
+otherFunc:
+  ret
+MessageBoxA:
+  ret

test/COFF/thunk-replace.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-win32 %s -filetype=obj -o %t.main.obj
+# RUN: llvm-mc -triple=x86_64-win32 %p/Inputs/otherFunc.s -filetype=obj -o %t.other.obj
+# RUN: llvm-ar rcs %t.other.lib %t.other.obj
+# RUN: not lld-link -out:%t.exe -entry:main %t.main.obj %p/Inputs/std64.lib %t.other.lib -opt:noref 2>&1 | FileCheck %s
+# CHECK: duplicate symbol: MessageBoxA
+
+.global main
+.text
+main:
+  callq MessageBoxA
+  callq ExitProcess
+  callq otherFunc
+  ret