[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL325704: Fix TestBreakpointInGlobalConstructor for Windows (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43419?vs=135272&id=135281#toc Repository: rL LLVM https://reviews.llvm.org/D43419 Files: lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,8 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we don't +check the actual number at all. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,7 +564,7 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") -else: +elif num_locations != -2: test.assertTrue( num_locations == out_num_locations, "Expecting %d locations, got %d." % Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,8 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we don't +check the actual number at all. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,7 +564,7 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") -else: +elif num_locations != -2: test.assertTrue( num_locations == out_num_locations, "Expecting %d locations, got %d." % ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth updated this revision to Diff 135272. amccarth added a comment. Per Pavel's suggestion, change special value to mean don't check the number of locations found. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py lldb/packages/Python/lldbsuite/test/lldbutil.py Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,8 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we don't +check the actual number at all. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,7 +564,7 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") -else: +elif num_locations != -2: test.assertTrue( num_locations == out_num_locations, "Expecting %d locations, got %d." % Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,8 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we don't +check the actual number at all. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,7 +564,7 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") -else: +elif num_locations != -2: test.assertTrue( num_locations == out_num_locations, "Expecting %d locations, got %d." % Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
labath added a comment. Yes, regardless of the target platform, if you specify num_locations = -2, then we just don't check the number of locations. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth added a comment. By unconditional, do you mean allowing any value for `out_num_locations` in these cases? I'm happy to do that, but I'm not sure if I've understood you correctly. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
labath added a comment. I was actually thinking of making this unconditional. It makes the behaviour easier to understand and this test really does not care about whether the libraries were resolved statically or not. I'm with Jim that we should make targeted tests for that functionality. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth added a comment.
In https://reviews.llvm.org/D43419#1013559, @jingham wrote:
> At some point we should introduce "platformCapacities" so that you could do:
>
> if not test.getPlatformCapacities("preload-dylibs"):
>
>
> so we are testing specific features not the whole platform.
Yes, I agree completely.
> It looks like your added comment has no line endings? Can you reformat the
> comment so it's easier to read? Then it will be good.
Done. I'm never sure how Python handles docstrings. The existing code looked
like it was trying to avoid line breaks that weren't separating paragraphs.
https://reviews.llvm.org/D43419
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth updated this revision to Diff 135124. amccarth added a comment. Wrapped the modified docstring. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py lldb/packages/Python/lldbsuite/test/lldbutil.py Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,9 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we check +that we got AT LEAST one location on platforms other than Windows. Otherwise, we check that num_expected_locations +equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,6 +565,12 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") +elif num_locations == -2: +# On Windows, breakpoints may remain pending until a DLL is loaded, +# so we allow zero. Other platforms expect at least one. +if test.getPlatform() != "windows": + test.assertTrue(out_num_locations > 0, + "Expecting one or more locations, got none.") else: test.assertTrue( num_locations == out_num_locations, Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,9 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we check +that we got AT LEAST one location on platforms other than Windows. Otherwise, we check that num_expected_locations +equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,6 +565,12 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") +elif num_locations == -2: +# On Windows, breakpoints may remain pending until a DLL is loaded, +# so we allow zero. Other platforms expect at least one. +if test.getPlatform() != "windows": + test.assertTrue(out_num_locations > 0, + "Expecting one or more locations, got none.") else: test.assertTrue( num_locations == out_num_locations, Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, e
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
At some point we should introduce "platformCapacities" so that you could do:
if not test.getPlatformCapacities("preload-dylibs"):
so we are testing specific features not the whole platform. But that seems a
bigger change than this fix warrants.
It looks like your added comment has no line endings? Can you reformat the
comment so it's easier to read? Then it will be good.
https://reviews.llvm.org/D43419
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth updated this revision to Diff 135120. amccarth added a comment. Added new special value to accommodate the fact that Windows may postpone resolving the breakpoints for DLLs that aren't yet loaded. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py lldb/packages/Python/lldbsuite/test/lldbutil.py Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,7 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we check that we got AT LEAST one location on platforms other than Windows. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,6 +563,12 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") +elif num_locations == -2: +# On Windows, breakpoints may remain pending until a DLL is loaded, +# so we allow zero. Other platforms expect at least one. +if test.getPlatform() != "windows": + test.assertTrue(out_num_locations > 0, + "Expecting one or more locations, got none.") else: test.assertTrue( num_locations == out_num_locations, Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=-2) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/packages/Python/lldbsuite/test/lldbutil.py === --- lldb/packages/Python/lldbsuite/test/lldbutil.py +++ lldb/packages/Python/lldbsuite/test/lldbutil.py @@ -343,7 +343,7 @@ If extra_options is not None, then we append it to the breakpoint set command. -If num_expected_locations is -1 we check that we got AT LEAST one location, otherwise we check that num_expected_locations equals the number of locations. +If num_expected_locations is -1, we check that we got AT LEAST one location. If num_expected_locations is -2, we check that we got AT LEAST one location on platforms other than Windows. Otherwise, we check that num_expected_locations equals the number of locations. If loc_exact is true, we check that there is one location, and that location must be at the input file and line number.""" @@ -563,6 +563,12 @@ if num_locations == -1: test.assertTrue(out_num_locations > 0, "Expecting one or more locations, got none.") +elif num_locations == -2: +# On Windows, breakpoints may remain pending until a DLL is loaded, +# so we allow zero. Other platforms expect at least one. +if test.getPlatform() != "windows": + test.assertTrue(out_num_locations > 0, + "Expecting one or more locations, got none.") else: test.assertTrue( num_locations == out_num_locations, Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,9 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_ex
Re: [Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
We don’t parse libraries beyond getting sections until we look for symbols, this doesn’t seem to slow startup noticeably, and since I find it super useful especially for a program like lldb, where the driver has very little code in it and most of the functionality is in libraries, I’m happy with the tradeoff. But I don’t think we should require all platforms to do this, nor is it expected to be exact (for instance, the darwin dynamic loader plugin doesn’t track setting environment variables in the run-args, re-calculating the loaded libraries based on new DYLD_LIBRARY_PATH variables… So the tests should be flexible and not require this. I think having a special value for num_locations is fine. We probably should add some tests that explicitly test this for platforms that do support the feature, I think we are only testing it by accident now. Jim > On Feb 20, 2018, at 9:37 AM, Adrian McCarthy via Phabricator > wrote: > > amccarth added a comment. > > I have reservations about making the debugger try to pre-locate the modules > and their PDBs before those modules are actually loaded. For a few reasons. > > (1) On Windows, module resolution is complex. Search paths, the safe search > path, manifests, the side-by-side cache, dynamic library link redirection > (.exe.local), Windows API sets, etc. It got to the point that Microsoft > dropped support for the venerable DependencyWalker tool (though there is an > open source community trying to keep it alive). That's a lot of logic to > bake into the debugger (and to create tests for), and the cost of getting it > wrong is that we think your breakpoint will resolve when actually it won't. > > (2) A typical program depends directly and indirectly on many DLLs. Even > with lazy-evaluation, trying to apply all the rules in 1 (which must be done > serially) to locate all of the dependents seems like a lot of unnecessary > work on startup. > > (3) It's different than how all the debuggers on Windows work, which might be > mildly surprising to users. > > If there's a strong will to head down this path, I think that'll be a > separate effort than my getting this test working again in the short term. > So I think I'll do something less invasive along the lines of Pavel's > suggestion to get this test working. Stay tuned. > > > https://reviews.llvm.org/D43419 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth added a comment. I have reservations about making the debugger try to pre-locate the modules and their PDBs before those modules are actually loaded. For a few reasons. (1) On Windows, module resolution is complex. Search paths, the safe search path, manifests, the side-by-side cache, dynamic library link redirection (.exe.local), Windows API sets, etc. It got to the point that Microsoft dropped support for the venerable DependencyWalker tool (though there is an open source community trying to keep it alive). That's a lot of logic to bake into the debugger (and to create tests for), and the cost of getting it wrong is that we think your breakpoint will resolve when actually it won't. (2) A typical program depends directly and indirectly on many DLLs. Even with lazy-evaluation, trying to apply all the rules in 1 (which must be done serially) to locate all of the dependents seems like a lot of unnecessary work on startup. (3) It's different than how all the debuggers on Windows work, which might be mildly surprising to users. If there's a strong will to head down this path, I think that'll be a separate effort than my getting this test working again in the short term. So I think I'll do something less invasive along the lines of Pavel's suggestion to get this test working. Stay tuned. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
labath requested changes to this revision. labath added a comment. This revision now requires changes to proceed. Yeah, setting this to zero would break other platforms that are able to locate shared libraries before running the executable. On linux, we try to locate them as well, but it's kind of on a best-effort basis -- it's quite hard to figure out what library will get loaded with absolute precision, as it can e.g. depend on the value of LD_LIBRARY_PATH env var that you run the process with (and you don't know that until the actual "process launch" command). In fact, it wouldn't be hard to come up with examples where this static resolution finds the wrong library. So, it sounds to me like we do need a special flag for "zero or more" breakpoint locations for tests which don't care about this behavior such as this one. Maybe num_expected_locations=-2 (I think we should leave "0" meaning "exactly zero", as that can be useful in some situations). In fact I know of at least one other test that could use something like this (I've had to disable some tests in TestLoadUnload on linux because the out-of-tree build caused the automatic library lookup to not work). Of course being able to resolve DLL dependencies statically like other platforms would be nice, but that's quite orthogonal to this what this test is doing. (Here the purpose is to check that we set the breakpoint early enough -- before the global constructor executes). https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jingham added a comment. The debug info is parsed only when needed. So for breakpoints, it will get parsed when the breakpoint is set. That's one good use of "break set -s " BTW, it will limit the debug info read in to just the library you expect to find the symbol in. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jingham added a comment. If you want to avoid loading dependent libraries, you can use the "-d" flag to "target create". This is sometimes handy, but shouldn't be the default behavior. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
zturner added a comment. I guess on the other hand, it's reasonable to assume that if you've set a breakpoint somewhere, you're more likely than not to need it since you probably had a reason for setting it. Is the debug info parsed when the executable is loaded, or when the breakpoint is set? https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
zturner added a comment. I guess one advantage to delaying it is that the debug info for the dynamic library could be large and slow to parse, and you don't know if you're even going to need it until you hit the breakpoint. So by delaying the resolution even to File address, you postpone parsing potentially expensive debug info until you know you're going to need it. It this not a consideration on OSX and/or other platforms that resolve it to a file address early? https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jingham added a comment. It is really handy to see the pre-load libraries in lldb, it means you can do "source list" to make sure the breakpoints you are planning to set are where you expect them to be, and tab completion can help you set breakpoints pre-run, etc. So if you can do this, your users will be happier. If you can't get to this, then it would be better to pass different values for num_expected_locations on Windows & other platforms. Finding the breakpoint pre-launch is expected behavior on macOS & Linux, so it would be good to keep testing that that continues to work. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
zturner added a comment. I'm not sure how hard it would be, but it would be better if we could do the same thing on Windows, for consistency of behavior. In principle it's straightforward, just scan through the IAT and load all of the imported modules. Probably the DynamicLoaderWindows plugin is doing this right now. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth added a comment. OK, so my fix is too simplistic. I'll have to (1) make LLDB on Windows also attempt to pre-load the DLLs, (2) make the test use platform-specific expectations, or (3) use a looser expectation. I'll give it some thought over the weekend. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jasonmolenda added a comment. Yeah, it has a location because we're resolved it to a File address (section+offset), and when the dylib actually gets loaded we'll resolve that to a load address and insert the breakpoint instruction. https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
zturner added a comment. In https://reviews.llvm.org/D43419#1011045, @jasonmolenda wrote: > On Darwin we load all the libraries that the binary links against > pre-execution, if possible. So I see: > > % lldb a.out > (lldb) ima li libfoo.dylib > [ 0] 35C5FE62-5327-3335-BBCF-5369CB07D1D6 0x > /Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib > > > /Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib.dSYM/Contents/Resources/DWARF/libfoo.dylib > > > (lldb) br s -f foo.cpp -p BR_foo > Breakpoint 1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = > 0x0f52 > (lldb) br li > Current breakpoints: > 1: source regex = "BR_foo", exact_match = 0, locations = 1 > > 1.1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = > libfoo.dylib[0x0f52], unresolved, hit count = 0 So IIUC it's still unresolved because you don't know the load address of the dylib until runtime, but it at least has a location because you know the RVA in the dylib? https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
jasonmolenda added a comment. On Darwin we load all the libraries that the binary links against pre-execution, if possible. So I see: % lldb a.out (lldb) ima li libfoo.dylib [ 0] 35C5FE62-5327-3335-BBCF-5369CB07D1D6 0x /Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib /Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib.dSYM/Contents/Resources/DWARF/libfoo.dylib (lldb) br s -f foo.cpp -p BR_foo Breakpoint 1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = 0x0f52 (lldb) br li Current breakpoints: 1: source regex = "BR_foo", exact_match = 0, locations = 1 1.1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = libfoo.dylib[0x0f52], unresolved, hit count = 0 https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
zturner added a reviewer: labath. zturner added a comment. What's the behavior on other platforms? When you set a breakpoint in a shared library before you've run the program, shouldn't it still be unresolved, in which case this test should have failed on those platforms too? https://reviews.llvm.org/D43419 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows
amccarth created this revision. amccarth added a reviewer: jasonmolenda. Herald added a subscriber: sanjoy. This test was failing on Windows because it expected the breakpoint in the dynamic library to be resolved before the process is launched. Since the DLL isn't loaded until the process is launched this didn't work. Changing the expectation solves the problem on Windows, but since I'm not sure how this works on other platforms, I'm afraid this could break it there. I'm considering whether we should treat 0 as a special value for `num_expected_locations` that means 0 or more. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,11 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + +# We don't expect any locations because the dynamic library isn't +# loaded until we launch the process. bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=0) process = target.LaunchSimple( None, env, self.get_process_working_directory()) Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py === --- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py +++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py @@ -29,8 +29,11 @@ bp_main = lldbutil.run_break_set_by_file_and_line( self, 'main.cpp', self.line_main) + +# We don't expect any locations because the dynamic library isn't +# loaded until we launch the process. bp_foo = lldbutil.run_break_set_by_file_and_line( -self, 'foo.cpp', self.line_foo) +self, 'foo.cpp', self.line_foo, num_expected_locations=0) process = target.LaunchSimple( None, env, self.get_process_working_directory()) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
