This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix crashes on `if consteval` in readability checks
ClosedPublic

Authored by rymiel on Sep 7 2022, 4:42 AM.

Details

Summary

The readability-braces-around-statements check tries to look at the
closing parens of the if condition to determine where to insert braces,
however, "consteval if" statements don't have a condition, and always
have braces regardless, so the skip can be checked.

The readability-simplify-boolean-expr check looks at the condition
of the if statement to determine what could be simplified, but as
"consteval if" statements do not have a condition that could be
simplified, they can also be skipped here.

There may still be more checks that try to look at the conditions of
ifs that aren't included here

Fixes https://github.com/llvm/llvm-project/issues/57568

Diff Detail

Event Timeline

rymiel created this revision.Sep 7 2022, 4:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 4:42 AM
rymiel requested review of this revision.Sep 7 2022, 4:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 4:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel added a comment.EditedSep 7 2022, 4:46 AM

Note for reviewing:

  • The C++ standard, from what I could tell, seems to imply the the else clause of a consteval if clause could be written without braces, however it seems that both GCC and Clang don't allow for this, so both clauses of constexpr if statements should be skipped in readability-braces-around-statement
  • I was really unsure what to do with the tests. Since consteval if is a C++23 feature, I felt like they needed to be in their own files, but these files are also super minimal, as they really only make sure that using them doesn't crash.
  • The tests use the flag -std=c++2b. Seeing as that flag will probably be renamed once 2023 actually rolls around, is that okay?

Ping? This is fixing a segmentation fault. Please let me know if there are other people I should add as reviewers

aaron.ballman accepted this revision.Sep 29 2022, 7:38 AM

LGTM, though please add a release note for the fix. Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

This revision is now accepted and ready to land.Sep 29 2022, 7:38 AM
rymiel updated this revision to Diff 464142.Sep 29 2022, 10:42 PM
rymiel edited the summary of this revision. (Show Details)

Add release note

Thank you, Aaron! Is the note I've added okay? I didn't expect to need to add a note to a bugfix, but I haven't written a release note before at all regardless. And yes, I'm able to commit it myself.

njames93 added inline comments.Sep 29 2022, 11:32 PM
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
471

Probably should add a check here.

494

And here.

rymiel marked 2 inline comments as done.Sep 30 2022, 1:18 AM

Thank you for pointing those out, I was indeed able to cause crashes from those locations

rymiel updated this revision to Diff 464176.Sep 30 2022, 1:18 AM

Extra consteval checks in SimplifyBooleanExprCheck