This is an archive of the discontinued LLVM Phabricator instance.

ObjCARC: Try to fix faulty tests
ClosedPublic

Authored by arsenm on Nov 28 2022, 5:19 AM.

Details

Summary

These were trying to check if there was not an llvm.objc call before a
closing "}", which presumably was intended to match the end of the
function. Really this was matching the closing } in "bitcast {}* %selfto i8*",
which can't be what anyone intended. This broke after converting the test
to opaque pointer deleted this bitcast.

There are in fact @llvm.obj calls remaining in the function, so this
may indicate the transform this was intended to check is actually
broken. In @"\01-[A z]" (great test name), the first retain call seems
to move down to the printf. The second case, @"\01-[Top0 _getX]", has
no change.

Diff Detail

Event Timeline

arsenm created this revision.Nov 28 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 5:19 AM
arsenm requested review of this revision.Nov 28 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 5:19 AM
Herald added a subscriber: wdng. · View Herald Transcript

@ahatanak or @rjmccall, can you confirm whether a bug has crept in while the test was broken? Is a retain/release around @printf and/or @puts expected? Or maybe there's no bug, but the printf/puts declarations need to be annotated somehow?

Regardless, this patch seems like an improvement. It make sense to me to commit this (maybe with FIXMEs in front of those check lines and/or an LLVM bug filed), although for now it'd be good to wait a bit to give them a chance to respond. I assume they're flooded with a holiday email deluge (IIRC, last week would have been a holiday for those at Apple).

llvm/test/Transforms/ObjCARC/basic.ll
2715

Can/should this be CHECK: [^]} to confirm it's a scoping brace instead of a type brace?

ARC optimizer was previously removing the retain/release pair, but it's not clear to me why it was safe to do so. There is a load that loads an ivar of the object before the call to @llvm.objc.release, so the object has to be kept alive at least until that point.

I have to investigate when it stopped removing the pair.

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

arsenm updated this revision to Diff 479730.Dec 2 2022, 1:25 PM
arsenm marked an inline comment as done.

Check brace is at start of line

llvm/test/Transforms/ObjCARC/basic.ll
2715

[^]} doesn't work, but {{^}}} does (which looks like a bug in the check itself)

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

Great progress; seems to me like the regression is old enough it might not be an easy fix. Are you okay with the tests being cleaned up to reflect reality in the meantime?

One option would be this commit as-is (maybe with a comment in the regressed test?). Another alternative would be to move the tests that had bitrotted to a different file, which is XFAIL'ed for now while you investigate how/whether to get them passing again; this has the benefit of making the expected/desired behaviour clear.

WDYT?

llvm/test/Transforms/ObjCARC/basic.ll
2715

Right, right; I misremembered how to get into regex mode. {{^}}} seems correct.

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

Great progress; seems to me like the regression is old enough it might not be an easy fix. Are you okay with the tests being cleaned up to reflect reality in the meantime?

Yes, I think that's okay. We are tracking this internally, so this can be committed as-is.

dexonsmith accepted this revision.Dec 6 2022, 12:56 PM

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

Great progress; seems to me like the regression is old enough it might not be an easy fix. Are you okay with the tests being cleaned up to reflect reality in the meantime?

Yes, I think that's okay. We are tracking this internally, so this can be committed as-is.

In that case, this LGTM.

@arsenm, I'd prefer that you add comments to the two broken cases saying that the optimizer should somehow cleanup those pairs (or move those cases to an XFAIL test), so that if some future person works on this it'll be more clear. Up to you though.

This revision is now accepted and ready to land.Dec 6 2022, 12:56 PM