This is an archive of the discontinued LLVM Phabricator instance.

Fix ARM attribute parsing for Android after rL267291
ClosedPublic

Authored by tberghammer on Apr 25 2016, 5:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Fix ARM attribute parsing for Android after rL267291.
tberghammer updated this object.
tberghammer added reviewers: labath, compnerd.
tberghammer added a subscriber: lldb-commits.
rengolin added inline comments.Apr 25 2016, 5:58 AM
source/Core/ArchSpec.cpp
1017 ↗(On Diff #54839)

Are you sure AndroidEabi and EabiHF are compatible? Android is always soft-float, is it not?

tberghammer added inline comments.Apr 25 2016, 6:20 AM
source/Core/ArchSpec.cpp
1017 ↗(On Diff #54839)

They are not always compatible but I don't see any way to detect it in the current system and it is possible to have code compiled with AndroidEabi and AndroidEabiHF inside the same application if all API crossing the ABI boundaries are annotated with attribute((pcs("aapcs"))).

The android platform libraries are soft float with all API with floating point argument is marked with attribute((pcs("aapcs"))) so user libraries can be hard float (not sure how well it works)

labath accepted this revision.Apr 25 2016, 6:38 AM
labath edited edge metadata.

Ideally, we'd be able to pick up the fact that we're dealing with an android shared library just from looking at it, but it seems they don't contain any information which would identify them as such. If anyone has a better solution, we'd like to hear about it.

This revision is now accepted and ready to land.Apr 25 2016, 6:38 AM

Don't Android libraries output build attributes for ARM? If they do, that's an easy way to know.

Both executables and shared libraries are containing ARM Attributes what contains the information about soft-float vs hard-float. I am not sure how accurate it is as in theory you can link together an object file compiled with soft float with a one compiled with hard float but most likely we can trust it to be good enough.

The problem is that currently this information is not stored in the llvm::Triple so if the Environment part of the Triple is set to Android then the information is lost. I can try to recover the data from the architecture flags but I am not convinced that it is always possible and considering that the same executable can contain soft-float and hard-float code on Android we have to treat them as compatible architectures at some point.

Both executables and shared libraries are containing ARM Attributes what contains the information about soft-float vs hard-float. I am not sure how accurate it is as in theory you can link together an object file compiled with soft float with a one compiled with hard float but most likely we can trust it to be good enough.

That's only true if one doesn't depend on the other, and only if you can guarantee that hard-float library functions will only be called using AAPCS_VFP and soft-float library functions will only be called with AAPCS.

This is not the kind of guarantee one can do easily. Nor it is recommended that one does so. If Android applications can guarantee that by having two different ABIs for Android specific and user specific, this could work in theory.

The problem is that currently this information is not stored in the llvm::Triple so if the Environment part of the Triple is set to Android then the information is lost. I can try to recover the data from the architecture flags but I am not convinced that it is always possible and considering that the same executable can contain soft-float and hard-float code on Android we have to treat them as compatible architectures at some point.

The information is not lost, because "Android EABI" is always soft-float, and by definition, you should not be linking hard-float objects with soft-float ones.

Can you elaborate which case you can reach that improbably scenario?

cheers,
--renato

Both executables and shared libraries are containing ARM Attributes what contains the information about soft-float vs hard-float. I am not sure how accurate it is as in theory you can link together an object file compiled with soft float with a one compiled with hard float but most likely we can trust it to be good enough.

That's only true if one doesn't depend on the other, and only if you can guarantee that hard-float library functions will only be called using AAPCS_VFP and soft-float library functions will only be called with AAPCS.

This is not the kind of guarantee one can do easily. Nor it is recommended that one does so. If Android applications can guarantee that by having two different ABIs for Android specific and user specific, this could work in theory.

The problem is that currently this information is not stored in the llvm::Triple so if the Environment part of the Triple is set to Android then the information is lost. I can try to recover the data from the architecture flags but I am not convinced that it is always possible and considering that the same executable can contain soft-float and hard-float code on Android we have to treat them as compatible architectures at some point.

The information is not lost, because "Android EABI" is always soft-float, and by definition, you should not be linking hard-float objects with soft-float ones.

Can you elaborate which case you can reach that improbably scenario?

This happens because there is a broken mode (supported by an older NDK) that allowed for Hard FP calling conventions to be mixed with Soft FP in a hybrid mode. Everything in the user's application would get to use Hard FP, while any calls to bionic (libc/libm), or platform native libraries would use Soft FP. This wasn't really well tested, nor is it something that I think we should be promoting. Does this patch only affect debugging? Could we hold off on this until a long term decision is made about supporting mixed hard/soft FP calling conventions simultaneously?

cheers,
--renato

This patch only effects debugging (all modified file is part of LLDB).

I want to get the Android <-> EABI part fixed ASAP as a recent LLDB change completely broke Android ARM debugging with adding ARM.Attributes support (causing Environment mismatch between EABI and Android when the executable contains an ".note.android.ident" while the shared library isn't).

I can change the patch to check only for the Android <-> EABI case and we can wait with the hard-float part as almost nobody using hard-float if you think that is a better idea.

This patch only effects debugging (all modified file is part of LLDB).

I want to get the Android <-> EABI part fixed ASAP as a recent LLDB change completely broke Android ARM debugging with adding ARM.Attributes support (causing Environment mismatch between EABI and Android when the executable contains an ".note.android.ident" while the shared library isn't).

I can change the patch to check only for the Android <-> EABI case and we can wait with the hard-float part as almost nobody using hard-float if you think that is a better idea.

I would prefer that this patch not address any of the Hard/Soft FP mismatch problem. If you can make it just fix the Android/EABI case you are looking at, that would be the best option.

tberghammer edited edge metadata.

I updated the diff to only address the soft-flat case (Android-EABI). This fixes the most important case but I would like to fix the hard-flat case as well in the not so distant future as we have the same problem (regression ) in that case as well (combined with the hard/soft mismatch)

This revision was automatically updated to reflect the committed changes.

I committed in this change based on the approval from @labath with the Android - EABI support (no hard float) to get the LLDB Android ARM buildbots green again. If you have any more question comment then please let me know and I am happy to address them with a separate commit.

I committed in this change based on the approval from @labath with the Android - EABI support (no hard float) to get the LLDB Android ARM buildbots green again. If you have any more question comment then please let me know and I am happy to address them with a separate commit.

Sounds good, thanks!