[Lldb-commits] [PATCH] D96637: Make sure the interpreter module was loaded before making checks against it

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have some improvements to the test suite -- it would be great if you could incorporate them into the next version of the patch.

BTW, it would be nice if the revert commit message included a (brief) explanation of why the patch is being reverted.



================
Comment at: lldb/test/API/functionalities/module_load_attach/Makefile:2
+C_SOURCES := main.c
+LD_EXTRAS := -Wl,-rpath "-Wl,$(shell pwd)"
+USE_LIBDL := 1
----------------
Delete, and use self.registerSharedLibrariesWithTarget in python code


================
Comment at: lldb/test/API/functionalities/module_load_attach/Makefile:5
+
+feature:
+	$(MAKE) -f $(MAKEFILE_RULES) \
----------------
`feature` sounds very specific and unusual. I guess that is inspired by whatever was the original use case that caused you to find this bug, but maybe you could pick a more generic name here: `liba`, or `libload_after_attach`, ...


================
Comment at: lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:32-33
+
+    @skipIfRemote
+    @skipUnlessLinux
+    @no_debug_info_test
----------------
With the other modifications, you should be able to drop these. The way this test is phrased, it should run everywhere, so it'd be a pity to not make use of that.


================
Comment at: lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:42
+        thread = process.GetSelectedThread()
+        self.assertModuleIsNotLoaded("libfeature.so")
+        thread.GetSelectedFrame().EvaluateExpression("flip_to_1_to_continue = 1")
----------------
Use `self.platformContext.shlib_prefix` and `.shlib_extension` instead of `"lib"` and `".so"`.


================
Comment at: lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:28
+    def test_x(self):
+        process = self.build_launch_and_attach()
+        thread = process.GetSelectedThread()
----------------
aadsm wrote:
> clayborg wrote:
> > Is this racy? What happens on a really slow system? Can we fail to attach? If we do attach, are we guaranteed to be at a place where we can set "flip_to_1_to_continue = 1"? The nice thing is it is a global variable that we should be able to set no matter where we stop. 
> > Is this racy?
> 
> I don't think so because we already have a pid at that point in time, so we should always be able to attach.
> 
> > If we do attach, are we guaranteed to be at a place where we can set "flip_to_1_to_continue = 1"?
> 
> yeah, that's exactly why I made it global. I could also wait until there's a `flip_to_1_to_continue` in the scope if you think it's worthwhile.
> I don't think so because we already have a pid at that point in time, so we should always be able to attach.
Attaching -- yes, but I think that if we attach _really_ early we may not be able to flip the variable, as the loader has not yet finished setting up the main module. It will also make the test nondeterministic, as (depending on how early we attach) we may or may not be getting notifications about the loading of dependent libraries (libc and stuff). Other attach tests use synchronization by having the inferior create a file when it's ready to be attached, and the test waits for this via `lldbutil.wait_for_file_on_target`. It would be good to use that here too..


================
Comment at: lldb/test/API/functionalities/module_load_attach/main.c:1
+#include <dlfcn.h>
+#include <assert.h>
----------------
replace with `"dylib.h"` and use `dylib_open` instead of `dlopen`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96637