Page MenuHomePhabricator

[analyzer] TrackConstraintBRVisitor: Do not track unknown values
ClosedPublic

Authored by Charusso on Aug 14 2019, 5:47 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Aug 14 2019, 5:47 PM

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

NoQ accepted this revision.Aug 15 2019, 2:30 PM

This code is super messy. It'd be great to actually test this change on something to see if it has any unwanted side effects.

Could you see if it also fixes https://bugs.llvm.org/show_bug.cgi?id=42938 and, if so, add it as a test?

This revision is now accepted and ready to land.Aug 15 2019, 2:30 PM
NoQ added a comment.Aug 15 2019, 2:31 PM

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

I'm actually curious about one particular mess that we have here. Namely, there's a visitor that says "assuming..." and there's checker notes when checkers themselves assume something; how can we be sure they don't duplicate each other?

In D66267#1632164, @NoQ wrote:

Shouldn't we just delete this entire visitor altogether and merge it into ConditionBRVisitor (like, eventually, not right now)? It seems to be a relic of the past.

I'm actually curious about one particular mess that we have here. Namely, there's a visitor that says "assuming..." and there's checker notes when checkers themselves assume something; how can we be sure they don't duplicate each other?

BugReporter.cpp does some deduplication of notes originating from TrackConstraintBRVisitor and ConditionBRVisitor, but I'm not sure how many other visitors/checkers do we have doing the same.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1898–1900 ↗(On Diff #215294)

When trackNulOrUndeflValue to trackExpressionValue, I think it was an oversight that the comments in it weren't changed accordingly. This makes a lot more sense now, cheers!

Charusso marked an inline comment as done.EditedAug 15 2019, 4:09 PM

@Szelethus pointed out well. My patch is about nullability, and it is perfect. The bug report you made is about deduplication rather than nullability. The fact is we fail to call the deduplication function: eventsDescribeSameCondition() from removeRedundantMsgs(PathPieces &path) because the path does not contain the path pieces. We call removeRedundantMsgs with passing the PD->getMutablePieces(), but this only contains one piece of the foo function call in the test case you have attached.

I would upstream my hotfix of nullability without any tests as the comment says the intention and also we have plenty of tests of TrackConstraintBRVisitor, none of them changed.

Szelethus added inline comments.Aug 15 2019, 4:43 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1898–1900 ↗(On Diff #215294)

was renamed to* oops.

Charusso updated this revision to Diff 215525.Aug 15 2019, 7:13 PM
Charusso marked an inline comment as done.
  • Rebased.
Szelethus accepted this revision.Aug 19 2019, 1:02 PM

I would upstream my hotfix of nullability without any tests as the comment says the intention and also we have plenty of tests of TrackConstraintBRVisitor, none of them changed.

Sounds fair.

Thanks for the reviews! I hope that mentioned error will be visible by the BugReporter revisions.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 5:09 PM