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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked an inline comment as done.
rampitec added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:543
+    Use &U = *UI;
+    ++UI;
+    if (!ShouldReplace(U))
----------------
efriedma wrote:
> 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.
Thanks! A good idea, here is the patch: D105061
For the handleOperandChange() I have left a FIXME for now in the same review. This is a very good point, although not essential for the current use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103051