[PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Matt Jacobson via Gcc-patches]

Fix protocol list layout for non-LP64.  clang and objc4 both give the `count` 
field as `long`, not `intptr_t`.  Those are the same on LP64, but not 
everywhere.  For non-LP64, this fixes binary compatibility with clang-built 
classes.

This was more complicated than I anticipated, because the relevant frontend 
code in fact had no AST type for `protocol_list_t`, instead emitting protocol 
lists as `protocol_t[]`, with the zeroth element actually being the integer 
count.  That made it nontrivial to change the count to `long`.  With this 
change, there is now a true `protocol_list_t` type in the AST.

Tested multiple ways.  On x86_64/Darwin, I confirmed with a test program that 
protocol conformances by classes, categories, and protocols works.  On AVR, I 
manually inspected the generated assembly to confirm that protocol lists gain 
an extra two bytes of `count`, matching clang.

Thank you for your time.




gcc/objc/ChangeLog:

2021-09-26  Matt Jacobson  

* objc-next-runtime-abi-02.c (enum objc_v2_tree_index): Add new global 
tree.
(static void next_runtime_02_initialize): Initialize protocol list type 
tree.
(struct class_ro_t): Fix type misspelling.
(build_v2_class_templates): Correct type in field declaration.
(build_v2_protocol_templates): Create actual protocol list type tree.
(build_v2_category_template): Correct type in field declaration.
(generate_v2_protocol_list): Emit protocol list count as `long`.
(generate_v2_protocols): Use correct type.
(build_v2_category_initializer): Use correct type.
(build_v2_class_ro_t_initializer): Use correct type.


diff --git a/gcc/objc/objc-next-runtime-abi-02.c 
b/gcc/objc/objc-next-runtime-abi-02.c
index c3af369ff0d..aadf1741676 100644
--- a/gcc/objc/objc-next-runtime-abi-02.c
+++ b/gcc/objc/objc-next-runtime-abi-02.c
@@ -92,6 +92,7 @@ enum objc_v2_tree_index
   OCTI_V2_CAT_TEMPL,
   OCTI_V2_CLS_RO_TEMPL,
   OCTI_V2_PROTO_TEMPL,
+  OCTI_V2_PROTO_LIST_TEMPL,
   OCTI_V2_IVAR_TEMPL,
   OCTI_V2_IVAR_LIST_TEMPL,
   OCTI_V2_MESSAGE_REF_TEMPL,
@@ -130,6 +131,8 @@ enum objc_v2_tree_index
objc_v2_global_trees[OCTI_V2_CAT_TEMPL]
 #define objc_v2_protocol_template \
objc_v2_global_trees[OCTI_V2_PROTO_TEMPL]
+#define objc_v2_protocol_list_template \
+   objc_v2_global_trees[OCTI_V2_PROTO_LIST_TEMPL]
 
 /* struct message_ref_t */
 #define objc_v2_message_ref_template \
@@ -196,7 +199,7 @@ static void build_v2_message_ref_templates (void);
 static void build_v2_class_templates (void);
 static void build_v2_super_template (void);
 static void build_v2_category_template (void);
-static void build_v2_protocol_template (void);
+static void build_v2_protocol_templates (void);
 
 static tree next_runtime_abi_02_super_superclassfield_id (void);
 
@@ -394,9 +397,9 @@ static void next_runtime_02_initialize (void)
build_pointer_type (xref_tag (RECORD_TYPE,
get_identifier ("_prop_list_t")));
 
+  build_v2_protocol_templates ();
   build_v2_class_templates ();
   build_v2_super_template ();
-  build_v2_protocol_template ();
   build_v2_category_template ();
 
   bool fixup_p = flag_next_runtime < USE_FIXUP_BEFORE;
@@ -636,7 +639,7 @@ struct class_ro_t
 const uint8_t * const ivarLayout;
 const char *const name;
 const struct method_list_t * const baseMethods;
-const struct objc_protocol_list *const baseProtocols;
+const struct protocol_list_t *const baseProtocols;
 const struct ivar_list_t *const ivars;
 const uint8_t * const weakIvarLayout;
 const struct _prop_list_t * const properties;
@@ -685,11 +688,9 @@ build_v2_class_templates (void)
   /* const struct method_list_t * const baseMethods; */
   add_field_decl (objc_method_list_ptr, "baseMethods", &chain);
 
-  /* const struct objc_protocol_list *const baseProtocols; */
-  add_field_decl (build_pointer_type
-   (xref_tag (RECORD_TYPE,
-  get_identifier (UTAG_V2_PROTOCOL_LIST))),
- "baseProtocols", &chain);
+  /* const struct protocol_list_t *const baseProtocols; */
+  add_field_decl (build_pointer_type (objc_v2_protocol_list_template),
+ "baseProtocols", &chain);
 
   /* const struct ivar_list_t *const ivars; */
   add_field_decl (objc_v2_ivar_list_ptr, "ivars", &chain);
