Fix ARM attribute parsing for Android after rL267291

[tberghammer]

Summary

Fix ARM attribute parsing for Android after rL267291

[rengolin]

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]

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]

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.

[rengolin]

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

[tberghammer]

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.

[rengolin]

In D19480#410620, @tberghammer wrote:

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

[srhines]

In D19480#410641, @rengolin wrote:

In D19480#410620, @tberghammer wrote:

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

[tberghammer]

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.

[srhines]

In D19480#410664, @tberghammer wrote:

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]

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)

[tberghammer]

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.

[rengolin]

In D19480#410762, @tberghammer wrote:

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!

lldb/trunk/source/Core/ArchSpec.cpp

 }
 
 bool
 ArchSpec::IsCompatibleMatch (const ArchSpec& rhs) const
 {
     return IsEqualTo (rhs, false);
 }
 
+static bool
+isCompatibleEnvironment(llvm::Triple::EnvironmentType lhs, llvm::Triple::EnvironmentType rhs)
+{
+    if (lhs == rhs)
+        return true;
+
+    // If any of the environment is unknown then they are compatible
+    if (lhs == llvm::Triple::UnknownEnvironment || rhs == llvm::Triple::UnknownEnvironment)
+        return true;
+
+    // If one of the environment is Android and the other one is EABI then they are considered to
+    // be compatible. This is required as a workaround for shared libraries compiled for Android
+    // without the NOTE section indicating that they are using the Android ABI.
+    if ((lhs == llvm::Triple::Android && rhs == llvm::Triple::EABI) ||
+        (rhs == llvm::Triple::Android && lhs == llvm::Triple::EABI))
+        return true;
+
+    return false;
+}
+
 bool
 ArchSpec::IsEqualTo (const ArchSpec& rhs, bool exact_match) const
 {
     // explicitly ignoring m_distribution_id in this method.
 
     if (GetByteOrder() != rhs.GetByteOrder())
         return false;
         
if (core_match)
             // Only fail if both os types are not unknown
             if (lhs_triple_os != llvm::Triple::UnknownOS &&
                 rhs_triple_os != llvm::Triple::UnknownOS)
                 return false;
         }
 
         const llvm::Triple::EnvironmentType lhs_triple_env = lhs_triple.getEnvironment();
         const llvm::Triple::EnvironmentType rhs_triple_env = rhs_triple.getEnvironment();
-            
-        if (lhs_triple_env != rhs_triple_env)
-        {
-            // Only fail if both environment types are not unknown
-            if (lhs_triple_env != llvm::Triple::UnknownEnvironment &&
-                rhs_triple_env != llvm::Triple::UnknownEnvironment)
-                return false;
-        }
+        if (!isCompatibleEnvironment(lhs_triple_env, rhs_triple_env))
         return true;
     }
     return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Helper methods.
 

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (strtab_idx && strtab_idx < section_headers.size())
                     }
                     arch_spec.SetFlags (arch_flags);
                 }
 
                 if (arch_spec.GetMachine() == llvm::Triple::arm || arch_spec.GetMachine() == llvm::Triple::thumb)
                 {
                     DataExtractor data;
 
-                    if (sheader.sh_type != SHT_ARM_ATTRIBUTES)
-                        continue;
-                    if (section_size == 0 || set_data(data, sheader.sh_offset, section_size) != section_size)
-                        continue;
-
+                    if (sheader.sh_type == SHT_ARM_ATTRIBUTES && section_size != 0 && 
+                        set_data(data, sheader.sh_offset, section_size) == section_size)
                         ParseARMAttributes(data, section_size, arch_spec);
                 }
 
                 if (name == g_sect_name_gnu_debuglink)
                 {
                     DataExtractor data;
                     if (section_size && (set_data (data, sheader.sh_offset, section_size) == section_size))
                     {
                         lldb::offset_t gnu_debuglink_offset = 0;