labath added a subscriber: spyffe.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In, @amccarth wrote:

> 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 Sean was the one who added this test, so we would have to ask him if 
this made a difference. What I know is that looking up things in the debug info 
and looking up via symbol table uses quite distinct code paths, and in fact I 
was not able to reproduce the bug in the second test in this test case 
(test_shadowed) without actually removing debug info (that is why I added the 
second test to this file).

So, not having a conflicting symbol in the debug info would *not* be invalid, 
and it sounds like a reasonable thing to test. However, I think we should do 
that via a separate test instead of modifying this test case.

> 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.

Fair enough, that's true. lgtm then.

lldb-commits mailing list

Reply via email to