Page MenuHomePhabricator

Analyzer: Calculate field offset correctly
Needs RevisionPublic

Authored by ismailp on Aug 21 2015, 3:12 PM.



StoreManager::getLValueFieldOrIvar should return loc as
base + field-offset, instead of just base.

Diff Detail

Event Timeline

ismailp updated this revision to Diff 32871.Aug 21 2015, 3:12 PM
ismailp retitled this revision from to Analyzer: Calculate field offset correctly.
ismailp updated this object.
ismailp added reviewers: krememek, zaks.anna.
ismailp added a subscriber: cfe-commits.
dcoughlin requested changes to this revision.Sep 25 2015, 6:35 PM
dcoughlin edited edge metadata.

Thanks for the patch Ismail! Some comments inline.


You can use auto *FD = dyn_cast<const FieldDecl>(D) here to avoid repeating the type.


getFieldOffset() will assert when the field is an Objective-C instance variable because it expects the field's parent to be a record. For example: int *x = &((SomeClass *)0x10)->someIVar;


This test doesn't cover the new logic you added in Store.cpp because it is never the case in this test that the base is a non-zero ConcreteInt Loc. In addition to your test with PFONull, you should add tests with something like struct FieldOffset *PFOConstant = (struct FieldOffset *) 0x22;. I think it is also important to add tests for Objective-C instance variables after addressing the assertion failure in that case.

This revision now requires changes to proceed.Sep 25 2015, 6:35 PM
ismailp updated this revision to Diff 36123.Sep 30 2015, 9:59 AM
ismailp edited edge metadata.
  • Don't try to calculate field offset for Objective-C instance variables
  • Added test for Objective-C instance variables
  • Added a non-null pointer in test
ismailp marked 3 inline comments as done.Sep 30 2015, 10:10 AM
ismailp added inline comments.

I might be missing something, and would be very happy if you could explain why it is necessary to add PFOConstant.

At line 122, for example, where &FO is always non-null. Likewise, I'd expect line 128 to be non-null, because new in this translation unit either throws -- in which case, we shouldn't be executing this line -- or succeeds -- in which case, PFONew is non-null.

dcoughlin added inline comments.Sep 30 2015, 11:53 AM

Sorry I wasn't clear. The branch of the case that you modified here only executes when the base is an int with a known value, such as '0xa' (see the code comment with the FIXME you removed.) This means that your test with PFONew doesn't exercise your new code at all. The PFONull case does exercise this case branch (because NULL is the 0 concreteInt) but it does not exercise your newly added logic for calculating field offsets (because for NULL, Base.isZeroConstant() is true). This means there were no tests exercising that logic (try removing the logic and adding assert(false) -- your old tests would still pass).

Separately, as I mentioned before, I think it is important to leave a FIXME here documenting that the NULL case still doesn't work properly. That is, even with your changes, the analyzer still incorrectly says that &(((struct foo *) NULL)->f == 0. (Fixing this would be quite an undertaking.) It would probably also be good to add comments to the tests saying the behavior they expect for PFONull is incorrect so that if we ever fix this issue we will know it is safe to update the tests:

clang_analyzer_eval((void *)&PFONull->secondField != (void *)&PFONull->thirdField); // expected-warning{{FALSE}}
clang_analyzer_eval((void *)&PFONull->secondField == (void *)0); // expected-warning{{TRUE}}

(If we handled NULL properly, the first would be TRUE and second would be FALSE.)

Note that your updated version doesn't calculate field offsets for ObjC ivars with concreteInt bases, so the analyzer also still won't properly say &((Root *)0x10)->uniqueID != (Root *)0x10). Please add it to the FIXME comment, as well.

This revision now requires changes to proceed.Mar 28 2022, 5:13 PM