ABISysV_arm64: compute return value for large vectors correctly

[labath]

Summary

Arm64 Procedure Call Standard specifies than only vectors up to 16 bytes
are stored in v0 (which makes sense, as that's the size of the
register). 32-byte vector types are passed as regular structs via x8
pointer. Treat them as such.

This fixes TestReturnValue for arm64-clang. I also split the test case
into two so I can avoid the if(gcc) line, and annotate each test
instead. (It seems the vector type tests fail with gcc only when
targetting x86 arches).

[tberghammer]

I am a bit confused by the correlation between your change and commit message. In the commit message you say that 32 byte structs are passed as x8 pointers but the implementation of LoadValueFromConsecutiveGPRRegisters seems to read it out from the v0-v8 registers for vectors of up to 8 elements independently of there size. Also based on that code I have the suspicion that the first branch (where byte_size <= 16) is not actually used or necessary and also I don't see anything in the ABI documentation indicating otherwise (it would be a pretty crazy ABI if they say that if you have 4 double then passed in a single 32 byte register while if you have 8 double then passed in 8 different 32 byte registers). Can you make sure that branch is necessary (e.g. removing it breaks at least 1 test)?

packages/Python/lldbsuite/test/functionalities/return-value/TestReturnValue.py

194 ↗

(On Diff #97676)

(nit): Not needed

[labath]

In D32813#746012, @tberghammer wrote:

I am a bit confused by the correlation between your change and commit message. In the commit message you say that 32 byte structs

I mean 32-byte vectors. I.e. variables declared as float foo __attribute__((__vector_size__(32)));

are passed as x8 pointers but the implementation of LoadValueFromConsecutiveGPRRegisters seems to read it out from the v0-v8 registers for vectors of up to 8 elements independently of there size.

LoadValueFromConsecutiveGPRRegisters does this for "homogeneous structs", which is a different concept than vector: """Note that for short-vector types the fundamental types are 64-bit vector and 128-bit vector; the type of the elements in the short vector does not form part of the test for homogeneity. """

So an 8-byte and 16-byte vector (and probably structures containing them) are passed in v0..v7 registers. However, a 32-byte vector is not a short-vector type, nor a homogeneous aggregate, so it is passed as a generic struct, via the v8 pointer.

Also based on that code I have the suspicion that the first branch (where byte_size <= 16) is not actually used or necessary and also I don't see anything in the ABI documentation indicating otherwise (it would be a pretty crazy ABI if they say that if you have 4 double then passed in a single 32 byte register while if you have 8 double then passed in 8 different 32 byte registers). Can you make sure that branch is necessary (e.g. removing it breaks at least 1 test)?

Removing the branch makes the test for 8 and 16-byte vectors fail.

[tberghammer]

Makes sense. Thank you for the explanation (I assumed homogeneous aggregate and vector are the same).

lldb/trunk/packages/Python/lldbsuite/test/functionalities/return-value/TestReturnValue.py

def test_with_python(self):
             "return_one_short_one_double_one_short")
         self.return_and_test_struct_value("return_one_float_one_int_one_float")
         self.return_and_test_struct_value("return_two_float")
         # I am leaving out the packed test until we have a way to tell CLANG
         # about alignment when reading DWARF for packed types.
         #self.return_and_test_struct_value ("return_one_int_one_double_packed")
         self.return_and_test_struct_value("return_one_int_one_long")
 
-        # icc and gcc don't support this extension.
-        if self.getCompiler().endswith('clang'):
-            self.return_and_test_struct_value("return_vector_size_float32_8")
-            self.return_and_test_struct_value("return_vector_size_float32_16")
-            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")
-            self.return_and_test_struct_value(
-                "return_ext_vector_size_float32_8")
+    @expectedFailureAll(oslist=["freebsd"], archs=["i386"])
+    @expectedFailureAll(oslist=["macosx"], archs=["i386"], bugnumber="<rdar://problem/28719652>")
+    @expectedFailureAll(
+        oslist=["linux"],
+        compiler="clang",
+        compiler_version=[
+            "<=",
+            "3.6"],
+        archs=["i386"])
+    @expectedFailureAll(
+        bugnumber="llvm.org/pr25785",
+        hostoslist=["windows"],
+        compiler="gcc",
+        archs=["i386"],
+        triple='.*-android')
+    @expectedFailureAll(compiler=["gcc"], archs=["x86_64", "i386"])
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24778")
+    def test_vector_values(self):
+        self.build()
+        exe = os.path.join(os.getcwd(), "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_ext_vector_size_float32_2")
+        self.return_and_test_struct_value("return_ext_vector_size_float32_4")
+        self.return_and_test_struct_value("return_ext_vector_size_float32_8")
 
     def return_and_test_struct_value(self, func_name):
         """Pass in the name of the function to return from - takes in value, returns value."""
 
         # Set the breakpoint, run to it, finish out.
         bkpt = self.target.BreakpointCreateByName(func_name)
         self.assertTrue(bkpt.GetNumResolvedLocations() > 0)
 

lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp

if (type_flags & eTypeIsInteger || type_flags & eTypeIsPointer) {
           }
         }
       }
     }
 
     if (success)
       return_valobj_sp = ValueObjectConstResult::Create(
           thread.GetStackFrameAtIndex(0).get(), value, ConstString(""));
-  } else if (type_flags & eTypeIsVector) {
+  } else if (type_flags & eTypeIsVector && byte_size <= 16) {
     if (byte_size > 0) {
       const RegisterInfo *v0_info = reg_ctx->GetRegisterInfoByName("v0", 0);
 
       if (v0_info) {
-        if (byte_size <= v0_info->byte_size) {
-          std::unique_ptr<DataBufferHeap> heap_data_ap(
-              new DataBufferHeap(byte_size, 0));
-          const ByteOrder byte_order = exe_ctx.GetProcessRef().GetByteOrder();
-          RegisterValue reg_value;
-          if (reg_ctx->ReadRegister(v0_info, reg_value)) {
-            Error error;
-            if (reg_value.GetAsMemoryData(v0_info, heap_data_ap->GetBytes(),
-                                          heap_data_ap->GetByteSize(),
-                                          byte_order, error)) {
-              DataExtractor data(DataBufferSP(heap_data_ap.release()),
-                                 byte_order,
-                                 exe_ctx.GetProcessRef().GetAddressByteSize());
-              return_valobj_sp = ValueObjectConstResult::Create(
-                  &thread, return_compiler_type, ConstString(""), data);
-            }
-          }
-        }
-      }
-    }
-  } else if (type_flags & eTypeIsStructUnion || type_flags & eTypeIsClass) {
+                                        heap_data_ap->GetByteSize(), byte_order,
+                                        error)) {
+            DataExtractor data(DataBufferSP(heap_data_ap.release()), byte_order,
+  } else if (type_flags & eTypeIsStructUnion || type_flags & eTypeIsClass ||
+             (type_flags & eTypeIsVector && byte_size > 16)) {
     DataExtractor data;
 
     uint32_t NGRN = 0; // Search ABI docs for NGRN
     uint32_t NSRN = 0; // Search ABI docs for NSRN
     const bool is_return_value = true;
     if (LoadValueFromConsecutiveGPRRegisters(
             exe_ctx, reg_ctx, return_compiler_type, is_return_value, NGRN, NSRN,
             data)) {