[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

Tue Feb 1 23:14:51 PST 2022

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"}


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D118755.405153.patch
Type: text/x-patch
Size: 4145 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220202/a1410775/attachment-0001.bin>
