Page MenuHomePhabricator

Use Android device serial number instead of hostname as a target identifier within module cache.
ClosedPublic

Authored by ovyalov on Mar 24 2015, 5:40 PM.

Details

Summary

Use Android device serial number instead of hostname as a target identifier within module cache - otherwise modules for all devices will be stored under localhost folder.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 22619.Mar 24 2015, 5:40 PM
ovyalov retitled this revision from to Use Android device serial number instead of hostname as a target identifier within module cache..
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: clayborg, tberghammer, vharron.
ovyalov added a subscriber: Unknown Object (MLST).
tberghammer added inline comments.Mar 25 2015, 3:39 AM
source/Plugins/Platform/Android/AdbClient.h
33–37

What is your opinion about using a proper factory pattern instead of this combined approach? If we worry about the performance implication caused by querying the device list every time then we can make that optional.

Note: I plan to add a few more factory method to create an AdbClient based on -e (emulator) and -d (device) flags the same way as adb do it.

38–40

Can we remove this function (or make it private)?

ovyalov updated this revision to Diff 22653.Mar 25 2015, 8:44 AM

Marked AdbClient::SetDeviceID as private.

Please see my comments.

source/Plugins/Platform/Android/AdbClient.h
33–37

Do you mean to use separate factory methods to attach either to emulator or device?

Performance shouldn't be an issue here since it's one-off call and then device_id is stored inside PlatformAndroid.

Could you give more context why we need to distinguish emulator and device flows? I mean, once we have device_id (either emulator and real device) we can distinguish Android instances even when there are a few connected devices. As for me, from LLDB perspective either emulator or device should be treated identically.

38–40

Made it private.

tberghammer added inline comments.Mar 25 2015, 9:03 AM
source/Plugins/Platform/Android/AdbClient.h
33–37

I plan to add some flags to "platform connect" in PlatformAndroid to specify which device we want to connect. My plan is to follow the same convention what is used by adb ([-s serial_number] [-e] [-d]) where if serial number is specified then connect based on that, if -e is specified then connect to the only running emulator (fail if more then 1 is running) and if -d is specified the connect to the only running device. Also fail if more then one flag is provided.

ovyalov added inline comments.Mar 25 2015, 9:14 AM
source/Plugins/Platform/Android/AdbClient.h
33–37

I see - makes sense.

clayborg accepted this revision.Mar 25 2015, 9:59 AM
clayborg edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 25 2015, 9:59 AM
ovyalov closed this revision.Mar 25 2015, 11:02 AM

AFFECTED FILES

/lldb/trunk/include/lldb/Target/Platform.h
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
/lldb/trunk/source/Target/Platform.cpp

USERS

ovyalov (Author)

http://reviews.llvm.org/rL233202