[PATCH] D103051: [IR] Allow Value::replaceUsesWithIf() to process constants

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:543
+    Use &U = *UI;
+    ++UI;
+    if (!ShouldReplace(U))
----------------
rampitec wrote:
> efriedma wrote:
> > I'm suspicious of the way you're iterating over the use list here; incrementing early isn't enough to avoid a use-after-free, I think.  The incremented `UI` might refer to the same constant.
> > 
> > `doRAUW` repeatedly accesses `*use_begin()` to avoid issues.
> I do not think this is currently exploitable as the only caller with constants `convertConstantExprsToInstructions()` guards it, but this may be true in general. Since not all uses are going to be replaced I cannot do the same thing as `doRAUW`. Do you think this will resolve the potential issue?
> 
> ```
> @@ -527,22 +527,26 @@ void Value::replaceNonMetadataUsesWith(Value *New) {
> 
>  void Value::replaceUsesWithIf(Value *New,
>                                llvm::function_ref<bool(Use &U)> ShouldReplace) {
>    assert(New && "Value::replaceUsesWithIf(<null>) is invalid!");
>    assert(New->getType() == getType() &&
>           "replaceUses of value with new value of different type!");
> 
> +  SmallPtrSet<Constant *, 8> Visited;
>    for (use_iterator UI = use_begin(), E = use_end(); UI != E;) {
>      Use &U = *UI;
>      ++UI;
> +    auto *C = dyn_cast<Constant>(U.getUser());
> +    if (C && !Visited.insert(C).second)
> +      continue;
>      if (!ShouldReplace(U))
>        continue;
>      // Must handle Constants specially, we cannot call replaceUsesOfWith on a
>      // constant because they are uniqued.
> -    if (auto *C = dyn_cast<Constant>(U.getUser())) {
> +    if (C) {
>        if (!isa<GlobalValue>(C)) {
>          C->handleOperandChange(this, New);
>          continue;
>        }
>      }
>      U.set(New);
>    }
> ```
I don't think that's quite right.  The problem is ensuring that UI points to something valid when we execute the `++UI`.  Adding a "continue" after that doesn't really help.

The most straightforward solution I can think of is to dodge the issue by not calling handleOperandChange with an active use_iterator.  Instead, we collect all the constant users into a vector of TrackingVH, then iterate over that.

Also, minor related issue: handleOperandChange updates all the uses in a given Constant, not just the one passed to ShouldReplace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103051/new/

https://reviews.llvm.org/D103051