This is an archive of the discontinued LLVM Phabricator instance.

[lldb-server] Update tests to use std::thread/mutex for all platforms
ClosedPublic

Authored by asmith on Apr 9 2019, 10:39 PM.

Diff Detail

Event Timeline

asmith created this revision.Apr 9 2019, 10:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 10:39 PM
labath accepted this revision.Apr 9 2019, 11:00 PM

LGTM, with some inline comments about additional c++11 goodies we can use to clean up this file further. (Also, it might be good to mention in the patch title that this is about modifying the test code, because my first though was that you are adding some locking to the actual lldb-server code.)

packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
37–45

Other tests already use std::this_thread::sleep_for(std::chrono::seconds(X)), so I expect you should be able to do the same here.

180–185

you could just make s_thread_index an atomic<int>, and avoid the manual locking around the increment operation.

188–192

use std::scoped_lock instead of manual lock/unlock calls (throughout the patch).

This revision is now accepted and ready to land.Apr 9 2019, 11:00 PM
asmith retitled this revision from [lldb-server] Use std::thread/mutex for all platforms to [lldb-server] Update tests to use std::thread/mutex for all platforms.Apr 10 2019, 12:30 AM
asmith updated this revision to Diff 194751.Apr 11 2019, 1:57 PM

Address reviewer comments

clayborg added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
80–84

Is there no llvm host code we can use as the porting layer instead of this?

87–107

No llvm host layer code for this too?

asmith marked an inline comment as done.Apr 11 2019, 2:22 PM
asmith added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
188–192

Is cxx17 now officially okay to use?

asmith marked an inline comment as done.Apr 11 2019, 7:51 PM
asmith added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
80–84

If the test harness was changed to link against the host support libraries and included the right header this could be factored out.

labath accepted this revision.Apr 11 2019, 11:23 PM

LGTM.

The ifdefs for the getpid() code and stuff are unfortunate, but I am not sure if using llvm libraries for that is a good idea. Right now we have the ability to compile the tests for a different architecture than the one lldb is built for. If we started using non-standard facilities in the tests, this would become a lot more complicated.

packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
188–192

Sorry, I meant std::lock_guard here (and I see that you have used that). c++17 is not ok do be used in real code. Tests can theoretically have different (stricter, or more lax) requirements, but I don't see a reason to deviate from the official policy here.

asmith closed this revision.Apr 12 2019, 12:46 AM