Bug 24574 - extern symbols in dlls are misleading under debugger
Summary: extern symbols in dlls are misleading under debugger
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: 2.33
Assignee: Eric Botcazou
URL: https://sourceware.org/ml/binutils/20...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-17 13:02 UTC by szabo bence
Modified: 2019-09-05 16:40 UTC (History)
3 users (show)

See Also:
Host: x86-64-mingw32
Target: x86-64-mingw32
Build: x86-64-mingw32
Last reconfirmed: 2019-05-20 00:00:00


Attachments
example (746 bytes, application/x-7z-compressed)
2019-05-17 13:02 UTC, szabo bence
Details

Note You need to log in before you can comment on or make changes to this bug.
Description szabo bence 2019-05-17 13:02:00 UTC
Created attachment 11782 [details]
example

With binutils 2.31 and later extern symbols in DLLs point to junk when inspected under a debugger. The source of the problem seems to be ld because the bug is reproducible with ld 2.31 and gdb 8.3 but it isn't with ld 2.30 with same gdb.

If your run make all in the attached example (after specifying the your toolchain) you find the the address of pcode_st is different in productionCode.dll and test.dll under a debugger.
If you change  extern prodCode_st pcode_st; in test.cpp  to __declspec(dllimport) prodCode_st pcode_st; the problem goes away.

linking with --enable-auto-import does not help.

toolchain for reproduction: mingw64 gcc 8.3 from msys2 + binutils 2.31.1 from http://repo.msys2.org/mingw/x86_64/
Comment 1 szabo bence 2019-05-18 14:13:06 UTC
@Eric:
Is this an intentional side effect of the "Speed up direct linking with DLLs on Windows" change?
Comment 2 szabo bence 2019-05-19 20:09:32 UTC
yep, bisected, started with 317ff0084bc68bfae2eabd82015e9239a68b5195
Comment 3 Eric Botcazou 2019-05-19 21:33:29 UTC
> Is this an intentional side effect of the "Speed up direct linking with DLLs
> on Windows" change?

Is that a rhetorical question?  If not, then I can assure you that I don't have the slightest idea about how GDB is supposed to inspect this kind of symbols...
Comment 4 Eric Botcazou 2019-05-20 10:03:16 UTC
I can reproduce.
Comment 5 Eric Botcazou 2019-05-20 10:03:36 UTC
Investigating.
Comment 6 szabo bence 2019-05-20 15:02:31 UTC
No, I honestly dont know if the change was aiming to remove pressure from the linker by e.g. ignoring the extra symbols only used by the debugger and so a patch in gdb is on it's way to address this or gdb is using something that it shouldn't. It's important as the llvm linker also implements the auto import and debugging those dlls have the same issue. I wanted to address the right audience with the ticket.
Comment 7 Martin Storsjö 2019-06-02 18:08:27 UTC
FWIW, I'm planning on looking into this in LLD, but if Eric is looking into it in binutils ld, I'd wait for progress on that first, as that's probably valuable info about what gdb requires to handle these symbols correctly.
Comment 8 szabo bence 2019-07-18 21:22:03 UTC
ping
Comment 9 Martin Storsjö 2019-08-03 20:59:19 UTC
I went ahead and dug into this a little bit more. First off, props for the great testcase, it was great to even have the gdb test routine automated.

I haven't dug into the gdb sources to figure out exactly how/why it does resolve those symbols, but the difference between what ld.bfd produces is easily visible.

With ld.bfd before the referenced commit, the symbol table for the linked test.dll looks like this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
000000006bec4260 R __fu0_pcode_st
000000006bec9300 I __imp_pcode_st
000000006bec9300 I __imp_pcode_st
000000006bec95ac I __nm_pcode_st
000000006bec4260 r .rdata$.refptr.pcode_st
000000006bec4260 R .refptr.pcode_st 

Note that there's two symbols with the name __imp_pcode_st.

With ld.bfd after that referenced commit, the symbol table looks like this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
000000006bec4260 R __fu0_pcode_st
000000006bec9300 I __imp_pcode_st
000000006bec95ac I __nm_pcode_st
000000006bec9300 I pcode_st
000000006bec4260 r .rdata$.refptr.pcode_st
000000006bec4260 R .refptr.pcode_st 

Now there's one symbol named __imp_pcode_st and one named pcode_st, but both with the same address. The unprefixed symbol probably is what gbb (rightly) picks up and considers the real address of the symbol, even if it points to the import address table.

