Page MenuHomePhabricator

[clang-tidy] new check: bugprone-branch-clone
ClosedPublic

Authored by Szelethus on Nov 20 2018, 8:15 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM?

Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM?

GCC 7 introduced -Wduplicated-branches, so will be good idea to run this check on associated regression(s).

Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM?

GCC 7 introduced -Wduplicated-branches, so will be good idea to run this check on associated regression(s).

Compared to Donát's checker, GCC's warning seems much more underperforming. See https://godbolt.org/z/Iq3FC9.

Implement suggested improvements

donat.nagy marked 6 inline comments as done.Nov 21 2018, 9:41 AM

An example of duplicated code in Clang (it appears in llvm/tools/clang/lib/Frontend/Rewrite/RewriteMacros.cpp starting from line 128):

// If this is a #warning directive or #pragma mark (GNU extensions),
// comment the line out.
if (RawTokens[CurRawTok].is(tok::identifier)) {
  const IdentifierInfo *II = RawTokens[CurRawTok].getIdentifierInfo();
  if (II->getName() == "warning") {
    // Comment out #warning.
    RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//");       // THIS IS...
  } else if (II->getName() == "pragma" &&
             RawTokens[CurRawTok+1].is(tok::identifier) &&
             (RawTokens[CurRawTok+1].getIdentifierInfo()->getName() ==
              "mark")) {
    // Comment out #pragma mark.
    RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//");       // IDENTICAL TO THIS
  }
}
MTC added a subscriber: MTC.Nov 21 2018, 6:41 PM

Thank you for this great patch!

Did you consider using clang/Tooling/ASTDiff/* functionality, as there is the possibility of just comparing the AST 'below' the branches?

Please add tests that contain macros as well, and what do you think should happen with them? I think it would be very valuable to figure out that different macro expansion result in the same code.

clang-tidy/bugprone/BranchCloneCheck.cpp
32

maybe plural for the typename would fit better, as its a vector of multiple elements?

47

You can ellide the single-stmt braces here, maybe its better to move the comment above the for then?

72

This if should always be true, no? The matcher will always bind ifParent

84

This detection can and should land in the matcher, to filter as much as possible already while matching.

89

Please don't use else after return. You can ellide the braces in the if as well.

114

you can ellide the braces.

117

This note is misplaced? The exit is at the break.

121

please dont use const for values, only pointers and references

125

braces, i will stop pointing at it. Please do it on the other parts as well

154

i think you can use early return here, if yes, no else afterwards.

165

typo, branhces

188

i think the name FamilyEnd is not very descriptive, maybe SubsequentBranch or so?

197–198

please use std::distance instead

docs/clang-tidy/checks/list.rst
20

spurious change

test/clang-tidy/bugprone-branch-clone.cpp
5

could you please add tests for ternary operators?

Remove braces, move if parent checking to ASTMatcher, other minor improvements

donat.nagy marked 15 inline comments as done.Nov 27 2018, 9:08 AM

I will add tests for macro handling later.

clang-tidy/bugprone/BranchCloneCheck.cpp
72

I moved this check to the AST matcher.

117

The note was placed correctly, but I replaced it with a break.

docs/clang-tidy/checks/list.rst
20

It was added automatically by the python script, which sorts this list alphabetically. If I remove it now, it will be automatically re-added later.

test/clang-tidy/bugprone-branch-clone.cpp
5

Currently the check does not handle ternary operators, but I added some checks reflecting this.

donat.nagy marked 5 inline comments as done.

Minor simplifications, add tests for macro handling

donat.nagy marked 3 inline comments as done.Nov 28 2018, 8:35 AM

AstDiff:

I looked at the AstDiff library, but I think that it is overkill for my goals. The main feat of the AstDiff library is analyzing and presenting the differences between two AST subtrees, while this checker only needs a simple yes/no answer for "Are these branches identical?".

Macros:

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior. I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.

clang-tidy/bugprone/BranchCloneCheck.cpp
32

This type represents one branch in a switch statement (which consists of multiple statements). I cannot think of a descriptive, short name which also happens to be plural and I do not want to use a monstrosity like StatementsInSwitchBranch.

By the way, can anyone think of shorter, but still clear names for areStatementsIdentical() and areSwitchBranchesIdentical() ?

188

I have renamed FamilyBegin/FamilyEnd to BeginCurrent/EndCurrent. I kept begin and end in the names to emphasize that this is is an iterator range (and altered the comment to include the expression "iterator range"). Is this naming choice clear enough?

test/clang-tidy/bugprone-branch-clone.cpp
201

The tests for macro handling start here.

Correct a typo (ELSE instead of else)

Macros:

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior.

The only option I see to detect macro related errors is to use a Lexer, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.

I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.

How about this?

#define PANDA_CUTE_FACTOR 9001
#define CAT_CUTE_FACTOR 9001

if (whatever())
  return PANDA_CUTE_FACTOR;
else
  return CAT_CUTE_FACTOR;

Your check would warn here, but it is possible, if not likely that the definition of those macros will eventually change. Again, it's fine not to cover this, but this is a false positive in a sense.

I have to agree with the folks before me, this patch is amazing, great job!

test/clang-tidy/bugprone-branch-clone.cpp
201

Maybe add dividers? :) The //===--------------===// thing, you know.

Macros:

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior.

The only option I see to detect macro related errors is to use a Lexer, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.

Agreed with the documentation note.
Starting around with the Lexer here will not help, it is too fragile, as macros can change between different builds and so on.

I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.

How about this?

#define PANDA_CUTE_FACTOR 9001
#define CAT_CUTE_FACTOR 9001

if (whatever())
  return PANDA_CUTE_FACTOR;
else
  return CAT_CUTE_FACTOR;

Your check would warn here, but it is possible, if not likely that the definition of those macros will eventually change. Again, it's fine not to cover this, but this is a false positive in a sense.

Yes, and it is potentially even build-configuration dependent. This is one of the big issues with macros that are impossible(?) to overcome. I would accept this case in the check. (Making it clear in the doc would not hurt).

docs/clang-tidy/checks/list.rst
20

yes. please remove it still, the python script is only run in add_new_check.py, i will commit the fix isolated to master.

test/clang-tidy/bugprone-branch-clone.cpp
5

Thank you. Could you please add a short note to the user-facing documentation that these are not handled.

JonasToth added inline comments.Nov 29 2018, 10:58 AM
clang-tidy/bugprone/BranchCloneCheck.cpp
32

The name is ok, I missunderstood the sense of the type.
Your other names are fine i think.

donat.nagy marked an inline comment as done.Nov 29 2018, 1:50 PM
donat.nagy added inline comments.
test/clang-tidy/bugprone-branch-clone.cpp
5

In fact, I could easily extend the functionality of the checker to cover ternary operators. Would it be an useful addition?

Eugene.Zelenko added inline comments.Nov 29 2018, 1:53 PM
test/clang-tidy/bugprone-branch-clone.cpp
5

Sure, it'll be great extension of functionality.

Handle ternary operators, improve documentation

donat.nagy marked 8 inline comments as done.Nov 30 2018, 2:43 AM

At the end of the documentation I stated that the check examines the code after macro expansion. Is this note clear enough?

test/clang-tidy/bugprone-branch-clone.cpp
5

I added support for ternary operators and documented this fact.

LGTM, could you please run this check over real world code and check that there are no obvious false positives?

docs/clang-tidy/checks/bugprone-branch-clone.rst
11

please do not indent the .. code-block:: c++ but just the code itself, (following too)

Please close PR30233 after committing patch.

donat.nagy updated this revision to Diff 176408.Dec 3 2018, 7:53 AM

Minor correction in documentation

donat.nagy marked 2 inline comments as done.EditedDec 3 2018, 8:31 AM

I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results together). I didn't go through the whole list, but after examining a few dozen random reports it seems that most of these are true positives: relatively short branches (usually one or two lines of code) are repeated for no obvious reason. I would guess that most of these fall into the "correct but too verbose" case, because otherwise the tests would have caught the problem, but I didn't try to understand their context.

I have seen a few false positives related to preprocessor trickery: when an .inc file is included to create a huge switch, sometimes it will become a huge switch with lots of identical branches. There were also some situations where the check reported identical branches which are annotated with different comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this patch for me as I don't have commit rights.

I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results together). I didn't go through the whole list, but after examining a few dozen random reports it seems that most of these are true positives: relatively short branches (usually one or two lines of code) are repeated for no obvious reason. I would guess that most of these fall into the "correct but too verbose" case, because otherwise the tests would have caught the problem, but I didn't try to understand their context.

I have seen a few false positives related to preprocessor trickery: when an .inc file is included to create a huge switch, sometimes it will become a huge switch with lots of identical branches. There were also some situations where the check reported identical branches which are annotated with different comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this patch for me as I don't have commit rights.

Thank you!
Are the cases with .inc files easily silenceable with // NOLINT comments? In general code-generation through macros is not easily handled with clang-tidy, so all checks have the problem (e.g. GoogleTest is extremly chatty with everything), but would be interesting to know :)

All other cases sound like "a human should look and evaluate" and that's ok for clang-tidy as it doesn't claim finding ONLY bugs, but all kind of issues.

Thank you for the patch, if there are no objections by other reviewers I will commit tomorrow( ~24 hours from now)

JonasToth accepted this revision.Dec 3 2018, 8:41 AM
This revision is now accepted and ready to land.Dec 3 2018, 8:41 AM
donat.nagy added a comment.EditedDec 3 2018, 2:34 PM

Macro-generated long switches: I cannot access the check results right now, but if I recall correctly, the check assigned these errors to certain lines in the .inc files. This means that (1) it is not very easy to to suppress them with //NOLINT comments, but (2) it should be relatively easy to filter them by location (ignore all reports coming from .inc files).

A quick, tangentially related idea: Would it be useful to implement multiline nolint sections (e.g. //BEGINNOLINT//ENDNOLINT or something similar)? This would be helpful for using clang-tidy on projects that contain some automatically generated fragments.

A quick, tangentially related idea: Would it be useful to implement multiline nolint sections (e.g. //BEGINNOLINT//ENDNOLINT or something similar)? This would be helpful for using clang-tidy on projects that contain some automatically generated fragments.

Yes, I think so. The filtering mechanism we have right now are not sufficient i think.
Features similar to pylint would be great, were you can disable certain checks for scopes (functions, modules, classes). With this kind of functionality we could bring clang-tidy to system-level libraries too, as the "unsafe" code is usually chatty in many regards, but with fine-grained and powerful filtering it should be easier to manage the output.

TL;DR I am in favor of such approaches, a short proposal (with what you plan) on the mailing-list wouldn't hurt for more opinions :)

This revision was automatically updated to reflect the committed changes.

I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio)

Could you please take a look at it? I could not reproduce locally though.

Szelethus reopened this revision.Apr 30 2019, 1:32 AM

Reverted in rCTE348344.

This revision is now accepted and ready to land.Apr 30 2019, 1:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 1:32 AM
Szelethus commandeered this revision.Apr 30 2019, 1:34 AM
Szelethus added a reviewer: donat.nagy.

I'll take a look at this.

I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio)

Could you please take a look at it? I could not reproduce locally though.

The logs are gone -- do you mind me commiting this again to retrieve the assertation?

I actually managed to reproduce the crash.

extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:200: clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' failed.
Szelethus added a comment.EditedMay 3 2019, 2:49 AM
$ cat orig.cpp
#define a(b) switch (b) {
#define c(b) \
b:
int d;
e() {
a(d) c(2);
c(3)
$ debugBuild/bin/clang-tidy orig.cpp -checks=bugprone-branch-clone --
clang-tidy: ../extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:200: clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' failed.
#0 0x00007f263c30ee79 llvm::sys::PrintStackTrace(llvm::raw_ostream&) debugBuild/../llvm/lib/Support/Unix/Signals.inc:494:11 #1 0x00007f263c30f029 PrintStackTraceSignalHandler(void*) debugBuild/../llvm/lib/Support/Unix/Signals.inc:558:1
#2 0x00007f263c30d936 llvm::sys::RunSignalHandlers() debugBuild/../llvm/lib/Support/Signals.cpp:67:5 
#3 0x00007f263c30f6db SignalHandler(int) debugBuild/../llvm/lib/Support/Unix/Signals.inc:357:1 #4 0x00007f263bd64890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
#5 0x00007f2637aefe97 gsignal /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0 
#6 0x00007f2637af1801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0 
#7 0x00007f2637ae139a __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:89:0 
#8 0x00007f2637ae1412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412) 
#9 0x00007f263a64c623 clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) debugBuild/../extra/clang-tidy/Cl
angTidyDiagnosticConsumer.cpp:201:17 #10 0x00007f263a647baf clang::tidy::ClangTidyCheck::diag(clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) debugBuild/../extra/clang-tidy/ClangTidyCheck.cpp:23
:3 #11 0x00007f263a28671e clang::tidy::bugprone::BranchCloneCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) debugBuild/../extra/clang-tidy/bugprone/BranchCloneCheck.cpp:200:9 
#12 0x00007f263a647be9 clang::tidy::ClangTidyCheck::run(clang::ast_matchers::MatchFinder::MatchResult const&) debugBuild/../extra/clang-tidy/ClangTidyCheck.cpp:31:1 #13 0x00007f263ae60174 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) debugBuild/../clang
/lib/ASTMatchers/ASTMatchFinder.cpp:742:7 
#14 0x00007f263af4e699 clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) debugBuild/../clang/lib/
ASTMatchers/ASTMatchersInternal.cpp:75:5 #15 0x00007f263ae5fe66 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::ast_type_traits::DynTypedNode const&) debugBuild/../clang/lib
/ASTMatchers/ASTMatchFinder.cpp:574:7 
#16 0x00007f263ae601c5 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchDispatch(clang::Stmt const*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder
.cpp:597:5
#17 0x00007f263ae5fa7d void clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::match<clang::Stmt>(clang::Stmt const&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:499:3
#18 0x00007f263ae6dedd clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:868:48
#19 0x00007f263ae904b5 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseCompoundStmt(clang::CompoundStmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2175:1
#20 0x00007f263ae806a2 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::dataTraverseNode(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/tools/clang/include/clang/AST/StmtNodes.inc:73:1
#21 0x00007f263ae7c95e clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:655:7
#22 0x00007f263ae6def9 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:868:3
#23 0x00007f263aeb71ca clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseFunctionHelper(clang::FunctionDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2020:5
#24 0x00007f263ae677be clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseFunctionDecl(clang::FunctionDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2025:1
#25 0x00007f263ae61df3 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDecl(clang::Decl*) debugBuild/tools/clang/include/clang/AST/DeclNodes.inc:389:1
#26 0x00007f263ae61281 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:860:3
#27 0x00007f263ae6dc58 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDeclContextHelper(clang::DeclContext*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:1387:7
#28 0x00007f263ae6a3a8 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:1479:1
#29 0x00007f263ae623f6 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDecl(clang::Decl*) debugBuild/tools/clang/include/clang/AST/DeclNodes.inc:571:1
#30 0x00007f263ae61281 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:860:3
#31 0x00007f263adf7e73 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseAST(clang::ASTContext&) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:183:11
#32 0x00007f263adf7c8d clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:1037:11
#33 0x00007f263ae60d05 clang::ast_matchers::internal::(anonymous namespace)::MatchASTConsumer::HandleTranslationUnit(clang::ASTContext&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:930:3
#34 0x00007f263608220b clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) debugBuild/../clang/lib/Frontend/MultiplexConsumer.cpp:291:23
#35 0x00007f263253655e clang::ParseAST(clang::Sema&, bool, bool) debugBuild/../clang/lib/Parse/ParseAST.cpp:178:12
#36 0x00007f26360432c2 clang::ASTFrontendAction::ExecuteAction() debugBuild/../clang/lib/Frontend/FrontendAction.cpp:1037:1
#37 0x00007f2636042ce0 clang::FrontendAction::Execute() debugBuild/../clang/lib/Frontend/FrontendAction.cpp:938:7
#38 0x00007f2635fbf6ae clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) debugBuild/../clang/lib/Frontend/CompilerInstance.cpp:945:7
#39 0x00007f2638998382 clang::tooling::FrontendActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) debugBuild/../clang/lib/Tooling/Tooling.cpp:369:14
#40 0x00007f263a5f48d1 clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, llvm::StringRef)::ActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) debugBuild/../extra/clang-tidy/ClangTidy.cpp:540:7
#41 0x00007f2638998218 clang::tooling::ToolInvocation::runInvocation(char const*, clang::driver::Compilation*, std::shared_ptr<clang::CompilerInvocation>, std::shared_ptr<clang::PCHContainerOperations>) debugBuild/../clang/lib/Tooling/Tooling.cpp:344:3
#42 0x00007f2638997385 clang::tooling::ToolInvocation::run() debugBuild/../clang/lib/Tooling/Tooling.cpp:329:3
#43 0x00007f2638999577 clang::tooling::ClangTool::run(clang::tooling::ToolAction*) debugBuild/../clang/lib/Tooling/Tooling.cpp:518:11
#44 0x00007f263a5f283a clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, llvm::StringRef) debugBuild/../extra/clang-tidy/ClangTidy.cpp:562:23
#45 0x0000000000243db8 clang::tidy::clangTidyMain(int, char const**) debugBuild/../extra/clang-tidy/tool/ClangTidyMain.cpp:430:7
#46 0x0000000000243342 main debugBuild/../extra/clang-tidy/tool/ClangTidyMain.cpp:491:3
#47 0x00007f2637ad2b97 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0
#48 0x000000000024302a _start (debugBuild/bin/clang-tidy+0x24302a)
Szelethus updated this revision to Diff 198025.May 3 2019, 8:51 AM

FIx the above mentioned crash.

diff --git a/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tidy/bugprone/BranchCloneCheck.cpp
index f371231..a898311 100644
--- a/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -197,8 +197,19 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
         diag(BeginCurrent->front()->getBeginLoc(),
              "switch has %0 consecutive identical branches")
             << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
-        diag(Lexer::getLocForEndOfToken((EndCurrent - 1)->back()->getEndLoc(),
-                                        0, *Result.SourceManager,
+
+        SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc();
+        // If the case statement is generated from a macro, it's SourceLocation
+        // may be invalid, resuling in an assertation failure down the line.
+        // While not optimal, try the begin location in this case, it's still
+        // better then nothing.
+        if (EndLoc.isInvalid())
+          EndLoc = (EndCurrent - 1)->back()->getBeginLoc();
+
+        if (EndLoc.isMacroID())
+          EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc);
+
+        diag(Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager,
                                         getLangOpts()),
              "last of these clones ends here", DiagnosticIDs::Note);
       }
Szelethus requested review of this revision.May 3 2019, 8:57 AM
Szelethus marked an inline comment as done.

I changed the testfiles quite a bit, surprisingly, I'd appreciate another accept on this one please :) Seems to me that the changes describe the actual reports far better...

test/clang-tidy/bugprone-branch-clone.cpp
349

...except the removal of these -- I guess

+        if (EndLoc.isMacroID())
+          EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc);

is responsible for this, but I'm not really sure how else can I avoid the assertation failure.

JonasToth accepted this revision.May 7 2019, 10:01 AM

Did you run the new version over a big project like llvm again?
If yes LGTM from my side, if not please do that before committing to ensure no new/other problems creeped in.

This revision is now accepted and ready to land.May 7 2019, 10:01 AM
This revision was automatically updated to reflect the committed changes.