Re: [patch] for PR 17531

[Richard Henderson <rth at redhat dot com>]

Since you refused to give me any real details about this bug, I was 
forced to debug it myself.  Not surprisingly, I find the problem to
be elsewhere.

Specifically, the first place I can identify where things go wrong is
in expand_expr_addr_expr_1, at 

          result = expand_simple_binop (tmode, PLUS, result, tmp, subtarget,
                                        1, OPTAB_LIB_WIDEN);

with tmode=HImode, and result and tmp having SImode.  The small tmode
gets propagated down from the cast expanding the inner expression.
The Pmode operands come from expand_expr deciding not to honor the
tmode that it was given.  Which while legal according to the description
of expand_expr, is sounding more and more questionable to me.

A further bug would have appeared for a different target because 
convert_memory_address only works on pointer modes, and HImode does 
not qualify.

My solution is to be more careful about what modes we allow during 
computation of the actual address.  If there's an outer cast, then
narrowing the mode gets taken care of by the cast.

I'm in the process of testing this on ia64, alphaev6 and i686 linux,
but it really should be tested on a target for which Pmode != ptr_mode.
One example would be ia64-hpux; it's possible that ppc64-darwin would
qualify, but I havn't checked.


r~


	* expr.c (expand_expr_addr_expr_1): Don't assemble_external for
	non-decls.  Don't handle tmode == VOIDmode.  Assert that the 
	modes are correct when generating a PLUS.
	(expand_expr_addr_expr): Move VOIDmode check earlier.  Force tmode
	to Pmode for invalid pointer modes.

Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.723
diff -c -p -d -r1.723 expr.c
*** expr.c	23 Sep 2004 14:34:15 -0000	1.723
--- expr.c	27 Sep 2004 21:58:24 -0000
*************** expand_expr_addr_expr_1 (tree exp, rtx t
*** 6088,6094 ****
      case CONST_DECL:
        /* Recurse and make the output_constant_def clause above handle this.  */
        return expand_expr_addr_expr_1 (DECL_INITIAL (exp), target,
! 				    tmode, modifier);
  
      case REALPART_EXPR:
        /* The real part of the complex number is always first, therefore
--- 6088,6094 ----
      case CONST_DECL:
        /* Recurse and make the output_constant_def clause above handle this.  */
        return expand_expr_addr_expr_1 (DECL_INITIAL (exp), target,
! 				      tmode, modifier);
  
      case REALPART_EXPR:
        /* The real part of the complex number is always first, therefore
*************** expand_expr_addr_expr_1 (tree exp, rtx t
*** 6126,6132 ****
  	  result = XEXP (result, 0);
  
  	  /* ??? Is this needed anymore?  */
! 	  if (!TREE_USED (exp) == 0)
  	    {
  	      assemble_external (exp);
  	      TREE_USED (exp) = 1;
--- 6126,6132 ----
  	  result = XEXP (result, 0);
  
  	  /* ??? Is this needed anymore?  */
! 	  if (DECL_P (exp) && !TREE_USED (exp) == 0)
  	    {
  	      assemble_external (exp);
  	      TREE_USED (exp) = 1;
*************** expand_expr_addr_expr_1 (tree exp, rtx t
*** 6149,6161 ****
    subtarget = offset || bitpos ? NULL_RTX : target;
    result = expand_expr_addr_expr_1 (inner, subtarget, tmode, modifier);
  
-   if (tmode == VOIDmode)
-     {
-       tmode = GET_MODE (result);
-       if (tmode == VOIDmode)
- 	tmode = Pmode;
-     }
- 
    if (offset)
      {
        rtx tmp;
--- 6149,6154 ----
*************** expand_expr_addr_expr_1 (tree exp, rtx t
*** 6164,6169 ****
--- 6157,6165 ----
  	result = force_operand (result, NULL);
        tmp = expand_expr (offset, NULL, tmode, EXPAND_NORMAL);
  
+       gcc_assert (general_operand (result, tmode));
+       gcc_assert (general_operand (tmp, tmode));
+ 
        if (modifier == EXPAND_SUM)
  	result = gen_rtx_PLUS (tmode, result, tmp);
        else
*************** expand_expr_addr_expr_1 (tree exp, rtx t
*** 6178,6184 ****
      {
        /* Someone beforehand should have rejected taking the address
  	 of such an object.  */
!       gcc_assert (!(bitpos % BITS_PER_UNIT));
  
        result = plus_constant (result, bitpos / BITS_PER_UNIT);
        if (modifier < EXPAND_SUM)
--- 6174,6180 ----
      {
        /* Someone beforehand should have rejected taking the address
  	 of such an object.  */
!       gcc_assert ((bitpos % BITS_PER_UNIT) == 0);
  
        result = plus_constant (result, bitpos / BITS_PER_UNIT);
        if (modifier < EXPAND_SUM)
*************** expand_expr_addr_expr (tree exp, rtx tar
*** 6198,6216 ****
    enum machine_mode rmode;
    rtx result;
  
    result = expand_expr_addr_expr_1 (TREE_OPERAND (exp, 0), target,
  				    tmode, modifier);
  
    /* Despite expand_expr claims concerning ignoring TMODE when not
!      strictly convenient, stuff breaks if we don't honor it.  */
!   if (tmode == VOIDmode)
!     tmode = TYPE_MODE (TREE_TYPE (exp));
    rmode = GET_MODE (result);
    if (rmode == VOIDmode)
      rmode = tmode;
    if (rmode != tmode)
      result = convert_memory_address (tmode, result);
!  
    return result;
  }
  
--- 6194,6221 ----
    enum machine_mode rmode;
    rtx result;
  
+   /* Target mode of VOIDmode says "whatever's natural".  */
+   if (tmode == VOIDmode)
+     tmode = TYPE_MODE (TREE_TYPE (exp));
+ 
+   /* We can get called with some Weird Things if the user does silliness
+      like "(short) &a".  In that case, convert_memory_address won't do
+      the right thing, so ignore the given target mode.  */
+   if (!targetm.valid_pointer_mode (tmode))
+     tmode = Pmode;
+ 
    result = expand_expr_addr_expr_1 (TREE_OPERAND (exp, 0), target,
  				    tmode, modifier);
  
    /* Despite expand_expr claims concerning ignoring TMODE when not
!      strictly convenient, stuff breaks if we don't honor it.  Note
!      that combined with the above, we only do this for pointer modes.  */
    rmode = GET_MODE (result);
    if (rmode == VOIDmode)
      rmode = tmode;
    if (rmode != tmode)
      result = convert_memory_address (tmode, result);
! 
    return result;
  }