Partial fix for TestConflictingSymbol.py on Windows

[amccarth]

Summary

Without this fix, the test ERRORs because the link of the inferior fails. This patch adds the LLDB_TEST_API macro where needed and uses the new -2 magic value for num_expected_locations to account for lazy-loading of module symbols on Windows.

With this fix, the test itself still fails: conflicting_symbol isn't in the debug info (at least, not as a global).
This indicates an incomplete implementation somewhere, and having the test fail at this point is more useful than a build error.

[labath]

This test deliberately compiles without -g, so the variable should not be in the debug info. 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? Maybe you need to lookup the symbol as _conflicting_symbol ?

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.

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

[amccarth]

In D43688#1017638, @labath wrote:

This test deliberately compiles without -g, so the variable should not be in the debug info.

Ah, that makes more sense than what I was thinking: that it was trying to make sure the unused symbol wasn't being optimized away.

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.

Maybe you need to lookup the symbol as _conflicting_symbol ?

No, that doesn't make a difference.

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.

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.

[labath]

In D43688#1017652, @amccarth wrote:

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

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.

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?

[amccarth]

In D43688#1017691, @labath wrote:

In D43688#1017652, @amccarth wrote:

In 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?

[labath]

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

lldb/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h

 #ifndef ONE_H
 #define ONE_H
-void one();
+LLDB_TEST_API void one();
 #endif

lldb/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py

def test_conflicting_symbols(self):
         self.assertTrue(target, VALID_TARGET)
 
         # Register our shared libraries for remote targets so they get
         # automatically uploaded
         environment = self.registerSharedLibrariesWithTarget(
             target, ['One', 'Two'])
 
         lldbutil.run_break_set_by_source_regexp(self, '// break here',
-                extra_options='-f One.c')
+                extra_options='-f One.c', num_expected_locations=-2)
         lldbutil.run_break_set_by_source_regexp(self, '// break here',
-                extra_options='-f Two.c')
+                extra_options='-f Two.c', num_expected_locations=-2)
         lldbutil.run_break_set_by_source_regexp(self, '// break here',
                 extra_options='-f main.c', num_expected_locations=1)
 
         process = target.LaunchSimple(
             None, environment, self.get_process_working_directory())
         self.assertTrue(process, PROCESS_IS_VALID)
 
         # The stop reason of the thread should be breakpoint.

lldb/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h

 #ifndef TWO_H
 #define TWO_H
-void two();
+LLDB_TEST_API void two();
 #endif