[Lldb-commits] [PATCH] D46395: Remove Process references from the Host module
labath marked an inline comment as done.
labath added a comment.
I've added a comment about the requirement to the `MonitoringProcessLauncher`
and to `Host::LaunchProcess` (most users go through the latter). I've also
tried to make it more obvious that the no-op callback still causes the process
to be reaped. Let me know if I succeeded.
Comment at: source/Host/macosx/Host.mm:1501
+bool monitoring = launch_info.MonitorProcess();
+(void)monitoring;
+assert(monitoring);
clayborg wrote:
> Do we not have a macro for silencing unused variables?
For me, the UNUSED_IF_ASSERT_DISABLED macro does not seem to add any value, and
it is inconsistent with the llvm style. But I don't feel that strongly, so I've
changed this to use the macro instead.
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:873-874
+const bool monitor_signals = false;
+launch_info.SetMonitorProcessCallback(
+[](lldb::pid_t, bool, int, int) { return true; }, monitor_signals);
process_sp = Platform::DebugProcess(launch_info, debugger, target, error);
clayborg wrote:
> Now we won't reap the process from within LLDB or is the comment is wrong?
> This looks wrong. Also, if we truly have places that don't need to reap the
> process, then we should add a "launch_info.SetDefaultDontReapCallback()" or
> something to launch_info that will install a default do nothing callback
> instead of one or more clients doing this
The comment is correct. We still reap (waitpid) the child; we don't set the
exit status on the Process class. The reaping is done in
Host::StartMonitoringChildProcess (on macosx), and this function calls the
callback with the waitpid result. Since all we're interested in here is the
reaping, we set the callback to a function which just ignores the result.
I tried to make this more clear by introducing a special NoOpMonitorCallback
function and making this code reference that.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:119
+ Info.SetMonitorProcessCallback(
+ [](lldb::pid_t, bool, int, int) { return true; }, false);
clayborg wrote:
> I know on Mac we still must reap the process from both the process that
> launches (lldb) it **and** the ptrace parent process (lldb-server or
> debugserver). This will create zombies and buildbots will die horribly if
> this is wrong
This is just launching the debugserver/lldb-server process, so there should be
no special reaping going on here.
What would be interesting (and hence the TODO) is to make the tests more
resilient to broken server binaries. Right now, if you break the server so much
it does not even connect to the client, this test will hang. If we could check
for the exit status, we could fail the test with a nice error message instead.
The reason I did not do that now is because we don't currently have an easy way
to wait for a process to exit *or* a socket to connect.
https://reviews.llvm.org/D46395
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46395: Remove Process references from the Host module
labath updated this revision to Diff 145163.
labath marked an inline comment as done.
labath added a comment.
Update the diff.
https://reviews.llvm.org/D46395
Files:
include/lldb/Host/Host.h
include/lldb/Host/MonitoringProcessLauncher.h
include/lldb/Target/ProcessLaunchInfo.h
source/Host/common/MonitoringProcessLauncher.cpp
source/Host/common/NativeProcessProtocol.cpp
source/Host/macosx/Host.mm
source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
source/Target/ProcessLaunchInfo.cpp
unittests/tools/lldb-server/tests/TestClient.cpp
Index: unittests/tools/lldb-server/tests/TestClient.cpp
===
--- unittests/tools/lldb-server/tests/TestClient.cpp
+++ unittests/tools/lldb-server/tests/TestClient.cpp
@@ -97,6 +97,11 @@
Info.SetArchitecture(arch_spec);
Info.SetArguments(args, true);
Info.GetEnvironment() = Host::GetEnvironment();
+ // TODO: Use this callback to detect botched launches. If lldb-server does not
+ // start, we can print a nice error message here instead of hanging in
+ // Accept().
+ Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback,
+ false);
status = Host::LaunchProcess(Info);
if (status.Fail())
Index: source/Target/ProcessLaunchInfo.cpp
===
--- source/Target/ProcessLaunchInfo.cpp
+++ source/Target/ProcessLaunchInfo.cpp
@@ -189,6 +189,13 @@
m_monitor_signals = monitor_signals;
}
+bool ProcessLaunchInfo::NoOpMonitorCallback(lldb::pid_t pid, bool exited, int signal, int status) {
+ Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS);
+ LLDB_LOG(log, "pid = {0}, exited = {1}, signal = {2}, status = {3}", pid,
+ exited, signal, status);
+ return true;
+}
+
bool ProcessLaunchInfo::MonitorProcess() const {
if (m_monitor_callback && ProcessIDIsValid()) {
Host::StartMonitoringChildProcess(m_monitor_callback, GetProcessID(),
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -866,11 +866,12 @@
if (IsHost()) {
// We are going to hand this process off to debugserver which will be in
-// charge of setting the exit status. We still need to reap it from lldb
-// but if we let the monitor thread also set the exit status, we set up a
-// race between debugserver & us for who will find out about the debugged
-// process's death.
-launch_info.GetFlags().Set(eLaunchFlagDontSetExitStatus);
+// charge of setting the exit status. However, we still need to reap it
+// from lldb. So, make sure we use a exit callback which does not set exit
+// status.
+const bool monitor_signals = false;
+launch_info.SetMonitorProcessCallback(
+&ProcessLaunchInfo::NoOpMonitorCallback, monitor_signals);
process_sp = Platform::DebugProcess(launch_info, debugger, target, error);
} else {
if (m_remote_platform_sp)
Index: source/Host/macosx/Host.mm
===
--- source/Host/macosx/Host.mm
+++ source/Host/macosx/Host.mm
@@ -57,7 +57,6 @@
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/HostInfo.h"
#include "lldb/Host/ThreadLauncher.h"
-#include "lldb/Target/Process.h"
#include "lldb/Utility/ArchSpec.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataBufferHeap.h"
@@ -68,6 +67,7 @@
#include "lldb/Utility/NameMatches.h"
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-defines.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Errno.h"
@@ -1497,15 +1497,9 @@
launch_info.SetProcessID(pid);
// Make sure we reap any processes we spawn or we will have zombies.
-if (!launch_info.MonitorProcess()) {
- const bool monitor_signals = false;
- Host::MonitorChildProcessCallback callback = nullptr;
-
- if (!launch_info.GetFlags().Test(lldb::eLaunchFlagDontSetExitStatus))
-callback = Process::SetProcessExitStatus;
-
- StartMonitoringChildProcess(callback, pid, monitor_signals);
-}
+bool monitoring = launch_info.MonitorProcess());
+UNUSED_IF_ASSERT_DISABLED(monitoring);
+assert(monitoring);
} else {
// Invalid process ID, something didn't go well
if (error.Success())
Index: source/Host/common/NativeProcessProtocol.cpp
===
--- source/Host/common/NativeProcessProtocol.cpp
+++ source/Host/common/NativeProcessProtocol.cpp
@@ -13,7 +13,6 @@
#include "lldb/Host/common/NativeRegisterContext.h"
#include "lldb/Host/common/NativeThreadProtocol.h"
#include "lldb/Host/common/SoftwareBreakpoint.h"
-#include "lldb/Target/Process.h"
#include "lldb/Utility/LLDBAssert.h
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
labath added a comment. Maybe we could start by splitting of the ObjectFile recognition code for the debug_types section into a separate patch. That should be sufficiently non-controversial and it would reduce the number of files touched by this patch a bit. https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
clayborg added a comment. In https://reviews.llvm.org/D32167#1087627, @labath wrote: > Maybe we could start by splitting of the ObjectFile recognition code for the > debug_types section into a separate patch. That should be sufficiently > non-controversial and it would reduce the number of files touched by this > patch a bit. Sure thing. Anything to get this moving. https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r331082 - Fix build bots after r331049 broke them.
ping.
On Thu, May 3, 2018 at 3:08 PM, Davide Italiano wrote:
> I would like to apologize for communicating this so late (mainly
> because I worked on the swift bits which are a little behind and I
> didn't update my checkout) but this commit completely broke debugging
> in some cases (on MacOS).
>
> Testcase:
>
> #import
>
> int main() {
> id keys[1] = { @"abc" };
> id values[1] = { @"def" };
> id d = CFBridgingRelease(CFDictionaryCreate(nil, (void *)keys, (void
> *)values, 1, nil, nil));
> NSLog(@"%@", d);
> NSLog(@"x");
> }
>
>
> $ clang patatino.m -framework Foundation -g -o blah
>
>
> dtdebugger2:bin davide$ ./lldb ./blah
> (lldb) target create "./blah"
> Current executable set to './blah' (x86_64).
> (lldb) b main
> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
> 0x00010fad
> (lldb) r
> error: No such file or directory
>
> Reverting yours (and a later change to avoid conflicts):
>
> dtdebugger2:llvm-project-20170507 davide$ git revert 57dce7084e7b
> [master 8be66051362] Revert "Fixup r331049 (FileSpec
> auto-normalization)"
> 2 files changed, 11 insertions(+), 5 deletions(-)
> dtdebugger2:llvm-project-20170507 davide$ git revert
> d5a834a73c0b37c3862daee260c416e7ea3b761c
> [master 5bd1fb68a69] Revert "Fix build bots after r331049 broke them."
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> works.
> I also noticed that if I pass a full path the thing works:
>
> Current executable set to
> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64).
> (lldb) b main
> Breakpoint 1: where = blah`main + 13 at blah.c:2, address = 0x00010fad
> (lldb) r
> Process 82833 launched:
> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64)
> Process 82833 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
> frame #0: 0x00010fad blah`main at blah.c:2
>1int main(void) {
> -> 2 return 0;
>3}
>
>
> Do you mind to take a look and add a lit test for this?
> (also, I guess you should change your mail address as it's still an @apple
> one).
>
> Thanks!
>
> --
> Davide
>
> On Thu, May 3, 2018 at 3:07 PM, Davide Italiano wrote:
>> On Fri, Apr 27, 2018 at 2:10 PM, Greg Clayton via lldb-commits
>> wrote:
>>> Author: gclayton
>>> Date: Fri Apr 27 14:10:07 2018
>>> New Revision: 331082
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=331082&view=rev
>>> Log:
>>> Fix build bots after r331049 broke them.
>>>
>>
>> I would like to apologize for communicating this so late (mainly
>> because I worked on the swift bits which are a little behind and I
>> didn't update my checkout) but this commit completely broke debugging
>> in some cases (on MacOS).
>>
>> Testcase:
>>
>> #import
>>
>> int main() {
>> id keys[1] = { @"abc" };
>> id values[1] = { @"def" };
>> id d = CFBridgingRelease(CFDictionaryCreate(nil, (void *)keys, (void
>> *)values, 1, nil, nil));
>> NSLog(@"%@", d);
>> NSLog(@"x");
>> }
>>
>>
>> $ clang patatino.m -framework Foundation -g -o blah
>>
>>
>> dtdebugger2:bin davide$ ./lldb ./blah
>> (lldb) target create "./blah"
>> Current executable set to './blah' (x86_64).
>> (lldb) b main
>> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
>> 0x00010fad
>> (lldb) r
>> error: No such file or directory
>>
>> Reverting yours (and a later change to avoid conflicts):
>>
>> dtdebugger2:llvm-project-20170507 davide$ git revert 57dce7084e7b
>> [master 8be66051362] Revert "Fixup r331049 (FileSpec
>> auto-normalization)"
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>> dtdebugger2:llvm-project-20170507 davide$ git revert
>> d5a834a73c0b37c3862daee260c416e7ea3b761c
>> [master 5bd1fb68a69] Revert "Fix build bots after r331049 broke them."
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> works.
>> I also noticed that if I pass a full path the thing works:
>>
>> Current executable set to
>> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64).
>> (lldb) b main
>> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
>> 0x00010fad
>> (lldb) r
>> Process 82833 launched:
>> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64)
>> Process 82833 stopped
>> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
>> frame #0: 0x00010fad blah`main at blah.c:2
>>1int main(void) {
>> -> 2 return 0;
>>3}
>>
>>
>> Do you mind to take a look and add a lit test for this?
>>
>> Thanks!
>>
>> --
>> Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r331082 - Fix build bots after r331049 broke them.
So it seems that if specify "./blah" as your program then this fails, but if
you specify just "blah" then this succeeds. Looks like we need to resolve the
path when creating a target with a local copy of the file. I will look into
this.
Greg
> On May 3, 2018, at 3:08 PM, Davide Italiano wrote:
>
> I would like to apologize for communicating this so late (mainly
> because I worked on the swift bits which are a little behind and I
> didn't update my checkout) but this commit completely broke debugging
> in some cases (on MacOS).
>
> Testcase:
>
> #import
>
> int main() {
> id keys[1] = { @"abc" };
> id values[1] = { @"def" };
> id d = CFBridgingRelease(CFDictionaryCreate(nil, (void *)keys, (void
> *)values, 1, nil, nil));
> NSLog(@"%@", d);
> NSLog(@"x");
> }
>
>
> $ clang patatino.m -framework Foundation -g -o blah
>
>
> dtdebugger2:bin davide$ ./lldb ./blah
> (lldb) target create "./blah"
> Current executable set to './blah' (x86_64).
> (lldb) b main
> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
> 0x00010fad
> (lldb) r
> error: No such file or directory
>
> Reverting yours (and a later change to avoid conflicts):
>
> dtdebugger2:llvm-project-20170507 davide$ git revert 57dce7084e7b
> [master 8be66051362] Revert "Fixup r331049 (FileSpec
> auto-normalization)"
> 2 files changed, 11 insertions(+), 5 deletions(-)
> dtdebugger2:llvm-project-20170507 davide$ git revert
> d5a834a73c0b37c3862daee260c416e7ea3b761c
> [master 5bd1fb68a69] Revert "Fix build bots after r331049 broke them."
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> works.
> I also noticed that if I pass a full path the thing works:
>
> Current executable set to
> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64).
> (lldb) b main
> Breakpoint 1: where = blah`main + 13 at blah.c:2, address = 0x00010fad
> (lldb) r
> Process 82833 launched:
> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64)
> Process 82833 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
>frame #0: 0x00010fad blah`main at blah.c:2
> 1int main(void) {
> -> 2 return 0;
> 3}
>
>
> Do you mind to take a look and add a lit test for this?
> (also, I guess you should change your mail address as it's still an @apple
> one).
>
> Thanks!
>
> --
> Davide
>
> On Thu, May 3, 2018 at 3:07 PM, Davide Italiano wrote:
>> On Fri, Apr 27, 2018 at 2:10 PM, Greg Clayton via lldb-commits
>> wrote:
>>> Author: gclayton
>>> Date: Fri Apr 27 14:10:07 2018
>>> New Revision: 331082
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=331082&view=rev
>>> Log:
>>> Fix build bots after r331049 broke them.
>>>
>>
>> I would like to apologize for communicating this so late (mainly
>> because I worked on the swift bits which are a little behind and I
>> didn't update my checkout) but this commit completely broke debugging
>> in some cases (on MacOS).
>>
>> Testcase:
>>
>> #import
>>
>> int main() {
>> id keys[1] = { @"abc" };
>> id values[1] = { @"def" };
>> id d = CFBridgingRelease(CFDictionaryCreate(nil, (void *)keys, (void
>> *)values, 1, nil, nil));
>> NSLog(@"%@", d);
>> NSLog(@"x");
>> }
>>
>>
>> $ clang patatino.m -framework Foundation -g -o blah
>>
>>
>> dtdebugger2:bin davide$ ./lldb ./blah
>> (lldb) target create "./blah"
>> Current executable set to './blah' (x86_64).
>> (lldb) b main
>> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
>> 0x00010fad
>> (lldb) r
>> error: No such file or directory
>>
>> Reverting yours (and a later change to avoid conflicts):
>>
>> dtdebugger2:llvm-project-20170507 davide$ git revert 57dce7084e7b
>> [master 8be66051362] Revert "Fixup r331049 (FileSpec
>> auto-normalization)"
>> 2 files changed, 11 insertions(+), 5 deletions(-)
>> dtdebugger2:llvm-project-20170507 davide$ git revert
>> d5a834a73c0b37c3862daee260c416e7ea3b761c
>> [master 5bd1fb68a69] Revert "Fix build bots after r331049 broke them."
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> works.
>> I also noticed that if I pass a full path the thing works:
>>
>> Current executable set to
>> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64).
>> (lldb) b main
>> Breakpoint 1: where = blah`main + 13 at blah.c:2, address =
>> 0x00010fad
>> (lldb) r
>> Process 82833 launched:
>> '/Users/davide/work/llvm-monorepo/build/bin/blah' (x86_64)
>> Process 82833 stopped
>> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
>>frame #0: 0x00010fad blah`main at blah.c:2
>> 1int main(void) {
>> -> 2 return 0;
>> 3}
>>
>>
>> Do you mind to take a look and add a lit test for this?
>>
>> Thanks!
>>
>> --
>> Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r331082 - Fix build bots after r331049 broke them.
On Fri, May 4, 2018 at 10:54 AM, Greg Clayton wrote: > So it seems that if specify "./blah" as your program then this fails, but if > you specify just "blah" then this succeeds. Looks like we need to resolve the > path when creating a target with a local copy of the file. I will look into > this. > > Greg > Please take a look at this quickly today, as it's breaking a user-visible feature. -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
