Page MenuHomePhabricator

[lldb] Enable FreeBSDRemote plugin by default and update test status
ClosedPublic

Authored by mgorny on Nov 4 2020, 3:34 AM.

Details

Summary

The new FreeBSDRemote plugin has reached feature parity on i386
and amd64 targets. Use it by default on these architectures, while
allowing the use of the legacy plugin via FREEBSD_LEGACY_PLUGIN envvar.

Revisit the method of switching plugins. Apparently, the return value
of PlatformFreeBSD::CanDebugProcess() is what really decides whether
the legacy or the new plugin is used.

Update the test status. Reenable the tests that were previously
disabled on FreeBSD and do not cause hangs or are irrelevant to FreeBSD.
Mark all tests that fail reliably as expectedFailure. For now, tests
that are flaky (i.e. produce unstable results) are left enabled
and cause unpredictable test failures.

Diff Detail

Event Timeline

mgorny created this revision.Nov 4 2020, 3:34 AM
mgorny requested review of this revision.Nov 4 2020, 3:34 AM
mgorny added inline comments.
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
250–262

@labath, any clue if I am missing something or is this really sufficient to switch between plugins?

lldb/test/API/functionalities/avoids-fd-leak/TestFdLeak.py
13

I've removed this entirely since LLDB does not support Python 2 anymore.

mgorny updated this revision to Diff 302810.Nov 4 2020, 3:39 AM

Missed one test.

labath added inline comments.Nov 4 2020, 4:49 AM
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
250–262

Yeah, this should be fine. I was thinking it should be possible to switch this on from a single place all along....

263–269

I think this ought to go into the if(host) block, as it's really not applicable to remote debugging.

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
39

These may be related to the if(host) thingy.

lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashingStep.py
53–54

I'm pretty sure the root cause here is the same for net/free bsd as it is for linux (it comes down to macos catching the "crashes" specially, before they even get turned to a SEGV -- something that's not possible elsewhere). I marked it skip because that's not something we should support, ever. I don't care that much which decorator (skip vs. xfail) is used here, but I think they should be consistent.

lldb/test/API/functionalities/inferior-crashing/recursive-inferior/TestRecursiveInferiorStep.py
30

ditto

lldb/test/API/tools/lldb-server/thread-name/TestGdbRemoteThreadName.py
32

What's up with this? I though you fixed thread names already..

mgorny marked 4 inline comments as done.Nov 4 2020, 8:19 AM
mgorny added inline comments.
lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
39

Doesn't seem to have changed after changing it. I will look into it while reiterating the whole test suite.

lldb/test/API/tools/lldb-server/thread-name/TestGdbRemoteThreadName.py
32

The test code is buggy. I want to fix it separately not to mix things up too much.

mgorny updated this revision to Diff 302859.Nov 4 2020, 8:47 AM

Applied changes as requested.

I added comments in the now-dereferenced bugs linking back to this review - most of them were submitted by me, and I'll double check and close them once this lands.

One minor update needed (restoring a comment that was partially deleted)

lldb/packages/Python/lldbsuite/test/dotest.py
955

It's a shame that these variables and the comments are inverted sense / double negatives, but not an issue in this patch.

lldb/test/API/commands/register/register/register_command/TestRegisters.py
31

there's no (existing) PR for the failure?

lldb/test/API/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py
25

was this one already failing, or is this new?

lldb/test/API/functionalities/exec/TestExec.py
23

So hangs on FreeBSD but reliably fails on NetBSD?

lldb/test/API/functionalities/thread/create_after_attach/TestCreateAfterAttach.py
24–25

part of comment here disappeared

I added comments in the now-dereferenced bugs linking back to this review - most of them were submitted by me, and I'll double check and close them once this lands.

I was planning to go through them once this is accepted. But if you could double check my results, that would be great.

When running locally with this change applied I get:

********************
Failed Tests (8):
  lldb-api :: commands/expression/multiline-completion/TestMultilineCompletion.py
  lldb-api :: functionalities/breakpoint/breakpoint_set_restart/TestBreakpointSetRestart.py
  lldb-api :: functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
  lldb-api :: iohandler/completion/TestIOHandlerCompletion.py
  lldb-api :: python_api/watchpoint/watchlocation/TestSetWatchlocation.py
  lldb-api :: python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
  lldb-api :: tools/lldb-server/thread-name/TestGdbRemoteThreadName.py
  lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/StandardStartupTest.TestStopReplyContainsThreadPcs

********************
Unexpectedly Passed Tests (2):
  lldb-api :: api/multiple-debuggers/TestMultipleDebuggers.py
  lldb-api :: lang/c/conflicting-symbol/TestConflictingSymbol.py


Testing Time: 367.34s
  Unsupported        :  422
  Passed             : 1741
  Expectedly Failed  :    5
  Failed             :    8
  Unexpectedly Passed:    2
mgorny marked an inline comment as done.Nov 4 2020, 12:43 PM
mgorny added inline comments.
lldb/packages/Python/lldbsuite/test/dotest.py
955

Yes, I had to look at it for a while to make sure I'm doing it right. I'll change them in a followup commit.

