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

[Tamas Berghammer via lldb-commits]

tberghammer 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;
----------------
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)
{
    ....
}

```

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

================
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)
----------------
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);
```


http://reviews.llvm.org/D16975