This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] ConditionBRVisitor: Fix HTML PathDiagnosticPopUpPieces
ClosedPublic

Authored by Charusso on Aug 2 2019, 9:56 AM.

Details

Summary

A condition could be a multi-line expression where we create the highlight
in separated chunks. PathDiagnosticPopUpPiece is not made for that purpose,
it cannot be added to multiple lines because we have only one ending part
which contains all the notes. So that it cannot have multiple endings and
therefore this patch narrows down the ranges of the highlight to the given
interesting variable of the condition. It prevents HTML-breaking injections.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Aug 2 2019, 9:56 AM

It also reverts https://reviews.llvm.org/rL362632 which is not a fix, but a problem instead.

NoQ added a comment.Aug 2 2019, 6:44 PM

Aha, aha, looks reasonable. If we're describing a variable, we should only highlight that variable.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2420 ↗(On Diff #213063)

Just curious, can BExpr->getLHS() potentially still be a multi-line expression? Or are we making sure it's always a DeclRefExpr/MemberExpr?

In case of MemberExpr i'm pretty sure you can fit a newline before/after . or ->.

2512 ↗(On Diff #213063)

It looks like you forgot to make this change popup-piece-specific. I think we should add our note to the whole condition in case of event pieces.

Charusso updated this revision to Diff 213610.Aug 6 2019, 8:17 AM
Charusso marked 4 inline comments as done.
  • Fix.

Here is an example of the new MemberExpr::getBase() based report:

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2420 ↗(On Diff #213063)

Previously I have focused on DeclRefExpr and MemberExpr value-evaluation, so that there the expression must be one of them. Because we have no getField() method for obtaining the field of the MemberExpr, I have picked getBase().

2512 ↗(On Diff #213063)

Hm, it probably makes sense. At this level 'Cond' and 'ME' are equals as I saw, but may in crazy conditions they are not.

NoQ added inline comments.Aug 6 2019, 4:43 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2420 ↗(On Diff #213063)

It's MemberExpr::getMemberLoc(). It's always a single token. Unlike the base, which may still be arbitrarily complex.

Charusso updated this revision to Diff 213763.Aug 6 2019, 5:35 PM
Charusso marked 2 inline comments as done.
  • Fix.

Thanks for the review! It is much better:

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2420 ↗(On Diff #213063)

I am thinking about the opposite when we have->a->lot->of->stuff. It is working fine, thanks for the ideas!

NoQ accepted this revision.Aug 6 2019, 5:37 PM

Yay!

This revision is now accepted and ready to land.Aug 6 2019, 5:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 7:20 PM