[lldb][COFF] Add note to forwarder export symbols in symtab

[alvinhochun]

Summary

Forwarder exports do not point to a real function or variable. Instead
they point to a string describing which DLL and symbol to forward to.
Any imports which uses them will be redirected by the loader
transparently. These symbols do not have much use in LLDB, but keep them
just in case someone find it useful. Also set a synthesized name with
the forwarder string for informational purpose.

Depends on D134426

[labath]

They may not be useful (at the moment), but if they're not actively causing harm (e.g. stopping some other feature from functioning, or if we're badly misrepresenting them in the symtab output), then I think we should keep them. You never know -- maybe someone will find a use for them.

[mstorsjo]

In D134518#3811153, @labath wrote:

They may not be useful (at the moment), but if they're not actively causing harm (e.g. stopping some other feature from functioning, or if we're badly misrepresenting them in the symtab output), then I think we should keep them. You never know -- maybe someone will find a use for them.

Hm, maybe, but they're just plain names even without an associated address? @alvinhochun If we keep them, do we need to adjust the code below to handle the fact that they're addressless?

[labath]

Where is the "dll description string" that they point to? Could they be made to point to that?

[alvinhochun]

In D134518#3811155, @mstorsjo wrote:

In D134518#3811153, @labath wrote:

They may not be useful (at the moment), but if they're not actively causing harm (e.g. stopping some other feature from functioning, or if we're badly misrepresenting them in the symtab output), then I think we should keep them. You never know -- maybe someone will find a use for them.

Hm, maybe, but they're just plain names even without an associated address? @alvinhochun If we keep them, do we need to adjust the code below to handle the fact that they're addressless?

Hmm I'm not sure how. They do have an address pointing to the forwarder string, but they are useless as it is now.

Mind that the DLL containing the forwarder export does not actually export the symbol. When another EXE or DLL imports the forwarder symbol, Windows sees that it is a forwarder and loads the forwarded target instead. Unless a real exported function is imported from this DLL, it isn't even loaded into the process (at least that's what I gather from Process Explorer's module search with "api-ms" -- DLLs using this as a prefix contains only forwarder symbols to ucrtbase.dll or other system DLLs).

