Re: [PATCH] Dissociate store_expr's temp from exp so that it is not marked as addressable

[Richard Guenther <rguenther at suse dot de>]

On Sat, 31 Mar 2012, Martin Jambor wrote:

> Hi,
> 
> On Fri, Mar 30, 2012 at 10:03:59AM +0200, Richard Guenther wrote:
> > On Fri, 30 Mar 2012, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > when testing a patch of mine on sparc64-linux, I came across an Ada
> > > bootstrap failure due to a structure DECL which was marked addressable
> > > but had a register DECL_RTL (and therefore mem_ref_refers_to_non_mem_p
> > > failed to trigger on it).
> > > 
> > > Mode of the structure was TI (16 bytes int) and it was mistakenly
> > > marked as addressable during expansion of an assignment statement in
> > > which a 12 byte portion of it was copied to another structure with
> > > BLKmode.  Specifically, this happened because expand_assignment called
> > > store_expr which loaded the required portion to temporary 12 byte
> > > BLKmode MEM_P variable and then called emit_block_move from the
> > > temporary to the destination.  emit_block_move_hints then marked
> > > MEM_EXPR of the temp as addressable because it handled the copy by
> > > emitting a library call.  And MEM_EXPR pointed to the DECL of the
> > > source of the assignment which I believe is the bug, thus this patch
> > > re-sets MEM_EXPR of temp in these cases.
> > > 
> > > However, if anybody believes the main issue is elsewhere and another
> > > component of this chain of events needs to be fixed, I'll be happy to
> > > come up with another patch.  so far this patch has passed bootstrap
> > > and testing on x86_64-linux and helped my patch which uncovered this
> > > issue to reach stage 3 of bootstrap.
> > > 
> > > What do you think, is it OK for trunk?
> > 
> > Hmm, I think this should be done at the point we create the mem - where
> > does that happen?
> 
> The function gets it by simply calling expand_expr_real:
> 
>       temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
> 			       (call_param_p
> 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
> 			       &alt_rtl);
> 
> I have reproduced the issue again and looked at how expand_expr_real_1
> comes up with the MEM attributes in the COMPONENT_REF case.
> The memory-backed temporary is created in:
> 
> 	/* Otherwise, if this is a constant or the object is not in memory
> 	   and need be, put it there.  */
> 	else if (CONSTANT_P (op0) || (!MEM_P (op0) && must_force_mem))
> 	  {
> 	    tree nt = build_qualified_type (TREE_TYPE (tem),
> 					    (TYPE_QUALS (TREE_TYPE (tem))
> 					     | TYPE_QUAL_CONST));
> 	    memloc = assign_temp (nt, 1, 1, 1);
> 	    emit_move_insn (memloc, op0);
> 	    op0 = memloc;
> 	  }
> 
> and at the end of the  case, the bitpos is added by adjust_address,
> followed by set_mem_attributes (op0, exp, 0);
> 
> Indeed it seems that we should not be calling set_mem_attributes if
> op0 is based on such temporary... or perhaps just make sure that we
> only clear MEM_EXPR afterwards?

Yes, either way I suppose.  The following also looks dangerous to me:

        /* If OFFSET is making OP0 more aligned than BIGGEST_ALIGNMENT,
           record its alignment as BIGGEST_ALIGNMENT.  */
        if (MEM_P (op0) && bitpos == 0 && offset != 0
            && is_aligning_offset (offset, tem))
          set_mem_align (op0, BIGGEST_ALIGNMENT);

Maybe we can fall through most of the rest of the function if we
canonicalized in the above way?  Eric?

Thanks,
Richard.