Skip to content

[clang-format] Update FormatToken::isSimpleTypeSpecifier() #80241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

owenca
Copy link
Contributor

@owenca owenca commented Feb 1, 2024

Now with a8279a8, we can make the update.

@llvmbot
Copy link
Member

llvmbot commented Feb 1, 2024

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Now with a8279a8, we can make the update.


Full diff: https://github.com/llvm/llvm-project/pull/80241.diff

4 Files Affected:

  • (modified) clang/include/clang/Format/Format.h (+2)
  • (modified) clang/lib/Format/FormatToken.cpp (+1-34)
  • (modified) clang/lib/Format/FormatTokenLexer.cpp (+4-3)
  • (modified) clang/lib/Format/FormatTokenLexer.h (-1)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index efcb4e1d87ea4..361fde1ffece6 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -5141,6 +5141,8 @@ tooling::Replacements sortUsingDeclarations(const FormatStyle &Style,
                                             ArrayRef<tooling::Range> Ranges,
                                             StringRef FileName = "<stdin>");
 
+extern LangOptions LangOpts;
+
 /// Returns the ``LangOpts`` that the formatter expects you to set.
 ///
 /// \param Style determines specific settings for lexing mode.
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index b791c5a26bbe3..69f751db89630 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -34,41 +34,8 @@ const char *getTokenTypeName(TokenType Type) {
   return nullptr;
 }
 
