[PATCH] D115959: [clangd] Fix undefined behavior when generating error message at rename with an invalid name

2021-12-19 Thread Aleksandr Platonov via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG555eacf75f21: [clangd] Fix undefined behavior when 
generating error message at rename with an… (authored by ArcsinX).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115959

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1060,6 +1060,11 @@
   )cpp",
"conflict", !HeaderFile, "Conflict"},
 
+  {R"cpp(
+int V^ar;
+  )cpp",
+   "\"const\" is a keyword", !HeaderFile, "const"},
+
   {R"cpp(// Trying to rename into the same name, SameName == SameName.
 void func() {
   int S^ameName;
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -455,7 +455,7 @@
 }
 
 llvm::Error makeError(InvalidName Reason) {
-  auto Message = [](InvalidName Reason) {
+  auto Message = [](const InvalidName &Reason) {
 switch (Reason.K) {
 case InvalidName::Keywords:
   return llvm::formatv("the chosen name \"{0}\" is a keyword",
@@ -733,7 +733,7 @@
 return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
-return makeError(*Invalid);
+return makeError(std::move(*Invalid));
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
   if (Reject)


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1060,6 +1060,11 @@
   )cpp",
"conflict", !HeaderFile, "Conflict"},
 
+  {R"cpp(
+int V^ar;
+  )cpp",
+   "\"const\" is a keyword", !HeaderFile, "const"},
+
   {R"cpp(// Trying to rename into the same name, SameName == SameName.
 void func() {
   int S^ameName;
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -455,7 +455,7 @@
 }
 
 llvm::Error makeError(InvalidName Reason) {
-  auto Message = [](InvalidName Reason) {
+  auto Message = [](const InvalidName &Reason) {
 switch (Reason.K) {
 case InvalidName::Keywords:
   return llvm::formatv("the chosen name \"{0}\" is a keyword",
@@ -733,7 +733,7 @@
 return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
-return makeError(*Invalid);
+return makeError(std::move(*Invalid));
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
   if (Reject)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115959: [clangd] Fix undefined behavior when generating error message at rename with an invalid name

2021-12-19 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Yikes, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115959

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

[PATCH] D115959: [clangd] Fix undefined behavior when generating error message at rename with an invalid name

2021-12-17 Thread Aleksandr Platonov via Phabricator via cfe-commits 
ArcsinX created this revision.
ArcsinX added reviewers: sammccall, kadircet, hokein.
Herald added subscribers: usaxena95, arphaman.
ArcsinX requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

`Message()` lambda uses `Reason.Details` as an input parameter for 
`llvm::formatv()`, but `Reason` in `Message()` is a local object.
Return value of `llvm::formatv()` contains references to its input arguments, 
thus `Message()` returns an object which contains a reference to `Details` 
field of the local object `Reason`.
This patch fixes this behavior by passing `Reason` as a reference to 
`Message()` to ensure that return value of `Message()` contains references to 
alive object and also prevents copying of `InvalidName` structure at passing it 
to `makeError()`.

Provided test passes on Linux+GCC with or without this patch, but fails on 
Windows+VisualStudio without this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115959

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1060,6 +1060,11 @@
   )cpp",
"conflict", !HeaderFile, "Conflict"},
 
+  {R"cpp(
+int V^ar;
+  )cpp",
+   "\"const\" is a keyword", !HeaderFile, "const"},
+
   {R"cpp(// Trying to rename into the same name, SameName == SameName.
 void func() {
   int S^ameName;
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -455,7 +455,7 @@
 }
 
 llvm::Error makeError(InvalidName Reason) {
-  auto Message = [](InvalidName Reason) {
+  auto Message = [](const InvalidName &Reason) {
 switch (Reason.K) {
 case InvalidName::Keywords:
   return llvm::formatv("the chosen name \"{0}\" is a keyword",
@@ -733,7 +733,7 @@
 return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
-return makeError(*Invalid);
+return makeError(std::move(*Invalid));
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
   if (Reject)


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1060,6 +1060,11 @@
   )cpp",
"conflict", !HeaderFile, "Conflict"},
 
+  {R"cpp(
+int V^ar;
+  )cpp",
+   "\"const\" is a keyword", !HeaderFile, "const"},
+
   {R"cpp(// Trying to rename into the same name, SameName == SameName.
 void func() {
   int S^ameName;
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -455,7 +455,7 @@
 }
 
 llvm::Error makeError(InvalidName Reason) {
-  auto Message = [](InvalidName Reason) {
+  auto Message = [](const InvalidName &Reason) {
 switch (Reason.K) {
 case InvalidName::Keywords:
   return llvm::formatv("the chosen name \"{0}\" is a keyword",
@@ -733,7 +733,7 @@
 return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
-return makeError(*Invalid);
+return makeError(std::move(*Invalid));
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
   if (Reject)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits