[MCU] PR26438: Fix assertion failure on function returning an empty struct or union

[• d.zobnin.bugzilla]

Summary

Add missing check for zero-sized type for MCU ABI in order to return zero-sized types (empty structs and unions) via memory.

[hjl.tools]

Why should empty structs and unions) be return in memory? I don't remember
I called for it in MCU psABI.

[rnk]

If the ABI is still open to small changes and clarifications, can we make sure that C and C++ empty structs are passed the same way? Unfortunately, on normal x86_64 SysV GCC doesn't pass them the same way and Clang has to follow suit.

[DavidKreitzer]

Denis, can you please explain your rationale for choosing to return 0-sized aggregates in memory for MCU? It doesn't match gcc behavior. For example,

int g;
union U {} u;
union U f(int a, int b, int c) { g = a + b + c; return u; }
void f1() { f(1, 2, 3); }

Produces this, where all three incoming arguments are in registers. With your patch, %eax will be used for the return value pointer.

f:
	addl	%eax, %edx
	subl	$4, %esp
	addl	%edx, %ecx
	movl	%ecx, g
	popl	%eax
	ret

(Using i586-intel-elfiamcu-gcc 5.2.1 20150820)

HJ, even though a strict reading of the ABI suggests that a 0-size aggregate should be returned in register, it would be a good idea to make this more explicit in the ABI here, by adding the underlined text, for example.

Returning Values
Table 2.4 lists the location used to return a value for each fundamental data type.
Aggregate types (structs and unions) are returned as follows:
• Short aggregate types no larger than 8 bytes, including 0-length aggregate types, are returned in %edx:%eax.
The most significant 32 bits are returned in %edx. The least significant 32
bits are returned in %eax.
• Other aggregate types are returned in memory.

[hjl.tools]

I am planning to update i386, x86-64 and IA MCU psABIs to address how to
pass and return C++ empty class after resolving:

https://llvm.org/bugs/show_bug.cgi?id=26337

by updating C++ ABI to explicitly pass and return C++ empty class like C empty
structure.

[• d.zobnin.bugzilla]

Thank you for the review!

David, I started from the assertion failure and was going up the call stack trying to understand what should be changed to make the assertion go away. In shouldReturnTypeInRegister() function there is isRegisterSize(Size) check for i386, which returns false for 0-sized types, so I naively assumed that for MCU we missed the 0-case check. I must have read the ABI more carefully...

While searching for a correct solution for this, I saw that 'void' return type is handled in classifyReturnType() by returning ABIArgInfo::getIgnore(). According to the comments:

/// Ignore - Ignore the argument (treat as void). Useful for void and
/// empty structs.
Ignore,

Does it mean that we can treat empty structs and unions in return type as void in this case?

[DavidKreitzer]

From an MCU ABI perspective, yes, returning an empty struct is equivalent to returning void.

[hjl.tools]

Empty struct/union should be passed and returned as void for all IA psABs.

[• d.zobnin.bugzilla]

Thank you for your reply!

Added a check for empty struct/union to treat them as void, updated the test accordingly.

Please take a look.

[rnk]

lgtm

[alexanderkyte]

This change is causing mono's native interop with C code compiled from clang to break. After some investigation, we're tracked bitcode differences between tests that fail versus not. When we upgrade the compiler to Xcode 8.3, we see that code generated to call functions returning empty structs will now generate code that crashes.

This seems to be us trying to pass arguments assuming that clang exhibits the same behavior it used to. It is easy enough for us to generate code for what clang does now, but that code would break if forced to interact with C compiled by gcc and older clangs.

Is there a way for callers of clang-emitted code to detect which ABI clang is going to use when returning an empty struct? This ABI change does not seem to admit backwards compatibility.

xcode73's clang -emit-llvm generates:

• define void @ret(%struct.AStruct* noalias sret %agg.result, i32 %a) #0 {

while xcode83's clang generates:

• define void @ret(i32) #0 {

cfe/trunk/lib/CodeGen/TargetInfo.cpp

 }
 
 /// isEmptyRecord - Return true iff a structure contains only empty
 /// fields. Note that a structure with a flexible array member is not
 /// considered empty.
 static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)
