On Wed, 2026-03-11 at 12:15 +0530, Saksham Gupta wrote:
> Hi David,
>
> Thank you for the detailed review! It is great to hear that the core
> logic
> is on the right track.
>
> I have incorporated all of your feedback into a v2 patch (attached).
> Specifically:
>
> 1. Renamed the class to kf_atoi_family.
> 2. Added support for atoll in both namespaces.
> 3. Removed the unnecessary -Wno-analyzer-too-complex pragma from
> the
> test.
> 4. Added test coverage for atoi without an LHS, atoi with an
> unterminated buffer, and valid calls to atol and atoll.
Thanks - the updated patch looks great. I've added it to my queue of
patches for GCC 17.
One minor nit: git is complaining (correctly) about whitespace in a
couple of places in the patch, within class kf_atoi_family:
Applying: analyzer: add known function handling for atoi, atol, and atoll
.git/rebase-apply/patch:27: trailing whitespace.
.git/rebase-apply/patch:28: trailing whitespace.
/* atoi returns an integer, but we don't know what it is statically.
warning: 2 lines add whitespace errors.
Ideally please turn on visual whitespace (or whatever such a mode is
called) in your editor to make it easier to keep things clean in new
code.
>
> Regarding the bootstrap build: I haven't done a full bootstrap yet,
> but I
> will kick one off locally on my machine today. I would also love to
> get
> access to the Compile Farm to test across different architectures
> like
> Linux/x86_64, so I will submit an account request there.
Great.
>
> Understood entirely on the Stage 4 feature freeze! I am perfectly
> happy for
> this to sit in the queue for GCC 17. Right now, my primary focus is
> diving
> deep into the CPython API checking for the GSoC proposal.
If you haven't tried writing a trivial CPython extension module in C by
hand yet, that would be a good exercise [1]. IIRC there's a link to
the tutorial for this on the project wiki page.
One other thing to note is that the CPython API changes somewhat from
release to release; see e.g. bug 112520 where the plugin broke due to
changes in CPython 3.11. It's best to pick one version of CPython 3
and focus on it, at least initially.
Thanks
Dave
[1] FWIW I recommend that people shouldn't write .c code for real
CPython extensions, and should instead use libffi, or write a .pyx file
and use Cython to translate it to .c - Cython tends to avoid the
mistakes that humans make when hand-authoring .c code. But doing it as
a learning exercise ought to be useful.
>
> Regards,
> Saksham Gupta
>
>
> On Tue, 10 Mar 2026 at 06:14, David Malcolm <[email protected]>
> wrote:
>
> > On Mon, 2026-03-09 at 12:09 +0530, Saksham Gupta wrote:
> > > Hi David,
> > >
> > > Following your suggestion to look into kf.cc, I noticed that the
> > > analyzer
> > > didn't have known function handlers for atoi or atol.
> > >
> > > I have implemented a kf_atoi class that verifies the incoming
> > > argument is a
> > > valid, null-terminated string and sets the LHS to a generic
> > > unknown
> > > value.
> > > I also added a regression test (atoi-1.c) which successfully
> > > flags
> > > uninitialized buffers passed to atoi.
> > >
> > > I've attached the patch (with the Signed-off-by tag and
> > > ChangeLog).
> > > I've
> > > run the DejaGnu test suite on this (make check-gcc
> > > RUNTESTFLAGS="analyzer.exp=atoi-1.c") and it passes with zero
> > > regressions.
> >
> > Thanks for the patch! The kf stuff is a major part of the CPython
> > API
> > project, so it's good to see you getting familiar with it.
> >
> > Overall, the patch looks great, but I have a few comments:
> >
> > > diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
> > > index b1ccbd6584a..bc625682717 100644
> > > --- a/gcc/analyzer/kf.cc
> > > +++ b/gcc/analyzer/kf.cc
> > > @@ -833,6 +833,25 @@ private:
> > > tree m_var_decl; // could be NULL
> > > };
> > >
> > > +class kf_atoi: public known_function
> >
> > How about naming this "kf_atoi_family" since it handles various
> > functions, not just "atoi" specifically.
> >
> > > +{
> > > +public:
> > > + bool matches_call_types_p (const call_details &cd) const final
> > override
> > > + {
> > > + return (cd.num_args () == 1 && cd.arg_is_pointer_p (0));
> > > + }
> > > +
> > > + void impl_call_pre (const call_details &cd) const final
> > > override
> > > + {
> > > + /* atoi expects a valid, null-terminated string. */
> > > + cd.check_for_null_terminated_string_arg (0, false, nullptr);
> > > +
> > > + /* atoi returns an integer, but we don't know what it is
> > statically.
> > > + Tell the analyzer to assume it returns a generic, unknown
> > > value.
> > */
> > > + cd.set_any_lhs_with_defaults ();
> > > + }
> > > +};
> >
> > Looks correct.
> >
> > > In theory we could try to model the state of the environment
> > variables
> > > @@ -2289,6 +2308,9 @@ register_known_functions
> > > (known_function_manager
> > &kfm,
> > > /* Known builtins and C standard library functions
> > > the analyzer has known functions for. */
> > > {
> > > + kfm.add ("atoi", std::make_unique<kf_atoi> ());
> > > + kfm.add ("atol", std::make_unique<kf_atoi> ());
> >
> > We could do "atoll" here for good measure as well.
> >
> >
> > > @@ -2388,6 +2410,9 @@ register_known_functions
> > > (known_function_manager
> > &kfm,
> > > from <cstdlib> etc for the C spellings of these headers
> > > (e.g.
> > <stdlib.h>),
> > > so we must match against these too. */
> > > {
> > > + kfm.add_std_ns ("atoi", std::make_unique<kf_atoi> ());
> > > + kfm.add_std_ns ("atol", std::make_unique<kf_atoi> ());
> >
> > Likewise here.
> >
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/atoi-1.c
> > b/gcc/testsuite/gcc.dg/analyzer/atoi-1.c
> > > new file mode 100644
> > > index 00000000000..67d7b103457
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/atoi-1.c
> > > @@ -0,0 +1,11 @@
> > > +/* { dg-additional-options "-Wno-analyzer-too-complex" } */
> >
> > Do we need -Wno-analyzer-too-complex here, or was this due to
> > copy&pasting from another example?
> >
> > > +#include <stdlib.h>
> > > +
> > > +void test_valid_atoi(void) {
> > > + int val = atoi("123"); /* Should be fine. */
> > > +}
> > > +
> > > +void test_uninitialized_atoi(void) {
> > > + char buf[10];
> > > + int val = atoi(buf); /* { dg-warning "use of uninitialized
> > > value" }
> > */
> > > +}
> >
> > This is good, but please add more test coverage:
> >
> > * a call to atoi for which the return value isn't taken
> > (set_any_lhs_with_defaults handles this, but in the past I've had
> > crashes in kf handlers due to assuming that there is an LHS to
> > write
> > the result to).
> >
> > * a call to atoi with an unterminated buffer
> >
> > * calls to atol and atoll to check that they're wired up.
> >
> > You should probably get familiar with how to do a bootstrap build
> > of
> > GCC. Have you tried this yet? Let us know if you need a more
> > powerful
> > machine; see e.g. https://portal.cfarm.net/ or want some pointers
> > on
> > how to do this.
> >
> > One other thing to mention - right now we're in feature freeze for
> > GCC
> > 16, trying to focus on stabilizing for the release - we call this
> > "stage 4" of the release cycle. Hence I plan to wait before
> > actually
> > pushing the patch. Hopefully we'll transition to stage 1 of the
> > GCC 17
> > release cycle sometime next month (when we're open to new
> > features).
> >
> > Hope this all makes sense; thanks again for the patch
> > Dave
> >
> >
