Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (1).

[David Malcolm <dmalcolm@redhat.com>]

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 <dmalcolm@redhat.com>
> wrote:
> 
> > > From: benjamin priour <priour.be@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