[clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[jspam]

Summary

Check llvm::Optional before dereferencing it.

Compute VirtualEndLoc differently to avoid an assertion failure
in clang::SourceManager::getFileIDLoaded:

Assertion `0 && "Invalid SLocOffset or bad function choice"' failed

[LegalizeAdulthood]

Please rebase this and fold your changes into checkers/cppcoreguidelines/virtual-class-destructor.cpp

[jspam]

Rebased

[jspam]

@LegalizeAdulthood Not sure what exactly you mean by "fold your changes into checkers/cppcoreguidelines/virtual-class-destructor.cpp". The only rebase conflict was in VirtualClassDestructorCheck.cpp. Please let me know if there is still something to do.

[LegalizeAdulthood]

In D128157#3605504, @jspam wrote:

@LegalizeAdulthood Not sure what exactly you mean by "fold your changes into checkers/cppcoreguidelines/virtual-class-destructor.cpp". The only rebase conflict was in VirtualClassDestructorCheck.cpp. Please let me know if there is still something to do.

Clang-Tidy tests and docs have moved to module subdirectories. You had originally edited the file checkers/cppcoreguidelines-virtual-class-destructor.cpp (no subdirectory, file named with module prefix) and this file was moved to checkers/cppcoreguidelines/virtual-class-destructor.cpp (now in a module subdirectory, no module prefix in file name).

Looks like git handled it automatically and you didn't notice :). This is good because it means I did the "right git thing" when I moved/renamed the files and that for most people, simply rebasing should be enough to ensure that their changes are following the file to the new location.

[LegalizeAdulthood]

LGTM

[jspam]

I see :) Yes, looks like Git's rename detection did its job.

Thanks for the review. Could you or someone else please merge this for me? Author: Joachim Priesner <[e-mail]>

[LegalizeAdulthood]

In D128157#3605829, @jspam wrote:

I see :) Yes, looks like Git's rename detection did its job.

Thanks for the review. Could you or someone else please merge this for me? Author: Joachim Priesner <[e-mail]>

I can submit, yes.

[LegalizeAdulthood]

I get a test failure when I run with your change:

  -- Testing: 972 tests, 12 workers --
  FAIL: Clang Tools :: clang-apply-replacements/basic.cpp (680 of 972)
  ******************** TEST 'Clang Tools :: clang-apply-replacements/basic.cpp' FAILED ********************
  Script:
  --
  : 'RUN: at line 1';   mkdir -p D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  : 'RUN: at line 2';   grep -Ev "// *[A-Z-]+:" D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h > D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
  : 'RUN: at line 3';   sed "s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file1.yaml > D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file1.yaml
  : 'RUN: at line 4';   sed "s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file2.yaml > D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file2.yaml
  : 'RUN: at line 5';   clang-apply-replacements D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  : 'RUN: at line 6';   FileCheck -input-file=D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h
  : 'RUN: at line 9';   ls -1 D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic | FileCheck D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp --check-prefix=YAML
  : 'RUN: at line 12';   grep -Ev "// *[A-Z-]+:" D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h > D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
  : 'RUN: at line 13';   clang-apply-replacements -remove-change-desc-files D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  : 'RUN: at line 14';   ls -1 D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic | FileCheck D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp --check-prefix=NO_YAML
  --
  Exit Code: 1

  Command Output (stdout):
  --
  $ ":" "RUN: at line 1"
  $ "mkdir" "-p" "D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ ":" "RUN: at line 2"
  $ "grep" "-Ev" "// *[A-Z-]+:" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ ":" "RUN: at line 3"
  $ "sed" "s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file1.yaml"
  $ ":" "RUN: at line 4"
  $ "sed" "s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file2.yaml"
  $ ":" "RUN: at line 5"
  $ "clang-apply-replacements" "D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ ":" "RUN: at line 6"
  $ "FileCheck" "-input-file=D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ ":" "RUN: at line 9"
  $ "ls" "-1" "D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ "FileCheck" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp" "--check-prefix=YAML"
  $ ":" "RUN: at line 12"
  $ "grep" "-Ev" "// *[A-Z-]+:" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ ":" "RUN: at line 13"
  $ "clang-apply-replacements" "-remove-change-desc-files" "D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ ":" "RUN: at line 14"
  $ "ls" "-1" "D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ "FileCheck" "D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp" "--check-prefix=NO_YAML"
  # command stderr:
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp(17,17): error G8807F445: NO_YAML-NOT: excluded string found in input [D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\check-clang-extra.vcxproj]
  // NO_YAML-NOT: {{^file.\.yaml$}}
                  ^
  <stdin>:2:1: note: found here
  file1.yaml
  ^~~~~~~~~~

  Input file: <stdin>
  Check file: D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp

  -dump-input=help explains the following input dump.

  Input was:
  <<<<<<
          1: basic.h
          2: file1.yaml
not : 17     !~~~~~~~~~ error : no match expected [D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\check-clang-extra.vcxproj]
  >>>>>>

CUSTOMBUILD : error : command failed with exit status: 1 [D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\check-clang-extra.vcxproj]

  --

  ********************
  ********************
  Failed Tests (1):
    Clang Tools :: clang-apply-replacements/basic.cpp


  Testing Time: 707.23s
    Unsupported      :   11
    Passed           : 1983
    Expectedly Failed:    2
    Failed           :    1
C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(245,5): error MSB8066: Custom build for 'D:\legalize\llvm\llvm-project\build\CMakeFiles\5761a0d8e5ced254ad62b101e70e3dea\check-clang-extra.rule' exited with code 1. [D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\check-clang-extra.vcxproj]

If I revert your change, then the tests pass.... can you take a look? Build the CMake target check-clang-extra

[LegalizeAdulthood]

Well, it must have been unrelated to your change or some weird interaction on my machine because I rebased and ran check-clang-extra again and it passed this time.

[LegalizeAdulthood]

I pushed this change, but for some reason phabricator doesn't show it and close the review... I wonder if it's because I rebased it?

[jspam]

No idea, I'll just close this then :)

[myhsu]

I pushed this change, but for some reason phabricator doesn't show it and close the review... I wonder if it's because I rebased it?

You forgot to add "Differential Revison: https://reviews.llvm.org/DXXXXXX" in the commit message, which is how Phabricator identifies the differential to close. I don't think it has anything to do with rebasing.

[LegalizeAdulthood]

In D128157#3612741, @myhsu wrote:

You forgot to add "Differential Revison: https://reviews.llvm.org/DXXXXXX" in the commit message, which is how Phabricator identifies the differential to close. I don't think it has anything to do with rebasing.

Oh, OK, that makes sense, oops.

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

 
 static Optional<CharSourceRange>
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
                        const SourceManager &SM, const LangOptions &LangOpts) {
   if (Destructor.getLocation().isMacroID())
     return None;
 
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
-  SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
-      Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
+  SourceLocation VirtualBeginSpellingLoc =
+      SM.getSpellingLoc(Destructor.getBeginLoc());
+  SourceLocation VirtualEndLoc = VirtualBeginSpellingLoc.getLocWithOffset(
+      Lexer::MeasureTokenLength(VirtualBeginSpellingLoc, SM, LangOpts));
 
   /// Range ends with \c StartOfNextToken so that any whitespace after \c
   /// virtual is included.
-  SourceLocation StartOfNextToken =
-      Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)->getLocation();
+  Optional<Token> NextToken = Lexer::findNextToken(VirtualEndLoc, SM, LangOpts);
+  if (!NextToken)
+    return None;
+  SourceLocation StartOfNextToken = NextToken->getLocation();
 
   return CharSourceRange::getCharRange(VirtualBeginLoc, StartOfNextToken);
 }
 
 static const AccessSpecDecl *
 getPublicASDecl(const CXXRecordDecl &StructOrClass) {
   for (DeclContext::specific_decl_iterator<AccessSpecDecl>
            AS{StructOrClass.decls_begin()},

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/virtual-class-destructor.cpp

 };
 
 using DerivedFromTemplateNonVirtualBaseStruct2Typedef = DerivedFromTemplateNonVirtualBaseStruct2<PublicNonVirtualBaseStruct>;
 DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
 
 } // namespace Bugzilla_51912
 
 namespace macro_tests {
+#define MY_VIRTUAL virtual
 #define CONCAT(x, y) x##y
 
 // CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
 // CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
 class FooBar1 {
 protected:
   CONCAT(vir, tual) CONCAT(~Foo, Bar1()); // no-fixit
 };

 
 // CHECK-MESSAGES: :[[@LINE+3]]:7: warning: destructor of 'FooBar5' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
 // CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected and non-virtual
 #define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2
 class FooBar5 {
 protected:
   XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
 };
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar6' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar6 {
+protected:
+  MY_VIRTUAL ~FooBar6(); // FIXME: We should have a fixit for this.
+};
+
 #undef XMACRO
 #undef CONCAT
+#undef MY_VIRTUAL
 } // namespace macro_tests
 
 namespace FinalClassCannotBeBaseClass {
 class Base {
 public:
   Base() = default;
   virtual void func() = 0;