Page MenuHomePhabricator

[MSVC] 'property' with an empty array in array subscript expression.

Authored by ABataev on Oct 1 2015, 4:00 AM.



MSVC supports 'property' attribute and allows to apply it to the declaration of an empty array in a class or structure definition.
For example:

__declspec(property(get=GetX, put=PutX)) int x[];

The above statement indicates that x[] can be used with one or more array indices. In this case, i=p->x[a][b] will be turned into i=p->GetX(a, b), and p->x[a][b] = i will be turned into p->PutX(a, b, i);

Diff Detail


Event Timeline

ABataev updated this revision to Diff 36215.Oct 1 2015, 4:00 AM
ABataev retitled this revision from to [MSVC] 'property' with an empty array in array subscript expression..
ABataev updated this object.
ABataev added a reviewer: rnk.
ABataev added a subscriber: cfe-commits.
rnk edited edge metadata.Oct 1 2015, 8:26 AM

I think fundamentally we are doing too much declspec property lowering in Sema. We might want to back up and figure out how to do it in IRGen. Right now we have bugs like this, which are probably more important than new functionality:

In D13336#257628, @rnk wrote:

I think fundamentally we are doing too much declspec property lowering in Sema. We might want to back up and figure out how to do it in IRGen. Right now we have bugs like this, which are probably more important than new functionality:

Currently declspec property is represented as a member function call and does not keep info about property itself. Array subscripts exprs add arguments to this member function call and it must be built in Sema, so we could use an existing codegen for CallExprs.

rnk edited reviewers, added: rjmccall; removed: rnk.Oct 19 2015, 5:07 PM
rnk added a subscriber: rnk.Oct 19 2015, 5:13 PM

I spent some time reading through the pseudo-object implementation in clang, but I still don't understand it. We should ask John if he can review this.

My gut feeling is that we shouldn't have a global DenseMap in Sema just for this. Should we have some other node, like an ObjCSubscriptRefExpr that describes this instead?

We need *some* AST for the as-written syntactic form. Can you send a dump of the AST for the test?

