Handle floating-point type homogeneous aggregate return values in ABISysV_arm

[omjavaid]

Summary

Arm hard float ABI can use floating point registers for returning structures containing all 4 or 8 byte floating point elements.

Arm ABI documentation call such structs a Homogeneous Aggregate, which is a Composite Type where all of the Fundamental Data Types that compose the type
are the same.

With Arm Hard float ABI a Homogeneous Aggregate with a Base Type of a single- or double-precision floating-point type with one to four Elements can be returned using register s0-s3 and d0-d3.

This patch updates ABISysV_arm::GetReturnValueObjectImpl to handle these cases.

ReturnValueTestCase passes on armhf based linux target after applying this patch.

[labath]

Do we have a test for this?

If not, it sounds like it would be an easy thing to add one.

[labath]

Nevermind, I now see you mention TestReturnValue. :/

[tberghammer]

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

581–591 ↗

(On Diff #47152)

I think if we are returning an aggregate containing 1 32 bit float (byte_size == 4) then it will be returned in s0 while your current implementation expect it to be returned in r0. I am not sure about it but please take a look.

If this is the case I would suggest to order the conditions the following way to decrees the nesting level of the ifs:

if (IsArmHardFloat(thread))
{
    ...
}
else if (byte_size <= 4)
{
    ....
}

597 ↗

(On Diff #47152)

What is float_count means here? Do we have to check its value to decide if we can print the return value (e.g. what happens when float_count == 2)?

612–619 ↗

(On Diff #47152)

I would suggest to use the dwarf register numbers instead of the register names for the lookup:

uint32_t regnum = 0;
if (byte_size == 4)
    regnum = dwarf_s0 + reg_index;
else if (base_byte_size == 8)
    regnum = dwarf_d0 + reg_index;
else
    break;
const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoBy(eRegisterKindDWARF, regnum);

[omjavaid]

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

581–591 ↗

(On Diff #47152)

Yes, you are right. I ll rearrange the code accordingly.

597 ↗

(On Diff #47152)

float_count is just used to fullfil argument requirements. It was already declared in the function so didnt have to define it here.

It returns 1 if type is a standard builtin type (float or double), returns 2 for complex and number of elements for vector types.

We are using homogeneous_count to tell the number of elements in our aggregate type.

612–619 ↗

(On Diff #47152)

Alright I ll make the appropriate change.

[tberghammer]

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

597 ↗

(On Diff #47152)

In that case I think we should check that its value after the call is 1 because we don't want to use the current implementation for vector types (we already check for complex types)

[omjavaid]

updated diff after incorporating suggested corrections.

[tberghammer]

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

592 ↗

(On Diff #47191)

Please check that "float_count == 1" as well (we don't want to use this code path for vector types)

592 ↗

(On Diff #47191)

What the ABI says about homogeneous complex types? The current implementation treats them as in-memory return types what I think is not the right thing to do (I am happy with saying we don't support it but then we should return an empty object)

636 ↗

(On Diff #47191)

If we failed to read out all data (this condition is false) then we will fall through to the non hard float case and will try to read out the return value as an in-memory return value. I think this isn't what we want to do based on the ABI.

[omjavaid]

Updated adding float_count check.

[clayborg]

I am fine as long as tberghammer is happy.

[omjavaid]

Is this good to go ?

[tberghammer]

Can you take a look for the 2 marked comment? I think you handling those error scenarios incorrectly

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

592 ↗

(On Diff #47198)

?

636 ↗

(On Diff #47198)

?

650 ↗

(On Diff #47198)

I think this should be an "else if" to fix the issue I mentioned in line 636

[omjavaid]

I have added an else case for unhandled or error cases.
I have added to Todo for handling complex and vector types. ABI document doesnt say much about how complex will be returned so I am doing a bit investigation with how they are being managed by compiler. I will handle them both in next patch.

[omjavaid]

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

658 ↗

(On Diff #47625)

Homogenous types with elements more than 4 will fall through and will be handled by this even in case of hardfloat ABI so we cannot make it an elseif here.

[tberghammer]

Looks good, thank you for fixing it

lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

else if (compiler_type.IsFloatingPointType(float_count, is_complex))
         {
             // not handled yet
             return return_valobj_sp;
         }
     }
     else if (compiler_type.IsAggregateType())
     {
         size_t byte_size = compiler_type.GetByteSize(&thread);
+        if (IsArmHardFloat(thread))
+        {
+            CompilerType base_type;
+            const uint32_t homogeneous_count = compiler_type.IsHomogeneousAggregate (&base_type);
+
+            if (homogeneous_count > 0 && homogeneous_count <= 4)
+            {
+                if (base_type.IsFloatingPointType(float_count, is_complex))
+                {
+                    if (float_count == 1 && !is_complex)
+                    {
+                        ProcessSP process_sp (thread.GetProcess());
+                        ByteOrder byte_order = process_sp->GetByteOrder();
+
+                        DataBufferSP data_sp (new DataBufferHeap(byte_size, 0));
+                        const size_t base_byte_size = base_type.GetByteSize(nullptr);
+                        uint32_t data_offset = 0;
+
+                        for (uint32_t reg_index = 0; reg_index < homogeneous_count; reg_index++)
+                        {
+                            uint32_t regnum = 0;
+
+                            if (base_byte_size == 4)
+                                regnum = dwarf_s0 + reg_index;
+                            else if (base_byte_size == 8)
+                                regnum = dwarf_d0 + reg_index;
+                            else
+                                break;
+
+                            const RegisterInfo *reg_info = reg_ctx->GetRegisterInfo (eRegisterKindDWARF, regnum);
+                            if (reg_info == NULL)
+                                break;
+
+                            RegisterValue reg_value;
+                            if (!reg_ctx->ReadRegister(reg_info, reg_value))
+                                break;
+
+                            // Make sure we have enough room in "data_sp"
+                            if ((data_offset + base_byte_size) <= data_sp->GetByteSize())
+                            {
+                                Error error;
+                                const size_t bytes_copied = reg_value.GetAsMemoryData (reg_info,
+                                                                                       data_sp->GetBytes() + data_offset,
+                                                                                       base_byte_size,
+                                                                                       byte_order,
+                                                                                       error);
+                                if (bytes_copied != base_byte_size)
+                                    break;
+
+                                data_offset += bytes_copied;
+                            }
+                        }
+
+                        if (data_offset == byte_size)
+                        {
+                            DataExtractor data;
+                            data.SetByteOrder(byte_order);
+                            data.SetAddressByteSize(process_sp->GetAddressByteSize());
+                            data.SetData(data_sp);
+
+                            return ValueObjectConstResult::Create (&thread, compiler_type, ConstString(""), data);
+                        }
+                        else
+                        {   // Some error occurred while getting values from registers
+                            return return_valobj_sp;
+                        }
+
+                    }
+                    else
+                    {   // TODO: Add code to handle complex and vector types.
+                        return return_valobj_sp;
+                    }
+                }
+            }
+        }
+
         if (byte_size <= 4)
         {
             RegisterValue r0_reg_value;
             uint32_t raw_value = reg_ctx->ReadRegisterAsUnsigned(r0_reg_info, 0) & UINT32_MAX;
             value.SetBytes(&raw_value, byte_size);
         }
         else
         {