[PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Martin Jambor]

Hi,

while compute_record_mode in stor-layout.c makes sure it assigns BLK
mode to structs with flexible arrays, it has no such provisions for
zero length arrays
(http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Zero-Length.html).  I
think that in order to avoid problems and surprises like PR 57748
(where this triggered code that was intended for small structures that
fit into a scalar mode and ICEd), we should assign both variable array
possibilities the same mode.

Bootstrapped and tested on x86_64-linux without any problems.  OK for
trunk and the 4.8 branch?  (I'm not sure about the 4.7, this PR does
not happen there despite the wrong mode so I'd ignore it for now.)

Thanks,

Martin


2013-08-01  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/57748
	* stor-layout.c (compute_record_mode): Treat zero-sized array fields
	like incomplete types.

testsuite/
	* gcc.dg/torture/pr57748.c: New test.


*** /tmp/lV6Ba8_stor-layout.c	Thu Aug  1 16:28:25 2013
--- gcc/stor-layout.c	Thu Aug  1 15:36:18 2013
*************** compute_record_mode (tree type)
*** 1604,1610 ****
  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
  	  || ! host_integerp (bit_position (field), 1)
  	  || DECL_SIZE (field) == 0
! 	  || ! host_integerp (DECL_SIZE (field), 1))
  	return;
  
        /* If this field is the whole struct, remember its mode so
--- 1604,1612 ----
  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
  	  || ! host_integerp (bit_position (field), 1)
  	  || DECL_SIZE (field) == 0
! 	  || ! host_integerp (DECL_SIZE (field), 1)
! 	  || (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
! 	      && tree_low_cst (DECL_SIZE (field), 1) == 0))
  	return;
  
        /* If this field is the whole struct, remember its mode so
*** /dev/null	Tue Jun  4 12:34:56 2013
--- gcc/testsuite/gcc.dg/torture/pr57748.c	Thu Aug  1 15:42:14 2013
***************
*** 0 ****
--- 1,45 ----
+ /* PR middle-end/57748 */
+ /* { dg-do run } */
+ 
+ #include <stdlib.h>
+ 
+ extern void abort (void);
+ 
+ typedef long long V
+   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+ 
+ typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
+ 
+ struct __attribute__((packed)) T { char c; P s; };
+ 
+ void __attribute__((noinline, noclone))
+ check (struct T *t)
+ {
+   if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
+     abort ();
+ }
+ 
+ int __attribute__((noinline, noclone))
+ get_i (void)
+ {
+   return 0;
+ }
+ 
+ void __attribute__((noinline, noclone))
+ foo (P *p)
+ {
+   V a = { 3, 4 };
+   int i = get_i();
+   p->b[i] = a;
+ }
+ 
+ int
+ main ()
+ {
+   struct T *t = (struct T *) malloc (128);
+ 
+   foo (&t->s);
+   check (t);
+ 
+   return 0;
+ }

Re: [PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[David Abdurachmanov]

Hi,

Ping. Any news of the following patch being included into the trunk?

Thanks,
david

On Aug 2, 2013, at 1:45 PM, Martin Jambor wrote:

> Hi,
> 
> while compute_record_mode in stor-layout.c makes sure it assigns BLK
> mode to structs with flexible arrays, it has no such provisions for
> zero length arrays
> (http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Zero-Length.html).  I
> think that in order to avoid problems and surprises like PR 57748
> (where this triggered code that was intended for small structures that
> fit into a scalar mode and ICEd), we should assign both variable array
> possibilities the same mode.
> 
> Bootstrapped and tested on x86_64-linux without any problems.  OK for
> trunk and the 4.8 branch?  (I'm not sure about the 4.7, this PR does
> not happen there despite the wrong mode so I'd ignore it for now.)
> 
> Thanks,
> 
> Martin
> 
> 
> 2013-08-01  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/57748
> 	* stor-layout.c (compute_record_mode): Treat zero-sized array fields
> 	like incomplete types.
> 
> testsuite/
> 	* gcc.dg/torture/pr57748.c: New test.
> 
> 
> *** /tmp/lV6Ba8_stor-layout.c	Thu Aug  1 16:28:25 2013
> --- gcc/stor-layout.c	Thu Aug  1 15:36:18 2013
> *************** compute_record_mode (tree type)
> *** 1604,1610 ****
>  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>  	  || ! host_integerp (bit_position (field), 1)
>  	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1))
>  	return;
> 
>        /* If this field is the whole struct, remember its mode so
> --- 1604,1612 ----
>  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>  	  || ! host_integerp (bit_position (field), 1)
>  	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1)
> ! 	  || (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
> ! 	      && tree_low_cst (DECL_SIZE (field), 1) == 0))
>  	return;
> 
>        /* If this field is the whole struct, remember its mode so
> *** /dev/null	Tue Jun  4 12:34:56 2013
> --- gcc/testsuite/gcc.dg/torture/pr57748.c	Thu Aug  1 15:42:14 2013
> ***************
> *** 0 ****
> --- 1,45 ----
> + /* PR middle-end/57748 */
> + /* { dg-do run } */
> + 
> + #include <stdlib.h>
> + 
> + extern void abort (void);
> + 
> + typedef long long V
> +   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> + 
> + typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
> + 
> + struct __attribute__((packed)) T { char c; P s; };
> + 
> + void __attribute__((noinline, noclone))
> + check (struct T *t)
> + {
> +   if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
> +     abort ();
> + }
> + 
> + int __attribute__((noinline, noclone))
> + get_i (void)
> + {
> +   return 0;
> + }
> + 
> + void __attribute__((noinline, noclone))
> + foo (P *p)
> + {
> +   V a = { 3, 4 };
> +   int i = get_i();
> +   p->b[i] = a;
> + }
> + 
> + int
> + main ()
> + {
> +   struct T *t = (struct T *) malloc (128);
> + 
> +   foo (&t->s);
> +   check (t);
> + 
> +   return 0;
> + }