3986–3987 ↗(On Diff #36215)

Will this affect the syntactic form of the expression?

32 ↗(On Diff #36215)

Can you add a ++ test?

rjmccall edited edge metadata.Oct 19 2015, 9:47 PM

I agree with Reid that you should not be adding a DenseMap to Sema for this. Just build a SubscriptExpr for the syntactic form and have it yield an expression of pseudo-object type; or you can make your own AST node for it if that makes things easier.

ABataev updated this revision to Diff 37854.Oct 20 2015, 4:36 AM
ABataev edited edge metadata.

Update after review

Needs more tests.

  1. Dependent template instantiation.
  2. Non-dependent template instantiation.
  3. Indexes which are themselves pseudo-objects.
  4. Templated getters and setters.
  5. Non-POD by-value argument types.
253 ↗(On Diff #37854)

Hmm. I don't think you need a new placeholder type. These subscripts are still an ordinary pseudo-object; you can load and store at any point. Instead, you should detect this case by the expression node used, which can either be an ArraySubscriptExpr or a new node if you find it necessary.

3934 ↗(On Diff #37854)

I think what you should do here is:

  1. Detect whether the base type has pseudo-object type.
  2. If so, give the pseudo-object code a chance to work on the expression. In most cases, it will just load; but if the base is an MSPropertyRefExpr of array type, or an ArraySubscriptExpr, it can leave it alone.
  3. If the pseudo-object code returned an expression that's still of pseudo-object type, just build an ArraySubscriptExpr of pseudo-object type after you've folded pseudo-objects in the index as well.
4115 ↗(On Diff #37854)

Placeholders should never get here.

1458 ↗(On Diff #37854)

Don't modify ArgExprs in buildGet or buildSet; you might need to build both, e.g. when building a compound assignment or inc/dec.

Since you need to copy into a new array anyway, you might as well accumulate them index expressions in stack order and then reverse them when adding them to this array.

ABataev updated this revision to Diff 38232.Oct 23 2015, 5:53 AM
ABataev marked 4 inline comments as done.

Update after review

rjmccall added inline comments.Oct 26 2015, 2:07 PM
689 ↗(On Diff #38232)

Please add to the comment here that this is a syntactic pseudo-object expression.

3926 ↗(On Diff #38232)

Please factor this check so that it only does extra work when the base has placeholder type, and please extract a function to compute IsMSPropertySubscript.

1419 ↗(On Diff #38232)

This code will look slightly nicer and be slightly more efficient if you maintain the invariant that parens have been ignored on Base. That is, IgnoreParens here and when assigning in the loop, and then you can remove them from the type-checks.

Oh. You also need to capture the index expressions so that they aren't evaluated multiple times when this expression is used as the l-value of an increment, decrement, or compound-assignment. You then need to un-capture them in the Rebuilder. Please add code-generation tests that check that the index expression is only evaluated once in these cases.

ABataev updated this revision to Diff 40253.Nov 16 2015, 12:52 AM
ABataev marked 3 inline comments as done.

Update after review

John, any comments?

rjmccall added inline comments.Nov 19 2015, 11:01 AM
1627 ↗(On Diff #40253)

Hmm. Just like the ObjCSubscriptRefExpr case, this will need to strip the OVEs off the index expressions, or else you'll end up stacking OVEs and asserting in IRGen.

The deeper problem here is that we have too much redundancy in all these places that have to walk over the different pseudo-object expressions. We can remove some of that by simplifying the design of Rebuilder, which is really over-engineered for its task. It should be fine to make it non-templated, merge all of the rebuildSpecific cases into it (and therefore remove all of the subclasses), and give it a callback to invoke at all the capture points in source order.

I think the callback can just be a llvm::function_ref<Expr*(Expr*, unsigned)>. The second parameter is the position of the capture point in source order, so for example in your case the base of the MSPropertyRefExpr will be 0, the index of the innermost MSPropertySubscriptExpr will be 1, and so on. That'll be important for callers that care about which capture point is which.

The various rebuildAndCaptureObject implementations should use a callback that returns the appropriate captured expression for the position.

stripOpaqueValuesFromPseudoObjectRef should use a callback that returns cast<OpaqueValueExpr>(e)->getSourceExpr(); it shouldn't need to split out the different cases at all.

ABataev marked an inline comment as done.Nov 23 2015, 12:11 AM
ABataev added inline comments.
1627 ↗(On Diff #40253)

John, we don't need to strip the OVEs off the index expressions, because index expressions in MSPropertySubscriptExpr are not folded to OVEs. They are folded to OVEs only for CallArgs array. But in rebuilded MSPropertySubscriptExpr only base MSPropertyRefExpr is folded to OVE (we don't need to put indexes in MSPropertySubscriptExpr, because they are not used during codegen and we can keep original expressions). So, the code should work as is.
I reworked Rebuilder just like you said.

ABataev updated this revision to Diff 40901.Nov 23 2015, 12:12 AM
ABataev marked an inline comment as done.
rjmccall added inline comments.Nov 23 2015, 4:14 PM
57 ↗(On Diff #40901)

Well, it should be okay to call this on something that doesn't have a base; it just doesn't need to do anything and can just return refExpr.

1591 ↗(On Diff #40901)

That's what I'm saying, though: the capture process should be building the syntactic MSPropertySubscriptExpr using the captured OVEs for the index expressions as well as the recursively-captured base, and Rebuilder will then need to undo that.

We want to consistently replace all the captured sub-trees with their OVEs so that clients can easily recover the relationship between the syntactic and semantic forms.

1621 ↗(On Diff #40901)

My hope is that you'll be able to get this whole function down to just this last line and won't need to split out any of the cases here.

ABataev updated this revision to Diff 41018.Nov 24 2015, 2:45 AM

Update after review

This looks fantastic, thanks for doing all that. Approved with the one minor change.

175 ↗(On Diff #41018)

One extremely minor tweak: could you organize this like the rest of the pseudo-object expressions, checking for it above and calling a method to rebuild it?

This revision was automatically updated to reflect the committed changes.

John, thanks a lot for the review!
Committed version has a fix according to your last comment.