This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr
ClosedPublic

Authored by MTC on Jan 19 2018, 7:01 AM.

Details

Summary

PreStmt and PostStmt callbacks for OffsetOfExpr are necessary to implement Cert ARR39-C: Do not add or subtract a scaled integer to a pointer. And should I define the offsetof macro in clang/test/Analysis/Inputs/system-header-simulator.h? Or, like clang/test/Analysis/malloc-sizeof.c, use #include <stddef.h>?

Diff Detail

Repository
rC Clang

Event Timeline

MTC created this revision.Jan 19 2018, 7:01 AM

Hello Henry,

The patch looks reasonable. I think it can be landed after comments are resolved.

lib/StaticAnalyzer/Core/ExprEngine.cpp
1497

Could you C++11-fy this loop?

test/Analysis/offsetofexpr-callback.c
4

Answering your question, we should put the declaration into system-header-simulator.h.

8

This decl seems unused. Should we remove it?

NoQ added a comment.Jan 19 2018, 11:29 AM

The patch seems correct, but i suspect that your overall approach to the checker you're trying to make is not ideal. offsetof returns a concrete value (because, well, symbolic offsetofs are not yet supported in the analyzer), and even if you see the concrete value, you'd be unable to track it, because when you get another concrete value with the same integer inside, you won't be able to easily figure out if its the same value or a different value. My intuition suggests that this checker shouldn't be path-sensitive; our path-sensitive analysis does very little to help you with this particular checker, and you might end up with a much easier and more reliable checker if you turn it into a simple AST visitor or an AST matcher. Just a heads up.

MTC updated this revision to Diff 130753.Jan 20 2018, 1:20 AM
  • Use C++11 range-based for loop to traverse ExplodedNodeSet.
  • Define the macro offsetof in system-header-simulator.h.
MTC marked 3 inline comments as done.Jan 20 2018, 1:22 AM
MTC added a comment.Jan 20 2018, 1:30 AM
In D42300#982187, @NoQ wrote:

My intuition suggests that this checker shouldn't be path-sensitive; our path-sensitive analysis does very little to help you with this particular checker, and you might end up with a much easier and more reliable checker if you turn it into a simple AST visitor or an AST matcher. Just a heads up.

This is a very useful suggestion, many thanks, Noq! Path-sensitive is really a bit too heavy for this checker.

MTC updated this revision to Diff 133421.Feb 8 2018, 7:29 AM

rebase

MTC added a comment.Feb 8 2018, 7:32 AM

@NoQ Sorry to bother you again. It seems that this patch is useless to analyzer temporarily, if you think so, I will abandon it : ).

NoQ accepted this revision.Feb 8 2018, 12:57 PM

All right, i guess we already do have a pair of callbacks for IntegerLiteral and it doesn't hurt, especially because here they'd eventually be actually useful. I think it should land, just to make it easier to add the actual support later.

This revision is now accepted and ready to land.Feb 8 2018, 12:57 PM
This revision was automatically updated to reflect the committed changes.