Re: [PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[David Abdurachmanov]

Hi,

Ping. Any news of the following patch being included into the trunk?

Thanks,
david

On Aug 2, 2013, at 1:45 PM, Martin Jambor wrote:

> Hi,
> 
> while compute_record_mode in stor-layout.c makes sure it assigns BLK
> mode to structs with flexible arrays, it has no such provisions for
> zero length arrays
> (http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Zero-Length.html).  I
> think that in order to avoid problems and surprises like PR 57748
> (where this triggered code that was intended for small structures that
> fit into a scalar mode and ICEd), we should assign both variable array
> possibilities the same mode.
> 
> Bootstrapped and tested on x86_64-linux without any problems.  OK for
> trunk and the 4.8 branch?  (I'm not sure about the 4.7, this PR does
> not happen there despite the wrong mode so I'd ignore it for now.)
> 
> Thanks,
> 
> Martin
> 
> 
> 2013-08-01  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/57748
> 	* stor-layout.c (compute_record_mode): Treat zero-sized array fields
> 	like incomplete types.
> 
> testsuite/
> 	* gcc.dg/torture/pr57748.c: New test.
> 
> 
> *** /tmp/lV6Ba8_stor-layout.c	Thu Aug  1 16:28:25 2013
> --- gcc/stor-layout.c	Thu Aug  1 15:36:18 2013
> *************** compute_record_mode (tree type)
> *** 1604,1610 ****
>  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>  	  || ! host_integerp (bit_position (field), 1)
>  	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1))
>  	return;
> 
>        /* If this field is the whole struct, remember its mode so
> --- 1604,1612 ----
>  		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>  	  || ! host_integerp (bit_position (field), 1)
>  	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1)
> ! 	  || (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
> ! 	      && tree_low_cst (DECL_SIZE (field), 1) == 0))
>  	return;
> 
>        /* If this field is the whole struct, remember its mode so
> *** /dev/null	Tue Jun  4 12:34:56 2013
> --- gcc/testsuite/gcc.dg/torture/pr57748.c	Thu Aug  1 15:42:14 2013
> ***************
> *** 0 ****
> --- 1,45 ----
> + /* PR middle-end/57748 */
> + /* { dg-do run } */
> + 
> + #include <stdlib.h>
> + 
> + extern void abort (void);
> + 
> + typedef long long V
> +   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> + 
> + typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
> + 
> + struct __attribute__((packed)) T { char c; P s; };
> + 
> + void __attribute__((noinline, noclone))
> + check (struct T *t)
> + {
> +   if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
> +     abort ();
> + }
> + 
> + int __attribute__((noinline, noclone))
> + get_i (void)
> + {
> +   return 0;
> + }
> + 
> + void __attribute__((noinline, noclone))
> + foo (P *p)
> + {
> +   V a = { 3, 4 };
> +   int i = get_i();
> +   p->b[i] = a;
> + }
> + 
> + int
> + main ()
> + {
> +   struct T *t = (struct T *) malloc (128);
> + 
> +   foo (&t->s);
> +   check (t);
> + 
> +   return 0;
> + }

