On Fri, 2023-08-25 at 14:48 +0200, Benjamin Priour wrote:
> Hi David,
>
> Thanks for the review.
>
> On Fri, Aug 25, 2023 at 2:12 AM David Malcolm <dmalc...@redhat.com>
> wrote:
>
> > > From: benjamin priour <priour...@gmail.com>
> > >
> > > Hi,
> > >
> > > Below the first batch of a serie of patches to transition
> > > the analyzer testsuite from gcc.dg/analyzer to c-c++-
> > > common/analyzer.
> > > I do not know how long this serie will be, thus the patch was not
> > > numbered.
> > >
> > > For the grand majority of the tests, the transition only required
> > > some
> > > adjustement over the syntax and casts to be C++-friendly, or to
> > > adjust
> > > the warnings regexes to fit the C++ FE.
> > >
> > > The most noteworthy change is in the handling of known_functions,
> > > as described in the below patch.
> >
> > Hi Benjamin.
> >
> > Many thanks for putting this together, it looks like it was a lot
> > of
> > work.
> >
> > > Successfully regstrapped on x86_64-linux-gnu off trunk
> > > 18befd6f050e70f11ecca1dd58624f0ee3c68cc7.
> >
> > Did you compare the before/after results from DejaGnu somehow?
> >
> > Note that I've pushed 9 patches to the analyzer since
> > 18befd6f050e70f11ecca1dd58624f0ee3c68cc7 and some of those touch
> > the
> > files below, so it's worth rebasing and double-checking the
> > results.
> >
> >
[...snip...]
>
> > I confess I'm still a little hazy as to the whole builtin_kf logic,
> > but
> > I trust you that this is needed.
> >
> > Please can you add a paragraph to this comment to explain the
> > motivation here (perhaps giving examples?)
> >
> > > +
> > > +const builtin_known_function *
> > > +region_model::get_builtin_kf (const gcall *call,
> > > + region_model_context *ctxt /* = NULL
> > > */)
> > const
> > > +{
> > > + region_model *mut_this = const_cast <region_model *> (this);
> > > + tree callee_fndecl = mut_this->get_fndecl_for_call (call,
> > > ctxt);
> > > + if (! callee_fndecl)
> > > + return NULL;
> > > +
> > > + call_details cd (call, mut_this, ctxt);
> > > + if (const known_function *kf = get_known_function
> > > (callee_fndecl, cd))
> > > + return kf->dyn_cast_builtin_kf ();
> > > +
> > > + return NULL;
> > > +}
> > > +
> >
> >
> The new comment is as follow:
>
> /* Get any builtin_known_function for CALL and emit any warning to
> CTXT
> if not NULL.
>
> The call must match all assumptions made by the known_function
> (such as
> e.g. "argument 1's type must be a pointer type").
>
> Return NULL if no builtin_known_function is found, or it does
> not match the assumption(s).
>
> Internally calls get_known_function to find a known_function and
> cast it
> to a builtin_known_function.
>
> For instance, calloc is a C builtin, defined in gcc/builtins.def
> by the DEF_LIB_BUILTIN macro. Such builtins are recognized by the
> analyzer by their name, so that even in C++ or if the user
> redeclares
> them but mismatch their signature, they are still recognized as
> builtins.
>
> Cases when a supposed builtin is not flagged as one by the FE:
>
> The C++ FE does not recognize calloc as a builtin if it has not
> been
> included from a standard header, but the C FE does. Hence in C++
> if
> CALL comes from a calloc and stdlib is not included,
> gcc/tree.h:fndecl_built_in_p (CALL) would be false.
>
> In C code, a __SIZE_TYPE__ calloc (__SIZE_TYPE__, __SIZE_TYPE__)
> user
> declaration has obviously a mismatching signature from the
> standard, and
> its function_decl tree won't be unified by
> gcc/c-decl.cc:match_builtin_function_types.
>
> Yet in both cases the analyzer should treat the calls as a builtin
> calloc
> so that extra attributes unspecified by the standard but added by
> GCC
> (e.g. sprintf attributes in gcc/builtins.def), useful for the
> detection
> of
> dangerous behavior, are indeed processed.
>
> Therefore for those cases when a "builtin flag" is not added by
> the FE,
> builtins' kf are derived from builtin_known_function, whose method
> builtin_known_function::builtin_decl returns the builtin's
> function_decl tree as defined in gcc/builtins.def, with all the
> extra
> attributes. */
>
> I hope it clarifies the new kf subclass's purpose.
Thanks!
[...snip...]
> >
>
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
> > b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
> > > index f8dc806d619..e94c0561665 100644
> > > --- a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
> > > @@ -1,53 +1,14 @@
> > > /* See e.g. https://en.cppreference.com/w/c/io/fprintf
> > > and
> > > https://www.man7.org/linux/man-pages/man3/sprintf.3.html */
> > >
> > > +/* { dg-skip-if "C++ fpermissive already throws an error" { c++
> > > } } */
> >
> > Given that this is in the gcc.dg directory, this directive
> > presumably
> > never skips.
> >
> > Is the intent here to document that
> > (a) this set of tests is just for C, and
> > (b) you've checked that this test has been examined, and isn't on
> > the
> > "TODO" list to be migrated?
> >
> > If so, could it just be a regular comment for humans?
> >
> >
> Nods. I will do the same for tests with transparent_union.
> FWIW I'm logging on my side which tests I have already checked for
> C++
> and discarded.
> FYI here is the new comment
>
> /* This test needs not be moved to c-c++-common/analyzer as C++
> fpermissive already throws errors. */
Thanks.
(though FWIW I'd say "emits" rather than "throws" here; "throws" makes
me think of exceptions, when you're talking about diagnostics, right?)
[...snip...]
> >
> > Looks like you split this out into sprintf-2.c, but note that I
> > recently touched sprintf-1.c in
> > 3b691e0190c6e7291f8a52e1e14d8293a28ff4ce and in
> > 5ef89c5c2f52a2c47fd26845d1f73e20b9081fc9. I think those changes
> > affect
> > the rest of the file below this hunk, but does the result still
> > work
> > after rebasing? Should any of those changes have been moved to c-
> > c++-
> > common?
> >
> >
> Your new change with strlen has been added to the C++-friendly bit.
>
>
> > [...snip...]
> >
> > Thanks again for the patch.
> >
> > This will be OK for trunk with the above issues fixed.
> >
> > Dave
> >
> >
> Updated patch will follow when done with regstrapping.
Thanks!
I have some low-urgency patches that touch analyzer testcases, so I'll
wait to push them until after your patch is in trunk. Once your patch
is in trunk I can try to ensure all my new testcases go in the c-c++-
common/analyzer subdirectory, from then on (using the support for this
that your patch adds).
Dave
