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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