Re: [PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[David Abdurachmanov]

ping^2

david

On Aug 12, 2013, at 2:31 PM, David Abdurachmanov wrote:

> Hi,
> 
> Ping. Any news of the following patch being included into the trunk?
> 
> Thanks,
> david
> 
> On Aug 2, 2013, at 1:45 PM, Martin Jambor wrote:
> 
>> Hi,
>> 
>> while compute_record_mode in stor-layout.c makes sure it assigns BLK
>> mode to structs with flexible arrays, it has no such provisions for
>> zero length arrays
>> (http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Zero-Length.html).  I
>> think that in order to avoid problems and surprises like PR 57748
>> (where this triggered code that was intended for small structures that
>> fit into a scalar mode and ICEd), we should assign both variable array
>> possibilities the same mode.
>> 
>> Bootstrapped and tested on x86_64-linux without any problems.  OK for
>> trunk and the 4.8 branch?  (I'm not sure about the 4.7, this PR does
>> not happen there despite the wrong mode so I'd ignore it for now.)
>> 
>> Thanks,
>> 
>> Martin
>> 
>> 
>> 2013-08-01  Martin Jambor  <mjambor@suse.cz>
>> 
>> 	PR middle-end/57748
>> 	* stor-layout.c (compute_record_mode): Treat zero-sized array fields
>> 	like incomplete types.
>> 
>> testsuite/
>> 	* gcc.dg/torture/pr57748.c: New test.
>> 
>> 
>> *** /tmp/lV6Ba8_stor-layout.c	Thu Aug  1 16:28:25 2013
>> --- gcc/stor-layout.c	Thu Aug  1 15:36:18 2013
>> *************** compute_record_mode (tree type)
>> *** 1604,1610 ****
>> 		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>> 	  || ! host_integerp (bit_position (field), 1)
>> 	  || DECL_SIZE (field) == 0
>> ! 	  || ! host_integerp (DECL_SIZE (field), 1))
>> 	return;
>> 
>>       /* If this field is the whole struct, remember its mode so
>> --- 1604,1612 ----
>> 		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>> 	  || ! host_integerp (bit_position (field), 1)
>> 	  || DECL_SIZE (field) == 0
>> ! 	  || ! host_integerp (DECL_SIZE (field), 1)
>> ! 	  || (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
>> ! 	      && tree_low_cst (DECL_SIZE (field), 1) == 0))
>> 	return;
>> 
>>       /* If this field is the whole struct, remember its mode so
>> *** /dev/null	Tue Jun  4 12:34:56 2013
>> --- gcc/testsuite/gcc.dg/torture/pr57748.c	Thu Aug  1 15:42:14 2013
>> ***************
>> *** 0 ****
>> --- 1,45 ----
>> + /* PR middle-end/57748 */
>> + /* { dg-do run } */
>> + 
>> + #include <stdlib.h>
>> + 
>> + extern void abort (void);
>> + 
>> + typedef long long V
>> +   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>> + 
>> + typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>> + 
>> + struct __attribute__((packed)) T { char c; P s; };
>> + 
>> + void __attribute__((noinline, noclone))
>> + check (struct T *t)
>> + {
>> +   if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
>> +     abort ();
>> + }
>> + 
>> + int __attribute__((noinline, noclone))
>> + get_i (void)
>> + {
>> +   return 0;
>> + }
>> + 
>> + void __attribute__((noinline, noclone))
>> + foo (P *p)
>> + {
>> +   V a = { 3, 4 };
>> +   int i = get_i();
>> +   p->b[i] = a;
>> + }
>> + 
>> + int
>> + main ()
>> + {
>> +   struct T *t = (struct T *) malloc (128);
>> + 
>> +   foo (&t->s);
>> +   check (t);
>> + 
>> +   return 0;
>> + }
> 

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Martin Jambor]

Hi Jakub and/or Joseph,

the reporter of this bug seems to be very anxious to have it fixed in
the repository.  While Richi is away, do you think you could have a
look?  It is very small.

Thanks a lot,

Martin


