Page MenuHomePhabricator

[ObjCARC] Require the function argument in the clang.arc.attachedcall bundle.
ClosedPublic

Authored by ab on Jan 25 2022, 9:00 PM.

Details

Summary

Currently, the clang.arc.attachedcall bundle takes an optional function
argument. Depending on whether the argument is present, calls with this
bundle have the following semantics:

  • on x86, with the argument present, the call is lowered to:
call _target
mov rax, rdi
call _objc_retainAutoreleasedReturnValue
  • on AArch64, without the argument, the call is lowered to:
bl _target
mov x29, x29

and the objc runtime call is expected to be emitted separately.

That's because, on x86, the objc runtime checks for both the mov and
the call on x86, and treats the combination as the ARC autorelease elision
marker.

But on AArch64, it only checks for the dedicated NOP marker, as that's
historically been sufficiently unique. Thanks to that, the runtime call
wasn't required to be adjacent to the NOP marker, so it wasn't emitted
as part of the bundle sequence.

This patch unifies both architectures: on AArch64, we now emit all
3 instructions for the bundle. This guarantees that the runtime call
is adjacent to the marker in the sequence, and that's information the
runtime can use to further optimize this.

This helps simplify some of the handling, in particular
BundledRetainClaimRVs, which no longer needs to know whether the bundle
is sufficient or not: it now always should be.

Note that this includes an AutoUpgrade that simply drops
clang.arc.attachedcall bundles that don't have an operand: if they
don't, we know that the semantically required call is already a separate
instruction, and the bundle is merely a NOP.
It's not ideal to drop it, but it's okay.

Diff Detail

Event Timeline

ab created this revision.Jan 25 2022, 9:00 PM
ab requested review of this revision.Jan 25 2022, 9:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2022, 9:00 PM
ab edited the summary of this revision. (Show Details)Jan 25 2022, 9:01 PM
fhahn accepted this revision.Jan 26 2022, 9:13 AM

LGTM, thanks! For the Transforms/ObjCARC stuff it would be good for Akira to also take a quick look.

