Re: [PATCH, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target

[Jeff Law <law at redhat dot com>]

On 10/09/14 10:54, Uros Bizjak wrote:
> On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> > It appeared I changed a semantics of BNDMK expand when replaced tree operations with rtl ones.
> >
> > Original code:
> >
> > +      op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1),
> > +                                       arg1, integer_minus_one_node));
> > +      op1 = force_reg (Pmode, op1);
> >
> > Modified code:
> >
> > +      op1 = expand_normal (arg1);
> > +
> > +      if (!register_operand (op1, Pmode))
> > +       op1 = ix86_zero_extend_to_Pmode (op1);
> > +
> > +      /* Builtin arg1 is size of block but instruction op1 should
> > +        be (size - 1).  */
> > +      op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
> > +                                op1, 1, OPTAB_DIRECT);
> >
> > The problem is that in the fixed version we may modify value of a pseudo register into which arg1 is expanded which means incorrect value for all following usages of arg1.
> >
> > Didn't reveal it early because programs surprisingly rarely hit this bug.  I do following change to fix it:
> >
> >         op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
> > -                                op1, 1, OPTAB_DIRECT);
> > +                                NULL, 1, OPTAB_DIRECT);
> >
> > Similar problem was also fixed for BNDNARROW.  Does it look OK?
>
> I'm not aware of this type of limitation, and there are quite some
> similar constructs in i386.c. It is hard to say without the testcase
> what happens, perhaps one of RTX experts (CC'd) can advise what is
> recommended here.
The problem is the call to expand_simple_binop.

The source (op1) and the destination (op1) are obviously the same, so 
its going to clobber whatever value is in there.  If there are other 
uses of the original value of op1, then things aren't going to work. 
But I'm a little unclear how there's be other later uses of that value. 
 Perhaps Ilya could comment on that.

Regardless, unless there's a strong reason to do so, I'd generally 
recommend passing a NULL_RTX as the target for expansions so that you 
always get a new pseudo.  Lots of optimizers in the RTL world work 
better if we don't have pseudos with multiple assignments.  By passing 
NULL_RTX for the target we get that property more often.  So a change 
like Ilya suggests (though I'd use NULL_RTX rather than NULL) makes sense.



Jeff