If anyone wants to list these symbols they can always do it with llvm-objdump. (llvm-readobj isn't handling this right now but I will make a patch.)

In D134518#3811160, @labath wrote:

Where is the "dll description string" that they point to? Could they be made to point to that?

The current symbol address is already pointing to it.

[labath]

In D134518#3811206, @alvinhochun wrote:

In D134518#3811160, @labath wrote:

Where is the "dll description string" that they point to? Could they be made to point to that?

The current symbol address is already pointing to it.

Ok, so is there any harm in keeping them this way?

The symbols may not be very useful, but I would not say that they are useless. If, for whatever reason, the user finds himself inspecting the part of the memory covered by the forwarder string, then with this symbol, that memory would symbolicate as forwarded_symbol+X, which seems nice.

[alvinhochun]

In D134518#3811218, @labath wrote:

Ok, so is there any harm in keeping them this way?

The symbols may not be very useful, but I would not say that they are useless. If, for whatever reason, the user finds himself inspecting the part of the memory covered by the forwarder string, then with this symbol, that memory would symbolicate as forwarded_symbol+X, which seems nice.

I guess you're right, we can keep these symbols. Though do you think it make sense to synthesize a demangled name for the symbol, let's say ExportName (forwarded to kernel32.LoadLibrary)?

[alvinhochun]

Changed the patch to not remove the forwarder symbol, but instead add a note to the symbol name.

[labath]

In D134518#3811382, @alvinhochun wrote:

In D134518#3811218, @labath wrote:

Ok, so is there any harm in keeping them this way?

The symbols may not be very useful, but I would not say that they are useless. If, for whatever reason, the user finds himself inspecting the part of the memory covered by the forwarder string, then with this symbol, that memory would symbolicate as forwarded_symbol+X, which seems nice.

I guess you're right, we can keep these symbols. Though do you think it make sense to synthesize a demangled name for the symbol, let's say ExportName (forwarded to kernel32.LoadLibrary)?

Possibly. Are these basically the same as "undefined" (i.e., defined by another shared library) symbols on ELF? If so, then it may make sense to mark them as eSymbolTypeUndefined, in case some generic algorithm wants to do something with them. I believe MachO also has this notion of forwarding a symbol to a particular shared library (I think they call it "two-level namespacing"), so you may want to look at how those are represented.

But overall, I don't think this is particularly important, and we can change this later if we have a need for it.

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

for (const auto &entry : m_binary->export_directories()) {
     Symbol symbol;
     // Note: symbol name may be empty if it is only exported by ordinal.
     symbol.GetMangled().SetValue(ConstString(sym_name));
 
     uint32_t ordinal;
     llvm::cantFail(entry.getOrdinal(ordinal));
     symbol.SetID(ordinal);
 
+    bool is_forwarder;
+    llvm::cantFail(entry.isForwarder(is_forwarder));
+    if (is_forwarder) {
+      // Forwarder exports are redirected by the loader transparently, but keep
+      // it in symtab and make a note using the symbol name.
+      llvm::StringRef forwarder_name;
+      if (auto err = entry.getForwardTo(forwarder_name)) {
+        LLDB_LOG(log,
+                 "ObjectFilePECOFF::AppendFromExportTable - failed to get "
+                 "forwarder name of forwarder export '{0}': {1}",
+                 sym_name, llvm::fmt_consume(std::move(err)));
+        continue;
+      }
+      llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
+                                         " (forwarded to ", forwarder_name,
+                                         ")"};
+      symbol.GetMangled().SetDemangledName(ConstString(new_name.str()));
+      symbol.SetDemangledNameIsSynthesized(true);
+    }
+
     uint32_t function_rva;
     if (auto err = entry.getExportRVA(function_rva)) {
       LLDB_LOG(log,
                "ObjectFilePECOFF::AppendFromExportTable - failed to get "
                "address of export entry '{0}': {1}",
                sym_name, llvm::fmt_consume(std::move(err)));
       continue;
     }
     // Skip the symbol if it doesn't look valid.
     if (function_rva == 0 && sym_name.empty())
       continue;
     symbol.GetAddressRef() =
         Address(m_coff_header_opt.image_base + function_rva, sect_list);
 
     // An exported symbol may be either code or data. Guess by checking whether
     // the section containing the symbol is executable.
     symbol.SetType(lldb::eSymbolTypeData);
+    if (!is_forwarder)
       if (auto section_sp = symbol.GetAddressRef().GetSection())
         if (section_sp->GetPermissions() & ePermissionsExecutable)
           symbol.SetType(lldb::eSymbolTypeCode);
     symbol.SetExternal(true);
     uint32_t idx = symtab.AddSymbol(symbol);
     export_list.push_back(std::make_pair(function_rva, idx));
   }
   std::stable_sort(export_list.begin(), export_list.end(),
                    RVASymbolListCompareRVA);
   return export_list;
 }

lldb/test/Shell/ObjectFile/PECOFF/symbols-forwarder-export.yaml

+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# CHECK:       UserID DSX Type File Address/Value {{.*}} Size            Flags           Name
+# CHECK-NEXT:  ------
+# CHECK-NEXT:       1   X Data 0x{{[0-9a-f]+}}           0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} LoadLibrary (forwarded to kernel32.LoadLibrary)
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -dll -out:test.dll -entry:entry -export:LoadLibrary=kernel32.LoadLibrary test.obj
+# test.c:
+#   void entry(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+    RelativeVirtualAddress: 8192
+    Size:            110
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     1
+    SectionData:     C3
+  - Name:            .rdata
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  8192
+    VirtualSize:     110
+    SectionData:     0000000000000000000000002820000001000000010000000100000043200000472000004B2000006578706F72742D666F727761726465722E632E746D702E646C6C00592000004D20000000004C6F61644C696272617279006B65726E656C33322E4C6F61644C69627261727900
+symbols:         []
+...