This is an archive of the discontinued LLVM Phabricator instance.

Move ADB communications to AdbClient class - to make it accessible by other components.
ClosedPublic

Authored by ovyalov on Mar 22 2015, 7:27 PM.

Details

Summary

Move ADB communications to AdbClient class - in order to make it accessible by other LLDB components.
I'm planning to use device serial number for Android devices instead of host name within module cache - will integrate AdbClient with PlatformAndroid with a next CL.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 22445.Mar 22 2015, 7:27 PM
ovyalov retitled this revision from to Move ADB communications to AdbClient class - to make it accessible by other components..
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: vharron, tberghammer.
ovyalov added a subscriber: Unknown Object (MLST).
tberghammer edited edge metadata.Mar 23 2015, 7:30 AM

Thanks for moving it out to a new class.

Please check that the adb port forward is set up correctly (not left over from a previous session) and also check that all of the port forwards are cleaned up after lldb exits (I suggest a manual test as full test suit might leak some adb forwards in case of a failure).

source/Plugins/Platform/Android/AdbClient.cpp
72–76

I think it isn't an error in this scenario if no device was found

110–122

What is the goal of this function? I think it will return m_device_id on success and if m_device_id is not set then it will fail.

source/Plugins/Platform/Android/AdbClient.h
36

Do we need a virtual destructor?

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
44–49

Please return an error if not exactly one device is found and remove the assert.

I think we should add asserts to check for bugs in lldb while having no connected device can be caused by almost anything.

If we found more then 1 device then (randomly) choosing one will cause some flakiness in our tests so I think it is better to report a failure here. I plan to add some way to specify the selected device from platform-android.

ovyalov added inline comments.Mar 23 2015, 11:32 AM
source/Plugins/Platform/Android/AdbClient.cpp
72–76

Removed.

110–122

Good point - removed it.

source/Plugins/Platform/Android/AdbClient.h
36

Removed it at all.

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
44–49

Makes sense - fixed.

ovyalov updated this revision to Diff 22495.Mar 23 2015, 11:33 AM
ovyalov edited edge metadata.

Fixed accordingly to suggestions.
Please take another look.

Thank you.

tberghammer accepted this revision.Mar 23 2015, 11:56 AM
tberghammer edited edge metadata.

LGTM, but please add back the xcode project files and the Makefile changes (included in #1, removed from #2)

This revision is now accepted and ready to land.Mar 23 2015, 11:56 AM
ovyalov updated this revision to Diff 22500.Mar 23 2015, 12:07 PM
ovyalov edited edge metadata.

Added missed lldb.xcodeproj/project.pbxpro - thanks for catching this!

ovyalov closed this revision.Mar 23 2015, 2:07 PM

AFFECTED FILES

/lldb/trunk/lldb.xcodeproj/project.pbxproj
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/CMakeLists.txt
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

USERS

ovyalov (Author)

http://reviews.llvm.org/rL233021