llvm/lib/IR/AutoUpgrade.cpp
4633 ↗(On Diff #403128)

Any reason not to use a SmallVector?

4634 ↗(On Diff #403128)

nit: stray new line?

This revision is now accepted and ready to land.Jan 26 2022, 9:13 AM

The ObjC ARC changes look good.

Are there any users who need the changes in autoupgrade? clang.arc.attachedcall bundles without operands are emitted only after ARC contract (ARC optimizer does't remove the operand), so it seems safe not to upgrade the IR.

ab added a comment.Jan 28 2022, 11:49 AM

Are there any users who need the changes in autoupgrade? clang.arc.attachedcall bundles without operands are emitted only after ARC contract (ARC optimizer does't remove the operand), so it seems safe not to upgrade the IR.

That's an interesting point. I looked for where the pass was inserted in the pipeline, and I thought it did run in the frontend when doing LTO, but digging a bit more, it sounds like it really only is before the backend, with or without LTO. So I think you're right, we don't need this after all! (and hopefully there are no non-clang users of the bundle with different expectations).

This revision was landed with ongoing or failed builds.Jan 28 2022, 12:42 PM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Mar 18 2022, 4:23 PM

@ab We started seeing crashes in Flutter Engine on iOS after the latest Clang roll. We did a bisect and found this change to be a culprit, see https://github.com/flutter/flutter/issues/99488 for more details. Would it be possible to take a look?

Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 4:23 PM

@ab We started seeing crashes in Flutter Engine on iOS after the latest Clang roll. We did a bisect and found this change to be a culprit, see https://github.com/flutter/flutter/issues/99488 for more details. Would it be possible to take a look?

Gentle ping, would it be fine if reverted this change in the meantime?

ab added a comment.Mar 23 2022, 11:55 AM

@ab We started seeing crashes in Flutter Engine on iOS after the latest Clang roll. We did a bisect and found this change to be a culprit, see https://github.com/flutter/flutter/issues/99488 for more details. Would it be possible to take a look?

Hm, there's not much to go on, is there any additional information aside from the partial stacktrace? Happy to look into this if y'all can reduce this to something manageable. Thanks!

ab added a comment.Mar 23 2022, 12:06 PM

Ah, further down, there's a different crash signature in bisect6.txt, in dyld strlen: that one does ring a bell. I assume it's handling a corrupt selector, but there's not much context to know for sure. I have a later change that may take care of that, hopefully that resolves your original crash as well!

In D118214#3403398, @ab wrote:

Ah, further down, there's a different crash signature in bisect6.txt, in dyld strlen: that one does ring a bell. I assume it's handling a corrupt selector, but there's not much context to know for sure. I have a later change that may take care of that, hopefully that resolves your original crash as well!

Thanks, which change is that? We can give it a try to see if it resolves the issue.

FWIW, We're seeing Firefox crashes on aarch64 macos that have been bisected to this change as well. Top frames look like:

Thread 94 VideoCapture (crashed)
 0  libsystem_platform.dylib!_platform_strlen + 0x4
      x0 = 0x00000000018963bf     x1 = 0x00000000018963b0
      x2 = 0x0000000000000001     x3 = 0x000000005650a7e3
      x4 = 0x0000000000000000     x5 = 0x0000000000000000 
      x6 = 0x0000000000000065     x7 = 0x0000000000000000
      x8 = 0x00000001e2a220c8     x9 = 0x00000000593e20c8
     x10 = 0x00000001f405db98    x11 = 0x0000000189494000
     x12 = 0x0000000217e4c000    x13 = 0x0000000100000000
     x14 = 0x000000000000000c    x15 = 0x000000018963bca1
     x16 = 0x00000001897c2fa0    x17 = 0x00000001ef3c5dc0
     x18 = 0x0000000000000000    x19 = 0x00000000018963bf
     x20 = 0x00000001e2a220c8    x21 = 0x0000000000000001
     x22 = 0x00000000018963bf    x23 = 0x00000001f3a6d898
     x24 = 0x00000001dfb46c13    x25 = 0x0000000000000000
     x26 = 0x00000001dfb46c42    x27 = 0x00000000ffffffff
     x28 = 0x0000000000000000     fp = 0x0000000172aba2a0
      lr = 0x000000018978586c     sp = 0x0000000172aba280
      pc = 0x00000001897c2fa4
    Found by: given as instruction pointer in context
 1  libdyld.dylib!objc_opt::objc_stringhash_t::getIndex(char const*) const + 0x20
      sp = 0x0000000172aba2b0     pc = 0x000000018978586c
    Found by: previous frame's frame pointer
 2  libdyld.dylib!_dyld_get_objc_selector + 0x3c
      sp = 0x0000000172aba2d0     pc = 0x00000001897857dc
    Found by: previous frame's frame pointer
 3  libobjc.A.dylib!__sel_registerName(char const*, bool, bool) + 0x2c
      sp = 0x0000000172aba320     pc = 0x00000001896367e8
    Found by: previous frame's frame pointer
 4  CoreFoundation!___forwarding___ + 0x54c
      sp = 0x0000000172aba490     pc = 0x0000000189858130
    Found by: previous frame's frame pointer
 5  CoreFoundation!__forwarding_prep_0___ + 0x5c
      sp = 0x0000000172aba570     pc = 0x0000000189857b30
    Found by: previous frame's frame pointer
 6  XUL!-[DeviceInfoIosObjC configureObservers] [device_info_objc.mm:6475d6ae523de8e02c1e5c20ddaab4593cc16495 : 131 + 0x14]
      sp = 0x0000000172aba650     pc = 0x00000001057860a0
    Found by: previous frame's frame pointer
 7  XUL!webrtc::videocapturemodule::DeviceInfoIos::Init() [device_info.mm:6475d6ae523de8e02c1e5c20ddaab4593cc16495 : 42 + 0x8]
     x19 = 0x000000014abdfdf0    x20 = 0x000000014a4457c0
     x21 = 0xdb95bce6c9860006    x22 = 0x000000010ab71c30
     x23 = 0x000000014a9e4d40    x24 = 0x00000000000041b7
     x25 = 0x00000001057856ac    x26 = 0x0000000172aba680
     x27 = 0x000000014a9e4d40    x28 = 0x000000014a9e4d40
      fp = 0x0000000172abaa70     sp = 0x0000000172aba690
      pc = 0x0000000105784a08
    Found by: call frame info
 8  XUL!webrtc::videocapturemodule::VideoCaptureImpl::CreateDeviceInfo() [device_info.mm:6475d6ae523de8e02c1e5c20ddaab4593cc16495 : 35 + 0x44]
     x19 = 0x000000014abdfdf0    x20 = 0x000000014a4457c0
     x21 = 0x000000014abdfe80    x22 = 0x000000010a41723c
ab added a comment.Apr 12 2022, 10:41 AM

D'oh, this fell through the cracks, sorry folks! I committed the fix I had in mind (cfa4fe7c5187). Long story short: this change exposed an old bug in the LOH pass: it wasn't honoring regmasks in bundled MIs, but since we used to split the bundle and emit the attached call separately, the (unbundled) latter was still visible to the pass. That's not true as of this change. I think for the sort of code that uses bundles on aarch64 it's usually selectors that get corrupted in a later msgSend, and that ends up exploding in dyld the way we're seeing in both cases.

In D118214#3446233, @ab wrote:

D'oh, this fell through the cracks, sorry folks! I committed the fix I had in mind (cfa4fe7c5187).

I can confirm this fixes the issue for Firefox. I guess it should be queued for 14.0.1?

ab added a comment.Apr 14 2022, 11:09 AM

Sure, filed 54914 to cherry-pick into 14.0.2