Fix alias.c ICE on inline-asm "=m" incomplete operand (PR inline-asm/85022)

Message ID 20180322213406.GX8577@tucnak
State New
Headers show
Series
  • Fix alias.c ICE on inline-asm "=m" incomplete operand (PR inline-asm/85022)
Related show

Commit Message

Jakub Jelinek March 22, 2018, 9:34 p.m.
Hi!

Something I wasn't really aware, apparently we allow extern vars with
incomplete types in "m" and "=m" constrained asm.  The problem is that
incomplete vars have VOIDmode mode and so their MEMs do as well, apparently
everything works with it except a 2013-ish assert in alias.c.

The following patch just makes sure x_mode is not VOIDmode if x doesn't have
VOIDmode, if x has VOIDmode, then it is fine for x_mode to be VOIDmode too.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-03-22  Jakub Jelinek  <jakub@redhat.com>

	PR inline-asm/85022
	* alias.c (write_dependence_p): Don't require for x_canonicalized
	non-VOIDmode if x has VOIDmode.

	* c-c++-common/torture/pr85022.c: New test.


	Jakub

Comments

Richard Biener March 23, 2018, 8:53 a.m. | #1
On Thu, 22 Mar 2018, Jakub Jelinek wrote:

> Hi!

> 

> Something I wasn't really aware, apparently we allow extern vars with

> incomplete types in "m" and "=m" constrained asm.  The problem is that

> incomplete vars have VOIDmode mode and so their MEMs do as well, apparently

> everything works with it except a 2013-ish assert in alias.c.

> 

> The following patch just makes sure x_mode is not VOIDmode if x doesn't have

> VOIDmode, if x has VOIDmode, then it is fine for x_mode to be VOIDmode too.

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


OK.

The question now is whether we handle dependences correctly for
VOIDmode MEMs?  Do they have !MEM_SIZE_KNOWN_P?

Thanks,
Richard.

> 2018-03-22  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR inline-asm/85022

> 	* alias.c (write_dependence_p): Don't require for x_canonicalized

> 	non-VOIDmode if x has VOIDmode.

> 

> 	* c-c++-common/torture/pr85022.c: New test.

> 

> --- gcc/alias.c.jj	2018-03-01 11:29:06.725104754 +0100

> +++ gcc/alias.c	2018-03-22 10:35:15.649546027 +0100

