public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Malcolm <dmalcolm@redhat.com>
To: Eric Feng <ef2648@columbia.edu>
Cc: gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
Date: Thu, 31 Aug 2023 13:01:51 -0400	[thread overview]
Message-ID: <50449b85cd6858842903ad09e102fce2de2785ed.camel@redhat.com> (raw)
In-Reply-To: <CANGHATWvgXEzDgMKKeCbYMRSXB2T2dxVcuTTCjBgRJb-2zA39Q@mail.gmail.com>

On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote:
> On Tue, Aug 29, 2023 at 5:14 PM David Malcolm <dmalcolm@redhat.com>
> wrote:
> > 
> > On Tue, 2023-08-29 at 13:28 -0400, Eric Feng wrote:
> > > Additionally, by using the old model and the pointer per your
> > > suggestion,
> > > we are able to find the representative tree and emit a more
> > > accurate
> > > diagnostic!
> > > 
> > > rc3.c:23:10: warning: expected ‘item’ to have reference count:
> > > ‘1’
> > > but ob_refcnt field is: ‘2’
> > >    23 |   return list;
> > >       |          ^~~~
> > >   ‘create_py_object’: events 1-4
> > >     |
> > >     |    4 |   PyObject* item = PyLong_FromLong(3);
> > >     |      |                    ^~~~~~~~~~~~~~~~~~
> > >     |      |                    |
> > >     |      |                    (1) when ‘PyLong_FromLong’
> > > succeeds
> > >     |    5 |   PyObject* list = PyList_New(1);
> > >     |      |                    ~~~~~~~~~~~~~
> > >     |      |                    |
> > >     |      |                    (2) when ‘PyList_New’ succeeds
> > >     |......
> > >     |   14 |   PyList_Append(list, item);
> > >     |      |   ~~~~~~~~~~~~~~~~~~~~~~~~~
> > >     |      |   |
> > >     |      |   (3) when ‘PyList_Append’ succeeds, moving buffer
> > >     |......
> > >     |   23 |   return list;
> > >     |      |          ~~~~
> > >     |      |          |
> > >     |      |          (4) here
> > >     |
> > 
> > Excellent, that's a big improvement.
> > 
> > > 
> > > If a representative tree is not found, I decided we should just
> > > bail
> > > out
> > > of emitting a diagnostic for now, to avoid confusing the user on
> > > what
> > > the problem is.
> > 
> > Fair enough.
> > 
> > > 
> > > I've attached the patch for this (on top of the previous one)
> > > below.
> > > If
> > > it also looks good, I can merge it with the last patch and push
> > > it in
> > > at
> > > the same time.
> > 
> > I don't mind either way, but please can you update the tests so
> > that we
> > have some automated test coverage that the correct name is being
> > printed in the warning.
> > 
> > Thanks
> > Dave
> > 
> 
> Sorry — forgot to hit 'reply all' in the previous e-mail. Resending
> to
> preserve our chain on the list:
> 
> ---
> 
> Thanks; pushed to trunk with nits fixed:
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=597b9ec69bca8acb7a3d65641c0a730de8b27ed4
> .

Thanks; looks good.

Do you want to add this to the GCC 14 part of the "History" section on
the wiki page:
  https://gcc.gnu.org/wiki/StaticAnalyzer
or should I?

> 
> Incidentally, I updated my formatting settings in VSCode, which I've
> previously mentioned in passing. In case anyone is interested:
> 
> "C_Cpp.clang_format_style": "{ BasedOnStyle: GNU, UseTab: Always,
> TabWidth: 8, IndentWidth: 2, BinPackParameters: false,
> AlignAfterOpenBracket: Align,
> AllowAllParametersOfDeclarationOnNextLine: true }",
> 
> This fixes some issues with the indent width and also ensures
> function
> parameters of appropriate length are aligned properly and on a new
> line each (like the rest of the analyzer code).

Thanks
Dave



  reply	other threads:[~2023-08-31 17:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <c720fc39d86cb3ae8041fe5c86dad34d71466cec.camel@redhat.com>
2023-08-29  4:31 ` Eric Feng
2023-08-29  4:35   ` Eric Feng
2023-08-29 17:28     ` Eric Feng
2023-08-29 21:14       ` David Malcolm
2023-08-30 22:15         ` Eric Feng
2023-08-31 17:01           ` David Malcolm [this message]
2023-08-29 21:08   ` David Malcolm
2023-09-01  2:49   ` Hans-Peter Nilsson
2023-09-01 14:51     ` David Malcolm
2023-09-01 21:07       ` Eric Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50449b85cd6858842903ad09e102fce2de2785ed.camel@redhat.com \
    --to=dmalcolm@redhat.com \
    --cc=ef2648@columbia.edu \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=gcc@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).