-    return 0;
+    return false;
   const RecordDecl *RD = RT->getDecl();
   if (RD->hasFlexibleArrayMember())
     return false;
 
   // If this is a C++ record, check the bases first.
   if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD))
     for (const auto &I : CXXRD->bases())
       if (!isEmptyRecord(Context, I.getType(), true))
if (const RecordType *RT = RetTy->getAs<RecordType>()) {
       if (RT->getDecl()->hasFlexibleArrayMember())
         return getIndirectReturnResult(RetTy, State);
     }
 
     // If specified, structs and unions are always indirect.
     if (!IsRetSmallStructInRegABI && !RetTy->isAnyComplexType())
       return getIndirectReturnResult(RetTy, State);
 
+    // Ignore empty structs/unions.
+    if (isEmptyRecord(getContext(), RetTy, true))
+      return ABIArgInfo::getIgnore();
+
     // Small structures which are register sized are generally returned
     // in a register.
     if (shouldReturnTypeInRegister(RetTy, getContext())) {
       uint64_t Size = getContext().getTypeSize(RetTy);
 
       // As a special-case, if the struct is a "single-element" struct, and
       // the field is of type "float" or "double", return it in a
       // floating-point register. (MSVC does not apply this special case.)

cfe/trunk/test/CodeGen/mcu-struct-return.c

+// RUN: %clang_cc1 -triple i386-pc-elfiamcu -emit-llvm %s -o - | FileCheck %s
+
+// Structure that is more than 8 byte.
+struct Big {
+  double a[10];
+};
+
+// Empty union with zero size must be returned as void.
+union U1 {
+} u1;
+
+// Too large union (80 bytes) must be returned via memory.
+union U2 {
+  struct Big b;
+} u2;
+
+// Must be returned in register.
+union U3 {
+  int x;
+} u3;
+
+// Empty struct with zero size, must be returned as void.
+struct S1 {
+} s1;
+
+// Must be returend in register.
+struct S2 {
+  int x;
+} s2;
+
+// CHECK: [[UNION1_TYPE:%.+]] = type {}
+// CHECK: [[UNION2_TYPE:%.+]] = type { [[STRUCT_TYPE:%.+]] }
+// CHECK: [[STRUCT_TYPE]] = type { [10 x double] }
+// CHECK: [[UNION3_TYPE:%.+]] = type { i32 }
+// CHECK: [[STRUCT1_TYPE:%.+]] = type {}
+// CHECK: [[STRUCT2_TYPE:%.+]] = type { i32 }
+
+union U1 foo1() { return u1; }
+union U2 foo2() { return u2; }
+union U3 foo3() { return u3; }
+struct S1 bar1() { return s1; }
+struct S2 bar2() { return s2; }
+struct S1 bar3(union U1 u) { return s1; }
+// CHECK: define void @foo1()
+// CHECK: define void @foo2([[UNION2_TYPE]]* noalias sret %{{.+}})
+// CHECK: define i32 @foo3()
+// CHECK: define void @bar1()
+// CHECK: define i32 @bar2()
+// CHECK: define void @bar3()
+
+void run() {
+  union U1 x1 = foo1();
+  union U2 x2 = foo2();
+  union U3 x3 = foo3();
+  struct S1 y1 = bar1();
+  struct S2 y2 = bar2();
+  struct S1 y3 = bar3(x1);
+
+  // CHECK: [[X1:%.+]] = alloca [[UNION1_TYPE]]
+  // CHECK: [[X2:%.+]] = alloca [[UNION2_TYPE]]
+  // CHECK: [[X3:%.+]] = alloca [[UNION3_TYPE]]
+  // CHECK: [[Y1:%.+]] = alloca [[STRUCT1_TYPE]]
+  // CHECK: [[Y2:%.+]] = alloca [[STRUCT2_TYPE]]
+  // CHECK: call void @foo1()
+  // CHECK: call void @foo2([[UNION2_TYPE]]* sret [[X2]])
+  // CHECK: {{.+}} = call i32 @foo3()
+  // CHECK: call void @bar1()
+  // CHECK: {{.+}} = call i32 @bar2()
+  // CHECK: call void @bar3()
+}