Don't try to get memory returns for ABIMacOSX_arm64

[jingham]

Summary

The ARM64 ABI does not require restoring the memory return register value (x8) on exit from a function, so lldb can't reliably fetch memory return values. We were still trying and that caused a bunch of tests in TestReturnValue to fail. I changed the ABI code to not try to fetch return values for memory returns. I also changed the tests to assert that for all the cases of memory returns we return no value on Darwin arm64, since we shouldn't be guessing...

[jasonmolenda]

Ach, that's unfortunate. So a caller can allocate space for the return object, pass that address to a callee in x8, but when callee returns, x8 has been overwritten. Agree that this is the only right way to behave.

[labath]

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

[jingham]

In D121348#3373170, @labath wrote:

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

The ABI docs say "you aren't required to restore the value of x8 on function return"... Not all that surprising everybody took the speed over debuggability option.

[JDevlieghere]

LGMT with the SysV ABI updated as well.

[jingham]

At Pavel's request I extended this to the SysV_arm64 ABI. Since I don't want to have to guess which arch's are which ABI's, I exposed the ABI plugin's name and then used that in the test.

[JDevlieghere]

lldb/source/API/SBTarget.cpp

1596–1597 ↗

(On Diff #414765)

I think ConstString has a ctor that takes a StringRef so you should be able to do ConstString abi_name(target_sp->GetABIName()) directly.

[labath]

In D121348#3373396, @jingham wrote:

In D121348#3373170, @labath wrote:

I don't think this is a peculiarity of the darwin ABI. I'm pretty sure i've ran into this on the "sysv" ABI as well.

If we're going to disallow this, then I think we should do the same for ABISysV_arm64 as well -- it looks like an identical change to ABISysV_arm64 should suffice.

The ABI docs say "you aren't required to restore the value of x8 on function return"... Not all that surprising everybody took the speed over debuggability option.

Yeah, I'm actually pretty fine with this. I dread the day when they start routinely (I think it already happens in a limited fashion) start using ABI's made up on the spot for translation-unit-local functions.

In D121348#3376345, @jingham wrote:

At Pavel's request I extended this to the SysV_arm64 ABI. Since I don't want to have to guess which arch's are which ABI's, I exposed the ABI plugin's name and then used that in the test.

Heh.. one of my reasons for requesting this was to avoid complex logic in the test -- I thought you'd just say that the functionality is not available on all arm64 architectures. Though I suppose having the plugin name around could be useful in general...

lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp

while (data_offset < *byte_size) {
         break;
       data_offset += bytes_copied;
       bytes_left -= bytes_copied;
       ++NGRN;
     }
   } else {
     const RegisterInfo *reg_info = nullptr;
     if (is_return_value) {
-      // We are assuming we are decoding this immediately after returning from
-      // a function call and that the address of the structure is in x8
-      reg_info = reg_ctx->GetRegisterInfoByName("x8", 0);
+      // The Darwin arm64 ABI doesn't write the return location back to x8
+      // before returning from the function the way the x86_64 ABI does.  So
+      // we can't reconstruct stack based returns on exit from the function:
+      return false;
     } else {
       // We are assuming we are stopped at the first instruction in a function
       // and that the ABI is being respected so all parameters appear where
       // they should be (functions with no external linkage can legally violate
       // the ABI).
       if (NGRN >= 8)
         return false;
 

lldb/test/API/functionalities/return-value/TestReturnValue.py

class ReturnValueTestCase(TestBase):
     def affected_by_pr33042(self):
         return ("clang" in self.getCompiler() and self.isAArch64() and
             self.getPlatform() == "linux")
 
     def affected_by_pr44132(self):
         return (self.getArchitecture() in ["aarch64", "arm"] and
                 self.getPlatform() in ["freebsd", "linux"])
 
-    # ABIMacOSX_arm(64) can't fetch simple values inside a structure
-    def affected_by_radar_34562999(self):
+    # ABIMacOSX_arm(64) doesn't restore the storage value for memory returns on function
+    # exit, so lldb shouldn't attempt to fetch memory for those return types, as there is
+    # no easy way to guarantee that they will be correct.  This is calculable, but I don't
+    # want to have to encode the arm64 ABI here, so I just listed the ones that we shouldn't
+    # be reporting a value for.
+    darwin_arm_no_return_values = ["return_five_int", "return_one_int_one_double_one_int",
+                                       "return_one_short_one_double_one_short", "return_vector_size_float32_32",
+                                       "return_ext_vector_size_float32_8"]
+    def should_report_return_value(self, func_name):
         arch = self.getArchitecture().lower()
-        return arch in ['arm64', 'arm64e', 'armv7', 'armv7k'] and self.platformIsDarwin()
+        if arch in ['arm64', 'arm64e', 'armv7', 'armv7k'] and self.platformIsDarwin():
+            return not func_name in self.darwin_arm_no_return_values
+        return True
 
     @expectedFailureAll(oslist=["freebsd"], archs=["i386"],
                         bugnumber="llvm.org/pr48376")
     @expectedFailureAll(oslist=["macosx"], archs=["i386"], bugnumber="<rdar://problem/28719652>")
     @expectedFailureAll(
         oslist=["linux"],
         compiler="clang",
         compiler_version=[
def test_with_python(self):
         self.assertEquals(fun_name, "outer_float(float)")
 
         #return_value = thread.GetStopReturnValue()
         #self.assertTrue(return_value.IsValid())
         #return_float = float(return_value.GetValue())
 
         #self.assertEqual(in_float, return_float)
 
-        if not self.affected_by_radar_34562999() and not self.affected_by_pr44132():
+        if not self.affected_by_pr44132():
             self.return_and_test_struct_value("return_one_int")
             self.return_and_test_struct_value("return_two_int")
             self.return_and_test_struct_value("return_three_int")
             self.return_and_test_struct_value("return_four_int")
             if not self.affected_by_pr33042():
                 self.return_and_test_struct_value("return_five_int")
-
             self.return_and_test_struct_value("return_two_double")
             self.return_and_test_struct_value("return_one_double_two_float")
             self.return_and_test_struct_value("return_one_int_one_float_one_int")
 
             self.return_and_test_struct_value("return_one_pointer")
             self.return_and_test_struct_value("return_two_pointer")
             self.return_and_test_struct_value("return_one_float_one_pointer")
             self.return_and_test_struct_value("return_one_int_one_pointer")
@expectedFailureAll(
         oslist=["linux"],
         compiler="clang",
         compiler_version=[
             "<=",
             "3.6"],
         archs=["i386"])
     @expectedFailureAll(compiler=["gcc"], archs=["x86_64", "i386"])
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24778")
-    @expectedFailureDarwin(archs=["arm64"]) # <rdar://problem/33976032> ABIMacOSX_arm64 doesn't get structs this big correctly
     def test_vector_values(self):
         self.build()
         exe = self.getBuildArtifact("a.out")
         error = lldb.SBError()
 
         self.target = self.dbg.CreateTarget(exe)
         self.assertTrue(self.target, VALID_TARGET)
 
         main_bktp = self.target.BreakpointCreateByName("main", exe)
         self.assertTrue(main_bktp, VALID_BREAKPOINT)
 
         self.process = self.target.LaunchSimple(
             None, None, self.get_process_working_directory())
         self.assertEqual(len(lldbutil.get_threads_stopped_at_breakpoint(
             self.process, main_bktp)), 1)
-
         self.return_and_test_struct_value("return_vector_size_float32_8")
         self.return_and_test_struct_value("return_vector_size_float32_16")
         if not self.affected_by_pr44132():
             self.return_and_test_struct_value("return_vector_size_float32_32")
         self.return_and_test_struct_value("return_ext_vector_size_float32_2")
         self.return_and_test_struct_value("return_ext_vector_size_float32_4")
         if not self.affected_by_pr44132():
             self.return_and_test_struct_value("return_ext_vector_size_float32_8")
def return_and_test_struct_value(self, func_name):
         # Assuming all these functions step out to main.  Could figure out the caller dynamically
         # if that would add something to the test.
         frame = thread.GetFrameAtIndex(0)
         fun_name = frame.GetFunctionName()
         self.assertEquals(fun_name, "main")
 
         frame = thread.GetFrameAtIndex(0)
         ret_value = thread.GetStopReturnValue()
+        if not self.should_report_return_value(func_name):
+            self.assertFalse(ret_value.IsValid(), "Shouldn't have gotten a value")
+            return
 
         self.assertTrue(ret_value.IsValid())
 
         num_ret_children = ret_value.GetNumChildren()
         self.assertEquals(num_in_children, num_ret_children)
         for idx in range(0, num_ret_children):
             in_child = in_value.GetChildAtIndex(idx)
             ret_child = ret_value.GetChildAtIndex(idx)
             in_child_str = in_child.GetValue()
             ret_child_str = ret_child.GetValue()
 
             self.assertEqual(in_child_str, ret_child_str)