[v2] inline-asm: Fix ICE with bitfields in "m" operands [PR100785]

On Wed, Jun 02, 2021 at 12:14:51PM -0400, Jason Merrill wrote:
> On 6/2/21 11:25 AM, Jakub Jelinek wrote:
> > On Wed, Jun 02, 2021 at 11:09:45AM -0400, Jason Merrill wrote:
> > > On 6/2/21 3:59 AM, Jakub Jelinek wrote:
> > > >    	      if (!allows_reg && !cxx_mark_addressable (*op))
> > > >    		operand = error_mark_node;
> > > > +	      else if (!allows_reg && bitfield_p (*op))
> > > > +		{
> > > > +		  error_at (loc, "attempt to take address of bit-field");
> > > 
> > > Hmm, why aren't we already catching this in cxx_mark_addressable?
> > 
> > That is certainly possible, but it goes against Eric's patch
> > https://gcc.gnu.org/pipermail/gcc-patches/2015-June/421483.html
> 
> Hmm, I wonder what his rationale was?

No idea (and see below, nothing in the testsuite seems to require that).

Anyway, sorry for forgetting about this.

Here is a variant of the patch that adds the errors to
{c,cxx}_mark_addressable but keeps them in
build_unary_op/cp_build_addr_expr_1 too where for C++ it handles SFINAE
there etc.

Bootstrapped/regtested on x86_64-linux and i686-linux.

2021-06-11  Jakub Jelinek  <jakub@redhat.com>

	PR inline-asm/100785
gcc/
	* gimplify.c (gimplify_asm_expr): Don't diagnose errors if
	output or input operands were already error_mark_node.
	* cfgexpand.c (expand_asm_stmt): If errors are emitted,
	remove all inputs, outputs and clobbers from the asm and
	set template to "".
gcc/c/
	* c-typeck.c (c_mark_addressable): Diagnose trying to make
	bit-fields addressable.
gcc/cp/
	* semantics.c (cxx_mark_addressable): Diagnose trying to make
	bit-fields addressable.
gcc/testsuite/
	* c-c++-common/pr100785.c: New test.
	* gcc.dg/pr48552-1.c: Don't expect invalid lvalue errors.
	* gcc.dg/pr48552-2.c: Likewise.



	Jakub

On 6/11/21 10:50 AM, Jakub Jelinek wrote:
> On Wed, Jun 02, 2021 at 12:14:51PM -0400, Jason Merrill wrote:
>> On 6/2/21 11:25 AM, Jakub Jelinek wrote:
>>> On Wed, Jun 02, 2021 at 11:09:45AM -0400, Jason Merrill wrote:
>>>> On 6/2/21 3:59 AM, Jakub Jelinek wrote:
>>>>>     	      if (!allows_reg && !cxx_mark_addressable (*op))
>>>>>     		operand = error_mark_node;
>>>>> +	      else if (!allows_reg && bitfield_p (*op))
>>>>> +		{
>>>>> +		  error_at (loc, "attempt to take address of bit-field");
>>>>
>>>> Hmm, why aren't we already catching this in cxx_mark_addressable?
>>>
>>> That is certainly possible, but it goes against Eric's patch
>>> https://gcc.gnu.org/pipermail/gcc-patches/2015-June/421483.html
>>
>> Hmm, I wonder what his rationale was?
> 
> No idea (and see below, nothing in the testsuite seems to require that).
> 
> Anyway, sorry for forgetting about this.
> 
> Here is a variant of the patch that adds the errors to
> {c,cxx}_mark_addressable but keeps them in
> build_unary_op/cp_build_addr_expr_1 too where for C++ it handles SFINAE
> there etc.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux.

C++ change is OK; the whole patch is OK on Tuesday if no other comments.

> 2021-06-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR inline-asm/100785
> gcc/
> 	* gimplify.c (gimplify_asm_expr): Don't diagnose errors if
> 	output or input operands were already error_mark_node.
> 	* cfgexpand.c (expand_asm_stmt): If errors are emitted,
> 	remove all inputs, outputs and clobbers from the asm and
> 	set template to "".
> gcc/c/
> 	* c-typeck.c (c_mark_addressable): Diagnose trying to make
> 	bit-fields addressable.
> gcc/cp/
> 	* semantics.c (cxx_mark_addressable): Diagnose trying to make
> 	bit-fields addressable.
> gcc/testsuite/
> 	* c-c++-common/pr100785.c: New test.
> 	* gcc.dg/pr48552-1.c: Don't expect invalid lvalue errors.
> 	* gcc.dg/pr48552-2.c: Likewise.
> 
> --- gcc/gimplify.c.jj	2021-06-10 15:27:35.141299425 +0200
> +++ gcc/gimplify.c	2021-06-11 13:27:51.212867419 +0200
> @@ -6321,12 +6321,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
>         if (!allows_reg && allows_mem)
>   	mark_addressable (TREE_VALUE (link));
>   
> +      tree orig = TREE_VALUE (link);
>         tret = gimplify_expr (&TREE_VALUE (link), pre_p, post_p,
>   			    is_inout ? is_gimple_min_lval : is_gimple_lvalue,
>   			    fb_lvalue | fb_mayfail);
>         if (tret == GS_ERROR)
>   	{
> -	  error ("invalid lvalue in %<asm%> output %d", i);
> +	  if (orig != error_mark_node)
> +	    error ("invalid lvalue in %<asm%> output %d", i);
>   	  ret = tret;
>   	}
>   
> @@ -6521,8 +6523,9 @@ gimplify_asm_expr (tree *expr_p, gimple_
>   	  mark_addressable (TREE_VALUE (link));
>   	  if (tret == GS_ERROR)
>   	    {
> -	      error_at (EXPR_LOC_OR_LOC (TREE_VALUE (link), input_location),
> -			"memory input %d is not directly addressable", i);
> +	      if (inputv != error_mark_node)
> +		error_at (EXPR_LOC_OR_LOC (TREE_VALUE (link), input_location),
> +			  "memory input %d is not directly addressable", i);
>   	      ret = tret;
>   	    }
>   	}
> --- gcc/cfgexpand.c.jj	2021-06-02 10:07:47.632826557 +0200
> +++ gcc/cfgexpand.c	2021-06-11 13:27:51.212867419 +0200
> @@ -3082,6 +3082,7 @@ expand_asm_stmt (gasm *stmt)
>     unsigned ninputs = gimple_asm_ninputs (stmt);
>     unsigned nlabels = gimple_asm_nlabels (stmt);
>     unsigned i;
> +  bool error_seen = false;
>   
>     /* ??? Diagnose during gimplification?  */
>     if (ninputs + noutputs + nlabels > MAX_RECOG_OPERANDS)
> @@ -3140,6 +3141,7 @@ expand_asm_stmt (gasm *stmt)
>   		{
>   		  /* ??? Diagnose during gimplification?  */
>   		  error ("unknown register name %qs in %<asm%>", regname);
> +		  error_seen = true;
>   		}
>   	      else if (j == -4)
>   		{
> @@ -3202,7 +3204,10 @@ expand_asm_stmt (gasm *stmt)
>   		&& REG_P (DECL_RTL (output_tvec[j]))
>   		&& HARD_REGISTER_P (DECL_RTL (output_tvec[j]))
>   		&& output_hregno == REGNO (DECL_RTL (output_tvec[j])))
> -	      error ("invalid hard register usage between output operands");
> +	      {
> +		error ("invalid hard register usage between output operands");
> +		error_seen = true;
> +	      }
>   
>   	  /* Verify matching constraint operands use the same hard register
>   	     and that the non-matching constraint operands do not use the same
> @@ -3225,13 +3230,19 @@ expand_asm_stmt (gasm *stmt)
>   		  }
>   		if (i == match
>   		    && output_hregno != input_hregno)
> -		  error ("invalid hard register usage between output operand "
> -			 "and matching constraint operand");
> +		  {
> +		    error ("invalid hard register usage between output "
> +			   "operand and matching constraint operand");
> +		    error_seen = true;
> +		  }
>   		else if (early_clobber_p
>   			 && i != match
>   			 && output_hregno == input_hregno)
> -		  error ("invalid hard register usage between earlyclobber "
> -			 "operand and input operand");
> +		  {
> +		    error ("invalid hard register usage between "
> +			   "earlyclobber operand and input operand");
> +		    error_seen = true;
> +		  }
>   	      }
>   	}
>   
> @@ -3307,7 +3318,10 @@ expand_asm_stmt (gasm *stmt)
>   	    op = validize_mem (op);
>   
>   	  if (! allows_reg && !MEM_P (op))
> -	    error ("output number %d not directly addressable", i);
> +	    {
> +	      error ("output number %d not directly addressable", i);
> +	      error_seen = true;
> +	    }
>   	  if ((! allows_mem && MEM_P (op) && GET_MODE (op) != BLKmode)
>   	      || GET_CODE (op) == CONCAT)
>   	    {
> @@ -3347,6 +3361,19 @@ expand_asm_stmt (gasm *stmt)
>   	inout_opnum.safe_push (i);
>       }
>   
> +  const char *str = gimple_asm_string (stmt);
> +  if (error_seen)
> +    {
> +      ninputs = 0;
> +      noutputs = 0;
> +      inout_opnum.truncate (0);
> +      output_rvec.truncate (0);
> +      clobber_rvec.truncate (0);
> +      constraints.truncate (0);
> +      CLEAR_HARD_REG_SET (clobbered_regs);
> +      str = "";
> +    }
> +
>     auto_vec<rtx, MAX_RECOG_OPERANDS> input_rvec;
>     auto_vec<machine_mode, MAX_RECOG_OPERANDS> input_mode;
>   
> @@ -3405,7 +3432,7 @@ expand_asm_stmt (gasm *stmt)
>       }
>   
>     /* For in-out operands, copy output rtx to input rtx.  */
> -  unsigned ninout = inout_opnum.length();
> +  unsigned ninout = inout_opnum.length ();
>     for (i = 0; i < ninout; i++)
>       {
>         int j = inout_opnum[i];
> @@ -3459,7 +3486,7 @@ expand_asm_stmt (gasm *stmt)
>   
>     rtx body = gen_rtx_ASM_OPERANDS ((noutputs == 0 ? VOIDmode
>   				    : GET_MODE (output_rvec[0])),
> -				   ggc_strdup (gimple_asm_string (stmt)),
> +				   ggc_strdup (str),
>   				   "", 0, argvec, constraintvec,
>   				   labelvec, locus);
>     MEM_VOLATILE_P (body) = gimple_asm_volatile_p (stmt);
> --- gcc/c/c-typeck.c.jj	2021-06-10 16:45:43.953519765 +0200
> +++ gcc/c/c-typeck.c	2021-06-11 13:32:33.953983430 +0200
> @@ -5034,8 +5034,17 @@ c_mark_addressable (tree exp, bool array
>   	    && TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
>   	    && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (x, 0))))
>   	  return true;
> -	/* FALLTHRU */
> +	x = TREE_OPERAND (x, 0);
> +	break;
> +
>         case COMPONENT_REF:
> +	if (DECL_C_BIT_FIELD (TREE_OPERAND (x, 1)))
> +	  {
> +	    error ("cannot take address of bit-field %qD",
> +		   TREE_OPERAND (x, 1));
> +	    return false;
> +	  }
> +	/* FALLTHRU */
>         case ADDR_EXPR:
>         case ARRAY_REF:
>         case REALPART_EXPR:
> --- gcc/cp/typeck.c.jj	2021-05-31 10:11:15.150978892 +0200
> +++ gcc/cp/typeck.c	2021-06-11 13:38:37.850984608 +0200
> @@ -7119,9 +7119,14 @@ cxx_mark_addressable (tree exp, bool arr
>   	    && TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
>   	    && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (x, 0))))
>   	  return true;
> +	x = TREE_OPERAND (x, 0);
> +	break;
> +
> +      case COMPONENT_REF:
> +	if (bitfield_p (x))
> +	  error ("attempt to take address of bit-field");
>   	/* FALLTHRU */
>         case ADDR_EXPR:
> -      case COMPONENT_REF:
>         case ARRAY_REF:
>         case REALPART_EXPR:
>         case IMAGPART_EXPR:
> --- gcc/testsuite/c-c++-common/pr100785.c.jj	2021-06-11 13:27:51.215867378 +0200
> +++ gcc/testsuite/c-c++-common/pr100785.c	2021-06-11 13:27:51.215867378 +0200
> @@ -0,0 +1,21 @@
> +/* PR inline-asm/100785 */
> +
> +struct S { int a : 1; };
> +
> +void
> +foo (struct S *x)
> +{
> +  __asm__ ("" : "+m" (x->a));	/* { dg-error "address of bit-field" } */
> +}
> +
> +void
> +bar (struct S *x)
> +{
> +  __asm__ ("" : "=m" (x->a));	/* { dg-error "address of bit-field" } */
> +}
> +
> +void
> +baz (struct S *x)
> +{
> +  __asm__ ("" : : "m" (x->a));	/* { dg-error "address of bit-field" } */
> +}
> --- gcc/testsuite/gcc.dg/pr48552-1.c.jj	2021-06-02 10:07:47.737825068 +0200
> +++ gcc/testsuite/gcc.dg/pr48552-1.c	2021-06-11 13:27:51.228867199 +0200
> @@ -15,7 +15,7 @@ f2 (void *x)
>   {
>     __asm volatile ("" : "=r" (*x));	/* { dg-warning "dereferencing" "deref" } */
>   }					/* { dg-error "invalid use of void expression" "void expr" { target *-*-* } .-1 } */
> -					/* { dg-error "invalid lvalue in 'asm' output 0" "invalid lvalue" { target *-*-* } .-2 } */
> +
>   void
>   f3 (void *x)
>   {
> @@ -39,7 +39,7 @@ f6 (void *x)
>   {
>     __asm volatile ("" : "=g" (*x));	/* { dg-warning "dereferencing" "deref" } */
>   }					/* { dg-error "invalid use of void expression" "void expr" { target *-*-* } .-1 } */
> -					/* { dg-error "invalid lvalue in 'asm' output 0" "invalid lvalue" { target *-*-* } .-2 } */
> +
>   void
>   f7 (struct S *x)
>   {
> --- gcc/testsuite/gcc.dg/pr48552-2.c.jj	2021-06-02 10:07:47.773824557 +0200
> +++ gcc/testsuite/gcc.dg/pr48552-2.c	2021-06-11 13:27:51.233867130 +0200
> @@ -15,7 +15,7 @@ f2 (void *x)
>   {
>     __asm ("" : "=r" (*x));	/* { dg-warning "dereferencing" "deref" } */
>   }				/* { dg-error "invalid use of void expression" "void expr" { target *-*-* } .-1 } */
> -				/* { dg-error "invalid lvalue in 'asm' output 0" "invalid lvalue" { target *-*-* } .-2 } */
> +
>   void
>   f3 (void *x)
>   {
> @@ -39,7 +39,7 @@ f6 (void *x)
>   {
>     __asm ("" : "=g" (*x));	/* { dg-warning "dereferencing" "deref" } */
>   }				/* { dg-error "invalid use of void expression" "void expr" { target *-*-* } .-1 } */
> -				/* { dg-error "invalid lvalue in 'asm' output 0" "invalid lvalue" { target *-*-* } .-2 } */
> +
>   void
>   f7 (struct S *x)
>   {
> 
> 
> 	Jakub
>

On Fri, 11 Jun 2021, Jakub Jelinek via Gcc-patches wrote:

> gcc/c/
> 	* c-typeck.c (c_mark_addressable): Diagnose trying to make
> 	bit-fields addressable.

The C front-end changes are OK.