lldb/test/API/commands/register/register/register_command/TestRegisters.py
31

No. I'm planning to file bugs during the next stage, after trying to fix the trivial ones, if that's ok with you.

lldb/test/API/commands/watchpoints/multiple_threads/TestWatchpointMultipleThreads.py
25

Yes, it is new. I must've attributed it to unstable tests when I reported no regressions yesterday. However, today I've confirmed it to fail reliably. Hopefully, it won't be hard to fix once I get to it.

lldb/test/API/functionalities/exec/TestExec.py
23

Yes, or at least it reliably failed on NetBSD the last time I checked.

lldb/test/API/functionalities/thread/create_after_attach/TestCreateAfterAttach.py
24–25

Thanks for noticing. Restored it and move nearer to the new skip.

mgorny updated this revision to Diff 302942.Nov 4 2020, 12:43 PM
mgorny marked an inline comment as done.

Fixed test comment.

mgorny updated this revision to Diff 302943.Nov 4 2020, 12:46 PM

I had accidentally included a commented out xfail for testing.

mgorny added a comment.Nov 4 2020, 1:05 PM

When running locally with this change applied I get:

********************
Failed Tests (8):
  lldb-api :: commands/expression/multiline-completion/TestMultilineCompletion.py
  lldb-api :: functionalities/breakpoint/breakpoint_set_restart/TestBreakpointSetRestart.py

These two pass reliably for me. You can check if it could be related to parallel testing. To start them individually:

ninja check-lldb-api-commands-expression-multiline-completion
ninja check-lldb-api-functionalities-breakpoint-breakpoint_set_restart
lldb-api :: functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py

This one's unstable.

lldb-api :: iohandler/completion/TestIOHandlerCompletion.py
lldb-api :: python_api/watchpoint/watchlocation/TestSetWatchlocation.py
lldb-api :: python_api/watchpoint/watchlocation/TestTargetWatchAddress.py

All three pass for me reliably right now but I recall that watchlocation test could be fragile to parallel runs.

lldb-api :: tools/lldb-server/thread-name/TestGdbRemoteThreadName.py

This one I've commented out for testing per earlier comments here and forget to revert.

lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/StandardStartupTest.TestStopReplyContainsThreadPcs

This one I know about. There's no easy way to xfail an unittest, and the hack to skip it is kinda ugly, so I kept it for now not to forget about it.

********************
Unexpectedly Passed Tests (2):
  lldb-api :: api/multiple-debuggers/TestMultipleDebuggers.py

I think this one's unstable in long runs. I recall I marked it xfail first but then removed the xfail because it succeeded. But then, it failed in a few consecutive, so I've xfailed it ;-).

lldb-api :: lang/c/conflicting-symbol/TestConflictingSymbol.py

This one also reliably XFAILs for me.

Testing Time: 367.34s
  Unsupported        :  422
  Passed             : 1741
  Expectedly Failed  :    5
  Failed             :    8
  Unexpectedly Passed:    2

What do you suggest? Is it fine to go with my results for as long as I work on it? I can update it to match CI/buildbot results later.

labath accepted this revision.Nov 4 2020, 11:56 PM

Looks ok to me. I'll leave it to you to sort out the specific annotations on individual tests.

lldb/test/API/tools/lldb-server/thread-name/TestGdbRemoteThreadName.py
32

Fixing it here is not a good idea, yes. But I think it would have been good to fix in the patch that was implementing thread name support.

This revision is now accepted and ready to land.Nov 4 2020, 11:56 PM
mgorny marked 2 inline comments as done.Nov 5 2020, 4:00 AM
mgorny added inline comments.
lldb/test/API/tools/lldb-server/thread-name/TestGdbRemoteThreadName.py
32

I'm planning to run the test suite a lot more once this lands, and we have more predictable results.

emaste accepted this revision.Nov 5 2020, 6:35 AM

What do you suggest? Is it fine to go with my results for as long as I work on it? I can update it to match CI/buildbot results later.

Yes absolutely. I pasted my results mainly as an FYI/for comparison, since I recall there were many intermittent or flaky tests. I definitely want to see this go in and we can iterate on the individual annotations.

emaste added inline comments.Nov 5 2020, 6:39 AM
lldb/packages/Python/lldbsuite/test/dotest.py
955

Thanks.

lldb/test/API/commands/register/register/register_command/TestRegisters.py
31

Yes absolutely. I was just curious because this was already failing for NetBSD

lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashingStep.py
53–54

Sounds reasonable, we may want to change the comment above to make it clear this is an explicit decision.

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2020, 8:50 AM

Does the new plugin work with processes that are created with pdfork? The last time I tried this, it caused the old plugin to lock up the debugger entirely. Please can you ensure that there are tests that cover pdfork and cap_enter in the child? These are currently quite badly broken.

Does the new plugin work with processes that are created with pdfork? The last time I tried this, it caused the old plugin to lock up the debugger entirely. Please can you ensure that there are tests that cover pdfork and cap_enter in the child? These are currently quite badly broken.

Can you show an example of a program (ideally minimal) that is supposed to work but hangs? Does GDB support pdfork-ed children?

There is also TRAP_CAP and I'm not sure what should we do upon it.