Patchwork [objc] : Fix issue about LLP64 and -Wpadded

login
register
mail settings
Submitter Kai Tietz
Date Sept. 30, 2010, 8:34 a.m.
Message ID <OF184808F8.06E0EB41-ONC12577AE.002EEBE7-C12577AE.002F174F@onevision.de>
Download mbox | patch
Permalink /patch/66138/
State New
Headers show

Comments

Kai Tietz - Sept. 30, 2010, 8:34 a.m.
"Nicola Pero" <nicola.pero@meta-innovation.com> wrote on 30.09.2010 
10:09:04:

> 
> > Well, the trouble is comming here by the fact that we have 3 long
> > types in series within this structure. For targets with sizeof (long)
> > == sizeof (void *) this code works. As windows 64 has sizeof (long) ==
> > 4 and sizeof (void *) == 8, there is an implicit alignment done.
> > To use here the packed attribute was one of my first tries to fix it,
> > but sadly it didn't lead to working executables, as compiler itself
> > emits data aligned, so declaration of structure in header doesn't fit
> > to actual generated data structure layout of compiler.
> 
> Thanks Kai
> 
> just for me to understand what you are trying to fix - without your 
patch, 
> everything works, but anything that compiles using -Wpadded and includes 

> objc/objc-api.h prints a warning ?  Is that what happens ?

Yes, you got it absolute right.

> I guess that makes it important to fix, since it would mean that using 
> -Wpadded with Objective-C produces lots of warning that the GCC user 
can't
> do anything about.
Yes, this can be noticed by some testcases in objc's testsuite, which are 
using -Wpadded option.

> If that's the case, your fix does look reasonable and meaningful, but I 
would
> suggest you add a fat, descriptive comment explaining it.  Can you 
repost it 
> with such a comment added ?
> 
> Thanks a lot for looking into this! :-)
> 

Does this comment looks ok for you? If so, ok for apply?

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.


                                                 variables in the class
@@ -42,3 +49,4 @@
   struct objc_protocol_list *protocols;              /* Protocols 
conformed to
*/
   void* gc_object_type;
 };
+
IainS - Sept. 30, 2010, 8:39 a.m.
On 30 Sep 2010, at 09:34, Kai Tietz wrote:

> "Nicola Pero" <nicola.pero@meta-innovation.com> wrote on 30.09.2010
> 10:09:04:
>
>>
>>> Well, the trouble is comming here by the fact that we have 3 long
>>> types in series within this structure. For targets with sizeof  
>>> (long)
>>> == sizeof (void *) this code works. As windows 64 has sizeof  
>>> (long) ==
>>> 4 and sizeof (void *) == 8, there is an implicit alignment done.
>>> To use here the packed attribute was one of my first tries to fix  
>>> it,
>>> but sadly it didn't lead to working executables, as compiler itself
>>> emits data aligned, so declaration of structure in header doesn't  
>>> fit
>>> to actual generated data structure layout of compiler.
>>
>> Thanks Kai
>>
>> just for me to understand what you are trying to fix - without your
> patch,
>> everything works, but anything that compiles using -Wpadded and  
>> includes
>
>> objc/objc-api.h prints a warning ?  Is that what happens ?
>
> Yes, you got it absolute right.
>
>> I guess that makes it important to fix, since it would mean that  
>> using
>> -Wpadded with Objective-C produces lots of warning that the GCC user
> can't
>> do anything about.
> Yes, this can be noticed by some testcases in objc's testsuite,  
> which are
> using -Wpadded option.
>
>> If that's the case, your fix does look reasonable and meaningful,  
>> but I
> would
>> suggest you add a fat, descriptive comment explaining it.  Can you
> repost it
>> with such a comment added ?
>>
>> Thanks a lot for looking into this! :-)
>>
>
> Does this comment looks ok for you? If so, ok for apply?
>
> Regards,
> Kai
>
> |  (\_/)  This is Bunny. Copy and paste Bunny
> | (='.'=) into your signature to help him gain
> | (")_(") world domination.
>
>
> --- struct_objc_class.h (revision 164743)
> +++ struct_objc_class.h (working copy)
> @@ -24,6 +24,13 @@
>                                                 The sum of the class
>                                                definition and all  
> super
>                                                class definitions. */
> +#ifdef _WIN64
> +  /* We do here structure padding manual.  This is done by compiler

Perhaps: "Here we do structure padding manually"

Looks OK to me - but I cannot approve it.

> +     automatically, but would lead in combination with -Wpadded to
> +     annoying warnings.  This padding is necessary as on LLP64  
> targets
> +     sizeof (long) isn't equal to sizeof (void *).  */
> +  long pad;
> +#endif
>   struct objc_ivar_list* ivars;               /* Pointer to a  
> structure
> that
>                                                 describes the instance
>                                                 variables in the class
> @@ -42,3 +49,4 @@
>   struct objc_protocol_list *protocols;              /* Protocols
> conformed to
> */
>   void* gc_object_type;
> };
> +
>

Patch

--- struct_objc_class.h (revision 164743)
+++ struct_objc_class.h (working copy)
@@ -24,6 +24,13 @@ 
                                                 The sum of the class
                                                definition and all super
                                                class definitions. */
+#ifdef _WIN64
+  /* We do here structure padding manual.  This is done by compiler
+     automatically, but would lead in combination with -Wpadded to
+     annoying warnings.  This padding is necessary as on LLP64 targets
+     sizeof (long) isn't equal to sizeof (void *).  */
+  long pad;
+#endif
   struct objc_ivar_list* ivars;               /* Pointer to a structure 
that
                                                 describes the instance