Re: [PATCH] analyzer: implement symbolic value support for CPython plugin's refcnt checker [PR107646]

[David Malcolm <dmalcolm@redhat.com>]

On Sun, 2023-09-10 at 22:12 -0400, Eric Feng wrote:
> On Thu, Sep 7, 2023 at 1:28 PM David Malcolm <dmalcolm@redhat.com>
> wrote:
> 
> > On Mon, 2023-09-04 at 22:13 -0400, Eric Feng wrote:
> > 

[...snip...]

> > 
> > 
> > I know you're emulating the old behavior I implemented way back in
> > cpychecker, but I don't like that behavior :(
> > 
> > Specifically, although the patch improves the behavior for symbolic
> > values, it regresses the precision of wording for the concrete
> > values
> > case.  If we have e.g. a concrete ob_refcnt of 2, whereas we only
> > have
> > 1 pointer, then it's more readable to say:
> > 
> >   warning: expected ‘obj’ to have reference count: ‘1’ but
> > ob_refcnt
> > field is ‘2’
> > 
> > than:
> > 
> >   warning: expected ‘obj’ to have reference count: N + ‘1’ but
> > ob_refcnt
> > field is N + ‘2’
> > 
> > ...and we shouldn't quote concrete numbers, the message should be:
> > 
> >   warning: expected ‘obj’ to have reference count of 1 but
> > ob_refcnt field
> > is 2
> 
> 
> > or better:
> > 
> >   warning: ‘*obj’ is pointed to by 1 pointer but 'ob_refcnt' field
> > is 2
> > 
> > 
> > Can you move the unwrapping of the svalue from the tests below into
> > the
> > emit vfunc?  That way the m_actual_refcnt doesn't have to be a
> > constant_svalue; you could have logic in the emit vfunc to print
> > readable messages based on what kind of svalue it is.
> > 
> > Rather than 'N', it might be better to say 'initial'; how about:
> > 
> >   warning: ‘*obj’ is pointed to by 0 additional pointers but
> > 'ob_refcnt'
> > field has increased by 1
> >   warning: ‘*obj’ is pointed to by 1 additional pointer but
> > 'ob_refcnt'
> > field has increased by 2
> >   warning: ‘*obj’ is pointed to by 1 additional pointer but
> > 'ob_refcnt'
> > field is unchanged
> >   warning: ‘*obj’ is pointed to by 2 additional pointers but
> > 'ob_refcnt'
> > field has decreased by 1
> >   warning: ‘*obj’ is pointed to by 1 fewer pointers but 'ob_refcnt'
> > field
> > is unchanged
> > 
> > and similar?
> > 
> 
> That makes sense to me as well (indeed I was just emulating the old
> behavior)! Will experiment and keep you posted on a revised patch
> with this
> in mind.  This is somewhat of a minor detail but can we emit ‘*obj’
> as
> bolded text in the diagnostic message? Currently, I can emit this
> (including the asterisk) like so: '*%E'. But unlike using %qE, it
> doesn't
> bold the body of the single quotations. Is this possible?

Yes.

You could use %< and %> to get the colorized (and localized) quotes
(see pretty-print.cc), but better would probably be to pass a tree for
the *obj, rather than obj.  You can make this by building a MEM_REF
tree node wrapping the pointer (you can see an example of this in the
RK_SYMBOLIC case of region_model::get_representative_path_var_1).

Dave