ivanmurashko created this revision.
ivanmurashko added reviewers: alexfh, DmitryPolukhin, sammccall, bruno.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
There is a clangd crash at `__memcmp_avx2_movbe`. Short problem description is
below.
The method `HeaderIncludes::addExistingInclude` stores `Include` objects by
reference at 2 places: `ExistingIncludes` (primary storage) and
`IncludesByPriority` (pointer to the object's location at ExistingIncludes).
`ExistingIncludes` is a map where value is a `SmallVector`. A new element is
inserted by `push_back`. The operation might do resize. As result pointers
stored at `IncludesByPriority` might become invalid.
Typical stack trace
frame #0: 0x00007f11460dcd94 libc.so.6`__memcmp_avx2_movbe + 308
frame #1: 0x00000000004782b8 clangd`llvm::StringRef::compareMemory(Lhs="
\"t2.h\"", Rhs="", Length=6) at StringRef.h:76:22
frame #2: 0x0000000000701253 clangd`llvm::StringRef::compare(this=0x0000
7f10de7d8610, RHS=(Data = "", Length = 7166742329480737377)) const at String
Ref.h:206:34
* frame #3: 0x00000000007603ab clangd`llvm::operator<(llvm::StringRef, llv
m::StringRef)(LHS=(Data = "\"t2.h\"", Length = 6), RHS=(Data = "", Length =
7166742329480737377)) at StringRef.h:907:23
frame #4: 0x0000000002d0ad9f clangd`clang::tooling::HeaderIncludes::inse
rt(this=0x00007f10de7fb1a0, IncludeName=(Data = "t2.h\"", Length = 4), IsAng
led=false) const at HeaderIncludes.cpp:365:22
frame #5: 0x00000000012ebfdd clangd`clang::clangd::IncludeInserter::inse
rt(this=0x00007f10de7fb148, VerbatimHeader=(Data = "\"t2.h\"", Length = 6))
const at Headers.cpp:262:70
A clangd lit test for the crash was created. The proposed solution uses copy by
value instead of copy by reference.
Test Plan
./bin/llvm-lit -v ../clang-tools-extra/clangd/test/repeated_includes.test
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D118755
Files:
clang-tools-extra/clangd/test/repeated_includes.test
clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
int Priority = Categories.getIncludePriority(
CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
CategoryEndOffsets[Priority] = NextLineOffset;
- IncludesByPriority[Priority].push_back(&CurInclude);
+ IncludesByPriority[Priority].push_back(CurInclude);
if (FirstIncludeOffset < 0)
FirstIncludeOffset = CurInclude.R.getOffset();
}
@@ -361,9 +361,9 @@
unsigned InsertOffset = CatOffset->second; // Fall back offset
auto Iter = IncludesByPriority.find(Priority);
if (Iter != IncludesByPriority.end()) {
- for (const auto *Inc : Iter->second) {
- if (QuotedName < Inc->Name) {
- InsertOffset = Inc->R.getOffset();
+ for (const auto &Inc : Iter->second) {
+ if (QuotedName < Inc.Name) {
+ InsertOffset = Inc.R.getOffset();
break;
}
}
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===================================================================
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -105,8 +105,7 @@
/// in the order they appear in the source file.
/// See comment for "FormatStyle::IncludeCategories" for details about
include
/// priorities.
- std::unordered_map<int, llvm::SmallVector<const Include *, 8>>
- IncludesByPriority;
+ std::unordered_map<int, llvm::SmallVector<Include, 8>> IncludesByPriority;
int FirstIncludeOffset;
// All new headers should be inserted after this offset (e.g. after header
Index: clang-tools-extra/clangd/test/repeated_includes.test
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/repeated_includes.test
@@ -0,0 +1,33 @@
+# RUN: rm -rf %/t
+# RUN: mkdir -p %t && touch %t/t.h && touch %t/t2.h && touch %t/t3.h
+# RUN: echo '#pragma once' > %t/t.h
+# RUN: echo '#include "t2.h"' >> %t/t.h
+# RUN: echo 'void bar();' >> %t/t.h
+# RUN: echo '#pragma once' > %t/t2.h
+# RUN: echo 'void bar2();' >> %t/t2.h
+
+
+# Run clangd
+# RUN: clangd -lit-test -log error --path-mappings 'C:\client=%t' < %s |
FileCheck -strict-whitespace %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":"file:///C:/client/main.cpp","languageId":"cpp","version":1,"text":"#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\nbar();\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///C:/client/main.cpp"},"position":{"line":40,"character":3}}}
+# CHECK: "id": 1
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": {
+# CHECK-NEXT: "isIncomplete": false,
+# CHECK-NEXT: "items": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "detail": "void",
+# CHECK-NEXT: "documentation": {
+# CHECK-NEXT: "kind": "plaintext",
+# CHECK-NEXT: "value": "From \"t.h\""
+# CHECK-NEXT: },
+# CHECK-NEXT: "filterText": "bar",
+# CHECK-NEXT: "insertText": "bar",
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===================================================================
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
int Priority = Categories.getIncludePriority(
CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
CategoryEndOffsets[Priority] = NextLineOffset;
- IncludesByPriority[Priority].push_back(&CurInclude);
+ IncludesByPriority[Priority].push_back(CurInclude);
if (FirstIncludeOffset < 0)
FirstIncludeOffset = CurInclude.R.getOffset();
}
@@ -361,9 +361,9 @@
unsigned InsertOffset = CatOffset->second; // Fall back offset
auto Iter = IncludesByPriority.find(Priority);
if (Iter != IncludesByPriority.end()) {
- for (const auto *Inc : Iter->second) {
- if (QuotedName < Inc->Name) {
- InsertOffset = Inc->R.getOffset();
+ for (const auto &Inc : Iter->second) {
+ if (QuotedName < Inc.Name) {
+ InsertOffset = Inc.R.getOffset();
break;
}
}
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===================================================================
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -105,8 +105,7 @@
/// in the order they appear in the source file.
/// See comment for "FormatStyle::IncludeCategories" for details about include
/// priorities.
- std::unordered_map<int, llvm::SmallVector<const Include *, 8>>
- IncludesByPriority;
+ std::unordered_map<int, llvm::SmallVector<Include, 8>> IncludesByPriority;
int FirstIncludeOffset;
// All new headers should be inserted after this offset (e.g. after header
Index: clang-tools-extra/clangd/test/repeated_includes.test
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/repeated_includes.test
@@ -0,0 +1,33 @@
+# RUN: rm -rf %/t
+# RUN: mkdir -p %t && touch %t/t.h && touch %t/t2.h && touch %t/t3.h
+# RUN: echo '#pragma once' > %t/t.h
+# RUN: echo '#include "t2.h"' >> %t/t.h
+# RUN: echo 'void bar();' >> %t/t.h
+# RUN: echo '#pragma once' > %t/t2.h
+# RUN: echo 'void bar2();' >> %t/t2.h
+
+
+# Run clangd
+# RUN: clangd -lit-test -log error --path-mappings 'C:\client=%t' < %s | FileCheck -strict-whitespace %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":"file:///C:/client/main.cpp","languageId":"cpp","version":1,"text":"#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\nbar();\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///C:/client/main.cpp"},"position":{"line":40,"character":3}}}
+# CHECK: "id": 1
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": {
+# CHECK-NEXT: "isIncomplete": false,
+# CHECK-NEXT: "items": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "detail": "void",
+# CHECK-NEXT: "documentation": {
+# CHECK-NEXT: "kind": "plaintext",
+# CHECK-NEXT: "value": "From \"t.h\""
+# CHECK-NEXT: },
+# CHECK-NEXT: "filterText": "bar",
+# CHECK-NEXT: "insertText": "bar",
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