@@ -763,25 +764,33 @@ build_v2_super_template (void)
  const char ** extended_method_types;
  const char * demangled_name;
  const struct _prop_list_t * class_properties;
-   }
+  };
+
+  struct protocol_list_t
+  {
+long count;
+struct protocol_t protocols[];
+  };
 */
 static void
-build_v2_protocol_template (void)
+build_v2_protocol_templates (void)
 {
-  tree decls, *chain = NULL;
+  tree decls, protolist_pointer

Re: [PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Matt Jacobson via Gcc-patches]

> On Oct 25, 2021, at 5:43 AM, Iain Sandoe  wrote:
> 
> Did you test objective-c++ on Darwin?
> 
> I see a lot of fails of the form:
> Excess errors:
> : error: initialization of a flexible array member [-Wpedantic]

Looked into this.  It’s happening because obj-c++.dg/dg.exp has:

set DEFAULT_OBJCXXFLAGS " -ansi -pedantic-errors -Wno-long-long"

Specifically, the `-pedantic-errors` argument prohibits initialization of a 
flexible array member.  Notably, this flag does *not* appear in objc/dg.exp.

Admittedly I didn’t know that initialization of a FAM was prohibited by the 
standard.  It’s allowed by GCC, though, as documented here:



Is it OK to use a GCC extension this way in the Objective-C frontend?

> For a patch that changes code-gen we should have a test that it produces 
> what’s
> expected (in general, a ‘torture' test would be preferrable so that we can be 
> sure the
> output is as expected for different optimisation levels). 

The output is different only for targets where 
sizeof (long) != sizeof (void *).  Do we have the ability to run “cross” 
torture tests?  Could such a test verify the emitted assembly (like LLVM’s 
FileCheck tests do)?  Or would it need to execute something?

Thanks for your help!

Matt

Re: [PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Iain Sandoe]

> On 7 Nov 2021, at 22:50, Matt Jacobson  wrote:
> 
> 
>> On Oct 25, 2021, at 5:43 AM, Iain Sandoe  wrote:
>> 
>> Did you test objective-c++ on Darwin?
>> 
>> I see a lot of fails of the form:
>> Excess errors:
>> : error: initialization of a flexible array member [-Wpedantic]
> 
> Looked into this.  It’s happening because obj-c++.dg/dg.exp has:
> 
>set DEFAULT_OBJCXXFLAGS " -ansi -pedantic-errors -Wno-long-long"
> 
> Specifically, the `-pedantic-errors` argument prohibits initialization of a 
> flexible array member.  Notably, this flag does *not* appear in objc/dg.exp.

I think -pedantic-errors might be applied at a higher level - if it is not, 
then perhaps that is an omission to rectify.
> 
> Admittedly I didn’t know that initialization of a FAM was prohibited by the 
> standard.  It’s allowed by GCC, though, as documented here:
> 
> 
> 
> Is it OK to use a GCC extension this way in the Objective-C frontend?

Well, I had two thoughts so far;

1/ allow the extension and suppress the warning on the relevant trees.

2/ maybe create a new type “ on the fly “ for each protocol list with a correct 
length [initialising this would not be an error] (since the type is only used 
once it can be local to the use).

— but I haven’t had time to implement either ….

>> For a patch that changes code-gen we should have a test that it produces 
>> what’s
>> expected (in general, a ‘torture' test would be preferrable so that we can 
>> be sure the
>> output is as expected for different optimisation levels). 
> 
> The output is different only for targets where 
> sizeof (long) != sizeof (void *).  
> Do we have the ability to run “cross” 
> torture tests?

For most platforms, the answer is pretty much “no” - GCC is built for a single 
target (unless you have a mutlilib with the necessary difference - which seems 
unlikely in this case).

For Darwin platforms, we can (in most cases) choose a different 
-mmacosx-version-min= from the current host) - but that doesn’t allow us really 
any more and the 32b multilibs have sizeof (long) == sizeof (ptr) so no help 
there.

>  Could such a test verify the emitted assembly (like LLVM’s 
> FileCheck tests do)?  

We have a similar set of possibilities to LLVM/clang (the tree check could be 
the right one here)

* we can verify at some appproriate IR stage [-fdump-tree-x] (check that 
the right trees are emitted)

* we can verify the expected assembler is emitted (scan-asm in dg-tests)

> Or would it need to execute something?

An execute test can be a good way for checking that code-gen is working 
properly (providing, of course, that a wrong codegen would in some way make the 
execute fail - the question is can one construct such a test for this)

sorry this is not an answer - just a list of possible ways forward.
Iain

Re: [PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Matt Jacobson via Gcc-patches]