> @@ -2999,7 +2999,8 @@ write_dependence_p (const_rtx mem,

>    int ret;

>  

>    gcc_checking_assert (x_canonicalized

> -		       ? (x_addr != NULL_RTX && x_mode != VOIDmode)

> +		       ? (x_addr != NULL_RTX

> +			  && (x_mode != VOIDmode || GET_MODE (x) == VOIDmode))

>  		       : (x_addr == NULL_RTX && x_mode == VOIDmode));

>  

>    if (MEM_VOLATILE_P (x) && MEM_VOLATILE_P (mem))

> --- gcc/testsuite/c-c++-common/torture/pr85022.c.jj	2018-03-22 10:44:40.520679173 +0100

> +++ gcc/testsuite/c-c++-common/torture/pr85022.c	2018-03-22 10:44:29.550675460 +0100

> @@ -0,0 +1,9 @@

> +/* PR inline-asm/85022 */

> +

> +extern struct B b;

> +

> +void

> +foo ()

> +{

> +  __asm ("" : "+m" (b));

> +}

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Jakub Jelinek March 23, 2018, 9:04 a.m. | #2
On Fri, Mar 23, 2018 at 09:53:53AM +0100, Richard Biener wrote:
> > Something I wasn't really aware, apparently we allow extern vars with

> > incomplete types in "m" and "=m" constrained asm.  The problem is that

> > incomplete vars have VOIDmode mode and so their MEMs do as well, apparently

> > everything works with it except a 2013-ish assert in alias.c.

> > 

> > The following patch just makes sure x_mode is not VOIDmode if x doesn't have

> > VOIDmode, if x has VOIDmode, then it is fine for x_mode to be VOIDmode too.

> > 

> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

> 

> OK.

> 

> The question now is whether we handle dependences correctly for

> VOIDmode MEMs?  Do they have !MEM_SIZE_KNOWN_P?


They have MEM_SIZE_KNOWN_P true unfortunately.
set_mem_attributes_minus_bitpos starts with:
      defattrs = mode_mem_attrs[(int) GET_MODE (ref)];
      gcc_assert (!defattrs->expr);
      gcc_assert (!defattrs->offset_known_p);

      /* Respect mode size.  */
      attrs.size_known_p = defattrs->size_known_p;
      attrs.size = defattrs->size;
and because both TYPE_SIZE_UNIT and DECL_SIZE_UNIT are NULL, nothing updates
the size_known_p.
Wonder if we just shouldn't make mode_mem_attrs[VOIDmode]->size_known_p
false and mode_mem_attrs[VOIDmode]->size NULL, like we do for BLKmode?

   for (i = 0; i < (int) MAX_MACHINE_MODE; i++)
     {
       mode = (machine_mode) i;
       attrs = ggc_cleared_alloc<mem_attrs> ();
       attrs->align = BITS_PER_UNIT;
       attrs->addrspace = ADDR_SPACE_GENERIC;
-      if (mode != BLKmode)
+      if (mode != BLKmode && mode != VOIDmode)
         {
           attrs->size_known_p = true;
           attrs->size = GET_MODE_SIZE (mode);
           if (STRICT_ALIGNMENT)
             attrs->align = GET_MODE_ALIGNMENT (mode);
         }

?

	Jakub
Richard Biener March 23, 2018, 9:06 a.m. | #3
On Fri, 23 Mar 2018, Jakub Jelinek wrote:

> On Fri, Mar 23, 2018 at 09:53:53AM +0100, Richard Biener wrote:

> > > Something I wasn't really aware, apparently we allow extern vars with

> > > incomplete types in "m" and "=m" constrained asm.  The problem is that

> > > incomplete vars have VOIDmode mode and so their MEMs do as well, apparently

> > > everything works with it except a 2013-ish assert in alias.c.

> > > 

> > > The following patch just makes sure x_mode is not VOIDmode if x doesn't have

> > > VOIDmode, if x has VOIDmode, then it is fine for x_mode to be VOIDmode too.

> > > 

> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

> > 

> > OK.

> > 

> > The question now is whether we handle dependences correctly for

> > VOIDmode MEMs?  Do they have !MEM_SIZE_KNOWN_P?

> 

> They have MEM_SIZE_KNOWN_P true unfortunately.

> set_mem_attributes_minus_bitpos starts with:

>       defattrs = mode_mem_attrs[(int) GET_MODE (ref)];

>       gcc_assert (!defattrs->expr);

>       gcc_assert (!defattrs->offset_known_p);

> 

>       /* Respect mode size.  */

>       attrs.size_known_p = defattrs->size_known_p;

>       attrs.size = defattrs->size;

> and because both TYPE_SIZE_UNIT and DECL_SIZE_UNIT are NULL, nothing updates

> the size_known_p.

> Wonder if we just shouldn't make mode_mem_attrs[VOIDmode]->size_known_p

> false and mode_mem_attrs[VOIDmode]->size NULL, like we do for BLKmode?

> 

>    for (i = 0; i < (int) MAX_MACHINE_MODE; i++)

>      {

>        mode = (machine_mode) i;

>        attrs = ggc_cleared_alloc<mem_attrs> ();

>        attrs->align = BITS_PER_UNIT;

>        attrs->addrspace = ADDR_SPACE_GENERIC;

> -      if (mode != BLKmode)

> +      if (mode != BLKmode && mode != VOIDmode)

>          {

>            attrs->size_known_p = true;

>            attrs->size = GET_MODE_SIZE (mode);

>            if (STRICT_ALIGNMENT)

>              attrs->align = GET_MODE_ALIGNMENT (mode);

>          }

> 

> ?


Yes, that sounds like a very good idea.  Pre-approved if it passes
testing.

Richard.

Patch

--- gcc/alias.c.jj	2018-03-01 11:29:06.725104754 +0100
+++ gcc/alias.c	2018-03-22 10:35:15.649546027 +0100
@@ -2999,7 +2999,8 @@  write_dependence_p (const_rtx mem,
   int ret;
 
   gcc_checking_assert (x_canonicalized
-		       ? (x_addr != NULL_RTX && x_mode != VOIDmode)
+		       ? (x_addr != NULL_RTX
+			  && (x_mode != VOIDmode || GET_MODE (x) == VOIDmode))
 		       : (x_addr == NULL_RTX && x_mode == VOIDmode));
 
   if (MEM_VOLATILE_P (x) && MEM_VOLATILE_P (mem))
--- gcc/testsuite/c-c++-common/torture/pr85022.c.jj	2018-03-22 10:44:40.520679173 +0100
+++ gcc/testsuite/c-c++-common/torture/pr85022.c	2018-03-22 10:44:29.550675460 +0100
@@ -0,0 +1,9 @@ 
+/* PR inline-asm/85022 */
+
+extern struct B b;
+
+void
+foo ()
+{
+  __asm ("" : "+m" (b));
+}