[LLD][COFF] Improve symbol table info for import thunk

[alvinhochun]

Summary

Import thunks themselves contain a jump or branch, which is code by
nature. Therefore the import thunk symbol should be marked as function
type in the symbol table to help with debugging.

The __imp_ import symbol associated to the import thunk is also useful
for debugging. However, when the import symbol isn't directly referenced
outside of the import thunk, it doesn't normally get added to the symbol
table. This change teaches LLD to add the import symbol explicitly.

[mstorsjo]

Import thunks themselves contain a jump or branch, which is code by nature. Therefore the import thunk symbol should be marked as function type in the symbol table to help with debugging.

Totally agree about this.

The __imp_ import symbol associated to the import thunk is also useful for debugging. However, when the import symbol isn't directly referenced outside of the import thunk, it doesn't normally get added to the symbol table. This change teaches LLD to add the import symbol explicitly.

My initial reaction here is that I wonder when you'd want to look at these symbols in "normal" application level debugging. But on the other hand, I presume we're already inconsistently adding some __imp_ symbols to the symbol table, but not all of them, so maybe it's just for the best to add all of them. Additionally, if it makes things more consistent, and is useful for lower level debugging (like when figuring out which part of the toolchain is misbehaving), then it's probably good to add. So yeah, +1 from me to this too.

lld/test/COFF/symtab.test

25

If the actual value isn't very important for the test, one can also just strip this down to CHECK-NEXT: Value:, leaving out the value itself.

[alvinhochun]

In D134169#3799044, @mstorsjo wrote:

My initial reaction here is that I wonder when you'd want to look at these symbols in "normal" application level debugging. But on the other hand, I presume we're already inconsistently adding some __imp_ symbols to the symbol table, but not all of them, so maybe it's just for the best to add all of them. Additionally, if it makes things more consistent, and is useful for lower level debugging (like when figuring out which part of the toolchain is misbehaving), then it's probably good to add. So yeah, +1 from me to this too.

It was more for the sake of completeness, and also because ld.bfd seems to do the same.

The real reason which led me to add this symbol is: When I was using LLDB to inspect binaries without the import thunk being marked as functions, calls into the import thunk do not have any comments, and disassembling the import thunk shows a bogus label with symbol+offset (from another import symbol). Although marking the import thunk symbol as function already solves the biggest issue, it just annoys me knowing that LLDB would still show the wrong label for the jump inside the import thunk when not having the __imp_ symbol. (There is another way to fix this, which is to have LLDB load symbols from the import table, but that's more complicated.)

lld/test/COFF/symtab.test

25

Right, I agree the value isn't really important.

[alvinhochun]

Changed test to be less specific

[mstorsjo]

In D134169#3799064, @alvinhochun wrote:

In D134169#3799044, @mstorsjo wrote:

My initial reaction here is that I wonder when you'd want to look at these symbols in "normal" application level debugging. But on the other hand, I presume we're already inconsistently adding some __imp_ symbols to the symbol table, but not all of them, so maybe it's just for the best to add all of them. Additionally, if it makes things more consistent, and is useful for lower level debugging (like when figuring out which part of the toolchain is misbehaving), then it's probably good to add. So yeah, +1 from me to this too.

It was more for the sake of completeness, and also because ld.bfd seems to do the same.

The real reason which led me to add this symbol is: When I was using LLDB to inspect binaries without the import thunk being marked as functions, calls into the import thunk do not have any comments, and disassembling the import thunk shows a bogus label with symbol+offset (from another import symbol). Although marking the import thunk symbol as function already solves the biggest issue, it just annoys me knowing that LLDB would still show the wrong label for the jump inside the import thunk when not having the __imp_ symbol. (There is another way to fix this, which is to have LLDB load symbols from the import table, but that's more complicated.)

Oh, that's a great reason for adding them indeed. Yes, for stepping through things like thunks, having it show the right symbol name can be very valuable.

LGTM, thanks!

lld/COFF/Writer.cpp

if (name.size() > COFF::NameSize) {
     memset(sym.Name.ShortName, 0, COFF::NameSize);
     memcpy(sym.Name.ShortName, name.data(), name.size());
   }
 
   if (auto *d = dyn_cast<DefinedCOFF>(def)) {
     COFFSymbolRef ref = d->getCOFFSymbol();
     sym.Type = ref.getType();
     sym.StorageClass = ref.getStorageClass();
+  } else if (def->kind() == Symbol::DefinedImportThunkKind) {
+    sym.Type = (IMAGE_SYM_DTYPE_FUNCTION << SCT_COMPLEX_TYPE_SHIFT) |
+               IMAGE_SYM_TYPE_NULL;
+    sym.StorageClass = IMAGE_SYM_CLASS_EXTERNAL;
   } else {
     sym.Type = IMAGE_SYM_TYPE_NULL;
     sym.StorageClass = IMAGE_SYM_CLASS_EXTERNAL;
   }
   sym.NumberOfAuxSymbols = 0;
   return sym;
 }
 
for (ObjFile *file : ctx.objFileInstances) {
           COFFSymbolRef symRef = dc->getCOFFSymbol();
           if (symRef.isSectionDefinition() ||
               symRef.getStorageClass() == COFF::IMAGE_SYM_CLASS_LABEL)
             continue;
         }
 
         if (Optional<coff_symbol16> sym = createSymbol(d))
           outputSymtab.push_back(*sym);
+
+        if (auto *dthunk = dyn_cast<DefinedImportThunk>(d)) {
+          if (!dthunk->wrappedSym->writtenToSymtab) {
+            dthunk->wrappedSym->writtenToSymtab = true;
+            if (Optional<coff_symbol16> sym = createSymbol(dthunk->wrappedSym))
+              outputSymtab.push_back(*sym);
+          }
+        }
       }
     }
   }
 
   if (outputSymtab.empty() && strtab.empty())
     return;
 
   // We position the symbol table to be adjacent to the end of the last section.

lld/test/COFF/symtab.test

 # RUN: llvm-readobj --symbols %t.exe | FileCheck -check-prefix=NO %s
 
 # CHECK:      Symbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: MessageBoxA
 # CHECK-NEXT:     Value: 80
 # CHECK-NEXT:     Section: .text (1)
 # CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Function (0x2)
+# CHECK-NEXT:     StorageClass: External (0x2)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: __imp_MessageBoxA
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Section: .rdata (2)
+# CHECK-NEXT:     BaseType: Null (0x0)
 # CHECK-NEXT:     ComplexType: Null (0x0)
 # CHECK-NEXT:     StorageClass: External (0x2)
 # CHECK-NEXT:     AuxSymbolCount: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: ExitProcess
 # CHECK-NEXT:     Value: 64
 # CHECK-NEXT:     Section: .text (1)
 # CHECK-NEXT:     BaseType: Null (0x0)
+# CHECK-NEXT:     ComplexType: Function (0x2)
+# CHECK-NEXT:     StorageClass: External (0x2)
+# CHECK-NEXT:     AuxSymbolCount: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: __imp_ExitProcess
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:     Section: .rdata (2)
+# CHECK-NEXT:     BaseType: Null (0x0)
 # CHECK-NEXT:     ComplexType: Null (0x0)
 # CHECK-NEXT:     StorageClass: External (0x2)
 # CHECK-NEXT:     AuxSymbolCount: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: message
 # CHECK-NEXT:     Value: 6
 # CHECK-NEXT:     Section: .text2