-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-format] Crash with Whitesmiths and labels in top-level block #50007
Comments
I tested this with 11 and 12 and can confirm it does indeed hang, I tested this with my latest build and it doesn't, but I build debug so its possible it doesn't manifest itself. |
I tested Debug and Release builds of the current tip of the main (Currently 13) and I cannot reproduce. I checked out the 12.0.1 branches and tested both debug and release 12.0.1 does indeed still crash (Debug and Release), my guess its more a crash caused by out of memory as the process rapid grows to many GB Just to clarify my previous comment 11 GA was fine 12 GA was not. I'm not sure if this was somehow a known issue, I don't remember it between 12 and 13 especially for clang-format. |
Did eventually get a stack from 12.0.1 /buildareas/clang/build_ninja/bin/clang-format.exe test1.cpp
|
I applied https://reviews.llvm.org/D94500 to the 12.0.1 branch and this fixes your issue. Can we consider this issue fixed? for LLVM 13.0 I would prefer that @timwoj fixed this in the branch if that's what needs to be done before 13 is released |
Would be nice to see an early fix since this is obviously broken and shouldn't crash the formatter. As long as this issue remains, I will not be able to adopt clang-format for my code. |
The fix does not apply cleanly, could someone backport this and push a branch to their local github fork? |
Tom am I allowed to simply commit/push it directly into the branch? do I need to go back round the Phabricator review cycle? if the changes to get into the branch are minimal? |
I should have mentioned, I don't use a fork. |
Yes, if you can backport the patch cleanly, go ahead and push it. Just make sure to use git cherry-pick -x so that the commit in main is referenced in the commit log and make sure the lit tests pass. |
Ok its pushed to the 12.x branch (cherry picked -x), for the life of me I hope I've done it correctly, the unit tests all pass. If I've messed anything up, I apologize in advance, but I guess we all have to do something scary the first time Thanks for your help. |
Is the a branch build bot? |
Yes, there are branch buildbots. They report status to github, so if you click on the commit in the web interface, you should be able to see the status. |
Merged: c7d7ace |
Thanks Tom for helping me out (I got my green tick in the end, took a while...) Thank you for everything you do. |
Extended Description
Formatting a file with a label within a top-level block causes clang-format to hang for an extended period of time and use a lot of memory. It then proceeds to crash:
PS C:\Users\olive\clang-format-repro> clang-format.exe --style='{BreakBeforeBraces: Whitesmiths}' goto.cpp
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0. Program arguments: "C:\Program Files\LLVM\bin\clang-format.exe" "--style={BreakBeforeBraces: Whitesmiths}" goto.cpp
PS C:\Users\olive\clang-format-repro> cat goto.cpp
int main() {
test:
return 0;
}
No actual backtrace is printed.
I have prepared test cases with both a simple function and a switch statement and both lead to a crash.
My suspicion is that this was caused by the patch that tried to fix case labels with Whitesmiths [1]. This bug does not occur with version 11.0 of clang-format.
[1] https://reviews.llvm.org/D94500
The text was updated successfully, but these errors were encountered: