[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG244dd2913a43: Dont pass uninitialized QueryKind 
(authored by vitalybuka).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

Files:
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1298,7 +1298,7 @@
   DeclRelationSet Relations =
   DeclRelation::TemplatePattern | DeclRelation::Alias;
   llvm::DenseSet IDs;
-  RelationKind QueryKind;
+  RelationKind QueryKind = RelationKind::OverriddenBy;
   for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations)) {
 if (const auto *CXXMD = llvm::dyn_cast(ND)) {
   if (CXXMD->isVirtual()) {


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1298,7 +1298,7 @@
   DeclRelationSet Relations =
   DeclRelation::TemplatePattern | DeclRelation::Alias;
   llvm::DenseSet IDs;
-  RelationKind QueryKind;
+  RelationKind QueryKind = RelationKind::OverriddenBy;
   for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations)) {
 if (const auto *CXXMD = llvm::dyn_cast(ND)) {
   if (CXXMD->isVirtual()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka updated this revision to Diff 398271.
vitalybuka added a comment.

update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

Files:
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1298,7 +1298,7 @@
   DeclRelationSet Relations =
   DeclRelation::TemplatePattern | DeclRelation::Alias;
   llvm::DenseSet IDs;
-  RelationKind QueryKind;
+  RelationKind QueryKind = RelationKind::OverriddenBy;
   for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations)) {
 if (const auto *CXXMD = llvm::dyn_cast(ND)) {
   if (CXXMD->isVirtual()) {


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1298,7 +1298,7 @@
   DeclRelationSet Relations =
   DeclRelation::TemplatePattern | DeclRelation::Alias;
   llvm::DenseSet IDs;
-  RelationKind QueryKind;
+  RelationKind QueryKind = RelationKind::OverriddenBy;
   for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations)) {
 if (const auto *CXXMD = llvm::dyn_cast(ND)) {
   if (CXXMD->isVirtual()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Kevin Athey via Phabricator via cfe-commits]

kda added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:1313
   }
+  if (IDs.empty())
+return {};

vitalybuka wrote:
> kda wrote:
> > 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.
> Sorry, I am no following how "Index" is related to uninitialized QueryKind?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:311
 llvm::StringRef MainFilePath) {
   if (IDs.empty() || !Index)
 return {};

here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D116827#3228594 , @vitalybuka 
wrote:

> In D116827#3228524 , @kda wrote:
>
>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D116827#3228524 , @kda wrote:

> 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.




Comment at: clang-tools-extra/clangd/XRefs.cpp:1313
   }
+  if (IDs.empty())
+return {};

kda wrote:
> 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.
Sorry, I am no following how "Index" is related to uninitialized QueryKind?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Kevin Athey via Phabricator via cfe-commits]

kda requested changes to this revision.
kda added a comment.
This revision now requires changes to proceed.

I think I forgot to 'Request Changes'.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Kevin Athey via Phabricator via cfe-commits]

kda added a comment.

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




Comment at: clang-tools-extra/clangd/XRefs.cpp:1313
   }
+  if (IDs.empty())
+return {};

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116827/new/

https://reviews.llvm.org/D116827

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116827: Don't pass uninitialized QueryKind

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka created this revision.
vitalybuka added reviewers: kda, eugenis.
Herald added subscribers: usaxena95, kadircet, arphaman.
vitalybuka requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116827

Files:
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1310,6 +1310,8 @@
   QueryKind = RelationKind::BaseOf;
 }
   }
+  if (IDs.empty())
+return {};
   return findImplementors(std::move(IDs), QueryKind, Index, *MainFilePath);
 }
 


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1310,6 +1310,8 @@
   QueryKind = RelationKind::BaseOf;
 }
   }
+  if (IDs.empty())
+return {};
   return findImplementors(std::move(IDs), QueryKind, Index, *MainFilePath);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits