Page MenuHomePhabricator

[Analyzer] Correct stack address escape diagnostic
ClosedPublic

Authored by FlameTop on May 3 2016, 7:00 AM.

Details

Summary

Leaking a stack address via a static variable refers to it in the diagnostic as a 'global'. This patch corrects the diagnostic for static variables.

Patch by Phil Camp, SN Systems

Diff Detail

Event Timeline

FlameTop updated this revision to Diff 55983.May 3 2016, 7:00 AM
FlameTop retitled this revision from to [Analyzer] Correct stack address escape diagnostic.
FlameTop updated this object.
FlameTop added reviewers: zaks.anna, dcoughlin.
FlameTop added a subscriber: cfe-commits.
zaks.anna edited edge metadata.May 3 2016, 9:09 AM

Thanks for fixing!

Devin, what do you think about the BugType wording?

llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
230

I don't like the '/' here. The only idea I have is to replace it with "into a variable with static allocation", which is also not ideal because it uses jargon.

dcoughlin added inline comments.May 6 2016, 5:24 PM
llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
230

We have to be careful about changing the 'Name' parameter here. This is supposed to be stable, because we use it for issue hashing. Any change will cause the issue hash to change and stymie use of the issue hash for baselining/issue suppression.

I think we should probably not change it. It isn't user facing, so improving the text won't make a difference in user experience (although to does show up in plists as the "type" of the bug (via the "BugType" field in PathDiagnostic).

232

This string here is actually never used, so you should just remove it and use the constructor of BuiltinBug that doesn't take a description.

242

This is great!

FlameTop updated this revision to Diff 56591.May 9 2016, 9:36 AM
FlameTop edited edge metadata.

Thank you for your review. I have removed the changes to the bug description. Apologies, I did not realize it would alter the hash.

zaks.anna accepted this revision.May 9 2016, 10:19 AM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.May 9 2016, 10:19 AM

Thank you for the acceptance.

May I please ask you to commit the change for me as I do not have commit access?

cheers

Phil Camp