On Fri, Aug 02, 2013 at 01:45:31PM +0200, Martin Jambor wrote:
> Hi,
> 
> while compute_record_mode in stor-layout.c makes sure it assigns BLK
> mode to structs with flexible arrays, it has no such provisions for
> zero length arrays
> (http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Zero-Length.html).  I
> think that in order to avoid problems and surprises like PR 57748
> (where this triggered code that was intended for small structures that
> fit into a scalar mode and ICEd), we should assign both variable array
> possibilities the same mode.
> 
> Bootstrapped and tested on x86_64-linux without any problems.  OK for
> trunk and the 4.8 branch?  (I'm not sure about the 4.7, this PR does
> not happen there despite the wrong mode so I'd ignore it for now.)
> 
> Thanks,
> 
> Martin
> 
> 
> 2013-08-01  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/57748
> 	* stor-layout.c (compute_record_mode): Treat zero-sized array fields
> 	like incomplete types.
> 
> testsuite/
> 	* gcc.dg/torture/pr57748.c: New test.
> 
> 
> *** /tmp/lV6Ba8_stor-layout.c	Thu Aug  1 16:28:25 2013
> --- gcc/stor-layout.c	Thu Aug  1 15:36:18 2013
> *************** compute_record_mode (tree type)
> *** 1604,1610 ****
>   		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>   	  || ! host_integerp (bit_position (field), 1)
>   	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1))
>   	return;
>   
>         /* If this field is the whole struct, remember its mode so
> --- 1604,1612 ----
>   		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>   	  || ! host_integerp (bit_position (field), 1)
>   	  || DECL_SIZE (field) == 0
> ! 	  || ! host_integerp (DECL_SIZE (field), 1)
> ! 	  || (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
> ! 	      && tree_low_cst (DECL_SIZE (field), 1) == 0))
>   	return;
>   
>         /* If this field is the whole struct, remember its mode so
> *** /dev/null	Tue Jun  4 12:34:56 2013
> --- gcc/testsuite/gcc.dg/torture/pr57748.c	Thu Aug  1 15:42:14 2013
> ***************
> *** 0 ****
> --- 1,45 ----
> + /* PR middle-end/57748 */
> + /* { dg-do run } */
> + 
> + #include <stdlib.h>
> + 
> + extern void abort (void);
> + 
> + typedef long long V
> +   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> + 
> + typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
> + 
> + struct __attribute__((packed)) T { char c; P s; };
> + 
> + void __attribute__((noinline, noclone))
> + check (struct T *t)
> + {
> +   if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
> +     abort ();
> + }
> + 
> + int __attribute__((noinline, noclone))
> + get_i (void)
> + {
> +   return 0;
> + }
> + 
> + void __attribute__((noinline, noclone))
> + foo (P *p)
> + {
> +   V a = { 3, 4 };
> +   int i = get_i();
> +   p->b[i] = a;
> + }
> + 
> + int
> + main ()
> + {
> +   struct T *t = (struct T *) malloc (128);
> + 
> +   foo (&t->s);
> +   check (t);
> + 
> +   return 0;
> + }

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Jakub Jelinek]

On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> Hi Jakub and/or Joseph,
> 
> the reporter of this bug seems to be very anxious to have it fixed in
> the repository.  While Richi is away, do you think you could have a
> look?  It is very small.

Isn't this ABI incompatible change (at least potential on various targets)?
If so, then it shouldn't be applied to release branches, because it would
create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.

	Jakub

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Martin Jambor]

Hi,

On Fri, Aug 23, 2013 at 05:29:23PM +0200, Jakub Jelinek wrote:
> On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> > Hi Jakub and/or Joseph,
> > 
> > the reporter of this bug seems to be very anxious to have it fixed in
> > the repository.  While Richi is away, do you think you could have a
> > look?  It is very small.
> 
> Isn't this ABI incompatible change (at least potential on various targets)?
> If so, then it shouldn't be applied to release branches, because it would
> create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.
> 

I don't know.  I did a few attempts to observe a change in the calling
convention of a function accepting a zero sized array terminated
structure by value (on x86_64) but I did not succeed.  On the other
hand, I assume there are many other ways how a mode can influence ABI.
So I'll try to have a look whether we can hack around this situation
in 4.8's expr.c instead.

Nevertheless, is this patch ok for trunk?

Thanks,

Martin

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Jakub Jelinek]

On Tue, Aug 27, 2013 at 04:03:42PM +0200, Martin Jambor wrote:
> On Fri, Aug 23, 2013 at 05:29:23PM +0200, Jakub Jelinek wrote:
> > On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> > > Hi Jakub and/or Joseph,
> > > 
> > > the reporter of this bug seems to be very anxious to have it fixed in
> > > the repository.  While Richi is away, do you think you could have a
> > > look?  It is very small.
> > 
> > Isn't this ABI incompatible change (at least potential on various targets)?
> > If so, then it shouldn't be applied to release branches, because it would
> > create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.
> > 
> 
> I don't know.  I did a few attempts to observe a change in the calling
> convention of a function accepting a zero sized array terminated
> structure by value (on x86_64) but I did not succeed.  On the other
> hand, I assume there are many other ways how a mode can influence ABI.
> So I'll try to have a look whether we can hack around this situation
> in 4.8's expr.c instead.