-// FIXME: This is copy&pasted from Sema. Put it in a common place and remove
-// duplication.
 bool FormatToken::isSimpleTypeSpecifier() const {
-  switch (Tok.getKind()) {
-  case tok::kw_short:
-  case tok::kw_long:
-  case tok::kw___int64:
-  case tok::kw___int128:
-  case tok::kw_signed:
-  case tok::kw_unsigned:
-  case tok::kw_void:
-  case tok::kw_char:
-  case tok::kw_int:
-  case tok::kw_half:
-  case tok::kw_float:
-  case tok::kw_double:
-  case tok::kw___bf16:
-  case tok::kw__Float16:
-  case tok::kw___float128:
-  case tok::kw___ibm128:
-  case tok::kw_wchar_t:
-  case tok::kw_bool:
-#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) case tok::kw___##Trait:
-#include "clang/Basic/TransformTypeTraits.def"
-  case tok::annot_typename:
-  case tok::kw_char8_t:
-  case tok::kw_char16_t:
-  case tok::kw_char32_t:
-  case tok::kw_typeof:
-  case tok::kw_decltype:
-  case tok::kw__Atomic:
-    return true;
-  default:
-    return false;
-  }
+  return Tok.isSimpleTypeSpecifier(LangOpts);
 }
 
 bool FormatToken::isTypeOrIdentifier() const {
diff --git a/clang/lib/Format/FormatTokenLexer.cpp b/clang/lib/Format/FormatTokenLexer.cpp
index d7de09ef0e12a..53fe8a19b2fba 100644
--- a/clang/lib/Format/FormatTokenLexer.cpp
+++ b/clang/lib/Format/FormatTokenLexer.cpp
@@ -22,18 +22,20 @@
 namespace clang {
 namespace format {
 
+LangOptions LangOpts;
+
 FormatTokenLexer::FormatTokenLexer(
     const SourceManager &SourceMgr, FileID ID, unsigned Column,
     const FormatStyle &Style, encoding::Encoding Encoding,
     llvm::SpecificBumpPtrAllocator<FormatToken> &Allocator,
     IdentifierTable &IdentTable)
     : FormatTok(nullptr), IsFirstToken(true), StateStack({LexerState::NORMAL}),
-      Column(Column), TrailingWhitespace(0),
-      LangOpts(getFormattingLangOpts(Style)), SourceMgr(SourceMgr), ID(ID),
+      Column(Column), TrailingWhitespace(0), SourceMgr(SourceMgr), ID(ID),
       Style(Style), IdentTable(IdentTable), Keywords(IdentTable),
       Encoding(Encoding), Allocator(Allocator), FirstInLineIndex(0),
       FormattingDisabled(false), MacroBlockBeginRegex(Style.MacroBlockBegin),
       MacroBlockEndRegex(Style.MacroBlockEnd) {
+  LangOpts = getFormattingLangOpts(Style);
   Lex.reset(new Lexer(ID, SourceMgr.getBufferOrFake(ID), SourceMgr, LangOpts));
   Lex->SetKeepWhitespaceMode(true);
 
@@ -1442,7 +1444,6 @@ void FormatTokenLexer::readRawToken(FormatToken &Tok) {
 
 void FormatTokenLexer::resetLexer(unsigned Offset) {
   StringRef Buffer = SourceMgr.getBufferData(ID);
-  LangOpts = getFormattingLangOpts(Style);
   Lex.reset(new Lexer(SourceMgr.getLocForStartOfFile(ID), LangOpts,
                       Buffer.begin(), Buffer.begin() + Offset, Buffer.end()));
   Lex->SetKeepWhitespaceMode(true);
diff --git a/clang/lib/Format/FormatTokenLexer.h b/clang/lib/Format/FormatTokenLexer.h
index 65dd733bd5335..52838f1d8a17f 100644
--- a/clang/lib/Format/FormatTokenLexer.h
+++ b/clang/lib/Format/FormatTokenLexer.h
@@ -120,7 +120,6 @@ class FormatTokenLexer {
   unsigned Column;
   unsigned TrailingWhitespace;
   std::unique_ptr<Lexer> Lex;
-  LangOptions LangOpts;
   const SourceManager &SourceMgr;
   FileID ID;
   const FormatStyle &Style;

@owenca owenca merged commit 763139a into llvm:main Feb 9, 2024
@owenca owenca deleted the simple-type branch February 9, 2024 05:42
owenca added a commit that referenced this pull request Feb 9, 2024
…80241)"

This reverts commit 763139a.

It seems that LangOpts is not initialized before use.
@hokein
Copy link
Collaborator

hokein commented Feb 12, 2024

This change introduced an asan crash when running the QualifierFixerTest.IsQualifierType unittest:

$ tools/clang/unittests/Format/FormatTests --gtest_filter="QualifierFixerTest.IsQualifierType"
Note: Google Test filter = QualifierFixerTest.IsQualifierType
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from QualifierFixerTest
[ RUN      ] QualifierFixerTest.IsQualifierType
=================================================================
==2418936==ERROR: AddressSanitizer: heap-use-after-free on address 0x621000007ca8 at pc 0x55ba1c653541 bp 0x7ffcf3b39400 sp 0x7ffcf3b393f8
READ of size 8 at 0x621000007ca8 thread T0
    #0 0x55ba1c653540 in getTokenID llvm-project/clang/include/clang/Basic/IdentifierTable.h:304:62
    #1 0x55ba1c653540 in clang::IdentifierInfo::isKeyword(clang::LangOptions const&) const llvm-project/clang/lib/Basic/IdentifierTable.cpp:345:38
    #2 0x55ba1c742ca6 in clang::format::LeftRightQualifierAlignmentFixer::isConfiguredQualifierOrType(clang::format::FormatToken const*, std::vector<clang::tok::TokenKind, std::allocator<clang::tok::TokenKind>> const&) llvm-project/clang/lib/Format/QualifierAlignmentFixer.cpp:620:23
    #3 0x55ba1c16dd29 in clang::format::test::(anonymous namespace)::QualifierFixerTest_IsQualifierType_Test::TestBody() llvm-project/clang/unittests/Format/QualifierFixerTest.cpp:1070:3
    #4 0x55ba1c5b2edc in testing::Test::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:5
    #5 0x55ba1c5b50b0 in testing::TestInfo::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2836:11
    #6 0x55ba1c5b73ee in testing::TestSuite::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:3015:30
    #7 0x55ba1c5e317f in testing::internal::UnitTestImpl::RunAllTests() llvm-project/third-party/unittest/googletest/src/gtest.cc:5920:44
    #8 0x55ba1c5e23f0 in testing::UnitTest::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:5484:10
    #9 0x55ba1c57dff0 in RUN_ALL_TESTS llvm-project/third-party/unittest/googletest/include/gtest/gtest.h:2317:73
    #10 0x55ba1c57dff0 in main llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:55:10
    #11 0x7f960e6456c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #12 0x7f960e645784 in __libc_start_main csu/../csu/libc-start.c:360:3
    #13 0x55ba1b9c42d0 in _start (llvm-project/build-asan/tools/clang/unittests/Format/FormatTests+0xa9a2d0) (BuildId: b18a4002905d1789605532475cf5513986b28718)

0x621000007ca8 is located 936 bytes inside of 4096-byte region [0x621000007900,0x621000008900)
freed by thread T0 here:
    #0 0x55ba1ba8f606 in operator delete(void*, std::align_val_t) (llvm-project/build-asan/tools/clang/unittests/Format/FormatTests+0xb65606) (BuildId: b18a4002905d1789605532475cf5513986b28718)
    #1 0x55ba1c03fef6 in Deallocate llvm-project/llvm/include/llvm/Support/AllocatorBase.h:99:5
    #2 0x55ba1c03fef6 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::DeallocateSlabs(void**, void**) llvm-project/llvm/include/llvm/Support/Allocator.h:356:28
    #3 0x55ba1c03f485 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::~BumpPtrAllocatorImpl() llvm-project/llvm/include/llvm/Support/Allocator.h:98:5
    #4 0x55ba1c16cb3f in ~IdentifierTable llvm-project/clang/include/clang/Basic/IdentifierTable.h:630:7
    #5 0x55ba1c16cb3f in ~TestLexer llvm-project/clang/unittests/Format/TestLexer.h:58:7
    #6 0x55ba1c16cb3f in annotate llvm-project/clang/unittests/Format/QualifierFixerTest.cpp:33:5
    #7 0x55ba1c16cb3f in clang::format::test::(anonymous namespace)::QualifierFixerTest_IsQualifierType_Test::TestBody() llvm-project/clang/unittests/Format/QualifierFixerTest.cpp:1056:17
    #8 0x55ba1c5b2edc in testing::Test::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:5
    #9 0x55ba1c5b50b0 in testing::TestInfo::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2836:11
    #10 0x55ba1c5b73ee in testing::TestSuite::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:3015:30
    #11 0x55ba1c5e317f in testing::internal::UnitTestImpl::RunAllTests() llvm-project/third-party/unittest/googletest/src/gtest.cc:5920:44
    #12 0x55ba1c5e23f0 in testing::UnitTest::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:5484:10
    #13 0x55ba1c57dff0 in RUN_ALL_TESTS llvm-project/third-party/unittest/googletest/include/gtest/gtest.h:2317:73
    #14 0x55ba1c57dff0 in main llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:55:10
    #15 0x7f960e6456c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

previously allocated by thread T0 here:
    #0 0x55ba1ba8eba6 in operator new(unsigned long, std::align_val_t) (llvm-project/build-asan/tools/clang/unittests/Format/FormatTests+0xb64ba6) (BuildId: b18a4002905d1789605532475cf5513986b28718)
    #1 0x55ba1c43d4bd in llvm::allocate_buffer(unsigned long, unsigned long) llvm-project/llvm/lib/Support/MemAlloc.cpp:16:10
    #2 0x55ba1bac21d0 in Allocate llvm-project/llvm/include/llvm/Support/AllocatorBase.h:92:12
    #3 0x55ba1bac21d0 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::StartNewSlab() llvm-project/llvm/include/llvm/Support/Allocator.h:339:42
    #4 0x55ba1bac1f6b in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::Allocate(unsigned long, llvm::Align) llvm-project/llvm/include/llvm/Support/Allocator.h:195:5
    #5 0x55ba1c3d1f64 in Allocate llvm-project/llvm/include/llvm/Support/Allocator.h:209:12
    #6 0x55ba1c3d1f64 in allocateWithKey<llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096UL, 4096UL, 128UL> > llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:52:32
    #7 0x55ba1c3d1f64 in llvm::StringMapEntry<clang::IdentifierInfo*>* llvm::StringMapEntry<clang::IdentifierInfo*>::create<llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>, std::nullptr_t>(llvm::StringRef, llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>&, std::nullptr_t&&) llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:128:17
    #8 0x55ba1c3d1d66 in std::pair<llvm::StringMapIterator<clang::IdentifierInfo*>, bool> llvm::StringMap<clang::IdentifierInfo*, llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>>::try_emplace_with_hash<std::nullptr_t>(llvm::StringRef, unsigned int, std::nullptr_t&&) llvm-project/llvm/include/llvm/ADT/StringMap.h:384:9
    #9 0x55ba1c3d1912 in try_emplace<std::nullptr_t> llvm-project/llvm/include/llvm/ADT/StringMap.h:368:12
    #10 0x55ba1c3d1912 in clang::IdentifierTable::get(llvm::StringRef) llvm-project/clang/include/clang/Basic/IdentifierTable.h:664:30
    #11 0x55ba1c63c207 in get llvm-project/clang/include/clang/Basic/IdentifierTable.h:688:26
    #12 0x55ba1c63c207 in AddKeyword llvm-project/clang/lib/Basic/IdentifierTable.cpp:261:13
    #13 0x55ba1c63c207 in clang::IdentifierTable::AddKeywords(clang::LangOptions const&) llvm-project/clang/include/clang/Basic/TokenKinds.def:290:1
    #14 0x55ba1c040b4e in clang::format::TestLexer::TestLexer(llvm::SpecificBumpPtrAllocator<clang::format::FormatToken>&, std::vector<std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>, std::allocator<std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer>>>>&, clang::format::FormatStyle) llvm-project/clang/unittests/Format/TestLexer.h:64:36
    #15 0x55ba1c16cae6 in annotate llvm-project/clang/unittests/Format/QualifierFixerTest.cpp:33:12
    #16 0x55ba1c16cae6 in clang::format::test::(anonymous namespace)::QualifierFixerTest_IsQualifierType_Test::TestBody() llvm-project/clang/unittests/Format/QualifierFixerTest.cpp:1056:17
    #17 0x55ba1c5b2edc in testing::Test::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:5
    #18 0x55ba1c5b50b0 in testing::TestInfo::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2836:11
    #19 0x55ba1c5b73ee in testing::TestSuite::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:3015:30
    #20 0x55ba1c5e317f in testing::internal::UnitTestImpl::RunAllTests() llvm-project/third-party/unittest/googletest/src/gtest.cc:5920:44
    #21 0x55ba1c5e23f0 in testing::UnitTest::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:5484:10
    #22 0x55ba1c57dff0 in RUN_ALL_TESTS llvm-project/third-party/unittest/googletest/include/gtest/gtest.h:2317:73
    #23 0x55ba1c57dff0 in main llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:55:10
    #24 0x7f960e6456c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: heap-use-after-free llvm-project/clang/include/clang/Basic/IdentifierTable.h:304:62 in getTokenID
Shadow bytes around the buggy address:
  0x621000007a00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007a80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007b00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007b80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007c00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x621000007c80: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd
  0x621000007d00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007d80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007e00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007e80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x621000007f00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):

hokein added a commit that referenced this pull request Feb 12, 2024
…ier() (#80241)""

The change caused an asan crash when running the `QualifierFixerTest.IsQualifierType` unittest, see
details: #80241 (comment)

This reverts commit 7f40c5c.
@hokein
Copy link
Collaborator

hokein commented Feb 12, 2024

I have reverted it in 50ed98f, feel free to reland it with a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants