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

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/bindings/interface/SBTarget.i

public:
     GetByteOrder ();
 
     uint32_t
     GetAddressByteSize();
 
     const char *
     GetTriple ();
 
+    const char *
+    GetABIName();
+
     %feature("docstring", "
     Architecture data byte width accessor
 
     :return: The size in 8-bit (host) bytes of a minimum addressable unit from the Architecture's data bus.
 
     ") GetDataByteSize;
     uint32_t
     GetDataByteSize ();

lldb/include/lldb/API/SBTarget.h

public:
   lldb::SBSymbolContextList
   FindCompileUnits(const lldb::SBFileSpec &sb_file_spec);
 
   lldb::ByteOrder GetByteOrder();
 
   uint32_t GetAddressByteSize();
 
   const char *GetTriple();
   
+  const char *GetABIName();
+
   /// Architecture data byte width accessor
   ///
   /// \return
   /// The size in 8-bit (host) bytes of a minimum addressable
   /// unit from the Architecture's data bus
   uint32_t GetDataByteSize();
 
   /// Architecture code byte width accessor

lldb/include/lldb/Target/Target.h

public:
   /// \param[in] module_sp
   ///     A shared pointer reference to the module that checked.
   ///
   /// \return \b true if the module should be excluded, \b false otherwise.
   bool
   ModuleIsExcludedForUnconstrainedSearches(const lldb::ModuleSP &module_sp);
 
   const ArchSpec &GetArchitecture() const { return m_arch.GetSpec(); }
   
+  /// Returns the name of the target's ABI plugin.
+  llvm::StringRef GetABIName() const;
+
   /// Set the architecture for this target.
   ///
   /// If the current target has no Images read in, then this just sets the
   /// architecture, which will be used to select the architecture of the
   /// ExecutableModule when that is set. If the current target has an
   /// ExecutableModule, then calling SetArchitecture with a different
   /// architecture from the currently selected one will reset the
   /// ExecutableModule to that slice of the file backing the ExecutableModule.

lldb/source/API/SBTarget.cpp

if (target_sp) {
     // strings put the string into the string pool once and the strings never
     // comes out
     ConstString const_triple(triple.c_str());
     return const_triple.GetCString();
   }
   return nullptr;
 }
 
+const char *SBTarget::GetABIName() {
+  LLDB_INSTRUMENT_VA(this);
+  
+  TargetSP target_sp(GetSP());
+  if (target_sp) {
+    std::string abi_name(target_sp->GetABIName().str());
+    ConstString const_name(abi_name.c_str());
+    return const_name.GetCString();
+  }
+  return nullptr;
+}
+
 uint32_t SBTarget::GetDataByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
   TargetSP target_sp(GetSP());
   if (target_sp) {
     return target_sp->GetArchitecture().GetDataByteSize();
   }
   return 0;

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/source/Plugins/ABI/AArch64/ABISysV_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 SysV arm64 ABI doesn't require you to write the return location 
+      // back to x8 before returning from the function the way the x86_64 ABI 
+      // does.  It looks like all the users of this ABI currently choose not to
+      // do that, and 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/source/Target/Target.cpp

void Target::Destroy() {
   m_last_created_watchpoint.reset();
   m_search_filter_sp.reset();
   m_image_search_paths.Clear(notify);
   m_stop_hooks.clear();
   m_stop_hook_next_id = 0;
   m_suppress_stop_hooks = false;
 }
 
+llvm::StringRef Target::GetABIName() const {
+  lldb::ABISP abi_sp;
+  if (m_process_sp)
+    abi_sp = m_process_sp->GetABI();
+  if (!abi_sp)
+    abi_sp = ABI::FindPlugin(ProcessSP(), GetArchitecture());
+  if (abi_sp)
+      return abi_sp->GetPluginName();
+  return {};
+}
+
 BreakpointList &Target::GetBreakpointList(bool internal) {
   if (internal)
     return m_internal_breakpoint_list;
   else
     return m_breakpoint_list;
 }
 
 const BreakpointList &Target::GetBreakpointList(bool internal) const {

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):
-        arch = self.getArchitecture().lower()
-        return arch in ['arm64', 'arm64e', 'armv7', 'armv7k'] and self.platformIsDarwin()
+    # ABIMacOSX_arm64 and the SysV_arm64 don'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 a list of the memory
+    # return functions defined in the test file:
+    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):
+        abi = self.target.GetABIName()
+        if not abi in ["SysV-arm64", "ABIMacOSX_arm64", "macosx-arm"]:
+            return True
+        return not func_name in self.arm_no_return_values
 
     @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)

lldb/test/API/python_api/target/TestTargetAPI.py

def test_resolve_file_address(self):
         self.assertTrue(res_file_addr.IsValid())
 
         self.assertEqual(data_section_addr, res_file_addr.file_addr)
 
         data_section2 = res_file_addr.section
         self.assertIsNotNone(data_section2)
         self.assertEqual(data_section.name, data_section2.name)
 
+    def test_get_ABIName(self):
+        d = {'EXE': 'b.out'}
+        self.build(dictionary=d)
+        self.setTearDownCleanup(dictionary=d)
+        target = self.create_simple_target('b.out')
+
+        abi_pre_launch = target.GetABIName()
+        self.assertTrue(len(abi_pre_launch) != 0, "Got an ABI string")
+        
+        breakpoint = target.BreakpointCreateByLocation(
+            "main.c", self.line_main)
+        self.assertTrue(breakpoint, VALID_BREAKPOINT)
+
+        # Put debugger into synchronous mode so when we target.LaunchSimple returns
+        # it will guaranteed to be at the breakpoint
+        self.dbg.SetAsync(False)
+
+        # Launch the process, and do not stop at the entry point.
+        process = target.LaunchSimple(
+            None, None, self.get_process_working_directory())
+        abi_after_launch = target.GetABIName()
+        self.assertEqual(abi_pre_launch, abi_after_launch, "ABI's match before and during run")
+
     def test_read_memory(self):
         d = {'EXE': 'b.out'}
         self.build(dictionary=d)
         self.setTearDownCleanup(dictionary=d)
         target = self.create_simple_target('b.out')
 
         breakpoint = target.BreakpointCreateByLocation(
             "main.c", self.line_main)