[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

Thanks for the updates and for writing the test. The code looks fine to me, the "request changes" is for making sure the test follows best practices and is portable.



================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/Makefile:10-17
+# The following shared library will be used to test breakpoints under dynamic loading
+libother.so:  other-copy.c
+	$(CC) $(CFLAGS) -fpic -o other.o -c other-copy.c
+	$(CC) $(CFLAGS) -shared -o libother.so other.o
+
+a.out: main-copy.cpp libother.so
+	echo $(CXXFLAGS) > ./flags
----------------
This is not the right way to build binaries --  it will fail on macos for instance.
If we were not dlopening, you should set CXX_SOURCES, and DYLIB_C_SOURCES and let the normal rules do build for you, but for dlopen you need to do something a tad more complicated and invoke a submake with the right arguments you can look at `functionalities/load_unload/Makefile` for an example of how to do that.

I don't think we currently have a test which uses both shared libraries, and relocated sources files, so it's possible that this may fail for some reason. But if that's the case, then I want to understand what's the problem first (so we can try to fix it).


================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py:32-33
+
+        new_main_folder = os.path.join(os.path.dirname(source_folder), 'moved_main')
+        new_other_folder = os.path.join(os.path.dirname(source_folder), 'moved_other')
+
----------------
If I follow this correctly this will escape from the build folder for this test (two `dirname`s). Can you stay within the confines of the build folder?

Among other things, then you won't need to `rmtree` these things as the build folder is automatically cleaned before each test.


================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py:229-230
            breakpoint, like 'conditions' and 'hitCondition' settings.'''
-        source_basename = 'main.cpp'
-        source_path = os.path.join(os.getcwd(), source_basename)
+        source_basename = 'main-copy.cpp'
+        source_path = self.getBuildArtifact(source_basename)
         loop_line = line_number('main.cpp', '// break loop')
----------------
maybe move these into the `setUp` function?


================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/main.cpp:19
 int main(int argc, char const *argv[]) {
+  void *handle = dlopen("libother.so", RTLD_NOW);
+  int (*foo)(int) = (int (*)(int))dlsym(handle, "foo");
----------------
Looking at the other dlopen tests (e.g., `functionalities/load_unload/main.cpp`), it looks like you'll need to ifdef the correct library name here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76968/new/

https://reviews.llvm.org/D76968