Is the check performed on the return value from force_const_mem in the plus_constant function correct?

[Jeff Law]

On 06/02/2015 05:22 AM, Andrew Bennett wrote:
> Hi,
>
> I am debugging a segmentation fault when compiling some code for MIPS.  The
> segmentation fault occurs in the plus_constant function in explow.c when it
> tries to update a constant pool value.  The offending code is below:
>
> case MEM:
>        /* If this is a reference to the constant pool, try replacing it with
>           a reference to a new constant.  If the resulting address isn't
>           valid, don't return it because we have no way to validize it.  */
>        if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
>            && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
>          {
>            tem = plus_constant (mode, get_pool_constant (XEXP (x, 0)), c);
>            tem = force_const_mem (GET_MODE (x), tem);
>            if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
>              return tem;
>          }
>
>
> The code fails for MIPS because its cannot_force_const_mem target function does
> not allow constants (so that the move expanders can deal with them later on),
> this then causes the force_const_mem function to return a NULL_RTX.
>
> What I can't understand is why the memory_address_p function is used to check
> the result from force_const_mem?  From looking at the return values from
> force_const_mem the only possibilies are what looks like a valid memory rtx,
> or a NULL_RTX.  Also there is no other instances in the code where this happens.
> I was therefore wondering is anyone knew why the check is done in this manner?
Right, force_const_mem will return a (mem ...)) or NULL_RTX.

In the case of (mem ...), the validity of the memory address is not 
checked by force_const_mem.  That's what the call to memory_address_p 
you cited above does.  If the address is valid for the target, then we 
can return the newly created (mem ...) from plus_constant.


> I think the fix should be to just check that the tem variable is not a NULL_RTX.
> I was also wondering if anyone had any issues/objections to checking the result
> in this manner?
Something like this seems right to me

/* Targets may disallow some constants in the constant pool, thus
    force_const_mem may return NULL_RTX.  */
if (tem && memory_address_p (...))

Jeff