[clangd] Allow hover over 128-bit variable without crashing #71415
Conversation
When hovering over variables larger than 64 bits, with more than 64 active bits, there were assertion failures since Hover is trying to print the value as a 64-bit hex value. There is already protection avoiding to call printHex if there is more than 64 significant bits. And we already truncate and print negative values using only 32 bits, when possible. So we can simply truncate values with more than 64 bits to avoid the assert when using getZExtValue. The result will be that for example a negative 128 bit variable is printed using 64 bits, when possible. There is still no support for printing more than 64 bits. That would involve more changes since for example llvm::FormatterNumber is limited to 64 bits.
|
@llvm/pr-subscribers-clangd Author: Björn Pettersson (bjope) ChangesWhen hovering over variables larger than 64 bits, with more than 64 active bits, there were assertion failures since Hover is trying to print the value as a 64-bit hex value. There is already protection avoiding to call printHex if there is more than 64 significant bits. And we already truncate and print negative values using only 32 bits, when possible. So we can simply truncate values with more than 64 bits to avoid the assert when using getZExtValue. The result will be that for example a negative 128 bit variable is printed using 64 bits, when possible. There is still no support for printing more than 64 bits. That would involve more changes since for example llvm::FormatterNumber is limited to 64 bits. Full diff: https://github.com/llvm/llvm-project/pull/71415.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 7f7b5513dff6fee..a868d3bb4e3fa1d 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -408,7 +408,9 @@ void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
// -2 => 0xfffffffe
// -2^32 => 0xffffffff00000000
static llvm::FormattedNumber printHex(const llvm::APSInt &V) {
- uint64_t Bits = V.getZExtValue();
+ assert(V.getSignificantBits() <= 64 && "Can't print more than 64 bits.");
+ uint64_t Bits =
+ V.getBitWidth() > 64 ? V.trunc(64).getZExtValue() : V.getZExtValue();
if (V.isNegative() && V.getSignificantBits() <= 32)
return llvm::format_hex(uint32_t(Bits), 0);
return llvm::format_hex(Bits, 0);
diff --git a/clang-tools-extra/clangd/test/hover2.test b/clang-tools-extra/clangd/test/hover2.test
new file mode 100644
index 000000000000000..24d82bde20a7823
--- /dev/null
+++ b/clang-tools-extra/clangd/test/hover2.test
@@ -0,0 +1,29 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"__int128_t bar = -4;\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":13}}}
+# CHECK: "id": 1,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": {
+# CHECK-NEXT: "contents": {
+# CHECK-NEXT: "kind": "plaintext",
+# CHECK-NEXT: "value": "variable bar\n\nType: __int128_t (aka __int128)\nValue = -4 (0xfffffffc)\n\n__int128_t bar = -4"
+# CHECK-NEXT: },
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 14,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 11,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
|
| @@ -0,0 +1,29 @@ | |||
| # RUN: clangd -lit-test < %s | FileCheck %s | |||
There was a problem hiding this comment.
Rather than creating a lit-test, could you please move it to the HoverTests.cpp under the unittests directory? That would be more straightforward, and I think we could add the case to Hover.NoCrashAPInt64 (or feel free to rename that if you don't like the name.)
There was a problem hiding this comment.
Thanks. I'm not that familiar with the clangd test cases. But I've update the pull-request with a unittest instead.
Replace the test case a with a unittest.
|
Thanks! Please go ahead and merge it. :-) |
When hovering over variables larger than 64 bits, with more than 64 active bits, there were assertion failures since Hover is trying to print the value as a 64-bit hex value. There is already protection avoiding to call printHex if there is more than 64 significant bits. And we already truncate and print negative values using only 32 bits, when possible. So we can simply truncate values with more than 64 bits to avoid the assert when using getZExtValue. The result will be that for example a negative 128 bit variable is printed using 64 bits, when possible. There is still no support for printing more than 64 bits. That would involve more changes since for example llvm::FormatterNumber is limited to 64 bits. This is a second attempt at landing this patch. Now with protection to ensure we use a triple that supports __int128_t.
When hovering over variables larger than 64 bits, with more than 64 active bits, there were assertion failures since Hover is trying to print the value as a 64-bit hex value.
There is already protection avoiding to call printHex if there is more than 64 significant bits. And we already truncate and print negative values using only 32 bits, when possible. So we can simply truncate values with more than 64 bits to avoid the assert when using getZExtValue. The result will be that for example a negative 128 bit variable is printed using 64 bits, when possible.
There is still no support for printing more than 64 bits. That would involve more changes since for example llvm::FormatterNumber is limited to 64 bits.