Page MenuHomePhabricator

Download symbol file for .oat files on android
ClosedPublic

Authored by tberghammer on Aug 11 2015, 4:34 AM.

Details

Summary

Download symbol file for .oat files on android

On android .oat files (compiled java code) don't have symbol
information but on SDK 23+ it can be generated by the oatdump tool
(based on the dex information).

This CL adds logic to download this information and store it in the
module cache.

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Download symbol file for .oat files on android.
tberghammer updated this object.
tberghammer added a reviewer: ovyalov.
tberghammer added a subscriber: lldb-commits.
ovyalov edited edge metadata.Aug 11 2015, 9:14 AM

Please see my comments.

source/Plugins/Platform/Android/PlatformAndroid.cpp
306 ↗(On Diff #31791)

indentation.

325 ↗(On Diff #31791)

Please add constant for timeout.

329 ↗(On Diff #31791)

Please check that tmpdir isn't empty.

331 ↗(On Diff #31791)

s/directroy/directory

337 ↗(On Diff #31791)

Please log an error if rm fails.

342 ↗(On Diff #31791)

Can we have a bigger timeout? 1 sec might not be enough.

352 ↗(On Diff #31791)

GetCString(false)?

353 ↗(On Diff #31791)

ditto - please verify that it works as expected from Windows.

source/Utility/ModuleCache.cpp
137 ↗(On Diff #31791)

Please add ".sym" constant

197 ↗(On Diff #31791)

s/modul/module

207 ↗(On Diff #31791)

Do we really want to have a link for sym file in sys root?
I'd rather moved code for sym file download from GetAndPut into Put method and skip link creation for symbol file.

tberghammer edited edge metadata.
tberghammer marked 8 inline comments as done.

Address comments

See responds inline

source/Plugins/Platform/Android/PlatformAndroid.cpp
325 ↗(On Diff #31791)

Added a comment next to the value instead. I prefer that style but I can use a constant if you want.

342 ↗(On Diff #31791)

I think 1 second should be enough in almost every case but increase it to 5 seconds just in case the device is very busy.

353 ↗(On Diff #31791)

At the moment I don't have Windows environment but I will try to find a way to test it.

source/Utility/ModuleCache.cpp
207 ↗(On Diff #31791)

At the moment I think we don't use the sys root at all, but I might be wrong. If anywhere we use the sys root then we will need the symbol file next to the actual file to find it. The reason I put the file download here because we need a Module object to do it (find out if we have symtab) what means we have to do it after the Get. I think moving it to Put will make the implementation more complicated.

ovyalov accepted this revision.Aug 11 2015, 11:11 AM
ovyalov edited edge metadata.

LGTM

source/Utility/ModuleCache.cpp
207 ↗(On Diff #31826)

s/.sym/kSymFileExtension
Or maybe introduce a function that returns symbol file spec by given module file spec in order to re-use it here and in ModuleCache::Get

This revision is now accepted and ready to land.Aug 11 2015, 11:11 AM
This revision was automatically updated to reflect the committed changes.