Page MenuHomePhabricator

[clang] Unify Sema and CodeGen implementation of isFlexibleArrayMemberExpr
ClosedPublic

Authored by serge-sans-paille on Sep 27 2022, 11:41 PM.

Details

Summary

Turn it into a single Expr::isFlexibleArrayMemberLike method, as discussed in

https://discourse.llvm.org/t/rfc-harmonize-flexible-array-members-handling

Keep different behavior with respect to macro / template substitution, and
harmonize sharp edges: ObjC interface now behave as C struct wrt. FAM and
-fstrict-flex-arrays.

This does not impact __builtin_object_size interactions with FAM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 11:41 PM
serge-sans-paille requested review of this revision.Sep 27 2022, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 11:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the heads up! My only general comment is that unless this change has no externally observable effect I'd suggest adding tests. (Otherwise, noting it has no such effect would be helpful.) When I'm not familiar with the code (and often even what I am) I usually look for tests to understand the impact of a change.

clang/include/clang/AST/Expr.h
526

I was a little unsure what "this array" means in the context of an expression but after reading the code I think the intent of the function is to "Return true if this expression designates an array fits the idiom of a flexible array member..." If I have that right then that's what I would suggest to change the comment to.

Thanks for the heads up! My only general comment is that unless this change has no externally observable effect I'd suggest adding tests. (Otherwise, noting it has no such effect would be helpful.) When I'm not familiar with the code (and often even what I am) I usually look for tests to understand the impact of a change.

Agredd! This change should only be a refactoring one, but I can see edge cases where it brings some subtle difference (e.g. for ObjCIvarRefExpr but I need to explore that. Will do!)

serge-sans-paille edited the summary of this revision. (Show Details)

Add test case for ObjC interface with FAM, as hinted by @msebor

tschuett added inline comments.
clang/lib/AST/Expr.cpp
225

Could this be an early exit?

267

Could this be an early exit by moving it up?

Add an early exit

serge-sans-paille marked an inline comment as done.Oct 1 2022, 6:11 AM
serge-sans-paille added inline comments.
clang/lib/AST/Expr.cpp
225

I don't think so, the first branch is more likely to trigger than this one, so keeping it here seems a better choice (and it's conservative wrt. previous implementation)

In general, this looks reasonable to me.

clang/include/clang/AST/Expr.h
529
531

Do we want to make the array levels into an enumeration instead of letting the user pass arbitrary integers? (Perhaps not as part of this review.)

clang/lib/AST/Expr.cpp
244–247
serge-sans-paille marked 3 inline comments as done.

Address reviewers comment.

clang/include/clang/AST/Expr.h
531

Something along https://reviews.llvm.org/D135107 :-) ?

aaron.ballman added inline comments.Oct 4 2022, 9:41 AM
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
31–36 ↗(On Diff #465053)

These changes seem unrelated?

clang/include/clang/AST/Expr.h
531

LOL and that's why my suggestion seemed so familiar. :-D

Removed a diff that sneaked in.

aaron.ballman accepted this revision.Oct 4 2022, 10:34 AM

Assuming precommit CI comes back clean, this LGTM. Thanks for the cleanup!

This revision is now accepted and ready to land.Oct 4 2022, 10:34 AM

Thanks for the reviews!

I wonder if we could come up with an overload or something for cases when we only have a FieldDecl.
For example, in the Clang Static Analyzer, we have something similar at MemRegion.cpp#L784-L811, and it would be great if we could also make use of this harmonized flexible-array-members detection.

Actually, @serge-sans-paille inserted a FIXME about this, and I believe it's correct.

const AnalyzerOptions &Opts = SVB.getAnalyzerOptions();
// FIXME: this option is probably redundant with -fstrict-flex-arrays=1.
if (Opts.ShouldConsiderSingleElementArraysAsFlexibleArrayMembers && Size.isOne())
  return true;

Unfortunately, in this context we don't have an Expr; we only have a FieldDecl, so I'm looking for some way to plug this isFlexibleArrayMemberLike() into this.
I'm thinking of like introducing another overload for it accepting FieldDecl instead of Expr along with the current one.

@serge-sans-paille what do you think about this?