This is an archive of the discontinued LLVM Phabricator instance.

[Attr] Support _attribute__ ((fallthrough))
AbandonedPublic

Authored by xbolva00 on Jun 13 2019, 6:20 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Jun 13 2019, 6:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 6:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 marked an inline comment as done.
xbolva00 added inline comments.
test/Sema/block-literal.c
44 ↗(On Diff #204509)

I tried look at this, but I have no idea how my change to parse GNU affects __block.

xbolva00 edited the summary of this revision. (Show Details)Jun 13 2019, 6:33 AM
xbolva00 added reviewers: rnk, rjmccall, efriedma.

Thanks for the patch, I look forward to this feature!

I think the changes in test/SemaCXX/warn-unused-label-error.cpp, test/Sema/block-literal.c, and test/Sema/address_spaces.c should not be committed (2 look like unrelated cleanups?).

lib/Parse/ParseStmt.cpp
110–111 ↗(On Diff #204509)

Wouldn't this statement have to change? __attribute__((fallthrough)) is an "attribute on [an] empty statement." Maybe this is not the correct place to try to parse a GNU attr?

lib/Sema/AnalysisBasedWarnings.cpp
1279–1280

Probably should additionally/instead check S.getLangOpts().GNUMode (since these are GNU C style attributes)? I guess we want these attributes to be supported in C regardless of -std=?

test/Sema/block-literal.c
44 ↗(On Diff #204509)

If you remove your change to lib/Parse/ParseStmt.cpp, does this test still fail in this way? Did you test this on an assertion enabled build (-DLLVM_ENABLE_ASSERTIONS=ON)? (I would have expected the assertion I commented on above to fail.)

test/Sema/fallthrough-attr.c
1 ↗(On Diff #204509)

would you mind adding a run for -std=gnu89? In particular, I'm worried about you're above change checking c99.

xbolva00 marked 5 inline comments as done.Jun 13 2019, 10:25 AM
xbolva00 added inline comments.
lib/Parse/ParseStmt.cpp
110–111 ↗(On Diff #204509)

Since we parse [[ ]] parse here too, I expect it could be correct place.

@aaron.ballman ?

lib/Sema/AnalysisBasedWarnings.cpp
1279–1280

Good point.

test/Sema/block-literal.c
44 ↗(On Diff #204509)

Yes, added line "MaybeParseGNUAttributes(Attrs);" cause this changes in block-literal.c and address_spaces.c.

44 ↗(On Diff #204509)

Yes, LLVM_ENABLE_ASSERTIONS is ticked in cmake-gui.. I have no assert failure in that place..

xbolva00 added a comment.EditedJun 13 2019, 10:51 AM

Well, I am not familiar with __block, but this is quite weird :)

__block y = 7;

if (Tok.is(tok::kw___attribute))

printf("__attr\n");

Output:
__attr

xbolva00 marked an inline comment as done.Jun 13 2019, 10:53 AM
xbolva00 added inline comments.
test/Sema/block-literal.c
44 ↗(On Diff #204509)

See my comment.. if __block is internally as "kw _ _ attribute", now I know why this was changed.

xbolva00 updated this revision to Diff 204583.Jun 13 2019, 10:54 AM

Addressed some review notes. Thanks.

xbolva00 updated this revision to Diff 204584.Jun 13 2019, 10:55 AM
xbolva00 marked 2 inline comments as done.Jun 13 2019, 10:57 AM
xbolva00 added inline comments.
test/Sema/address_spaces.c
12 ↗(On Diff #204584)

I think this is an acceptable change..

test/SemaCXX/warn-unused-label-error.cpp
23 ↗(On Diff #204584)

I think this is an improvement.

lib/Sema/AnalysisBasedWarnings.cpp
1279–1280

IIUC, we allow GNU C attributes for C regardless of ISO vs GNU C. What we want to express is "if c++11 and newer, or c".

I think this might be better expressed as:

if (S.getLangOpts().CPlusPlus11 || (!S.getLangOpts().CPlusPlus && !S.getLangOpts().ObjC) {

But maybe there's a more canonical way to express "if the language is C." (If not, maybe we can consider such a change to LangOpt to make this easier, but as part of this commit).

but as part of this commit

but *not* as part of this commit

efriedma added inline comments.Jun 13 2019, 11:19 AM
test/Sema/address_spaces.c
12 ↗(On Diff #204584)

This is scary. gcc and clang both parse void f() { __attribute((aligned)) *x; } etc. as a declaration; I don't think we want to change that, even if that usage is a bit dubious in modern C. And it's not clear to me if there are other implications here; does this affect the handling of statement/declaration ambiguity in C++?

test/Sema/address_spaces.c
12 ↗(On Diff #204584)

It's a pointer to implicit int. Either way, I think the change and the comments are polluting this code review, hence the suggestion to submit as a separate individual patch.

xbolva00 marked an inline comment as done.Jun 13 2019, 11:32 AM
xbolva00 added inline comments.
test/Sema/address_spaces.c
12 ↗(On Diff #204584)

But this patch causes changes in those files. So I dont know what to split here...

xbolva00 marked an inline comment as done.Jun 13 2019, 11:34 AM
xbolva00 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
1279–1280

Yeah, I wanted to check if lang is C but I didnt find a nice way :(

efriedma added inline comments.Jun 13 2019, 11:36 AM
test/Sema/address_spaces.c
12 ↗(On Diff #204584)

This is caused by the change to lib/Parse/ParseStmt.cpp, right? You should be able to split that into a separate patch, even if there aren't any usable statement attributes without the other parts of the patch.

test/Sema/address_spaces.c
12 ↗(On Diff #204584)

Is it because the added call to MaybeParseGNUAttributes() in lib/Parse/ParseStmt.cpp can eventually produce this error? Your change as is seems to be breaking declarations of implicit integers.

xbolva00 updated this revision to Diff 204630.Jun 13 2019, 2:39 PM

Split.

Now patch only adds _attribute__ ((fallthrough)) support. Parsing will be hopefully solved in second patch.

xbolva00 updated this revision to Diff 204631.Jun 13 2019, 2:40 PM

Fixed EOF in test case.

test/Sema/fallthrough-attr.c
48 ↗(On Diff #204631)

Seems like lines 24-48 were an accidental copy+paste?

xbolva00 updated this revision to Diff 204644.Jun 13 2019, 3:33 PM

fixed testcase

xbolva00 updated this revision to Diff 204646.Jun 13 2019, 3:44 PM
ojeda added a subscriber: ojeda.Jun 14 2019, 1:46 AM
aaron.ballman added inline comments.Jun 17 2019, 12:15 PM
lib/Sema/AnalysisBasedWarnings.cpp
1233

I'd prefer to see this be __attribute__((fallthrough)) without the extra whitespace.

1279–1280

I think we want to support this attribute in ObjC because that language is based on C. In fact, I think we are supporting this attribute in all language modes regardless of -std level because the user can always use __attribute__((fallthrough)) as a spelling.

As noted in D63299, seems like this would require more surgery - I cannot continue D63299 since I have no enough knowledge to rework it fully.

I will update this patch, in case somebody picks D63299.

xbolva00 updated this revision to Diff 205349.Jun 18 2019, 7:55 AM

Addressed notes. Thanks.

rjmccall added inline comments.Jun 27 2019, 11:10 PM
lib/Sema/AnalysisBasedWarnings.cpp
1279–1280

As of a few weeks ago, you can provide custom code in a LangOpt; see how ObjCNonFragileRuntime is defined.

xbolva00 abandoned this revision.Jul 30 2019, 2:37 PM