[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll requested review of this revision.
gkll marked an inline comment as done.
gkll added a comment.

Mhm, I think the change here has gone under the radar 


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

https://reviews.llvm.org/D126498

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

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll marked 2 inline comments as done.
gkll added a comment.

Thank you, and sorry for the chaos, this is my first submission to llvm :D




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3221
+  TestTU TU = TestTU::withCode(T.code());
+  TU.OmitPredefinedMacros = false;
+  auto AST = TU.build();

sammccall wrote:
> As an alternative (just to avoid adding this option), I think adding 
> "-target=x86_64-pc-linux-gnu" would force intptr_t to be long.
> Up to you.
I prefer using the predefine.


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

https://reviews.llvm.org/D126498

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

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll updated this revision to Diff 433304.
gkll added a comment.

Avoid double negation.


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

https://reviews.llvm.org/D126498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -59,6 +59,9 @@
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
+  // Predefine macros such as __UINTPTR_TYPE__.
+  bool PredefineMacros = false;
+
   TidyProvider ClangTidyProvider = {};
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -40,9 +40,12 @@
   ParseInputs Inputs;
   Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
-  // In tests, omit predefined macros (__GNUC__ etc) for a 25% speedup.
-  // There are hundreds, and we'd generate, parse, serialize, and re-parse them!
-  Argv = {"clang", "-Xclang", "-undef"};
+  Argv = {"clang", "-Xclang"};
+  // In tests, unless explicitly specified otherwise, omit predefined macros
+  // (__GNUC__ etc) for a 25% speedup. There are hundreds, and we'd generate,
+  // parse, serialize, and re-parse them!
+  if (!PredefineMacros)
+Argv.push_back("-undef");
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,41 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+enum Test : uintptr_t {};
+unsigned global_var;
+void foo() {
+  Test v^al = static_cast(reinterpret_cast(_var));
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.PredefineMacros = true;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+unsigned global_var;
+void foo() {
+  uintptr_t a^ddress = reinterpret_cast(_var);
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.PredefineMacros = true;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll updated this revision to Diff 433217.
gkll added a comment.

Fixed formatting of `Argv` array.


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

https://reviews.llvm.org/D126498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -59,6 +59,9 @@
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
+  // Omit predefined macros.
+  bool OmitPredefinedMacros = true;
+
   TidyProvider ClangTidyProvider = {};
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -40,9 +40,12 @@
   ParseInputs Inputs;
   Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
-  // In tests, omit predefined macros (__GNUC__ etc) for a 25% speedup.
-  // There are hundreds, and we'd generate, parse, serialize, and re-parse them!
-  Argv = {"clang", "-Xclang", "-undef"};
+  Argv = {"clang", "-Xclang"};
+  // In tests, unless explicitly specified otherwise, omit predefined macros
+  // (__GNUC__ etc) for a 25% speedup. There are hundreds, and we'd generate,
+  // parse, serialize, and re-parse them!
+  if (OmitPredefinedMacros)
+Argv.push_back("-undef");
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,41 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+enum Test : uintptr_t {};
+unsigned global_var;
+void foo() {
+  Test v^al = static_cast(reinterpret_cast(_var));
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.OmitPredefinedMacros = false;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+unsigned global_var;
+void foo() {
+  uintptr_t a^ddress = reinterpret_cast(_var);
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.OmitPredefinedMacros = false;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll updated this revision to Diff 433211.
gkll added a comment.

Use `uintptr_t` instead of `unsigned long` for the casts, to ensure the pointer 
always fits into the integer type.
For example on Windows x64 that was previously not the case, because there 
`unsigned long` is only 32-bits wide.
Thus the pointer got truncated during the cast, which resulted in `getHover()` 
returning `None` instead of `_var`.


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

https://reviews.llvm.org/D126498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -59,6 +59,9 @@
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
+  // Omit predefined macros.
+  bool OmitPredefinedMacros = true;
+
   TidyProvider ClangTidyProvider = {};
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -40,9 +40,12 @@
   ParseInputs Inputs;
   Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
-  // In tests, omit predefined macros (__GNUC__ etc) for a 25% speedup.
-  // There are hundreds, and we'd generate, parse, serialize, and re-parse them!
-  Argv = {"clang", "-Xclang", "-undef"};
+  Argv = {"clang", "-Xclang", };
+  // In tests, unless explicitly specified otherwise, omit predefined macros
+  // (__GNUC__ etc) for a 25% speedup. There are hundreds, and we'd generate,
+  // parse, serialize, and re-parse them!
+  if (OmitPredefinedMacros)
+Argv.push_back("-undef");
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,41 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+enum Test : uintptr_t {};
+unsigned global_var;
+void foo() {
+  Test v^al = static_cast(reinterpret_cast(_var));
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.OmitPredefinedMacros = false;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+using uintptr_t = __UINTPTR_TYPE__;
+unsigned global_var;
+void foo() {
+  uintptr_t a^ddress = reinterpret_cast(_var);
+}
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.OmitPredefinedMacros = false;
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll updated this revision to Diff 433174.
gkll added a comment.

Changed return type of non-returning function in test code to void.


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

https://reviews.llvm.org/D126498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,33 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+enum Test : unsigned long {};
+unsigned global_var;
+void foo() { Test v^al = (Test)(unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+unsigned global_var;
+void foo() { unsigned long a^ddress = (unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,33 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+enum Test : unsigned long {};
+unsigned global_var;
+void foo() { Test v^al = (Test)(unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+unsigned global_var;
+void foo() { unsigned long a^ddress = (unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll added a comment.

In D126498#3548168 , @sammccall wrote:

> In D126498#3548132 , @gkll wrote:
>
>> In D126498#3548013 , @sammccall 
>> wrote:
>>
>>> Oh wow, my mental model of these was all wrong.
>>>
>>> Thank you! Do you want me to land this for you?
>>
>> Yeah, that would be great, thank you!
>
> Sorry, can you provide an email address for attribution?
>
> (Normally I can pick this up with `arc patch`, but not this time apparently)

Maybe because I didn't upload the patch with the `arc` tooling.
You can attribute the patch to: g...@mailbox.org


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126498

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

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll added a comment.

In D126498#3548013 , @sammccall wrote:

> Oh wow, my mental model of these was all wrong.
>
> Thank you! Do you want me to land this for you?

Yeah, that would be great, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126498

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

[PATCH] D126498: [clangd] Fix hover crashing on integral or enumeral casts

[Georg Kotheimer via Phabricator via cfe-commits]

gkll created this revision.
gkll added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
gkll requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

When pretty printing the value of an expression, we cannot infer from
the type of the expression the type of the constant that the expression
evaluates to, as the expression might contain a type cast.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,33 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+enum Test : unsigned long {};
+unsigned global_var;
+void foo() { Test v^al = (Test)(unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+unsigned global_var;
+int foo() { unsigned long a^ddress = (unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&
   Constant.Val.getInt().uge(10))
 return llvm::formatv("{0} ({1})", Constant.Val.getAsString(Ctx, T),


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -3206,6 +3206,33 @@
   ASSERT_TRUE(H);
   EXPECT_EQ(H->Definition, "int arr[]");
 }
+
+TEST(Hover, GlobalVarEnumeralCastNoCrash) {
+  Annotations T(R"cpp(
+enum Test : unsigned long {};
+unsigned global_var;
+void foo() { Test v^al = (Test)(unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+unsigned global_var;
+int foo() { unsigned long a^ddress = (unsigned long)_var; }
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  ASSERT_TRUE(HI);
+  EXPECT_EQ(*HI->Value, "_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -429,7 +429,8 @@
 return llvm::None;
 
   // Show enums symbolically, not numerically like APValue::printPretty().
-  if (T->isEnumeralType() && Constant.Val.getInt().getMinSignedBits() <= 64) {
+  if (T->isEnumeralType() && Constant.Val.isInt() &&
+  Constant.Val.getInt().getMinSignedBits() <= 64) {
 // Compare to int64_t to avoid bit-width match requirements.
 int64_t Val = Constant.Val.getInt().getExtValue();
 for (const EnumConstantDecl *ECD :
@@ -440,7 +441,7 @@
 .str();
   }
   // Show hex value of integers if they're at least 10 (or negative!)
-  if (T->isIntegralOrEnumerationType() &&
+  if (T->isIntegralOrEnumerationType() && Constant.Val.isInt() &&
   Constant.Val.getInt().getMinSignedBits() <= 64 &&