This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] pr34404: Fix a crash on pointers to members in nested anonymous structures.
ClosedPublic

Authored by NoQ on Nov 8 2017, 6:53 AM.

Details

Summary

pr34404 points out that given

class C {
  struct {
    int x;
  };
};

, taking pointer-to-member &C::x would crash the analyzer. We were not ready to stumble upon an IndirectFieldDecl, which is used for representing a field within an anonymous structure (or union) nested into a class.

Even if it wasn't anonymous, our new pointer-to-member support is not yet enabled for this case, so the value of the expression would be a conjured symbol of type void *. Being quite vague, it fits equally poorly to the anonymous case (in general this behavior makes very little sense, since pointer-to-member must be a NonLoc), so for the purposes of fixing the crash i guess i'd just keep it.

Actually constructing a nonloc::PointerToMember in the regular field case should be trivial. However, in the anonymous field case it requires more work, because IndirectFieldDecl is not a DeclaratorDecl. And simply calling getAnonField() over IndirectFieldDecl to obtain FieldDecl is incorrect, because it'd discard the offset to the anonymous structure within the class, leading to incorrect results on the attached tests for m and n.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Nov 8 2017, 6:53 AM
george.karpenkov accepted this revision.Nov 8 2017, 3:38 PM
This revision is now accepted and ready to land.Nov 8 2017, 3:38 PM
kromanenkov edited edge metadata.EditedNov 9 2017, 9:02 AM

@NoQ Do we need to change a DeclaratorDecl field in PointerToMember SVal to something more common, like ValueDecl, to support IndirectFieldDecl as well?
Of course in another patch, this one LGTM.

kromanenkov accepted this revision.Nov 9 2017, 12:31 PM
dcoughlin accepted this revision.Nov 10 2017, 11:00 AM

LGTM.

I suppose we could move the logic that constructs pointers to members for fields here (right now that logic is in ExprEngine::VisitUnaryOperator()'s handling of '&'). However, since the AST's DeclRefExpr for the field is marked as an lvalue this would be slightly odd -- we would have the value of an lvalue expression be a non-Loc.

Is anything holding this patch?

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Nov 27 2017, 9:33 AM

Sorry, i wanted to quickly look at why this thing is lvalue, but this didn't seem to be happening, so i guess i'd just commit for now.