[Lldb-commits] [PATCH] D43688: Partial fix for TestConflictingSymbol.py on Windows

Fri Feb 23 14:18:57 PST 2018

amccarth added a comment.

In https://reviews.llvm.org/D43688#1017691, @labath wrote:

> In https://reviews.llvm.org/D43688#1017652, @amccarth wrote:
>
> > In https://reviews.llvm.org/D43688#1017638, @labath wrote:
> >
> > > On non-windows systems we're expected to pick up the name from the symbol table. Is there such a thing on windows as well?
> >
> >
> > Given that constant is declared with visibility("hidden"), why would it end up in a symbol table?  Windows DLLs have tables for items explicitly exported, but that's contrary to asking the symbol to be hidden.
>
>
> visibility((hidden)) means it won't end up in the *exported* symbol table (.dynsym on linux). However, the compiler will still put it into the local symbol table (.symtab, which is not used by the linker or at runtime, but the debugger can use it to get more insight).

OK, as far as I can tell, Windows doesn't have anything equivalent to the .symtab:  a DLL has exported symbols and it might have debug info (usually in a PDB).  There's no "third place" to look.

This test seems intent on making sure this symbol is neither exported nor placed in the debug info.  I can see why that would be an interesting test case on Linux, since there you've got .symtab.

But that doesn't seem to be the intent of _this_ test.  It seems that the intent here is to make sure the debugger can distinguish between two identically named symbols that happen to reside in different shared libraries/DLLs.  I don't understand why the test is trying to keep these symbols out of the debug info.

> I have no idea how these things work on windows. If we have no way of finding this variable without debug info, then possibly the test just does not makes sense on windows.

(And I have no ideas how things work on Linux.)

What I think the test is supposed to check (distinguishing between symbols in different libraries) _is_ interesting on Windows.  Finding symbols that are not in the exported symbol table nor in the debug info would not be interesting on Windows.  Unfortunately, this test is trying to do both of those.

I'm tempted to remove the extra work the test does to keep the symbols out of the debug info, so that we can test what the test actually purports to test.  But I don't know enough about Linux.  Would having the conflicting symbol in the debug info of both libraries be invalid?

> 
> 
>>> I think this could also be related to your  breakpoint location resolving problem. The test explicitly calls `self.registerSharedLibrariesWithTarget` which explicitly adds the shared library to the list of target modules, so (assuming registerSharedLibrariesWithTarget works on windows) this test should not depend on automatic library searching in lldb. I'm guessing you have some problems looking things up when debug info is absent.
>> 
>> No, registering the libraries doesn't work on Windows (as discussed in the other patch were we introduced the -2 magic value).  The Windows search rules for DLLs are quite complex -- it would be very difficult to replicate them in lldb.
> 
> Yes, but here I am not talking about parsing the imports table of the EXE and trying to replicate the OS search logic. Here, the test explicitly calls SBTarget.AddModule and specifies the full path of the module to add. I think this should work regardless of the platform.

While it's unlikely in this case, I believe the Windows rules are convoluted enough that, in the general case, you might not get the exact DLL you request even when using a full path.  And, anyway, pre-loading the module doesn't seem to be the point of _this_ test.

> 
> 
>>> With all that in mind, I don't think this patch would hurt this test, but I don't believe it will help you getting the test passing either, so I would keep the num_expected_locations as-is (the LLDB_TEST_API thingy is obviously fine).
>> 
>> But that would leave the test failing for a reason other than the underlying problem:  that we can't resolve the symbol, which probably indicates an incomplete implementation somewhere.  I think a test that failure that points to the root problem is more useful than a general this-doesn't-even-link-on-Windows error.
> 
> IIUC, the LLDB_TEST_API is the thing which fixes the link error. I have no objections to that.
> 
> The part that is not clear to me is why the test is failing to resolve the breakpoint even though the module was explicitly added through SBTarget.AddImage. Can you check at what the module list looks like after the AddImage call?

https://reviews.llvm.org/D43688