Skip to content

Test cleanups #3399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Test cleanups #3399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cef7f6
Fix two race conditions and clean up testcase.
adrian-prantl Oct 12, 2021
14fe0b3
Fix out-of-bounds memory access
adrian-prantl Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions lldb/test/API/lang/swift/deployment_target/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
all: a.out dlopen_module
all: libNewerTarget.dylib a.out dlopen_module

SWIFT_SOURCES=main.swift
SWIFTFLAGS_EXTRAS=-target x86_64-apple-macosx10.10 -Xfrontend -disable-target-os-checking -I$(BUILDDIR)
LD_EXTRAS=-lNewerTarget -L$(BUILDDIR)

include Makefile.rules

# This test only works on macOS 10.11+.

a.out: main.swift libNewerTarget.dylib
$(SWIFTC) -sdk "$(SWIFTSDKROOT)" -target x86_64-apple-macosx10.10 -g -Onone $^ -lNewerTarget -L$(shell pwd) -o $@ $(SWIFTFLAGS) -I$(shell pwd) -Xfrontend -disable-target-os-checking
ifneq "$(CODESIGN)" ""
$(CODESIGN) -s - "$@"
endif

dlopen_module: main.m libNewerTarget.dylib
$(CC) -framework Foundation $^ -mmacos-version-min=10.8 -o $@ $(CFLAGS)
ifneq "$(CODESIGN)" ""
$(CODESIGN) -s - "$@"
endif
dlopen_module: dlopen_module.m libNewerTarget.dylib
$(MAKE) -f $(MAKEFILE_RULES) \
CXX= \
OBJC_SOURCES=dlopen_module.m \
EXE=dlopen_module \
LD_EXTRAS="-framework Foundation -mmacos-version-min=10.8"

lib%.dylib: %.swift
$(SWIFTC) -sdk "$(SWIFTSDKROOT)" -target x86_64-apple-macosx10.11 -g -Onone $^ -emit-library -module-name $(shell basename $< .swift) -emit-module -Xlinker -install_name -Xlinker @executable_path/$@
ifneq "$(CODESIGN)" ""
$(CODESIGN) -s - "$@"
endif
$(MAKE) -f $(MAKEFILE_RULES) \
DYLIB_ONLY=YES \
DYLIB_NAME=$(patsubst lib%.dylib,%,$@) \
DYLIB_SWIFT_SOURCES=$(patsubst lib%.dylib,%.swift,$@) \
SWIFTFLAGS_EXTRAS="-target x86_64-apple-macosx10.11"

