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.
>
>
Thanks for the info, I've rebased off it and I'm regstrapping.
For tests I didn't touch the warnings, I have checked they still pass in C
and C++.
Except for pr96841.c, C tests most notable update was to add a { target c }.
Every previous PASS and XFAIL remained as such in gcc.dg output.
For C++ I went with a no failure policy. Some tests weren't making sense
in C++,
such as transparent_unions. I've just skipped those.
For some tests C++ fpermissive would throw out an error. These tests I've
tried
to smuggle them out with const_cast and the likes, but never disabled
fpermissive.
> Please add
> PR analyzer/96395
> to the ChangeLog entries, and [PR96395] to the end of the Subject of
> the commit, so that these get tracked within that bug as they get
> pushed.
>
>
Nods.
[...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.
[...snip...]
>
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/aliasing-3.c
> b/gcc/testsuite/c-c++-common/analyzer/aliasing-3.c
> > similarity index 85%
> > rename from gcc/testsuite/gcc.dg/analyzer/aliasing-3.c
> > rename to gcc/testsuite/c-c++-common/analyzer/aliasing-3.c
> > index 003077ad5c1..f78fea64fbe 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/aliasing-3.c
> > +++ b/gcc/testsuite/c-c++-common/analyzer/aliasing-3.c
> > @@ -1,6 +1,14 @@
> > #include "analyzer-decls.h"
> >
> > -#define NULL ((void *)0)
> > +#ifdef __cplusplus
> > + #if __cplusplus >= 201103L
> > + #define NULL nullptr
> > + #else
> > + #define NULL 0
> > + #endif
> > +#else
> > + #define NULL ((void *)0)
> > +#endif
>
> The above fragment of code gets used a lot; can it be moved into
> analyzer-decls.h, perhaps wrapped in a "#ifndef NULL"?
>
> That could be done as a followup patch if it's easier.
>
>
Done.
[...snip...]
>
> > diff --git a/gcc/testsuite/c-c++-common/analyzer/analyzer-decls.h
> b/gcc/testsuite/c-c++-common/analyzer/analyzer-decls.h
> > new file mode 100644
> > index 00000000000..d9a32ed9370
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/analyzer/analyzer-decls.h
> > @@ -0,0 +1,56 @@
> > +#ifndef ANALYZER_DECLS_H
> > +#define ANALYZER_DECLS_H
> > +
> [...snip...]
> > +
> > +/* Obtain an "unknown" void *. */
> > +extern void *__analyzer_get_unknown_ptr (void);
> > +
> > +#endif /* #ifndef ANALYZER_DECLS_H. */
>
> We don't want to have a copy of the file here (the "DRY principle").
> For example, I added a __analyzer_get_strlen in
> 325f9e88802daaca0a4793ca079bb504f7d76c54 which doesn't seem to be in
> this copy.
>
> Can we simply have something like
>
> #include "../../gcc.dg/analyzer/analyzer-decls.h"
>
> here so that we're getting the "real" analyzer-decls.h by reference.
>
> The relative path in the above #include might need tweaking as I'm not
> sure exactly what "-I" directives are passed in when running the c-c++-
> common tests.
>
>
Thanks for noticing that, I had basically copied them to a test subfolder
and was in auto mode when writing the Changelog, I didn't even notice the
copy.
I will double check all the duplicates.
[...snip...]
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96841.c
> b/gcc/testsuite/c-c++-common/analyzer/pr96841.c
> > similarity index 77%
> > rename from gcc/testsuite/gcc.dg/analyzer/pr96841.c
> > rename to gcc/testsuite/c-c++-common/analyzer/pr96841.c
> > index 14f3f7a86a3..dd61176b73b 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/pr96841.c
> > +++ b/gcc/testsuite/c-c++-common/analyzer/pr96841.c
> > @@ -12,7 +12,7 @@ th (int *);
> > void
> > bv (__SIZE_TYPE__ ny, int ***mf)
> > {
> > - while (l8 ())
> > + while (l8 ()) /* { dg-warning "terminating analysis for this program
> point" } */
> > {
> > *mf = 0;
> > (*mf)[ny] = (int *) malloc (sizeof (int));
>
> Does this warning happen for both C and C++?
>
> It's probably better to simply add -Wno-analyzer-too-complex to this
> case, as we don't want to be fussy about precisely which line the
> message is emitted at (or, indeed, that the warning is emitted at all).
>
>
OK. The warning does appear both for C and C++ now, and I wanted to
emphasis this, yet
it is not relevant for the long-term.
Note this new warning is the only divergence in C Dejagnu tests between
trunk and my patch,
and was added after discussion
https://gcc.gnu.org/pipermail/gcc/2023-August/242317.html.
[...snip...]
> Why the rename from -1.c to to -1bis.c ?
>
>
I had to split it up at some point between C-only (-1bis.c) and
C++-friendly (-1.c still there).
The Changelog didn't reflect that though.
Actually double-checked and merged them back, as a const_cast made -1bis
work in C++.
> [...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. */
> +
> > extern int
> > sprintf(char* dst, const char* fmt, ...)
> > __attribute__((__nothrow__));
> >
> > -#define NULL ((void *)0)
> > -
> > -int
> > -test_passthrough (char* dst, const char* fmt)
> > -{
> > - /* This assumes that fmt doesn't have any arguments. */
> > - return sprintf (dst, fmt);
> > -}
> > -
> > -void
> > -test_known (void)
> > -{
> > - char buf[10];
> > - int res = sprintf (buf, "foo");
> > - /* TODO: ideally we would know the value of "res" is 3,
> > - and known the content and strlen of "buf" after the call */
> > -}
> > -
> > -int
> > -test_null_dst (void)
> > -{
> > - return sprintf (NULL, "hello world"); /* { dg-warning "use of NULL
> where non-null expected" } */
> > -}
> >
> > -int
> > -test_null_fmt (char *dst)
> > -{
> > - return sprintf (dst, NULL); /* { dg-warning "use of NULL where
> non-null expected" } */
> > -}
> > -
> > -int
> > -test_uninit_dst (void)
> > -{
> > - char *dst;
> > - return sprintf (dst, "hello world"); /* { dg-warning "use of
> uninitialized value 'dst'" } */
> > -}
> > -
> > -int
> > -test_uninit_fmt_ptr (char *dst)
> > -{
> > - const char *fmt;
> > - return sprintf (dst, fmt); /* { dg-warning "use of uninitialized
> value 'fmt'" } */
> > -}
> > +#define NULL ((void *)0)
> >
> > int
> > test_uninit_fmt_buf (char *dst)
>
> 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,
Benjamin.
