This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Crash in __memcmp_avx2_movbe
ClosedPublic

Authored by ivanmurashko on Feb 1 2022, 11:14 PM.

Details

Summary

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 unit test test for the crash was created (HeaderIncludesTest.RepeatedIncludes). The proposed solution is to use std::list instead of llvm::SmallVector

Test Plan

./tools/clang/unittests/Tooling/ToolingTests --gtest_filter=HeaderIncludesTest.RepeatedIncludes

Diff Detail

Event Timeline

ivanmurashko created this revision.Feb 1 2022, 11:14 PM
ivanmurashko requested review of this revision.Feb 1 2022, 11:14 PM
ivanmurashko added inline comments.Feb 1 2022, 11:24 PM
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
328 ↗(On Diff #405153)

The crash is caused by vector resize initiated by this line

@ivanmurashko

Overall looks good to me but please re-upload patch on top of some stable revision where failed test passes (it seems unrelated to your changes).

struct Include is a light weight structure so having a copy seems to be the simplest solution.

Thanks! This fix makes sense to me.

clang-tools-extra/clangd/test/repeated_includes.test
1 ↗(On Diff #405886)

This should be tested more directly rather than via clangd, e.g. rather in clang/unittests/Tooling/HeaderIncludesTest.cpp

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
101–102

An alternative would be to use a std::forward_list<Include> here.

This guarantees pointer stability, it's an extra allocation but seems unlikely to matter.

It would be more robust if the data structure changes (e.g. becomes large, or is mutated after creation) but probably none of this will ever happen.

Up to you.

There are some changes:

  • unit test for HeaderIncludes was added
  • lit test for clangd was removed
  • Solution uses std::list instead of copying by value
ivanmurashko added inline comments.Feb 7 2022, 5:35 AM
clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
101–102

I changed my solution accordingly this suggestion.
Small note: The std::list seems to be more suitable here as soon as the last element of the structure is used at the code.

ivanmurashko marked an inline comment as done.Feb 7 2022, 5:36 AM
ivanmurashko marked an inline comment as not done.
sammccall accepted this revision.Feb 7 2022, 5:47 AM

Thanks, looks good!

This revision is now accepted and ready to land.Feb 7 2022, 5:47 AM
sammccall added inline comments.Feb 7 2022, 5:48 AM
clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
101–102

May be worth a comment here that std::list is used for pointer stability, since it looks a little odd at first glance.

ivanmurashko edited the summary of this revision. (Show Details)Feb 7 2022, 5:56 AM
ivanmurashko edited the summary of this revision. (Show Details)

Comment about std::list was added

comment update

ivanmurashko marked an inline comment as done.Feb 7 2022, 9:42 AM

Do you have access to commit this or should I land it for you?

ivanmurashko added a comment.EditedFeb 7 2022, 10:30 AM

Do you have access to commit this or should I land it for you?

@sammccall, I would appreciate your assistance in the land. You can use the following email address: ivan.murashko at gmail.com

This revision was automatically updated to reflect the committed changes.