labath added a subscriber: spyffe.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D43688#1017831, @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
> 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