Page MenuHomePhabricator

Please use GitHub pull requests for new 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

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.