clean::
rm -rf *.swiftmodule *.swiftdoc *.dSYM *~ lib*.dylib a.out dlopen_module *.o
10 changes: 6 additions & 4 deletions lldb/test/API/lang/swift/deployment_target/TestSwiftDeploymentTarget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,15 @@ def test_swift_deployment_target(self):
@skipIf(bugnumber="rdar://60396797", # should work but crashes.
setting=('symbols.use-swift-clangimporter', 'false'))
@skipUnlessDarwin
@skipIfDarwinEmbedded # This test uses macOS triples explicitely.
@skipIfDarwinEmbedded # This test uses macOS triples explicitly.
@skipIf(macos_version=["<", "10.11"])
@swiftTest
def test_swift_deployment_target_dlopen(self):
self.build()
lldbutil.run_to_source_breakpoint(
self, 'break here', lldb.SBFileSpec('NewerTarget.swift'),
exe_name="dlopen_module")
target, process, _, _, = lldbutil.run_to_name_breakpoint(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimingham I don't feel good about this change, but it was necessary. Is this supposed to work for a regex breakpoint in a dylib that is going to be dlopened later?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this worked due to a freak accident before: main.swift main.o file replaced the main.m main.o and so the breakpoint was resolved at startup even though the dylib wasn't yet loaded?

Copy link

@jimingham jimingham Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work but it looks like there's a bug in the SBTarget bindings. This works:

(lldb) break set -p "Stop here if you can" -f dylib.c -s libdylib.dylib
Breakpoint 1: no locations (pending).
WARNING:  Unable to resolve breakpoint to any actual locations.
(lldb) run
Process 44860 launched: '/tmp/had_dylib/loadit' (x86_64)
1 location added to breakpoint 1
Process 44860 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x000000010015bf64 libdylib.dylib`my_function at dylib.c:5
   2   	
   3   	void
   4   	my_function() {
-> 5   	  printf("Stop here if you can.\n");
    	  ^
   6   	}
Target 0: (loadit) stopped.

But this, which should be equivalent, does not:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> files = lldb.SBFileSpecList()
>>> files.Append(lldb.SBFileSpec("dylib.c"))
>>> shlibs = lldb.SBFileSpecList()
>>> shlibs.Append(lldb.SBFileSpec("libdylib.dylib"))
>>> bkpt = lldb.target.BreakpointCreateBySourceRegex("Stop here if you can", shlibs, files)
>>> quit
(lldb) break list
Current breakpoints:
1: source regex = "Stop here if you can", exact_match = 0, module = libdylib.dylib, locations = 0 (pending)

(lldb) run
Process 44892 launched: '/tmp/had_dylib/loadit' (x86_64)
Stop here if you can.
Process 44892 exited with status = 0 (0x00000000)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also check that run_to_source_breakpoint does the right thing with the exe_module & file arguments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure this code is not using run_to_source_breakpoint correctly. The exe_name argument is the name of the module you are going to use to create the target. The bkpt_module argument is for the name of the module you are supposed to set the breakpoint in. If that's not specified, we set it in the main executable module.

Copy link

@jimingham jimingham Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. The test code you had above used a different version of BreakpointCreateBySourceRegex, not passing lists but a FileSpec for the file and a char * for the module name. That one does seem to work, and also doesn't require the module, only the file name:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> bkpt = lldb.target.BreakpointCreateBySourceRegex("Stop here if you can", lldb.SBFileSpec("dylib.c"))
>>> quit
(lldb) run
Process 44942 launched: '/tmp/had_dylib/loadit' (x86_64)
1 location added to breakpoint 1
Process 44942 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x000000010015bf64 libdylib.dylib`my_function at dylib.c:5
   2   	
   3   	void
   4   	my_function() {
-> 5   	  printf("Stop here if you can.\n");
    	  ^
   6   	}
Target 0: (loadit) stopped.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC, this should fail at the point where you try to set the breakpoint in run_to_source_breakpoint with the error that the breakpoint has no locations, but if you changed run_to_source_breakpoint to take a parameter to turn off the test, this should work.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR

The overload of SBTarget::CreateBreakpointWithSourceRegex that takes a list of files & a list of modules doesn't work, but lldbutil.run_to_source_breakpoint doesn't use that one, it uses the one that takes on file and one module, and that one does work.

lldbutil.run_to_source_breakpoint asserts that the breakpoint have at least one location pre run, which won't be the case for your test. Probably best to fix that by adding a has_locations_before_run parameter to run_to_source_breakpoint for the case where the breakpoint is expected to only take in a dynamically loaded library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason your fix works is because it DOESN'T check that bpkt has any locations when you set it. It would be better to fix that by fixing run_to_source_breakpoint.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like:

diff --git a/lldb/packages/Python/lldbsuite/test/lldbutil.py b/lldb/packages/Python/lldbsuite/test/lldbutil.py
index 20a6d28274b3..2ab372d82b72 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbutil.py
@@ -953,7 +953,8 @@ def run_to_source_breakpoint(test, bkpt_pattern, source_spec,
                              bkpt_module = None,
                              in_cwd = True,
                              only_one_thread = True,
-                             extra_images = None):
+                             extra_images = None,
+                             has_locations_before_run = True):
     """Start up a target, using exe_name as the executable, and run it to
        a breakpoint set by source regex bkpt_pattern.
 
@@ -964,9 +965,10 @@ def run_to_source_breakpoint(test, bkpt_pattern, source_spec,
     # Set the breakpoints
     breakpoint = target.BreakpointCreateBySourceRegex(
             bkpt_pattern, source_spec, bkpt_module)
-    test.assertTrue(breakpoint.GetNumLocations() > 0,
-        'No locations found for source breakpoint: "%s", file: "%s", dir: "%s"'
-        %(bkpt_pattern, source_spec.GetFilename(), source_spec.GetDirectory()))
+    if has_locations_before_run:
+        test.assertTrue(breakpoint.GetNumLocations() > 0,
+                        'No locations found for source breakpoint: "%s", file: "%s", dir: "%s"'
+                        %(bkpt_pattern, source_spec.GetFilename(), source_spec.GetDirectory()))
     return run_to_breakpoint_do_run(test, target, breakpoint, launch_info,
                                     only_one_thread, extra_images)

self, 'main', exe_name="dlopen_module")
bkpt = target.BreakpointCreateBySourceRegex(
'break here', lldb.SBFileSpec('NewerTarget.swift'))
lldbutil.continue_to_breakpoint(process, bkpt)
self.expect("p self", substrs=['i = 23'])

18 changes: 18 additions & 0 deletions lldb/test/API/lang/swift/deployment_target/dlopen_module.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#include <Foundation/Foundation.h>
#include <dlfcn.h>
#include <libgen.h>
#include <limits.h>

@protocol FooProtocol
- (void)f;
@end

int main(int argc, const char **argv) {
char dylib[PATH_MAX];
strlcpy(dylib, dirname(argv[0]), PATH_MAX);
strlcat(dylib, "/libNewerTarget.dylib", PATH_MAX);
dlopen(dylib, RTLD_LAZY);
Class<FooProtocol> fooClass = NSClassFromString(@"NewerTarget.Foo");
[[[fooClass alloc] init] f];
return 0;
}
14 changes: 0 additions & 14 deletions lldb/test/API/lang/swift/deployment_target/main.m
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions lldb/test/API/lang/swift/late_dylib_clangdeps/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
#include <dlfcn.h>
#include <libgen.h>
#include <limits.h>
#include <stdio.h>
#include <string.h>
#include <libgen.h>

int main(int argc, const char **argv) {
char *dylib_name = strcat(dirname(argv[0]),"/dylib.dylib");
char dylib_name[PATH_MAX];
strlcpy(dylib_name, dirname(argv[0]), PATH_MAX);
strlcat(dylib_name, "/dylib.dylib", PATH_MAX);
void *dylib = dlopen(dylib_name, RTLD_NOW);
void (*f)() = dlsym(dylib, "f");
f();
Expand Down
10 changes: 6 additions & 4 deletions lldb/test/API/lang/swift/unit-tests/xctest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@
//===----------------------------------------------------------------------===//

#include <dlfcn.h>
#include <libgen.h>
#include <limits.h>
#include <stdio.h>
#include <string.h>
#include <libgen.h>

int main(int argc, const char **argv)
{
char *dylib = strcat(dirname(argv[0]),"/UnitTest.xctest/Contents/MacOS/test");
int main(int argc, const char **argv) {
char dylib[PATH_MAX];
strlcpy(dylib, dirname(argv[0]), PATH_MAX);
strlcat(dylib, "/UnitTest.xctest/Contents/MacOS/test", PATH_MAX);
void *test_case = dlopen(dylib, RTLD_NOW);

printf("%p\n", test_case); // Set breakpoint here
Expand Down