[COFF] Add address-taken import thunks to the fid table

[rnk]

Summary

Fixes PR39799

[dmajor]

So after some further digging I was wrong, and my particular bug isn't to do with S->kind(). :(

The function that I'm interested in (a Rust function, on arm64, if it matters) is apparently failing the IMAGE_SYM_DTYPE_FUNCTION test. Is there any other category of thing that it could be?

[dmajor]

The function that I'm interested in (a Rust function, on arm64, if it matters) is apparently failing the IMAGE_SYM_DTYPE_FUNCTION test. Is there any other category of thing that it could be?

Its getType() is 0, if that means anything.

[rnk]

lld/test/COFF/guardcf-thunk.s

28 ↗

(On Diff #188625)

I think IMAGE_SYM_DTYPE_FUNCTION is set by the .type directive in this .def/.endef block. I guess what we're doing doesn't match MSVC, but it also seems reasonable. Is the Rust function written in assembly, or is it emitted in LLVM? We should be setting the appropriate type...

[dmajor]

lld/test/COFF/guardcf-thunk.s

28 ↗

(On Diff #188625)

The function is on_tls_callback at https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/thread_local.rs#L201.

(It's unclear to me if the linker magic related to TLS callbacks is related to this issue. This is just the first function we fail at. I don't know if there are more "normal" functions that we'd fail on after it.)

[rnk]

lld/test/COFF/guardcf-thunk.s

28 ↗

(On Diff #188625)

Well, most global variables also seem to have type "notype", so the only way we could make this work is by dropping the symbol type check completely and just looking at the section executability.

[rnk]

lld/test/COFF/guardcf-thunk.s

28 ↗

(On Diff #188625)

I checked, and MSVC performs this symbol type check, so we are being compatible, and I don't think we should remove it.

[dmajor]

Oh you know what, I bet it's https://reviews.llvm.org/D53540#1326473. Rust has an LLVM roll in progress as I write this, so this should just sort itself out.

Sorry for making you write a "needless" patch, though I'm glad that at least the original bug you filed is fixed!

COFF/Writer.cpp

static void addSymbolToRVASet(SymbolRVASet &RVASet, Defined *S) {
   uint32_t Off = S->getRVA() - (C ? C->getRVA() : 0);
   RVASet.insert({C, Off});
 }
 
 // Given a symbol, add it to the GFIDs table if it is a live, defined, function
 // symbol in an executable section.
 static void maybeAddAddressTakenFunction(SymbolRVASet &AddressTakenSyms,
                                          Symbol *S) {
-  auto *D = dyn_cast_or_null<DefinedCOFF>(S);
-
-  // Ignore undefined symbols and references to non-functions (e.g. globals and
-  // labels).
-  if (!D ||
-      D->getCOFFSymbol().getComplexType() != COFF::IMAGE_SYM_DTYPE_FUNCTION)
+  if (!S)
     return;
 
-  // Mark the symbol as address taken if it's in an executable section.
+  switch (S->kind()) {
+  case Symbol::DefinedLocalImportKind:
+  case Symbol::DefinedImportDataKind:
+    // Defines an __imp_ pointer, so it is data, so it is ignored.
+    break;
+  case Symbol::DefinedCommonKind:
+    // Common is always data, so it is ignored.
+    break;
+  case Symbol::DefinedAbsoluteKind:
+  case Symbol::DefinedSyntheticKind:
+    // Absolute is never code, synthetic generally isn't and usually isn't
+    // determinable.
+    break;
+  case Symbol::LazyKind:
+  case Symbol::UndefinedKind:
+    // Undefined symbols resolve to zero, so they don't have an RVA. Lazy
+    // symbols shouldn't have relocations.
+    break;
+
+  case Symbol::DefinedImportThunkKind:
+    // Thunks are always code, include them.
+    addSymbolToRVASet(AddressTakenSyms, cast<Defined>(S));
+    break;
+
+  case Symbol::DefinedRegularKind: {
+    // This is a regular, defined, symbol from a COFF file. Mark the symbol as
+    // address taken if the symbol type is function and it's in an executable
+    // section.
+    auto *D = cast<DefinedRegular>(S);
+    if (D->getCOFFSymbol().getComplexType() == COFF::IMAGE_SYM_DTYPE_FUNCTION) {
       Chunk *RefChunk = D->getChunk();
       OutputSection *OS = RefChunk ? RefChunk->getOutputSection() : nullptr;
       if (OS && OS->Header.Characteristics & IMAGE_SCN_MEM_EXECUTE)
         addSymbolToRVASet(AddressTakenSyms, D);
     }
+    break;
+  }
+  }
+}
 
 // Visit all relocations from all section contributions of this object file and
 // mark the relocation target as address-taken.
 static void markSymbolsWithRelocations(ObjFile *File,
                                        SymbolRVASet &UsedSymbols) {
   for (Chunk *C : File->getChunks()) {
     // We only care about live section chunks. Common chunks and other chunks
     // don't generally contain relocations.

test/COFF/guardcf-thunk.s

+# REQUIRES: x86
+
+# Make a DLL that exports exportfn1.
+# RUN: yaml2obj < %p/Inputs/export.yaml > %t.obj
+# RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1 /implib:%t.lib
+
+# Make an obj that takes the address of that exported function.
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc %s -o %t2.obj
+# RUN: lld-link -entry:main -guard:cf %t2.obj %t.lib -nodefaultlib -out:%t.exe
+# RUN: llvm-readobj -coff-load-config %t.exe | FileCheck %s
+
+# Check that the gfids table contains *exactly* two entries, one for exportfn1
+# and one for main.
+# CHECK: GuardFidTable [
+# CHECK-NEXT: 0x{{[0-9A-Fa-f]+0$}}
+# CHECK-NEXT: 0x{{[0-9A-Fa-f]+0$}}
+# CHECK-NEXT: ]
+
+
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 0x001
+
+        .section .text,"rx"
+        .def     main; .scl    2; .type   32; .endef
+        .global main
+main:
+        leaq exportfn1(%rip), %rax
+        retq
+
+        .section .rdata,"dr"
+.globl _load_config_used
+_load_config_used:
+        .long 256
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 128, 1, 0
+