Objective-C: fix class_ro layout for non-LP64

  Fix class_ro layout for non-LP64.  On LP64, the requisite padding is added at a
lower level.  For non-LP64, this fixes binary compatibility with clang-built
classes/runtimes.

Tested by examining the generated assembly for a class_ro in both cases (and in 
the case of clang), for both x86_64 (64-bit pointers) and AVR (16-bit pointers).
Tested by running a program on AVR with a GCC-built class using a clang-built 
Objective-C runtime.  Tested by running a program on x86_64/Darwin with a GCC-
built class and the clang-built system Objective-C runtime.

Patch also available at:
<https://github.com/mhjacobson/gcc/commit/917dc8bb2f3265c2ca899ad750c5833b0161a11e>

I don't have commit access, so if this patch is suitable, I'd need someone else 
to commit it for me.  Thanks.


gcc/objc/ChangeLog:

2021-09-21  Matt Jacobson  <mhjacobson@me.com>

	* objc-next-runtime-abi-02.c (struct class_ro_t): Remove explicit alignment 
	padding.
	(build_v2_class_templates): Remove explicit alignment padding.
	(build_v2_class_ro_t_initializer): Adjust initializer.
  

Hi Matt,

thanks for the patch.

> On 21 Sep 2021, at 23:29, Matt Jacobson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> Fix class_ro layout for non-LP64.  

> On LP64, the requisite padding is added at a lower level.  

However, the behaviour is changed - the existing implementation is explicit about the fields and
clears the reserved ones (and, ISTR, that was based on what the gcc-4.2.1 compiler did).

(as an aside, in general, I have to test changes in the runtime across the range of supported
 Darwin versions, since there have been clang changes too).

> For non-LP64, this fixes binary compatibility with clang-built classes/runtimes.

> Tested by examining the generated assembly for a class_ro in both cases (and in 
> the case of clang), for both x86_64 (64-bit pointers) and AVR (16-bit pointers).
> Tested by running a program on AVR with a GCC-built class using a clang-built 
> Objective-C runtime.  Tested by running a program on x86_64/Darwin with a GCC-
> built class and the clang-built system Objective-C runtime.
> 
> Patch also available at:
> <https://github.com/mhjacobson/gcc/commit/917dc8bb2f3265c2ca899ad750c5833b0161a11e>
> 
> I don't have commit access, so if this patch is suitable, I'd need someone else 
> to commit it for me.  Thanks.

how about we keep the LP64 behaviour unchanged and amend the comments and ifdefs as below,
does this work for your case?
(typed into email and thus untested).

thanks
Iain

> gcc/objc/ChangeLog:
> 
> 2021-09-21  Matt Jacobson  <mhjacobson@me.com>
> 
> 	* objc-next-runtime-abi-02.c (struct class_ro_t): Remove explicit alignment 
> 	padding.
> 	(build_v2_class_templates): Remove explicit alignment padding.
> 	(build_v2_class_ro_t_initializer): Adjust initializer.
> 
> 
> diff --git a/gcc/objc/objc-next-runtime-abi-02.c b/gcc/objc/objc-next-runtime-abi-02.c
> index 42645e22316..c3af369ff0d 100644
> --- a/gcc/objc/objc-next-runtime-abi-02.c
> +++ b/gcc/objc/objc-next-runtime-abi-02.c
> @@ -632,9 +632,7 @@ struct class_ro_t
>     uint32_t const flags;
>     uint32_t const instanceStart;
>     uint32_t const instanceSize;\

> -#ifdef __LP64__
> -    uint32_t const reserved;
> -#endif
> +    // [32 bits of reserved space here on LP64 platforms]
^revert this

>     const uint8_t * const ivarLayout;
>     const char *const name;
>     const struct method_list_t * const baseMethods;
> @@ -677,11 +675,6 @@ build_v2_class_templates (void)
>   /* uint32_t const instanceSize; */
>   add_field_decl (integer_type_node, "instanceSize", &chain);
> 
#ifdef __LP64__
/* For compatibility with existing implementations of the 64 bit NeXT
   library, explicitly describe reserved fileds used for alignment
   padding.  */

