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

  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.

<https://github.com/mhjacobson/gcc/commit/5ebc95dc726f0745ebdf003093f1b8d7720ce32f>


gcc/objc/ChangeLog:

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

	* 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.
  

> On Sep 26, 2021, at 11:45 PM, Matt Jacobson <mhjacobson@me.com> 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.
> 
> <https://github.com/mhjacobson/gcc/commit/5ebc95dc726f0745ebdf003093f1b8d7720ce32f>

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

Original mail:
<https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580280.html>

Thanks.
  

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 <gcc-patches@gcc.gnu.org> wrote:
> 
> 
>> On Sep 26, 2021, at 11:45 PM, Matt Jacobson <mhjacobson@me.com> 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.
>> 
>> <https://github.com/mhjacobson/gcc/commit/5ebc95dc726f0745ebdf003093f1b8d7720ce32f>
> 
> 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
  

Hi Matt,

> On 23 Oct 2021, at 09:46, Iain Sandoe via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
>> On 20 Oct 2021, at 04:51, Matt Jacobson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> 
>> 
>>> On Sep 26, 2021, at 11:45 PM, Matt Jacobson <mhjacobson@me.com> 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:
<built-in>: error: initialization of a flexible array member [-Wpedantic]


>>> Thank you for your time.
>>> 
>>> <https://github.com/mhjacobson/gcc/commit/5ebc95dc726f0745ebdf003093f1b8d7720ce32f>
>> 
>> 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
  

> On Oct 25, 2021, at 5:43 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
> Did you test objective-c++ on Darwin?
> 
> I see a lot of fails of the form:
> Excess errors:
> <built-in>: 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:

<https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>

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
  

> On 7 Nov 2021, at 22:50, Matt Jacobson <mhjacobson@me.com> wrote:
> 
> 
>> On Oct 25, 2021, at 5:43 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> Did you test objective-c++ on Darwin?
>> 
>> I see a lot of fails of the form:
>> Excess errors:
>> <built-in>: 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:
> 
> <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>
> 
> 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-xxxxx] (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