This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Use a custom program point for the check::NewAllocator callback.
ClosedPublic

Authored by NoQ on Jan 5 2018, 5:49 PM.

Details

Summary

This addresses a TODO from D41406. I re-used PostImplicitCall program point when calling the new callback, but it indeed causes problems: because implicit call has no associated statement, MallocChecker was unable to put visitor diagnostics over these nodes. Create a new class of ProgramPoint, namely PostAllocatorCall, which is a sub-class of StmtPoint, and use that when calling the callback.

This patch might cause slight conflicts with D41150.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Jan 5 2018, 5:49 PM
NoQ added inline comments.Jan 5 2018, 5:50 PM
lib/StaticAnalyzer/Core/ExprEngine.cpp
2933–2934

dcoughlin accepted this revision.Jan 8 2018, 6:22 PM
dcoughlin added inline comments.
lib/StaticAnalyzer/Core/CheckerManager.cpp
490–491

Can we get rid of the TODO now?

This revision is now accepted and ready to land.Jan 8 2018, 6:22 PM
xazax.hun accepted this revision.Jan 9 2018, 5:49 AM

LG!

include/clang/Analysis/ProgramPoint.h
592

Maybe = default is getting more canonical within LLVM? But that would not match the rest of the file, so I am fine with not touching this.

lib/StaticAnalyzer/Core/ExprEngine.cpp
2934

I think this is fine for now, but I wonder if in the future it would make more sense to have a getName or similar method for ProgramPoints.

NoQ updated this revision to Diff 129215.Jan 9 2018, 7:41 PM
NoQ marked an inline comment as done.

Fix the comment.

NoQ added inline comments.Jan 9 2018, 7:43 PM
include/clang/Analysis/ProgramPoint.h
592

Yeah, i guess i'd do a separate cleanup for these things.

a.sidorin accepted this revision.Jan 11 2018, 5:15 AM
a.sidorin added inline comments.
test/Analysis/NewDelete-path-notes.cpp
44

Not even a minor concern for this patch, but I think that placing //RUN and //CHECK after the code being tested could save us from massive changes of line numbers.

NoQ added inline comments.Jan 11 2018, 9:29 AM
test/Analysis/NewDelete-path-notes.cpp
44

Hmm, not sure if i understand, you mean before the code? (it would save us from line number changes in plists, but it'd make the tests harder to read because you'd have to scroll all the way down through the plist to find the actual code).

a.sidorin added inline comments.Jan 11 2018, 9:36 AM
test/Analysis/NewDelete-path-notes.cpp
44

I mean placing RUNs after the program code (but before // CHECK. Anyway, moving RUNs below will cause... line changes so it is not an important issue.

NoQ added inline comments.Jan 11 2018, 10:52 AM
test/Analysis/NewDelete-path-notes.cpp
44

Hmm, yeah, right, maybe. So there would be no line number issues until we stick a test in between two other tests or modify existing tests. But i'm still too used to having RUNs above everything, i guess, to just quickly figure out what does the test do. Also it's not hard to regenerate line numbers (even if it looks scary).

This revision was automatically updated to reflect the committed changes.