Hi David, Thanks for the review. On Fri, Aug 25, 2023 at 2:12 AM David Malcolm wrote: > > From: benjamin priour > > > > 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 (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.