[patch] Check that we don't try to RAUW a value with a ConstantExpr that uses Value.

[Duncan P. N. Exon Smith]

On 2014-May-12, at 12:21, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

> I am pretty sure this is an invalid use of RAUW. The current
> implementation also goes in an infinite loop if that happens.

Even if we wanted to support that, the assert is strictly better than
an infinite loop.

I wonder if you can make this cheaper, though.  Am I right that all
sub-exprs of a `ConstantExpr` must also be `ConstantExpr`s?  If so:

> diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp
> index 2ebdb70..db41d76 100644
> --- a/lib/IR/Value.cpp
> +++ b/lib/IR/Value.cpp
> @@ -301,10 +301,33 @@ void Value::takeName(Value *V) {
>      ST->reinsertValue(this);
>  }
>  
> +#ifndef NDEBUG
> +static bool contains(SmallPtrSet<Value *, 4> &Cache, Value *Expr, Value *V) {

You can change to this:

    static bool contains(SmallPtrSet<ConstantExpr *, 4> &Cache, ConstantExpr *Expr,
                         ConstantExpr *V) {

> +  if (!Cache.insert(Expr))
> +    return false;
> +
> +  if (Expr == V)
> +    return true;
> +  auto *CE = dyn_cast<ConstantExpr>(Expr);

No need for CE anymore, since `Expr` is `ConstantExpr*`.

> +  if (!CE)
> +    return false;
> +  for (auto &O : CE->operands()) {
> +    if (contains(Cache, O, V))

Change this to:

    if (contains(Cache, cast<ConstantExpr>(O), V))

> +      return true;
> +  }
> +  return false;
> +}
> +
> +static bool contains(Value *Expr, Value *V) {
> +  SmallPtrSet<Value *, 4> Cache;
> +  return contains(Cache, Expr, V);

Check up front if they're both `ConstantExpr`; otherwise there's no
need for the recursive descent.

    if (auto *CE = dyn_cast<ConstantExpr>(Expr))
      if (auto *CV = dyn_cast<ConstantExpr>(V)) {
        SmallPtrSet<ConstantExpr *, 4> Cache;
        return contains(Cache, CE, CV);
      }
    return Expr == V;

> +}
> +#endif
>  
>  void Value::replaceAllUsesWith(Value *New) {
>    assert(New && "Value::replaceAllUsesWith(<null>) is invalid!");
> -  assert(New != this && "this->replaceAllUsesWith(this) is NOT valid!");
> +  assert(!contains(New, this) &&
> +         "this->replaceAllUsesWith(expr(this)) is NOT valid!");
>    assert(New->getType() == getType() &&
>           "replaceAllUses of value with new value of different type!");
>  
>