[clangd] Fix crash in bugprone-bad-signal-to-kill-thread clang-tidy check.

[ArcsinX]

Summary

Inside clangd, clang-tidy checks don't see preprocessor events in the preamble.
This leads to Token::PtrData == nullptr for tokens that the macro is defined to.
E.g. #define SIGTERM 15:

• Token::Kind == tok::numeric_constant (Token::isLiteral() == true)
• Token::UintData == 2
• Token::PtrData == nullptr

As the result of this, bugprone-bad-signal-to-kill-thread check crashes at null-dereference inside clangd.

[hokein]

Thanks, this is a nice catch.

Looks like NotNullTerminatedResultCheck.cpp also has this pattern, we may want to fix that as well.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

441

ClangTidyBadSignalToKillThread doesn't seen ti provide much information about what is testcase testing. Maybe NoLiteralDataInMacroToken?

[ArcsinX]

Test rename: ClangTidyBadSignalToKillThread => ClangTidyNoLiteralDataInMacroToken

[ArcsinX]

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

441

Thanks, renamed. But I want to keep ClangTidy prefix because the problem related with clang-tidy limitations inside clangd.

[ArcsinX]

In D85417#2199694, @hokein wrote:

Looks like NotNullTerminatedResultCheck.cpp also has this pattern, we may want to fix that as well.

Yes, you are right. I will fix this in the next patch.

[ArcsinX]

In D85417#2199830, @ArcsinX wrote:

In D85417#2199694, @hokein wrote:

Looks like NotNullTerminatedResultCheck.cpp also has this pattern, we may want to fix that as well.

Yes, you are right. I will fix this in the next patch.

@hokein Sorry for bother you. Seems bugprone-not-null-terminated-result has the same 3 problems as bugprone-bad-signal-to-kill-thread:

• #undef __STDC_WANT_LIB_EXT1__
• #define __STDC_WANT_LIB_EXT1__ ((unsigned)1)
• Token::PtrData == nullptr inside clangd

Do you expect separate 3 patches for this (as we did for bugprone-bad-signal-to-kill-thread) ?

[hokein]

In D85417#2201953, @ArcsinX wrote:

In D85417#2199830, @ArcsinX wrote:

In D85417#2199694, @hokein wrote:

Looks like NotNullTerminatedResultCheck.cpp also has this pattern, we may want to fix that as well.

Yes, you are right. I will fix this in the next patch.

@hokein Sorry for bother you. Seems bugprone-not-null-terminated-result has the same 3 problems as bugprone-bad-signal-to-kill-thread:

• #undef __STDC_WANT_LIB_EXT1__
• #define __STDC_WANT_LIB_EXT1__ ((unsigned)1)
• Token::PtrData == nullptr inside clangd

Do you expect separate 3 patches for this (as we did for bugprone-bad-signal-to-kill-thread) ?

yeah, that would be nice. Thanks for digging into this.

[hokein]

Thinking more about this,

Inside clangd, clang-tidy checks don't see preprocessor events in the preamble.
This leads to Token::PtrData == nullptr for tokens that the macro is defined to.
E.g. #define SIGTERM 15:

Token::Kind == tok::numeric_constant (Token::isLiteral() == true)
Token::UintData == 2
Token::PtrData == nullptr

The token is in a pretty-broken state. Do you know why the UintData is set to 2, I suppose this is the length of the macro definition (15). If the PtrData is nullptr, I'd expect the UintData is 0.

[ArcsinX]

In D85417#2201973, @hokein wrote:

Thinking more about this,

Inside clangd, clang-tidy checks don't see preprocessor events in the preamble.
This leads to Token::PtrData == nullptr for tokens that the macro is defined to.
E.g. #define SIGTERM 15:

Token::Kind == tok::numeric_constant (Token::isLiteral() == true)
Token::UintData == 2
Token::PtrData == nullptr

The token is in a pretty-broken state. Do you know why the UintData is set to 2, I suppose this is the length of the macro definition (15). If the PtrData is nullptr, I'd expect the UintData is 0.

I think it's here:

Token ASTReader::ReadToken(ModuleFile &F, const RecordDataImpl &Record,
                           unsigned &Idx) {
  Token Tok;
  Tok.startToken();
  Tok.setLocation(ReadSourceLocation(F, Record, Idx));
  Tok.setLength(Record[Idx++]);
  if (IdentifierInfo *II = getLocalIdentifier(F, Record[Idx++]))
    Tok.setIdentifierInfo(II);
  Tok.setKind((tok::TokenKind)Record[Idx++]);
  Tok.setFlag((Token::TokenFlags)Record[Idx++]);
  return Tok;
}

So, we set Token::UintData via Token::setLength() at preamble read, but do not set Token::PtrData without preprocessor events.

[hokein]

In D85417#2202046, @ArcsinX wrote:

In D85417#2201973, @hokein wrote:

Thinking more about this,

Inside clangd, clang-tidy checks don't see preprocessor events in the preamble.
This leads to Token::PtrData == nullptr for tokens that the macro is defined to.
E.g. #define SIGTERM 15:

Token::Kind == tok::numeric_constant (Token::isLiteral() == true)
Token::UintData == 2
Token::PtrData == nullptr

The token is in a pretty-broken state. Do you know why the UintData is set to 2, I suppose this is the length of the macro definition (15). If the PtrData is nullptr, I'd expect the UintData is 0.

I think it's here:

Token ASTReader::ReadToken(ModuleFile &F, const RecordDataImpl &Record,
                           unsigned &Idx) {
  Token Tok;
  Tok.startToken();
  Tok.setLocation(ReadSourceLocation(F, Record, Idx));
  Tok.setLength(Record[Idx++]);
  if (IdentifierInfo *II = getLocalIdentifier(F, Record[Idx++]))
    Tok.setIdentifierInfo(II);
  Tok.setKind((tok::TokenKind)Record[Idx++]);
  Tok.setFlag((Token::TokenFlags)Record[Idx++]);
  return Tok;
}

So, we set Token::UintData via Token::setLength() at preamble read, but do not set Token::PtrData without preprocessor events.

I see, thanks! I think this is a missing feature in AST serialization, see https://github.com/llvm/llvm-project/blob/master/clang/lib/Serialization/ASTWriter.cpp#L4260-L4261.

clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp

return KeyValue.first->getName() == "SIGTERM" &&
            KeyValue.first->hasMacroDefinition();
   };
   const auto TryExpandAsInteger =
       [](Preprocessor::macro_iterator It) -> Optional<unsigned> {
     if (It == PP->macro_end())
       return llvm::None;
     const MacroInfo *MI = PP->getMacroInfo(It->first);
     const Token &T = MI->tokens().back();
-    if (!T.isLiteral())
+    if (!T.isLiteral() || !T.getLiteralData())
       return llvm::None;
     StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
 
     llvm::APInt IntValue;
     constexpr unsigned AutoSenseRadix = 0;
     if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
       return llvm::None;
     return IntValue.getZExtValue();

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Annotations Main(R"cpp(
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyChecks = "bugprone-integer-division";
   TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
   EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
 }
 
+TEST(DiagnosticTest, ClangTidyBadSignalToKillThread) {
+  Annotations Main(R"cpp(
+    #define SIGTERM 15
+    using pthread_t = int;
+    int pthread_kill(pthread_t thread, int sig);
+    int func() {
+      pthread_t thread;
+      return pthread_kill(thread, 0);
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "-*,bugprone-bad-signal-to-kill-thread";
+  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO