[Lldb-commits] [PATCH] D16975: Handle floating-point type homogeneous aggregate return values in ABISysV_arm

[Muhammad Omair Javaid via lldb-commits]

omjavaid added inline comments.

================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:581-591
@@ -580,10 +580,13 @@
     {
         size_t byte_size = compiler_type.GetByteSize(&thread);
         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
         {
+            if (IsArmHardFloat(thread))
+            {
+                CompilerType base_type;
----------------
tberghammer wrote:
> 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)
> {
>     ....
> }
> 
> ```
Yes, you are right. I ll rearrange the code accordingly.



================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:597
@@ +596,3 @@
+                {
+                    if (base_type.IsFloatingPointType(float_count, is_complex))
+                    {
----------------
tberghammer wrote:
> 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)?
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.

================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:612-619
@@ +611,10 @@
+
+                                if (base_byte_size == 4)
+                                    ::snprintf (reg_name, sizeof(reg_name), "s%u", reg_index);
+                                else if (base_byte_size == 8)
+                                    ::snprintf (reg_name, sizeof(reg_name), "d%u", reg_index);
+                                else
+                                    break;
+
+                                const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoByName(reg_name, 0);
+                                if (reg_info == NULL)
----------------
tberghammer wrote:
> 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);
> ```
Alright I ll make the appropriate change.


http://reviews.llvm.org/D16975