Page MenuHomePhabricator

[Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header
ClosedPublic

Authored by ahatanak on Jul 24 2019, 4:34 PM.

Details

Summary

r365985 stopped marking those fields as unavailable, which caused the union's NonTrivialToPrimitive* bits to be set to true. This patch restores the behavior prior to r365985, except that users can explicitly specify the ownership qualification of the field to instruct the compiler not to mark it as unavailable.

rdar://problem/53420753

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Jul 24 2019, 4:34 PM
ahatanak edited the summary of this revision. (Show Details)Jul 24 2019, 4:36 PM
ahatanak edited the summary of this revision. (Show Details)Jul 24 2019, 4:42 PM
jordan_rose added inline comments.Jul 24 2019, 5:34 PM
lib/Sema/SemaDecl.cpp
16379 ↗(On Diff #211632)

This check seems a little worrisome since we could have some other kind of attributed type. Is there an easy way to check specifically for a lifetime qualifier?

ahatanak marked an inline comment as done.Jul 24 2019, 7:08 PM
ahatanak added inline comments.
lib/Sema/SemaDecl.cpp
16379 ↗(On Diff #211632)

I think I can make IsObjCOwnershipAttributedType in SemaType.cpp a method of ASTContext or Sema and call it here.

ahatanak updated this revision to Diff 211663.Jul 24 2019, 8:00 PM
ahatanak marked an inline comment as done.

Check that the attributed type is an ObjC qualified type. Do not diagnose fields that are unavailable.

ahatanak updated this revision to Diff 211778.Jul 25 2019, 9:19 AM

Mark fields that don't have an explicit __strong qualifier as unavailable.

ahatanak marked an inline comment as done.Jul 25 2019, 9:41 AM
ahatanak added inline comments.
test/SemaObjC/Inputs/non-trivial-c-union.h
10 ↗(On Diff #211778)

Rather than making an exception for explicit __strong fields, should we have an attribute that can be used to instruct the compiler not to mark the field as unavailable?

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

That's the right answer on paper, but it's source-breaking in practice, for both Swift and Objective-C. On the Objective-C side, someone could have been using, say, glob_t in a copyable way in their ARC code, never touching the block field, and it would have been working fine. On the Swift side, we won't be able to import such a union at all when previously we would have.

I'm personally still of the opinion that allowing non-trivial fields in unions was a mistake, but it's too late to change that as well.

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

That's the right answer on paper, but it's source-breaking in practice, for both Swift and Objective-C. On the Objective-C side, someone could have been using, say, glob_t in a copyable way in their ARC code, never touching the block field, and it would have been working fine. On the Swift side, we won't be able to import such a union at all when previously we would have.

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Apparently that was not the case if it was in a system header. Instead, Clang marked the member unavailable rather than the entire union.

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Apparently that was not the case if it was in a system header. Instead, Clang marked the member unavailable rather than the entire union.

Ah, that's unfortunate. It also just seems like a bug.

I guess my questions are whether we're fixing a specific source-compatibility problem here and, if so, whether this is the only reasonable approach for solving it.

Okay, now that I understand the source-compatibility issues a little better, I think this approach is probably okay. If it causes trouble, we can consider special-casing these headers to treat the member as __unsafe_unretained implicitly — the special case isn't great, but it's better than the potential unsoundness.

include/clang/AST/ASTContext.h
2060 ↗(On Diff #211778)

How about something like: "Return true if the type has been explicitly qualified with ObjC ownership. A type may be implicitly qualified with ownership under ObjC ARC, and in some cases the compiler treats these differently".

Could you look for other places where we look for an explicit qualifier? I'm pretty sure I remember that happening once or twice.

lib/Sema/SemaDecl.cpp
11176 ↗(On Diff #211778)

Can we make these exceptions only apply to the attributes we synthesize rather than arbitrary uses of __attribute__((unavailable))? These aren't really good semantics in general.

You can do the check in a well-named function like isSuppressedNonTrivialMember, which would be a good place for a comment explaining what's going on here and why this seemed like the most reasonable solution.

ahatanak updated this revision to Diff 218767.Wed, Sep 4, 12:14 PM
ahatanak marked 3 inline comments as done.

Address review comments.

include/clang/AST/ASTContext.h
2060 ↗(On Diff #211778)

I found that there is a function named hasDirectOwnershipQualifier in SemaType.cpp which also detects explicit ownership qualifiers, so I'm using that instead of isObjCOwnershipAttributedType. The difference between isObjCOwnershipAttributedType is that it detects paren types like I*(__strong x) and doesn't look through typedefs (see the test case in non-trivial-c-union.h).

lib/Sema/SemaDecl.cpp
11176 ↗(On Diff #211778)

We are ignoring unavailable fields here since they don't make the containing union non-trivial and we don't want the user to think that the unavailable field has to be a trivial field in order to fix a compile error. For example, without the check for UnavailableAttr, clang will diagnose the unavailable field f0 when the following code is compiled:

union U3 {
  id f0 __attribute__((unavailable)); // expected-note {{f0 has type '__strong id' that is non-trivial to default-initialize}}
  __weak id f1; // expected-note {{f1 has type '__weak id' that is non-trivial to default-initialize}}
};

void test(void) {
  union U3 b; // expected-error {{cannot default-initialize an object of type 'union U3' since it is a union that is non-trivial to default-initialize}}
}

In that case, does it matter whether the field is explicitly annotated unavailable in the source code or implicitly by the compiler?

rjmccall added inline comments.Wed, Sep 4, 12:57 PM
include/clang/AST/ASTContext.h
2060 ↗(On Diff #211778)

Okay. I don't know what the right rule here is, but I agree we should be using the same rule in both places.

lib/Sema/SemaDecl.cpp
11176 ↗(On Diff #211778)

Oh, I didn't realize we were already honoring the ordinary attribute in the ABI, but from the code it looks we do. In that case, yeah, it makes sense to ignore it here. Should we extract a function to ask whether a FieldDecl should be ignored for triviality computations so that it's more self-documenting that there's a consistent logic here?

ahatanak updated this revision to Diff 218826.Wed, Sep 4, 6:54 PM
ahatanak marked 2 inline comments as done.

Address review comments.

lib/Sema/SemaDecl.cpp
11176 ↗(On Diff #211778)

I added function shouldDiagnoseNonTrivialField, which returns false if the field is unavailable.

Could you give it a slightly more general name and then use it in the main semantic check in ActOnFields?

ahatanak updated this revision to Diff 219008.Thu, Sep 5, 5:09 PM

Rename function to ignoreForTrivialityComputation.

rjmccall added inline comments.Fri, Sep 6, 11:14 AM
lib/Sema/SemaDecl.cpp
11142 ↗(On Diff #219008)

shouldIgnoreForRecordTriviality or something like that, please.

11144 ↗(On Diff #219008)

The "since" clause here is circular: this function *defines* what affects the triviality. The comment should talk about the motivations (e.g. fields that aren't available in particular language modes) and why this might be okay or is the best-available option (despite e.g. the potential for ABI incompatibility across language modes),

ahatanak updated this revision to Diff 219191.Fri, Sep 6, 3:39 PM
ahatanak marked 2 inline comments as done.

Rename function and fix comments.

lib/Sema/SemaDecl.cpp
11144 ↗(On Diff #219008)

I commented on the ABI incompatibility issue in ActOnFields where the unavailable attribute is added to the field.

rjmccall accepted this revision.Fri, Sep 6, 4:09 PM

Okay, thanks. LGTM.

This revision is now accepted and ready to land.Fri, Sep 6, 4:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 6, 5:33 PM