[clangd] Fix hover crashing on integral or enumeral casts

[gkll]

Summary

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.

[sammccall]

Oh wow, my mental model of these was all wrong.

Thank you! Do you want me to land this for you?

[gkll]

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!

[sammccall]

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)

[gkll]

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: [e-mail]

[sammccall]

Landed as ce5ebf0b9113df8ccaec1bcfd6804fb16cdef69d but I had to revert due to test failures on windows:

• https://buildkite.com/llvm-project/premerge-checks/builds/94783
• http://45.33.8.238/win/59124/step_9.txt

Unfortunately neither of these give a stack trace, so I'm not yet sure what's wrong, beyond "some Optional is null when it shouldn't be".
A wild guess: long is shorter than a pointer on 64-bit windows, so that code doesn't even parse.
Indeed (int)&global_var doesn't parse for me on a 64-bit linux machine (32-bit int, 64-bit pointer).
The mystery in that case, though, is why the test output doesn't show parse error before crashing (TestTU is supposed to dump this unless you set ErrorOK).

It's late, but I'll try fixing and re-landing tomorrow.

[gkll]

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

[gkll]

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

[gkll]

Fixed formatting of Argv array.

[sammccall]

Neat, looks like the pre-merge windows buildbot at least is happy!
I'll land this tomorrow.

clang-tools-extra/clangd/unittests/HoverTests.cpp

3221

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.

clang-tools-extra/clangd/unittests/TestTU.h

63

If you do add this option, please invert the sense, i.e. bool PredefineMacros = false;

(The double negative of Omit = false is a bit confusing to read)

And please expand the comment slightly: "... such as __INTPTR_TYPE__"

[gkll]

Avoid double negation.

[gkll]

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

clang-tools-extra/clangd/unittests/HoverTests.cpp

3221

I prefer using the predefine.

[gkll]

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

[sammccall]

Sorry about this! Landing now

clang-tools-extra/clangd/Hover.cpp

llvm::Optional<std::string> printExprValue(const Expr *E,
   // Attempt to evaluate. If expr is dependent, evaluation crashes!
   if (E->isValueDependent() || !E->EvaluateAsRValue(Constant, Ctx) ||
       // Disable printing for record-types, as they are usually confusing and
       // might make clang crash while printing the expressions.
       Constant.Val.isStruct() || Constant.Val.isUnion())
     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 :
          T->castAs<EnumType>()->getDecl()->enumerators())
       if (ECD->getInitVal() == Val)
         return llvm::formatv("{0} ({1})", ECD->getNameAsString(),
                              printHex(Constant.Val.getInt()))
             .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),
                          printHex(Constant.Val.getInt()))
         .str();
   return Constant.Val.getAsString(Ctx, T);
 }
 

clang-tools-extra/clangd/unittests/HoverTests.cpp

TEST(Hover, HideBigInitializers) {
 
   TestTU TU = TestTU::withCode(T.code());
   auto AST = TU.build();
   auto H = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
 
   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<Test>(reinterpret_cast<uintptr_t>(&global_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, "&global_var");
+}
+
+TEST(Hover, GlobalVarIntCastNoCrash) {
+  Annotations T(R"cpp(
+    using uintptr_t = __UINTPTR_TYPE__;
+    unsigned global_var;
+    void foo() {
+      uintptr_t a^ddress = reinterpret_cast<uintptr_t>(&global_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, "&global_var");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TestTU.h

struct TestTU {
   std::string HeaderFilename = "TestTU.h";
 
   // Name and contents of each file.
   llvm::StringMap<std::string> AdditionalFiles;
 
   // Extra arguments for the compiler invocation.
   std::vector<std::string> ExtraArgs;
 
+  // Omit predefined macros.
+  bool OmitPredefinedMacros = true;
+
   TidyProvider ClangTidyProvider = {};
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
   // Parse options pass on to the ParseInputs

clang-tools-extra/clangd/unittests/TestTU.cpp

ParseInputs TestTU::inputs(MockFS &FS) const {
   FS.Files = AdditionalFiles;
   FS.Files[FullFilename] = Code;
   FS.Files[FullHeaderName] = HeaderCode;
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
   Inputs.FeatureModules = FeatureModules;
   auto &Argv = 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()) {
     Argv.push_back("-include");
     Argv.push_back(ImplicitHeaderGuard ? ImportThunk : FullHeaderName);
     // ms-compatibility changes the meaning of #import.
     // The default is OS-dependent (on on windows), ensure it's off.
     if (ImplicitHeaderGuard)