> On Sep 26, 2021, at 11:45 PM, Matt Jacobson  wrote:
> 
> Fix protocol list layout for non-LP64.  clang and objc4 both give the `count` 
> field as `long`, not `intptr_t`.  Those are the same on LP64, but not 
> everywhere.  For non-LP64, this fixes binary compatibility with clang-built 
> classes.
> 
> This was more complicated than I anticipated, because the relevant frontend 
> code in fact had no AST type for `protocol_list_t`, instead emitting protocol 
> lists as `protocol_t[]`, with the zeroth element actually being the integer 
> count.  That made it nontrivial to change the count to `long`.  With this 
> change, there is now a true `protocol_list_t` type in the AST.
> 
> Tested multiple ways.  On x86_64/Darwin, I confirmed with a test program that 
> protocol conformances by classes, categories, and protocols works.  On AVR, I 
> manually inspected the generated assembly to confirm that protocol lists gain 
> an extra two bytes of `count`, matching clang.
> 
> Thank you for your time.
> 
> 

Friendly ping.  Please let me know if there’s anything I can clarify.

Original mail:


Thanks.

Re: [PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Iain Sandoe via Gcc-patches]

Hi Matt,

sorry for slow response,
unavoidable external factors have been keeping me away from the computer (for 
both $dayjob and and volunteer stuff).

> On 20 Oct 2021, at 04:51, Matt Jacobson via Gcc-patches 
>  wrote:
> 
> 
>> On Sep 26, 2021, at 11:45 PM, Matt Jacobson  wrote:
>> 
>> Fix protocol list layout for non-LP64.  clang and objc4 both give the 
>> `count` 
>> field as `long`, not `intptr_t`.  Those are the same on LP64, but not 
>> everywhere.  For non-LP64, this fixes binary compatibility with clang-built 
>> classes.
>> 
>> This was more complicated than I anticipated, because the relevant frontend 
>> code in fact had no AST type for `protocol_list_t`, instead emitting 
>> protocol 
>> lists as `protocol_t[]`, with the zeroth element actually being the integer 
>> count.  That made it nontrivial to change the count to `long`.  With this 
>> change, there is now a true `protocol_list_t` type in the AST.
>> 
>> Tested multiple ways.  On x86_64/Darwin, I confirmed with a test program 
>> that 
>> protocol conformances by classes, categories, and protocols works.  On AVR, 
>> I 
>> manually inspected the generated assembly to confirm that protocol lists 
>> gain 
>> an extra two bytes of `count`, matching clang.
>> 
>> Thank you for your time.
>> 
>> 
> 
> Friendly ping.  Please let me know if there’s anything I can clarify.

The patch is in my queue (it will not get lost), the rationale seems reasonable,
Iain

Re: [PATCH] Objective-C: fix protocol list count type (pertinent to non-LP64)

[Iain Sandoe]

Hi Matt,

> On 23 Oct 2021, at 09:46, Iain Sandoe via Gcc-patches 
>  wrote:
> 
>> On 20 Oct 2021, at 04:51, Matt Jacobson via Gcc-patches 
>>  wrote:
>> 
>> 
>>> On Sep 26, 2021, at 11:45 PM, Matt Jacobson  wrote:
>>> 
>>> Fix protocol list layout for non-LP64.  clang and objc4 both give the 
>>> `count` 
>>> field as `long`, not `intptr_t`.  Those are the same on LP64, but not 
>>> everywhere.  For non-LP64, this fixes binary compatibility with clang-built 
>>> classes.
>>> 
>>> This was more complicated than I anticipated, because the relevant frontend 
>>> code in fact had no AST type for `protocol_list_t`, instead emitting 
>>> protocol 
>>> lists as `protocol_t[]`, with the zeroth element actually being the integer 
>>> count.  That made it nontrivial to change the count to `long`.  With this 
>>> change, there is now a true `protocol_list_t` type in the AST.
>>> 
>>> Tested multiple ways.  On x86_64/Darwin, I confirmed with a test program 
>>> that 
>>> protocol conformances by classes, categories, and protocols works.

see ** below …

>>>  On AVR, I 
>>> manually inspected the generated assembly to confirm that protocol lists 
>>> gain 
>>> an extra two bytes of `count`, matching clang.

Did you test objective-c++ on Darwin?

I see a lot of fails of the form:
Excess errors:
: error: initialization of a flexible array member [-Wpedantic]


>>> Thank you for your time.
>>> 
>>> 
>> 
>> Friendly ping.  Please let me know if there’s anything I can clarify.
> 
> The patch is in my queue (it will not get lost), the rationale seems 
> reasonable,

For a patch that changes code-gen we should have a test that it produces what’s
expected (in general, a ‘torture' test would be preferrable so that we can be 
sure the
output is as expected for different optimisation levels). 

** If you have a test program that could be the basis for this - but it needs 
to be
integrated into the testsuite with scans for the required output.

Have to give some thought to how to solve the obj-c++ issue.
Iain