Page MenuHomePhabricator

Redeclare Objective-C property accessors inside the ObjCImplDecl in which they are synthesized.
ClosedPublic

Authored by aprantl on Sep 26 2019, 4:50 PM.

Details

Summary

This patch is motivated by (and factored out from) https://reviews.llvm.org/D66121 which is a debug info bugfix. Starting with DWARF 5 all Objective-C methods are nested inside their containing type, and that patch implements this for synthesized Objective-C properties.

This patch became much longer than I hoped it would be but most of the changes are mechanical in nature.

  1. SemaObjCProperty populates a list of synthesized accessors that may need to inserted into an ObjCImplDecl.
  2. SemaDeclObjC::ActOnEnd inserts forward-declarations for all accessors for which no override was provided into their ObjCImplDecl. This patch does *not* synthesize AST function *bodies*. Moving that code from the static analyzer into Sema may be a good idea though.
  3. Places that expect all methods to have bodies have been updated.

Most of the updates are very straightforward, the most irritating part was updating the static analyzer, there may be a more elegant way to do this.
I'm somewhat concerned that I didn't have to update the GNU Objective-C runtime for the testsuite to pass. I did not update the static analyzer's inliner for synthesized properties to point back to the property declaration (see test/Analysis/Inputs/expected-plists/nullability-notes.m.plist), which I believed to be more bug than a feature.

Diff Detail

Event Timeline

aprantl created this revision.Sep 26 2019, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 4:50 PM
aprantl updated this revision to Diff 222056.Sep 26 2019, 5:08 PM

Remove #if 0 from test I used during debugging. (It still passes!)

The changes for the GNU runtimes look correct to me. We're missing a bunch of tests there, unfortunately.

rjmccall added inline comments.Sep 27 2019, 11:54 PM
clang/include/clang/AST/DeclObjC.h
2797

Are these comments right? The DC is the implementation's interface?

Regardless, I think these comments are misleading: the right way to put this is that these are the getter and setter method *definitions* (even if they don't necessarily have a body). And, relatedly, you should mention whether these are set even if the methods are defined explicitly or only if they're synthesized.

clang/include/clang/Sema/Sema.h
8522 ↗(On Diff #222056)

Property accessors are created in the @implementation if they aren't defined explicitly there.

Does this really need to be global state on Sema? Does it theoretically need to be serialized and restored if e.g. a translation-unit prefix is cut after an @implementation is seen?

clang/lib/AST/DeclObjC.cpp
1386

Can you just do this as a simple check to look through the ObjCImplDecl to the ObjCInterfaceDecl as the first thing, immediately after you initialize Container?

clang/lib/Analysis/BodyFarm.cpp
852

Why did this logic need to change?

clang/lib/CodeGen/CGObjCGNU.cpp
1888

This is only correct if these methods aren't defined explicitly by the user, right? Or do this methods return null in that case?

Same question goes for the CGObjCMac code.

clang/lib/CodeGen/CGObjCMac.cpp
6697

Spurious change.

clang/lib/CodeGen/CodeGenFunction.cpp
694

Spurious change.

clang/lib/CodeGen/CodeGenModule.cpp
5124

Is this special treatment still necessary? Won't we encounter the getter and setter on the normal pass over the method definitions in the @implementation?

Also, I think it would be good if there was a more explicit method for asking whether a method is a synthesized definition. Basically, there's a tri-state: there are pure declarations, user-provided definitions, and synthetic definitions. IRGen should emit a function when it sees either of the latter two, but it might have to use special code for synthetic definitions.

aprantl marked 8 inline comments as done.Fri, Oct 18, 5:57 PM
aprantl added inline comments.
clang/lib/Analysis/BodyFarm.cpp
852

I don't have a satisfying answer for this.

Without this extra code, the assertion in https://github.com/llvm/llvm-project/blob/b081220cfd46965fa25dbf826cd3f42f4f9e54cd/clang/test/Analysis/properties.m#L1035 (and only that one) turns from UNKNOWN to TRUE.

There is a similar workaround with a similar comment in
https://github.com/llvm/llvm-project/blob/b081220cfd46965fa25dbf826cd3f42f4f9e54cd/clang/lib/StaticAnalyzer/Core/CallEvent.cpp#L1291
that looks like it is related.

clang/lib/CodeGen/CodeGenModule.cpp
5124

We don't know which ObjMethodDecls without bodies are property accessor implementations since there is no pointer from the ObjCMethodDecl back to the ObjCPropertyImplDecl.

aprantl updated this revision to Diff 225727.Fri, Oct 18, 6:00 PM

Addressed most of the feedback, explained the rest.

aprantl updated this revision to Diff 225729.Fri, Oct 18, 6:01 PM
NoQ edited reviewers, added: NoQ; removed: dergachev.a.Fri, Oct 18, 6:11 PM
NoQ added a subscriber: NoQ.
NoQ added inline comments.
clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
191

It looks like the body farm for the property accessor has stopped working here. The code under analysis (from test/Analysis/nullability-notes.m) is:

 7	void takesNonnull(NSObject *_Nonnull y);
 8
 9	@interface ClassWithProperties: NSObject
10	@property(copy, nullable) NSObject *x; // plist check ensures no control flow piece from here to 'self.x'.
11	-(void) method;
12	@end;
13	@implementation ClassWithProperties
14	-(void) method {
15	  // no-crash
16	  NSObject *x = self.x; // expected-note{{Nullability 'nullable' is inferred}}
17	  takesNonnull(x); // expected-warning{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
18	                   // expected-note@-1{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
19	}
20	@end

The point of the test is to check how control flow is displayed to the user, and this change in the plist file will not really be visible to the user. However i'm worried that we've lost precision of the analysis on similar examples. I'll double-check.

rjmccall added inline comments.Mon, Oct 21, 11:26 AM
clang/lib/CodeGen/CodeGenModule.cpp
5124

I mean, this doesn't seem like an unreasonable thing to be able to discover given just an ObjCMethodDecl. We can certainly set something on the declaration that says that it represents a synthesized method definition (and is therefore a definition for the purposes of isDefined or anything similar).

aprantl updated this revision to Diff 227799.Mon, Nov 4, 5:02 PM

Rebased patch and made it slightly more explicit whether an ObjCMethodDecl is a synthesized accessor stub by reserving a bit for it in ObjCMethodDecl.

aprantl marked an inline comment as done.Mon, Nov 4, 5:10 PM
aprantl added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
5124

If we were to move this into the loop / function that emits each method decl, we would still somehow need to get back to the ObjCPropertyImplDecl . There is no pointer in that direction, so we'd have to loop over all property impls until we find it. This here is going to be faster. Storing a pointer to a possible ObjCPropertyImplDecl in ObjCMethodDecl seems wasteful. Let me know if I misunderstood what you were suggesting.

aprantl updated this revision to Diff 227801.Mon, Nov 4, 5:12 PM

Accidentally sent my diff-upon-previous-diff instead of diff against master.

martong resigned from this revision.Tue, Nov 5, 2:16 AM
rjmccall accepted this revision.Tue, Nov 5, 12:08 PM

Okay, thanks for patiently working through all this review. I'm happy with this now.

clang/lib/CodeGen/CodeGenModule.cpp
5124

Oh, in order to find the associated ivar? I hadn't considered that. Okay, WFM.

This revision is now accepted and ready to land.Tue, Nov 5, 12:08 PM

Thanks for patiently working through this with me.

This revision was automatically updated to reflect the committed changes.