>   /* uint32_t const reserved; */
>   add_field_decl (integer_type_node, "reserved", &chain);
#endif
> 
>   /* const uint8_t * const ivarLayout; */
>   cnst_strg_type = build_pointer_type (unsigned_char_type_node);
>   add_field_decl (cnst_strg_type, "ivarLayout", &chain);
> @@ -3225,12 +3218,6 @@ build_v2_class_ro_t_initializer (tree type, tree name,
>   CONSTRUCTOR_APPEND_ELT (initlist, NULL_TREE,
> 			  build_int_cst (integer_type_node, instanceSize));
#ifdef __LP64__
/* For compatibility with existing implementations of the 64 bit NeXT
   library, ensure that reserved padding fields are 0-initialized.  */

>   CONSTRUCTOR_APPEND_ELT (initlist, NULL_TREE,
> 			    build_int_cst (integer_type_node, 0));
#endif
> 
>   /* ivarLayout */
>   unsigned_char_star = build_pointer_type (unsigned_char_type_node);
>   if (ivarLayout)
  

Hi Iain,

Thanks for reviewing.  I’m happy to make the suggested changes.  One comment 
inline.

> On Sep 22, 2021, at 2:49 PM, Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
> However, the behaviour is changed - the existing implementation is explicit about the fields and
> clears the reserved ones (and, ISTR, that was based on what the gcc-4.2.1 compiler did).

My original change does in fact clear the reserved bytes on LP64 platforms.  
The padding space compiles down to a `.space` assembler directive, and GNU as 
is documented to fill that space with zeros.  So the reserved bits are indeed 
cleared.

However, I understand the argument that this is too implicit, in that the C 
standard makes no guarantee about the contents of padding bytes.  So future 
standard-conforming changes to GCC *could* cause that space to be filled with 
other values (however unlikely that may actually be).

(Of course, clang -- which also does not explicitly declare this field --
essentially faces this same theoretical peril...)

One problem with the proposed diff: `__LP64__` there refers to the host, not 
the target.  What's the right way to refer to the LP64-ness of the target?  I 
see `TARGET_LP64`, but it's only defined for Intel.  I'm using it below (and 
backstopping it to zero), but I'm not sure if that's correct.  Note that it's a 
run-time-of-compiler (not build-time-of-compiler) check.

===

Here's v2.

<https://github.com/mhjacobson/gcc/commit/8193903a1d5a1569a6799174e13cb22925f1f428>


gcc/objc/ChangeLog:

2021-09-26  Matt Jacobson  <mhjacobson@me.com>

	* objc-next-runtime-abi-02.c (build_v2_class_templates): Remove explicit
	padding on non-LP64.
	(build_v2_class_ro_t_initializer): Remove initialization of explicit padding on
	non-LP64.


diff --git a/gcc/objc/objc-next-runtime-abi-02.c b/gcc/objc/objc-next-runtime-abi-02.c
index 42645e22316..22d5232614d 100644
--- a/gcc/objc/objc-next-runtime-abi-02.c
+++ b/gcc/objc/objc-next-runtime-abi-02.c
@@ -85,6 +85,10 @@ along with GCC; see the file COPYING3.  If not see
 
 #define OBJC2_CLS_HAS_CXX_STRUCTORS	0x0004L
 
+#ifndef TARGET_LP64
+#define TARGET_LP64 0
+#endif
+
 enum objc_v2_tree_index
 {
   /* Templates.  */
@@ -677,10 +681,12 @@ build_v2_class_templates (void)
   /* uint32_t const instanceSize; */
   add_field_decl (integer_type_node, "instanceSize", &chain);
 
-  /* This ABI is currently only used on m64 NeXT.  We always
-     explicitly declare the alignment padding.  */
+  /* For compatibility with existing implementations of the 64-bit NeXT v2
+     ABI, explicitly declare reserved fields that otherwise would be filled
+     with alignment padding. */
   /* uint32_t const reserved; */
-  add_field_decl (integer_type_node, "reserved", &chain);
+  if (TARGET_LP64)
+    add_field_decl (integer_type_node, "reserved", &chain);
 
   /* const uint8_t * const ivarLayout; */
   cnst_strg_type = build_pointer_type (unsigned_char_type_node);
@@ -3225,10 +3231,12 @@ build_v2_class_ro_t_initializer (tree type, tree name,
   CONSTRUCTOR_APPEND_ELT (initlist, NULL_TREE,
 			  build_int_cst (integer_type_node, instanceSize));
 
-  /* This ABI is currently only used on m64 NeXT.  We always
-     explicitly declare the alignment padding.  */
-  /* reserved, pads alignment.  */
-  CONSTRUCTOR_APPEND_ELT (initlist, NULL_TREE,
+  /* For compatibility with existing implementations of the 64-bit NeXT v2
+     ABI, explicitly zero-fill reserved fields that otherwise would be filled
+     with alignment padding. */
+  /* reserved */
+  if (TARGET_LP64)
+    CONSTRUCTOR_APPEND_ELT (initlist, NULL_TREE,
 			    build_int_cst (integer_type_node, 0));
 
   /* ivarLayout */
  

Hi Matt

> On 26 Sep 2021, at 09:10, Matt Jacobson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 

> Thanks for reviewing.  I’m happy to make the suggested changes.  One comment 
> inline.
> 
>> On Sep 22, 2021, at 2:49 PM, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> However, the behaviour is changed - the existing implementation is explicit about the fields and
>> clears the reserved ones (and, ISTR, that was based on what the gcc-4.2.1 compiler did).
> 
> My original change does in fact clear the reserved bytes on LP64 platforms.  
> The padding space compiles down to a `.space` assembler directive, and GNU as 
> is documented to fill that space with zeros.  So the reserved bits are indeed 
> cleared.

Apologies for iterating here ..

So it does, indeed, seem that clang does:

target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128”
<snip>
%struct._class_ro_t = type { i32, i32, i32, i8*, i8*, %struct.__method_list_t*, %struct._objc_protocol_list*, 

(for -m64 -fobjc-runtime=macosx -emit-llvm -S)

and,...

target datalayout = "e-m:o-p:32:32-f64:32:64-f80:128-n8:16:32-S128”
<snip>
%struct._class_ro_t = type { i32, i32, i32, i8*, i8*, %struct.__method_list_t*, %struct._objc_protocol_list*, 

(for -m32 -fobjc-runtime=macosx -emit-llvm -S), even if the target platform will fail to link that (at least macOS will).

So the padding is not explicit in this and will accommodate whatever the target uses.

Now - to change that such that the previously reserved field could be used on platforms which don’t have implicit padding would be an ABI break, so isn’t going to happen by accident (or at all, I imagine).

Since we produce .space for Darwin9 and Darwin10 (where the system compiler is GCC) I am no longer concerned about dropping the reserved field.

This is a long-winded way of saying that I now think your original approach is fine, and will put that patch into my testing queue (spot checks only so far) and apply if all goes without regression.

thanks for the patch,
Iain