Page MenuHomePhabricator

[lldb][testsuite] Create a SBDebugger instance for each test
ClosedPublic

Authored by tatyana-krasnukha on Feb 20 2020, 7:45 AM.

Details

Summary

Some tests set settings and don't clean them up, this leads to side effects in other tests.
The patch removes a global debugger instance with a per-test debugger to avoid such effects.

From what I see, lldb.DBG was needed to determine the platform before a test is run, lldb.selected_platform is used for this purpose now. Though, this required adding a new function to the SBPlatform interface.

Diff Detail

Event Timeline

tatyana-krasnukha retitled this revision from [lldb][test] Add two wrapper-functions to manage settings in test-suite to [lldb][test] Add two wrapper functions to manage settings in test-suite.Feb 20 2020, 8:01 AM

I'm kinda curious how much we actually support this? I know we do a lot of random fiddling with settings and what not, but I feel if we anyway say that each test needs to have a clean setup, can't we just create like a new SBDebugger instance or something like that at test start? That would solve all the cleanup issues and cover all the hidden settings that LLDB supports (e.g. the amazing platform settings command and all that).

I like this idea and ready to implement, but I'm not aware of the reasons why the current implementation was chosen. Suppose that initializing the Debugger for each test will slow down the test-suite significantly.

labath added a subscriber: jingham.Feb 21 2020, 5:03 AM

We (mostly me and @jingham) discussed that idea (very) briefly a couple of weeks ago, and my impression was that this (reinitializing SBDebugger for every test) would be a good way to go. There are just too many things that can leak from one session into another, and since none of the default bots run in the monolithic mode, I am frankly surprised that this mode is still even remotely usable.

One thing to be aware of is that SBDebuggers are not perfectly isolated either -- some settings will leak from one debugger into another and so one still has to be careful when setting those...

tatyana-krasnukha retitled this revision from [lldb][test] Add two wrapper functions to manage settings in test-suite to [lldb][testsuite] Create a SBDebugger instance for each test.
tatyana-krasnukha edited the summary of this revision. (Show Details)
tatyana-krasnukha planned changes to this revision.Mar 3 2020, 7:36 AM

As Pavel wrote, there are global properties that all debuggers share. That's why this approach doesn't work for me.
I'm going to add settings clear mode without arguments that will revert all the settings to their default values.

Clear all settings during a test's setUp

labath added a comment.Mar 3 2020, 8:43 AM

Overall, I don't have any big issues with this patch, but overall I see at least two distinct issues being handled here: a) changing how the platform selection works; and b) auto-clearing of settings. Please split those up into separate patches.

Moved settings clearing to the separate patch D75537

labath accepted this revision.Mar 4 2020, 4:10 AM
labath added a reviewer: jingham.

This is a great cleanup.

I don't believe the addition of the new SBPlatform interface is an issue, but I've added @jingham just in case he has any thoughts on that. Please give him a chance to comment on this.

This revision is now accepted and ready to land.Mar 4 2020, 4:10 AM
jingham accepted this revision.Mar 4 2020, 10:22 AM

Seems great to me. Getting the host platform is a generally useful thing to be able to do, and I can't think of anything you would want to add to this, so that is fine to.

This revision was automatically updated to reflect the committed changes.