[Lldb-commits] [PATCH] AArch64 LLDB Support for aarch64-linux-gnu abi support (SysV)

[Tamas Berghammer]

I added a few inline comments but overall it looks good. I think you haven't seen any improvement in the test results because this class isn't used by any of the tests (not sure who else use this class except ThreadElfCore).

P.S.: Next time please upload the diff with full context to make it easier to review.


================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:264-265
@@ +263,4 @@
+    // x0 - x7 contain first 8 simple args
+    if (args.size() > 8) // TODO handle more than 6 arguments
+        return false;
+
----------------
typo: 6 or 8 arguments are handled?

================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:353-360
@@ +352,10 @@
+                        {
+                            case 0: reg_info = reg_ctx->GetRegisterInfoByName("x0"); break;
+                            case 1: reg_info = reg_ctx->GetRegisterInfoByName("x1"); break;
+                            case 2: reg_info = reg_ctx->GetRegisterInfoByName("x2"); break;
+                            case 3: reg_info = reg_ctx->GetRegisterInfoByName("x3"); break;
+                            case 4: reg_info = reg_ctx->GetRegisterInfoByName("x4"); break;
+                            case 5: reg_info = reg_ctx->GetRegisterInfoByName("x5"); break;
+                            case 6: reg_info = reg_ctx->GetRegisterInfoByName("x6"); break;
+                            case 7: reg_info = reg_ctx->GetRegisterInfoByName("x7"); break;
+                        }
----------------
Please use reg_ctx->GetRegisterInfo() if possible (e.g.: based on dwarf numbers) as it don't have to iterate over all registers in the regiters contexs

================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:454
@@ +453,3 @@
+                {
+                    const RegisterInfo *x0_info = reg_ctx->GetRegisterInfoByName("x0", 0);
+                    if (byte_size <= 8)
----------------
Please use reg_ctx->GetRegisterInfo() if possible (same for other cases)

================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:635
@@ +634,3 @@
+        {
+            // Volatile registers: x0-x18, x30 (lr)
+            // Return false for the non-volatile gpr regs, true for everything else
----------------
Please fix the comment to match with the one in line 666 (I don't know witch one is the correct)

================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:667-668
@@ +666,4 @@
+                case '3':                             // x30 aka lr treat as non-volatile
+                    if (name[2] == '0')
+                      return false;
+                default:
----------------
Please add a comment about fall through or prevent it with a return

================
Comment at: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.h:27-48
@@ +26,24 @@
+    
+    virtual size_t 
+    GetRedZoneSize () const;
+    
+    virtual bool
+    PrepareTrivialCall (lldb_private::Thread &thread,
+                        lldb::addr_t sp,
+                        lldb::addr_t functionAddress,
+                        lldb::addr_t returnAddress,
+                        llvm::ArrayRef<lldb::addr_t> args) const;
+
+    virtual bool
+    GetArgumentValues (lldb_private::Thread &thread,
+                       lldb_private::ValueList &values) const;
+    
+    virtual bool
+    CreateFunctionEntryUnwindPlan (lldb_private::UnwindPlan &unwind_plan);
+    
+    virtual bool
+    CreateDefaultUnwindPlan (lldb_private::UnwindPlan &unwind_plan);
+    
+    virtual bool
+    RegisterIsVolatile (const lldb_private::RegisterInfo *reg_info);
+
----------------
Please remove virtual and add override markers where applicable (in the entry file)

http://reviews.llvm.org/D8538

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/