Page MenuHomePhabricator

[lldb] [Process/NetBSD] Enable reporting of new and exited threads [WIP]
Needs ReviewPublic

Authored by mgorny on Wed, Jul 31, 9:21 PM.

Details

Summary

// NB:
Now, this patch is most likely correct. However, I've tested it only with the other patch applied, and many tests started hanging forever. If you have any comments, I'd be happy to process them but I won't commit it until all the issues have been resolved.

Diff Detail

Event Timeline

mgorny created this revision.Wed, Jul 31, 9:21 PM

Looks fine to me. Minor comments inline.

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
271

I don't think you need this. Given that you never reported a stop event, as far as the rest of the world is concerned the process is still "running" (i.e., this "stop to notice a new thread" is just an implementation detail).

736

It looks like the rest of the code assumes (as I think it should) that the thread list does not contain null pointers. See e.g. the loop on line 276.

mgorny marked an inline comment as done.Thu, Aug 1, 12:18 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
737

Note to self, this should be it =...

mgorny marked an inline comment as done.Fri, Aug 2, 7:16 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
736

For the record, I've copied it from Linux ;-P.

jfb requested changes to this revision.Fri, Aug 2, 9:35 AM
jfb added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
736

Are you serious? Because you absolutely cannot copy code from Linux into LLVM.

This revision now requires changes to proceed.Fri, Aug 2, 9:35 AM
mgorny marked an inline comment as done.Fri, Aug 2, 10:07 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
736

I meant LLDB Linux Process plugin.

jingham added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
736

I believe Michal means he copied it from the Linux NativeProcess implementation in lldb to the NetBSD Process plugin in lldb.

jfb added inline comments.Fri, Aug 2, 10:21 AM
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
736

OK, thank you for clarifying.

mgorny marked 6 inline comments as done.Sat, Aug 3, 11:17 PM

@jfb, could you please remove your red flag now? ;-)

lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
737

Nevermind myself. It breaks immediately, so it doesn't matter.

mgorny updated this revision to Diff 213231.Sat, Aug 3, 11:37 PM
krytarowski added inline comments.Sun, Aug 4, 9:23 AM
lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
104

I would go for more generic PT_GET_EVENT_MASK and then |=. In theory we could add more bits and make them enabled by default.

jfb resigned from this revision.Sun, Aug 4, 8:39 PM