This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Sema] Diagnose indeterminately sequenced accesses
Needs ReviewPublic

Authored by rZhBoYao on Jul 23 2023, 8:29 AM.

Details

Reviewers
aaron.ballman
Endill
Group Reviewers
Restricted Project
Summary

Add -Windeterminately-sequenced and fix -Wunsequenced according to P0400R0 and CWG2571.

Fixes #63935

Diff Detail

Event Timeline

rZhBoYao created this revision.Jul 23 2023, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2023, 8:29 AM
rZhBoYao requested review of this revision.Jul 23 2023, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2023, 8:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rZhBoYao edited the summary of this revision. (Show Details)Jul 23 2023, 8:31 AM
rZhBoYao updated this revision to Diff 543295.Jul 23 2023, 9:54 AM

Put -Windeterminately-sequenced under -Wsequence-point's control

Thank you for working on this!

clang/test/SemaCXX/warn-unsequenced.cpp
2–6

Checking for cxx17 prefix in C++23 mode is misleading. I suggest to rename is to since-cxx17, and have a new run line for C++23 mode, leaving check in C++17 mode intact.

I'm sorry for hand-waving, but I also remember @aaron.ballman saying that code related to -Wunsequenced or around it was missing checks for C++17.
I can't say whether you addressed that or not, so just leaving it here for you and reviewers.

rZhBoYao updated this revision to Diff 543315.Jul 23 2023, 11:30 AM
rZhBoYao marked an inline comment as done.

Improve the test.

Not sure what the lack of C++17 checks was referring to in your conversation with Aaron.
Can @aaron.ballman confirm whether it is addressed by this patch?

BTW, I am not sure if CWG2571 is implemented by @cor3ntin? If so, can we mark it as done on https://clang.llvm.org/cxx_dr_status.html#2571? This patch handles the warning around it tho.

BTW, I am not sure if CWG2571 is implemented by @cor3ntin? If so, can we mark it as done on https://clang.llvm.org/cxx_dr_status.html#2571? This patch handles the warning around it tho.

As a reminder, we don't change cxx_dr_status.html directly. Instead, we add a special comment in clang/test/CXX/drs/dr25xx.cpp (for the case of CWG2571), which should be accompanied by test case(s) if possible. You can follow the pattern of existing tests. After that clang/www/make_cxx_dr_status script should be run to update the HTML.

Thanks for the reminder. I am aware of that. Browsing through 762672a73a1e and a560ccf2af7a, I believe the indeterminately sequenced requirement is met, neither of which test the codegen so I might just put the example shown in the release note in dr25xx.cpp.

BTW, I am not sure if CWG2571 is implemented by @cor3ntin? If so, can we mark it as done on https://clang.llvm.org/cxx_dr_status.html#2571? This patch handles the warning around it tho.

As a reminder, we don't change cxx_dr_status.html directly. Instead, we add a special comment in clang/test/CXX/drs/dr25xx.cpp (for the case of CWG2571), which should be accompanied by test case(s) if possible. You can follow the pattern of existing tests. After that clang/www/make_cxx_dr_status script should be run to update the HTML.

rZhBoYao updated this revision to Diff 543559.Jul 24 2023, 8:12 AM

Refactor VisitCXXOperatorCallExpr and mark CWG2571 as done in Clang 15 (deliberately the same version P2128R6 was implemented) since this patch only fix the diagnostics around it.

@aaron.ballman that's been waiting for a while - i don't feel comfortable reviewing it, mind looking at it?