This is an archive of the discontinued LLVM Phabricator instance.

[clang-doc][NFC] Fix reference invalidation assertion failure.
Needs ReviewPublic

Authored by MTC on Jun 20 2022, 7:28 AM.

Details

Summary

Fix the assertion failure caused by reference invalidation in SmallString. It seems that this bug was not caught when adding the assertion, see https://reviews.llvm.org/D91744.

We can reproduce this bug trivially through the following usage.

$ clang-doc  --format=html test.cpp
Emiting docs in html format.
clang-doc: llvm-project/llvm/include/llvm/ADT/SmallVector.h:173: void llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::assertSafeToReferenceAfterResize(const void*, size_t) [with T = char; <template-parameter-1-2> = void; size_t = long unsigned int]: Assertion `isSafeToReferenceAfterResize(Elt, NewSize) && "Attempting to reference an element of the vector in an operation " "that invalidates it"' failed.
 #0 0x000055ce4806b843 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:569:22
 #1 0x000055ce4806b8fa PrintStackTraceSignalHandler(void*) llvm-project/llvm/lib/Support/Unix/Signals.inc:636:1
 #2 0x000055ce4806989a llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:103:20
 #3 0x000055ce4806b277 SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007f72a4533730 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12730)
 #5 0x00007f72a3e1a7bb raise /build/glibc-fWwxX8/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #6 0x00007f72a3e05535 abort /build/glibc-fWwxX8/glibc-2.28/stdlib/abort.c:81:7
 #7 0x00007f72a3e0540f _nl_load_domain /build/glibc-fWwxX8/glibc-2.28/intl/loadmsgcat.c:1177:9
 #8 0x00007f72a3e13102 (/lib/x86_64-linux-gnu/libc.so.6+0x30102)
 #9 0x000055ce47f76f1a llvm::SmallVectorTemplateCommon<char, void>::assertSafeToReferenceAfterResize(void const*, unsigned long) llvm-project/llvm/include/llvm/ADT/SmallVector.h:171:5
#10 0x000055ce47f79efc llvm::SmallVectorTemplateCommon<char, void>::assertSafeToReferenceAfterClear(char const*, char const*) llvm-project/llvm/include/llvm/ADT/SmallVector.h:187:47
#11 0x000055ce47f743ed void llvm::SmallVectorImpl<char>::assign<char const*, void>(char const*, char const*) llvm-project/llvm/include/llvm/ADT/SmallVector.h:713:5
#12 0x000055ce47f6c587 llvm::SmallString<128u>::assign(llvm::StringRef) llvm-project/llvm/include/llvm/ADT/SmallString.h:53:3
#13 0x000055ce47f64338 llvm::SmallString<128u>::operator=(llvm::StringRef) llvm-project/llvm/include/llvm/ADT/SmallString.h:279:13
#14 0x000055ce47f4ab4c main llvm-project/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:226:28
#15 0x00007f72a3e0709b __libc_start_main /build/glibc-fWwxX8/glibc-2.28/csu/../csu/libc-start.c:342:3
#16 0x000055ce47f4963a _start (./bin/clang-doc+0x54563a)
[1]    1443196 abort (core dumped)  ./bin/clang-doc --format=html test.cpp

Diff Detail

Event Timeline

MTC created this revision.Jun 20 2022, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 7:28 AM
MTC requested review of this revision.Jun 20 2022, 7:28 AM

Is there a test that can be added so this doesn’t regress?

Also, I wonder if assign could/should add logic to be resilient to this.

MTC added a comment.Jun 22 2022, 4:48 AM

Sorry for the delayed response. Adding the regression test is not as simple as I thought, first of all clang-tools-extra does not enable the assert, see https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/lit.site.cfg.py.in . And add the clang-doc when enable --format=html is little bit of tricky.

Also, I wonder if assign could/should add logic to be resilient to this.

Eventhough destroy_range() does nothing for now, there are indeed potential risks.

void assign(in_iter in_start, in_iter in_end) {
  this->assertSafeToReferenceAfterClear(in_start, in_end);
  clear();
  append(in_start, in_end);
}
void clear() {
  this->destroy_range(this->begin(), this->end());
  this->Size = 0;
}