Prevent MDNode's RAUW from introducing a reference to a void Value.

[friss]

Summary

It seems this isn't allowed: the IR dumper would render it as !{void <badref>}
and it would assert when serialized to bitcode.

The specifc case that triggers the wrong behavior is related to debug
information (as shown in the testcase), but the fix is in the general metadata
code.

[echristo]

How'd we get the void anyhow?

[friss]

If you look at the added testcase, it cast a void (*func)() to a int (*func)(). The int typed result is referenced in a debug info metadata node (an argument to the dbg.value intrinsic). Then instcombine would remove the bitcast and turn the int returning function into a void returning function that gets RAUWd over the value in the MDNode. And at this point in the current code you have invalid IR.

The code in the testcase is reduced from a huge LTO link that encountered this situation in a much more complicated setup.

[friss]

New try to fix this issue with a different stategy: add an assert to enforce
that ValueHandleBase::ValueIsRAUWd isn't called with a new value that hasn't
the same type as the old one. Fix InstCombine to not violate the invariant.

How does that look?

[dblaikie]

Generally looks OK - but I'll leave it to someone more confident with the ValueHandle, RAUW, etc semantics to sign off on.

Ideally I'd probably like a more valid test case as, if we optimize away this scenario at some point the test case might become invalid (either testing parts of instcombine we don't actually ever need it to handle, or maybe instcombine would just eliminate such instructions - in which case this test case would no longer be testing this codepath, for example)

[friss]

Actually I should be able to easily extend the testcase to include a valid call to a bitcasted function type also (the fix changed scope, and the new one should be easier to test). I'd like to leave the UB call there for now as it is the initial issue that I chased, the one that caused an assert during bitcode serialization.

[friss]

Seems I was wrong about being able to extend the testcase. Changing the scope of the fix didn't change the fact that I need a constant bitcast inside a call to trigger that code path. And I'm not able to produce this without resorting to UB at the C level.

Unless you have another idea, I think I'll commit the test I have.

[dblaikie]

In D5828#10, @friss wrote:

Seems I was wrong about being able to extend the testcase. Changing the scope of the fix didn't change the fact that I need a constant bitcast inside a call to trigger that code path. And I'm not able to produce this without resorting to UB at the C level.

Unless you have another idea, I think I'll commit the test I have.

Nope, no particular ideas short of mucking with the assertion (perhaps so it triggers on anything other than this case) & running over a selfhost or other corpus to see if anything more realistic/reasonable/future-proof crops up.

Not a requirement, just a desire/preference if it were possible/practical.

[friss]

Closed by commit rL220468 (authored by @friss).

llvm/trunk/lib/IR/Value.cpp

#endif
     llvm_unreachable("All references to V were not removed?");
   }
 }
 
 
 void ValueHandleBase::ValueIsRAUWd(Value *Old, Value *New) {
   assert(Old->HasValueHandle &&"Should only be called if ValueHandles present");
   assert(Old != New && "Changing value into itself!");
+  assert(Old->getType() == New->getType() &&
+         "replaceAllUses of value with new value of different type!");
 
   // Get the linked list base, which is guaranteed to exist since the
   // HasValueHandle flag is set.
   LLVMContextImpl *pImpl = Old->getContext().pImpl;
   ValueHandleBase *Entry = pImpl->ValueHandles[Old];
 
   assert(Entry && "Value bit set but no entries exist");
 

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

if (OldRetTy != NewRetTy) {
 
     if (!CastInst::isBitCastable(NewRetTy, OldRetTy)) {
       if (Callee->isDeclaration())
         return false;   // Cannot transform this return value.
 
       if (!Caller->use_empty() &&
           // void -> non-void is handled specially
           !NewRetTy->isVoidTy())
         return false;   // Cannot transform this return value.
     }
 
     if (!CallerPAL.isEmpty() && !Caller->use_empty()) {
       AttrBuilder RAttrs(CallerPAL, AttributeSet::ReturnIndex);
       if (RAttrs.
           hasAttributes(AttributeFuncs::
                         typeIncompatible(NewRetTy, AttributeSet::ReturnIndex),
                         AttributeSet::ReturnIndex))
if (!NV->getType()->isVoidTy()) {
       Worklist.AddUsersToWorkList(*Caller);
     } else {
       NV = UndefValue::get(Caller->getType());
     }
   }
 
   if (!Caller->use_empty())
     ReplaceInstUsesWith(*Caller, NV);
-  else if (Caller->hasValueHandle())
+  else if (Caller->hasValueHandle()) {
+    if (OldRetTy == NV->getType())
       ValueHandleBase::ValueIsRAUWd(Caller, NV);
+    else
+      // We cannot call ValueIsRAUWd with a different type, and the
+      // actual tracked value will disappear.
+      ValueHandleBase::ValueIsDeleted(Caller);
+  }
 
   EraseInstFromFunction(*Caller);
   return true;
 }
 
 // transformCallThroughTrampoline - Turn a call to a function created by
 // init_trampoline / adjust_trampoline intrinsic pair into a direct call to the
 // underlying function.