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 > 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. https://reviews.llvm.org/D43688 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
