Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Fix an assertion failure in unwrapSugar
ClosedPublic

Authored by ahatanak on Mar 30 2023, 1:18 PM.

Details

Summary

An assertion in Qualifiers::addObjCLifetime fails when the ObjC lifetime bits are already set.

Instead of calling operator+=, call addConsistentQualifiers, which allows the lifetime bits to be set again as long the new value doesn't conflict with the old value.

This fixes https://github.com/llvm/llvm-project/issues/61419.

Diff Detail

Event Timeline

ahatanak created this revision.Mar 30 2023, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 1:18 PM
ahatanak requested review of this revision.Mar 30 2023, 1:18 PM
mizvekov added inline comments.Apr 12 2023, 4:56 AM
clang/test/SemaObjCXX/arc-objc-lifetime.mm
69–79 ↗(On Diff #509793)

I think the fix looks good, although this test may be out of place in this file.

Ideally, if I had thought of this test case, it would have gone into clang/test/SemaCXX/sugared-auto.cpp.

The existing tests there would try to trigger this bug through return type deduction, and not through the ternary operator as in your reduction.

A second good option would be to add it to clang/test/SemaCXX/sugar-common-types.cpp, as that tests ternary operator cases, but you would have to extend the compile flags to accept the ObjC++ stuff.

Also, if you want to add an isolated test for a bug in a file where it otherwise doesn't follow the style of the existing tests, we would isolate it in a namespace named after the PR.

Something like:

namespace PR61419 {
....
} // namespace PR61419

This makes it easier to extend and not accidentally break existing tests.

Also, it looks like this test could be a bit more minimal, as suggested in the edit.

This revision is now accepted and ready to land.Apr 12 2023, 5:03 AM
ahatanak updated this revision to Diff 513010.Apr 12 2023, 4:40 PM
ahatanak marked an inline comment as done.

Simplify test case and move it to clang/test/SemaCXX/sugar-common-types.cpp.

This revision was landed with ongoing or failed builds.Apr 12 2023, 4:48 PM
This revision was automatically updated to reflect the committed changes.