[Lldb-commits] [PATCH] D75877: [lldb/Reproducers] Fix replay for process attach workflows

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75877#1914755 <https://reviews.llvm.org/D75877#1914755>, @JDevlieghere wrote:

> In D75877#1913959 <https://reviews.llvm.org/D75877#1913959>, @labath wrote:
>
> > A more principled way to make this work would be to intercept (record) the Host::FindProcesses api. That way other functionality pertaining to running processes (e.g. the "platform process list" command) would also work. But if this is all you care about right now, then maybe this is fine...
>
>
> We could totally add a provider for that. I didn't because it seemed like overkill but if you're on board I also prefer that over a random PID.


In general, I am in favor of doing the capture at the lowest level possible. For this particular feature/bug, it is overkill, but OTOH, this will also make it possible to support things other things without adding hacks into random pieces of code.

> 
> 
>> The part that worries me more is the test. There are a lot of subtleties involved in making attach tests (and attach-by-name tests in particular) work reliably everywhere. I think this should be a dotest test, as there we already have some machinery to do these kinds of things (`lldb_enable_attach`, `wait_for_file_on_target`), and python is generally much better at complex control flow (I am very much against subshells and background processes in lit tests). Reproducers make this somewhat complicated because you cannot use the liblldb instance already loaded into the python process. But maybe you could run lldb in a subprocess similar to how the pexpect tests do it?
> 
> The problem is the `SBDebugger::Initialize()` that's called from the SWIG bindings, as soon as you import lldb it's already too late for the reproducers. I'm working on being able to capture/replay the test suite and currently I have the following hack:
> 
>   if 'DOTEST_CAPTURE_PATH' in os.environ:
>      SBReproducer.Capture(os.environ['DOTEST_CAPTURE_PATH'])
>   SBDebugger.Initialize()
> 
> 
> If you're fine with having that in `python.swig` unconditionally we could make a dotest-test work.

I'm not sure how that would help because for the test you need to run lldb both in capture and replay mode, and I don't think you can currently do that within a single process. It would be cool if that was possible, but even then we'd have the impendance mismatch because we'd need to run `SBDebugger.Initialize` inside a specific test method, whereas normally it gets run much earlier.

That's why I was talking about subprocesses in the previous patch. The test would only be responsible for building the inferior and driving the whole thing, while capture/replay would happen inside separate processes:

  self.spawnSubproces(randomized_inferior_name, [token_path])
  lldbutil.wait_for_file_on_target(token_path)
  self.spawnSubprocess(lldbtest_config.lldbExec, ['--capture', reproducer, '-n', randomized_inferior_name, ...])
  ...
  self.spawnSubprocess(lldbtest_config.lldbExec, ['--replay', reproducer])



================
Comment at: lldb/test/Shell/Reproducer/TestAttach.test:7
+# RUN: %clang_host %S/Inputs/sleep.c -g -o %t/attach.out
+# RUN: python -c 'import os; os.system("%t/attach.out &")'
+
----------------
JDevlieghere wrote:
> labath wrote:
> > How is this different from a plain `%t/attach.out &` ?
> Should that work? That's the first thing I tried and lit complained about invalid syntax.
I've seen tests do that (`RUN: setsid %run %t/LFSIGUSR -merge=1 -merge_control_file=%t/MCF %t/C1 %t/C2 2>%t/log & export PID=$!` in  `./compiler-rt/test/fuzzer/merge-sigusr.test`), but as I said, I don't think that is a good idea, so I don't really want to encourange it...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D75877