Likewise if LLD links against an import library generated by ld.bfd (LLD doesn't support linking directly against DLLs), gdb also picks up the wrong address of the symbol (using the IAT address instead), and the symbol table contains this:
$ x86_64-w64-mingw32-nm test.dll | grep pcode_st
00000001800038e0 R __imp_pcode_st
0000000180003b8c R __nm_pcode_st
00000001800038e0 R pcode_st
00000001800030b0 r .rdata$.refptr.pcode_st
00000001800030b0 R .refptr.pcode_st 

Now I can easily make LLD stop including the unprefixed symbol in the symbol table, and then gdb manages to figure out the address of the symbol correctly. I'll send a patch to LLD for this after I've made an accompanying mandatory testcase, but the fix essentially looks like this:

diff --git a/COFF/Writer.cpp b/COFF/Writer.cpp
index 3da8b98d3..e157d6dff 100644
--- a/COFF/Writer.cpp
+++ b/COFF/Writer.cpp
@@ -1095,6 +1095,9 @@ Optional<coff_symbol16> Writer::createSymbol(Defined *def) { 
   }
   }

+  if (def->isRuntimePseudoReloc)
+    return None;
+ 
   StringRef name = def->getName();
   if (name.size() > COFF::NameSize) {
     sym.Name.Offset.Zeroes = 0;


Even more surprisingly though, if I compile test.cpp with clang instead of gcc, the gdb test succeeds, with both LLD (without this fix) and ld.bfd after the change. Not sure if this stems from slightly different structure of .refptr and similar structures, or different debug info that gdb manages to use better wrt this.
Comment 10 Martin Storsjö 2019-08-06 18:12:50 UTC
The corresponding issue in LLD is fixed in https://reviews.llvm.org/D65727, both in trunk and in the upcoming 9.x release branch.
Comment 11 Eric Botcazou 2019-09-05 14:43:56 UTC
> I went ahead and dug into this a little bit more. First off, props for the
> great testcase, it was great to even have the gdb test routine automated.

Thanks for the analysis!  The __imp prefix has been dropped because I have dropped this piece of code from pe_find_data_imports in the patch:

-	      /* We replace original name with __imp_ prefixed, this
-		 1) may trash memory 2) leads to duplicate symbol generation.
-		 Still, IMHO it's better than having name polluted.  */
-	      undef->root.string = sym->root.string;
Comment 12 cvs-commit@gcc.gnu.org 2019-09-05 16:33:12 UTC
The master branch has been updated by Eric Botcazou <ebotcazou@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=de07a745805b28ed2c973635752719c4a6a32b1d

commit de07a745805b28ed2c973635752719c4a6a32b1d
Author: Eric Botcazou <ebotcazou@gcc.gnu.org>
Date:   Thu Sep 5 18:23:37 2019 +0200

    Fix PR ld/24574
    
    This restores a line that has been dropped when the auto-import feature
    of the PE-COFF linker was overhauled about one year.  It is necessary
    for GDB to properly resolve extern symbol in DLLs.
    
    ld/ChangeLog
    	* pe-dll.c (pe_find_data_imports): Replace again the original name
    	of the undefined symbol with the __imp_ prefixed one after it is
    	resolved.
Comment 13 cvs-commit@gcc.gnu.org 2019-09-05 16:36:36 UTC
The binutils-2_32-branch branch has been updated by Eric Botcazou <ebotcazou@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6f7a2922078bde30ca6ec1391a30d79f59c87b9a

commit 6f7a2922078bde30ca6ec1391a30d79f59c87b9a
Author: Eric Botcazou <ebotcazou@gcc.gnu.org>
Date:   Thu Sep 5 18:23:37 2019 +0200

    Fix PR ld/24574
    
    This restores a line that has been dropped when the auto-import feature
    of the PE-COFF linker was overhauled about one year.  It is necessary
    for GDB to properly resolve extern symbol in DLLs.
    
    ld/ChangeLog
    	* pe-dll.c (pe_find_data_imports): Replace again the original name
    	of the undefined symbol with the __imp_ prefixed one after it is
    	resolved.
Comment 14 cvs-commit@gcc.gnu.org 2019-09-05 16:38:43 UTC
The binutils-2_31-branch branch has been updated by Eric Botcazou <ebotcazou@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=34a30b6758db4fe53b2872d7bd3824c778f6118c

commit 34a30b6758db4fe53b2872d7bd3824c778f6118c
Author: Eric Botcazou <ebotcazou@gcc.gnu.org>
Date:   Thu Sep 5 18:23:37 2019 +0200

    Fix PR ld/24574
    
    This restores a line that has been dropped when the auto-import feature
    of the PE-COFF linker was overhauled about one year.  It is necessary
    for GDB to properly resolve extern symbol in DLLs.
    
    ld/ChangeLog
    	* pe-dll.c (pe_find_data_imports): Replace again the original name
    	of the undefined symbol with the __imp_ prefixed one after it is
    	resolved.
Comment 15 Eric Botcazou 2019-09-05 16:40:59 UTC
This should work again now.