Page MenuHomePhabricator

Do not call replaceAllUsesWith to upgrade calls to ARC runtime functions to intrinsic calls
ClosedPublic

Authored by ahatanak on Aug 9 2019, 6:58 PM.

Details

Summary

This fixes a bug in r368311.

It turns out that the ARC runtime functions in the IR can have pointer parameter types that are not i8* or i8**. Instead of calling replaceAllUsesWith, manually bitcast the arguments and return values before and after calling the intrinsic functions.

rdar://problem/54125406

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Aug 9 2019, 6:58 PM
rjmccall accepted this revision.Aug 12 2019, 10:26 AM

LGTM, although I'm not generally familiar with the upgrader code.

llvm/lib/IR/AutoUpgrade.cpp
3871

Intrinsic functions can't be used in any way except calling them, right?

This revision is now accepted and ready to land.Aug 12 2019, 10:26 AM
steven_wu added inline comments.Aug 12 2019, 10:32 AM
llvm/lib/IR/AutoUpgrade.cpp
3857

I probably missed this from previous commit, shouldn't this function return bool?

ahatanak marked 2 inline comments as done.Aug 12 2019, 11:21 AM
ahatanak added inline comments.
llvm/lib/IR/AutoUpgrade.cpp
3857

I changed the return type to void in a follow-up commit since nothing seemed to be using the return value. Do these functions have to return anything?

3871

Intrinsic functions can only be called, but it's possible to use ObjC ARC functions without calling it. For example, if the code below was compiled by clang and the auto-upgrader was reading the bitcode, the check would prevent the auto-upgrader from replacing the reference to @objc_autorelease:

id objc_autorelease(id);

void foo2(void *);

void foo1(void) {
  //   call void @foo2(i8* bitcast (i8* (i8*)* @objc_autorelease to i8*))
  foo2(&objc_autorelease);
}

I'm not sure why anyone would do something like this, but currently clang doesn't reject it and I don't think we can rule out the possibility that someone is generating an IR like this.

Ah, yes, if this isn't just working on intrinsic functions then that makes total sense.

steven_wu accepted this revision.Aug 12 2019, 12:26 PM
steven_wu added inline comments.
llvm/lib/IR/AutoUpgrade.cpp
3857

I guess it is just the convention and the return value is not used anymore. This is fine.

ahatanak closed this revision.Aug 12 2019, 4:55 PM

Thanks, committed in r368634.