This is an archive of the discontinued LLVM Phabricator instance.

Don't pass uninitialized QueryKind
ClosedPublic

Authored by vitalybuka on Jan 7 2022, 11:01 AM.

Details

Summary

Even if findImplementors does not use
uninitialized parameter it's still UB and
it's going to be detected by msan with:
-Xclang -enable-noundef-analysis -mllvm -msan-eager-checks=1

Diff Detail

Event Timeline

vitalybuka created this revision.Jan 7 2022, 11:01 AM
vitalybuka requested review of this revision.Jan 7 2022, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2022, 11:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kda added a comment.Jan 7 2022, 2:22 PM

This seems to introduce a new branch, should there be a new unit test in: XRefsTests.cpp?

clang-tools-extra/clangd/XRefs.cpp
1313

It seems like the first line of 'findImplementors' is 'if (IDs.empty() || !Index)`.
I wonder if the correct fix is to drop the '!Index' check in findImplementors.

kda requested changes to this revision.Jan 7 2022, 2:28 PM

I think I forgot to 'Request Changes'.

This revision now requires changes to proceed.Jan 7 2022, 2:28 PM

This seems to introduce a new branch, should there be a new unit test in: XRefsTests.cpp?

It's not functional change, test should not be able to see a difference after this patch.

clang-tools-extra/clangd/XRefs.cpp
1313

Sorry, I am no following how "Index" is related to uninitialized QueryKind?

This seems to introduce a new branch, should there be a new unit test in: XRefsTests.cpp?

It's not functional change, test should not be able to see a difference after this patch.

to clarify: it's not realy new branch, as findImplementors already does exactly the same check.
I considered removing that check, but it will complicated other callers.

vitalybuka added inline comments.Jan 7 2022, 3:14 PM
clang-tools-extra/clangd/XRefs.cpp
311

here

vitalybuka requested review of this revision.Jan 7 2022, 3:14 PM
kda added inline comments.Jan 7 2022, 3:35 PM
clang-tools-extra/clangd/XRefs.cpp
1313

Oh. Ignore some of my previous discussion, I missed the summary.

Why not just initialize QueryKind?
That seems like a more direct solution for the specific issue.
I believe it would be safe, as IDs is only inserted into when QueryKind is explicitly set.

vitalybuka marked an inline comment as done.Jan 7 2022, 4:39 PM
kda accepted this revision.Jan 7 2022, 4:51 PM
This revision is now accepted and ready to land.Jan 7 2022, 4:51 PM
This revision was automatically updated to reflect the committed changes.