Page MenuHomePhabricator

[clangd] Return earlier when snippet is empty
ClosedPublic

Authored by tom-anders on Sep 18 2022, 8:03 AM.

Details

Summary

Fixes github.com/clangd/clangd/issues/1216

If the Snippet string is empty, Snippet.front() would trigger a crash.
Move the Snippet->empty() check up a few lines to avoid this. Should not
break any existing behavior.

Diff Detail

Event Timeline

tom-anders created this revision.Sep 18 2022, 8:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2022, 8:03 AM
tom-anders requested review of this revision.Sep 18 2022, 8:03 AM
nridge added inline comments.Sep 19 2022, 1:58 AM
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

Does this test trigger the crash for you when run without the fix?

tom-anders marked an inline comment as done.Sep 19 2022, 2:53 AM
tom-anders added inline comments.
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

Yes it does, I double checked. Not sure why it didn't work for you

kadircet added inline comments.Sep 19 2022, 3:00 AM
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

it doesn't trigger the crash for me either.

tom-anders marked an inline comment as done.Sep 19 2022, 3:53 AM
tom-anders added inline comments.
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

Huh, maybe this is a b

1017

Hmm could be a build type thing? I ran the tests for a Debug build, maybe it's different in Release?

Maybe we could also ask the author of the GitHub issue to check if it fixes the crash for them?

nridge accepted this revision.Sep 20 2022, 1:52 AM
nridge added inline comments.
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

Ok, I verified that the test case does produce completion proposals with empty snippets, i.e. it does trigger the problematic code path, it just happens not to crash for me. I think that's fine.

Thank you for tracking this down and fixing!

This revision is now accepted and ready to land.Sep 20 2022, 1:52 AM
This revision was landed with ongoing or failed builds.Sep 20 2022, 1:48 PM
This revision was automatically updated to reflect the committed changes.
tom-anders added inline comments.Sep 20 2022, 1:51 PM
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1017

np, thanks for the review :)

It's not surprising that we often don't crash here, the "obvious" lowering of S.front() is *S.data() which is perfectly valid (will be 0 for an empty string).

One way to crash on this is to build with libstdc++'s debug iterators, which assert on this. Fedora seems to be building (some of) their packages in this mode, such that they crash but (sometimes?) do not print the assertion message. This was the cause in https://github.com/clangd/vscode-clangd/issues/400.

It's not surprising that we often don't crash here, the "obvious" lowering of S.front() is *S.data() which is perfectly valid (will be 0 for an empty string).

One way to crash on this is to build with libstdc++'s debug iterators, which assert on this. Fedora seems to be building (some of) their packages in this mode, such that they crash but (sometimes?) do not print the assertion message. This was the cause in https://github.com/clangd/vscode-clangd/issues/400.

Interesting, FYI I was able to reproduce this on Arch Linux.