All I remember that e.g. the number of regressions from PR20020 was big,
and any kind of TYPE_MODE changes are just extremely risky.
Perhaps x86_64 in the ABI decisions never uses mode, but we have tons of
other targets and it would surprise me if none of those were affected.

> Nevertheless, is this patch ok for trunk?

I'll defer that to Richard now that he is back ;)

	Jakub

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Richard Biener]

On Tue, 27 Aug 2013, Jakub Jelinek wrote:

> On Tue, Aug 27, 2013 at 04:03:42PM +0200, Martin Jambor wrote:
> > On Fri, Aug 23, 2013 at 05:29:23PM +0200, Jakub Jelinek wrote:
> > > On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> > > > Hi Jakub and/or Joseph,
> > > > 
> > > > the reporter of this bug seems to be very anxious to have it fixed in
> > > > the repository.  While Richi is away, do you think you could have a
> > > > look?  It is very small.
> > > 
> > > Isn't this ABI incompatible change (at least potential on various targets)?
> > > If so, then it shouldn't be applied to release branches, because it would
> > > create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.
> > > 
> > 
> > I don't know.  I did a few attempts to observe a change in the calling
> > convention of a function accepting a zero sized array terminated
> > structure by value (on x86_64) but I did not succeed.  On the other
> > hand, I assume there are many other ways how a mode can influence ABI.
> > So I'll try to have a look whether we can hack around this situation
> > in 4.8's expr.c instead.
> 
> All I remember that e.g. the number of regressions from PR20020 was big,
> and any kind of TYPE_MODE changes are just extremely risky.
> Perhaps x86_64 in the ABI decisions never uses mode, but we have tons of
> other targets and it would surprise me if none of those were affected.
> 
> > Nevertheless, is this patch ok for trunk?
> 
> I'll defer that to Richard now that he is back ;)

Eh ... :/

I'm extremely nervous about this change.  I also believe the change
is unrelated to the issue in the bugreport (even if it happens to
fix the ICE).

Let me have a (short) look.

Richard.

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Richard Biener]

On Wed, 28 Aug 2013, Richard Biener wrote:

> On Tue, 27 Aug 2013, Jakub Jelinek wrote:
> 
> > On Tue, Aug 27, 2013 at 04:03:42PM +0200, Martin Jambor wrote:
> > > On Fri, Aug 23, 2013 at 05:29:23PM +0200, Jakub Jelinek wrote:
> > > > On Fri, Aug 23, 2013 at 05:11:22PM +0200, Martin Jambor wrote:
> > > > > Hi Jakub and/or Joseph,
> > > > > 
> > > > > the reporter of this bug seems to be very anxious to have it fixed in
> > > > > the repository.  While Richi is away, do you think you could have a
> > > > > look?  It is very small.
> > > > 
> > > > Isn't this ABI incompatible change (at least potential on various targets)?
> > > > If so, then it shouldn't be applied to release branches, because it would
> > > > create (potential?) ABI incompatibility between 4.8.[01] and 4.8.2.
> > > > 
> > > 
> > > I don't know.  I did a few attempts to observe a change in the calling
> > > convention of a function accepting a zero sized array terminated
> > > structure by value (on x86_64) but I did not succeed.  On the other
> > > hand, I assume there are many other ways how a mode can influence ABI.
> > > So I'll try to have a look whether we can hack around this situation
> > > in 4.8's expr.c instead.
> > 
> > All I remember that e.g. the number of regressions from PR20020 was big,
> > and any kind of TYPE_MODE changes are just extremely risky.
> > Perhaps x86_64 in the ABI decisions never uses mode, but we have tons of
> > other targets and it would surprise me if none of those were affected.
> > 
> > > Nevertheless, is this patch ok for trunk?
> > 
> > I'll defer that to Richard now that he is back ;)
> 
> Eh ... :/
> 
> I'm extremely nervous about this change.  I also believe the change
> is unrelated to the issue in the bugreport (even if it happens to
> fix the ICE).
> 
> Let me have a (short) look.

Everything looks ok up until

      if (offset != 0)
        {
          enum machine_mode address_mode;

where we are supposed to factor in offset into the memory address.
This code doesn't handle the movmisalign path which has the memory
in 'mem' instead of in to_rtx.

And indeed a hack like

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 202043)
+++ gcc/expr.c  (working copy)
@@ -4753,6 +4753,9 @@ expand_assignment (tree to, tree from, b
        {
          enum machine_mode address_mode;
          rtx offset_rtx;
+         rtx saved_to_rtx = to_rtx;
+         if (misalignp)
+           to_rtx = mem;
 
          if (!MEM_P (to_rtx))
            {
@@ -4785,6 +4788,11 @@ expand_assignment (tree to, tree from, b
          to_rtx = offset_address (to_rtx, offset_rtx,
                                   highest_pow2_factor_for_target (to,
                                                                   
offset));
+         if (misalignp)
+           {
+             mem = to_rtx;
+             to_rtx = saved_to_rtx;
+           }
        }
 
       /* No action is needed if the target is not a memory and the field

fixes the testcase and results in (at -O)

foo:
.LFB1:
        .cfi_startproc
        pushq   %rbx
        .cfi_def_cfa_offset 16
        .cfi_offset 3, -16
        movq    %rdi, %rbx
        call    get_i
        cltq
        addq    $1, %rax
        salq    $4, %rax
        movdqa  .LC0(%rip), %xmm0
        movdqu  %xmm0, (%rbx,%rax)
        popq    %rbx
        .cfi_def_cfa_offset 8
        ret

exactly what is expected.

Martin, do you want to audit the (few) places we do the movmisalign
game for the same problem or shall I put it on my TODO?  The audit
should look for all MEM_P (to_rtx) tests which really should look
at 'mem' for the unaligned move case (I see a volatile bitfiled
case for example ...).

Still need to figure a "nice" way to restructure the code ...

Any ideas?

Thanks,
Richard.

Re: [PING PATCH, PR 57748] Set mode of structures with zero sized arrays to be BLK

[Martin Jambor]

On Wed, Aug 28, 2013 at 10:17:52AM +0200, Richard Biener wrote:
> On Wed, 28 Aug 2013, Richard Biener wrote:
> > Eh ... :/
> > 
> > I'm extremely nervous about this change.  I also believe the change
> > is unrelated to the issue in the bugreport (even if it happens to
> > fix the ICE).
> > 
> > Let me have a (short) look.
> 
> Everything looks ok up until
> 
>       if (offset != 0)
>         {
>           enum machine_mode address_mode;
> 
> where we are supposed to factor in offset into the memory address.
> This code doesn't handle the movmisalign path which has the memory
> in 'mem' instead of in to_rtx.
> 
> And indeed a hack like
> 
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  (revision 202043)
> +++ gcc/expr.c  (working copy)
> @@ -4753,6 +4753,9 @@ expand_assignment (tree to, tree from, b
>         {
>           enum machine_mode address_mode;
>           rtx offset_rtx;
> +         rtx saved_to_rtx = to_rtx;
> +         if (misalignp)
> +           to_rtx = mem;
>  
>           if (!MEM_P (to_rtx))
>             {
> @@ -4785,6 +4788,11 @@ expand_assignment (tree to, tree from, b
>           to_rtx = offset_address (to_rtx, offset_rtx,
>                                    highest_pow2_factor_for_target (to,
>                                                                    
> offset));
> +         if (misalignp)
> +           {
> +             mem = to_rtx;
> +             to_rtx = saved_to_rtx;
> +           }
>         }
>  
>        /* No action is needed if the target is not a memory and the field
> 
> fixes the testcase and results in (at -O)
> 
> foo:
> .LFB1:
>         .cfi_startproc
>         pushq   %rbx
>         .cfi_def_cfa_offset 16
>         .cfi_offset 3, -16
>         movq    %rdi, %rbx
>         call    get_i
>         cltq
>         addq    $1, %rax
>         salq    $4, %rax
>         movdqa  .LC0(%rip), %xmm0
>         movdqu  %xmm0, (%rbx,%rax)
>         popq    %rbx
>         .cfi_def_cfa_offset 8
>         ret
> 
> exactly what is expected.
> 
> Martin, do you want to audit the (few) places we do the movmisalign
> game for the same problem or shall I put it on my TODO?  The audit
> should look for all MEM_P (to_rtx) tests which really should look
> at 'mem' for the unaligned move case (I see a volatile bitfiled
> case for example ...).

And I thought I had an easy way out.  Well, let me see what I can do.

Thanks,

Martin