From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1C9573858C53 for ; Fri, 25 Aug 2023 00:12:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1C9573858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1692922330; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FK5XJ7Aux2vPv5f5ApTQ+vR56h0Id+jWY66bx09QdIc=; b=Hss8R11k7PNbzYPsvJ7TdchhxCxWUWKWMbw98Bp79Tdb+mRB3kfAYnAoMSXuQCVU1m9XgY LqA6FK0mNKZCbNKFS/TmOccFdvoABL742PmxrhbUgCWyLXnSFCjbJQl5VvHdmhu9EkgQm8 slbjjfLG8DNGD9X5YjRbMqkX2RFFxBM= Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-690-XRljA0LWNDmN8JCf2OQ1FQ-1; Thu, 24 Aug 2023 20:12:08 -0400 X-MC-Unique: XRljA0LWNDmN8JCf2OQ1FQ-1 Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-410f0988ea6so3430561cf.3 for ; Thu, 24 Aug 2023 17:12:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692922328; x=1693527128; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=OUu8V4CyYXuNmX5e8VNvuEiWZzLJOq9FfSriMHK3Nrs=; b=X7G5LitbUr8ljkFkELQqcm00/y2/rG99mLVLi3XQLaB6qMSE7cbo1xThTxJLOCTzDM +TGedD7E7aRgMQ6OoVG3s0Hpm/glCP97O+SUUT6+UvtmYK+x1RSGQAplDi1df5BBA8yK pv0+6BFTGTYB4hdxiSLnZQk/vz33Q1qzJ8iCp+FnK4j6Ah+h9XV3p7VkxI3RxbG02zqV +kB1MltjY44vJrJFpoJdewOnkl0gJNyVtuzalNLyZCtouIIuJwshZAP6+TnV5qK8EWX7 LUlrGWGxFljF87Y9Gy9iQtHU8hohIzYoJBcWvw/yE6cK5A5GrpK2l5+bFTQdvKNI9OHt zgng== X-Gm-Message-State: AOJu0YzRWVfiJfpCr9Xq8PXczQC0ilG4ihjm0Ba/s1eTOC1Y23kwZdTX x9NjvFG1ssArQnJJ7UUSaInRfUOULw4FF7j8XXZexxbLKj2m9zuME5lR4x5VIS/lCnzw8qww4mL McIjuxpJOQ9ilmKL4wQ== X-Received: by 2002:a05:622a:1182:b0:410:9855:aae with SMTP id m2-20020a05622a118200b0041098550aaemr15021542qtk.19.1692922328309; Thu, 24 Aug 2023 17:12:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG5L4/VjhMtZH5f3MwS1dF8D/QxhAbw55Rn0b5RIZ+MOqZXjGclQUX7WPJEUTuCbC7/Z8j83g== X-Received: by 2002:a05:622a:1182:b0:410:9855:aae with SMTP id m2-20020a05622a118200b0041098550aaemr15021521qtk.19.1692922327815; Thu, 24 Aug 2023 17:12:07 -0700 (PDT) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id q13-20020ac8410d000000b004116b082feesm180147qtl.75.2023.08.24.17.12.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Aug 2023 17:12:07 -0700 (PDT) Message-ID: Subject: Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (1). From: David Malcolm To: priour.be@gmail.com, gcc-patches@gcc.gnu.org Date: Thu, 24 Aug 2023 20:12:05 -0400 In-Reply-To: <20230824183919.2485913-1-vultkayn@gcc.gnu.org> References: <20230824183919.2485913-1-vultkayn@gcc.gnu.org> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_LOTSOFHASH,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > From: benjamin priour >=20 > Hi, >=20 > 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. >=20 > 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. >=20 > 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. > Is it OK for trunk ? It's *almost* ready; various comments inline below, throughout... >=20 > Thanks, > Benjamin. >=20 > Patch below. > --- >=20 > First batch of moving tests from under gcc.dg/analyzer into > c-c++-common/analyzer. >=20 > C builtins are not recognized as such by C++, therefore > this patch no longer uses tree.h:fndecl_built_in_p to recognize > a builtin function, but rather the function names. >=20 > Thus functions named as C builtins - such as calloc, sprintf ... - > are recognized as such both in C and C++ sources by the analyzer. >=20 > For user-declared functions named after builtins, the latters' function_d= ecl > tree are now preferred over the function_decl the user declared, even > when the FE consider their declaration to mismatch > (Wbuiltin-declaration-mismatch emitted). This mainly comes into account > in the handling of these function attributes : the analyzer uses > the builtin's attributes defined in gcc/builtins.def. >=20 > Signed-off-by: benjamin priour >=20 > gcc/analyzer/ChangeLog: Please add =09PR 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. >=20 > =09* analyzer.h (class known_function): Add virtual casts to > =09builtin_known_function. > =09(class builtin_known_function): New subclass of known_function > =09for builtins. > =09* kf.cc (class kf_alloca): Now derived from > =09builtin_known_function > =09(class kf_calloc): Likewise. > =09(class kf_free): Likewise. > =09(class kf_malloc): Likewise. > =09(class kf_memcpy_memmove): Likewise. > =09(class kf_memset): Likewise. > =09(class kf_realloc): Likewise. > =09(class kf_strchr): Likewise. > =09(class kf_sprintf): Likewise. > =09(class kf_strcpy): Likewise. > =09(class kf_strdup): Likewise. > =09(class kf_strlen): Likewise. > =09(class kf_strndup): Likewise. > =09(register_known_functions): Builtins are now registered as > =09known_functions by name rather than by their BUILTIN_CODE. > =09* known-function-manager.cc (get_normal_builtin): New overload. > =09* known-function-manager.h: New overload declaration. > =09* region-model.cc (region_model::get_builtin_kf): New function. > =09* region-model.h (class region_model): Add declaration of > =09get_builtin_kf. > =09* sm-fd.cc: For called recognized as builtins, use the attributes > =09of that builtin as defined in gcc/builtins.def rather than the user's. > =09* sm-malloc.cc (malloc_state_machine::on_stmt): Likewise. >=20 > gcc/testsuite/ChangeLog: Add =09PR analyzer/96395 here, as well, please. >=20 > =09* gcc.dg/analyzer/aliasing-3.c: Moved to... > =09* c-c++-common/analyzer/aliasing-3.c: ...here. [...snip...] > diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h > index 93a28b4b5cf..63a220c9b6d 100644 > --- a/gcc/analyzer/analyzer.h > +++ b/gcc/analyzer/analyzer.h > @@ -128,6 +128,10 @@ struct interesting_t; > =20 > class feasible_node; > =20 > +class known_function; > + class builtin_known_function; > + class internal_known_function; > + > /* Forward decls of functions. */ > =20 > extern void dump_tree (pretty_printer *pp, tree t); > @@ -279,6 +283,28 @@ public: > { > return; > } > + > + virtual const builtin_known_function * > + dyn_cast_builtin_kf () const { return NULL; } > + virtual builtin_known_function * > + dyn_cast_builtin_kf () { return NULL; } I don't think we ever work with non-const known_function pointers, so we don't need this non-const version of the vfunc. > +}; > + > +/* Subclass of known_function for builtin functions. */ > + > +class builtin_known_function : public known_function > +{ > +public: > + virtual enum built_in_function builtin_code () const =3D 0; > + tree builtin_decl () const { > + gcc_assert (builtin_code () < END_BUILTINS); > + return builtin_info[builtin_code ()].decl; > + } > + > + virtual const builtin_known_function * > + dyn_cast_builtin_kf () const { return this; } This doesn't need a "virtual", but should have a "final override". > + virtual builtin_known_function * > + dyn_cast_builtin_kf () { return this; } Drop the non-const overload, as noted above. > }; > =20 > /* Subclass of known_function for IFN_* functions. */ > diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc > index 59f46bab581..6c58a1b20e5 100644 > --- a/gcc/analyzer/kf.cc > +++ b/gcc/analyzer/kf.cc [...snip...] > @@ -1406,41 +1491,69 @@ register_known_functions (known_function_manager = &kfm) > kfm.add (IFN_UBSAN_BOUNDS, make_unique ()); > } > =20 > - /* Built-ins the analyzer has known_functions for. */ > + /* GCC built-ins that does not correspond to a function > + in the standard library. */ "does not" -> "do not" [...snip...] > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc > index 99817aee3a9..3a8f07525ba 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -1514,6 +1514,34 @@ region_model::get_known_function (enum internal_fn= ifn) const > return known_fn_mgr->get_internal_fn (ifn); > } > =20 > +/* 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. */ 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, > +=09=09=09 region_model_context *ctxt /* =3D NULL */) const > +{ > + region_model *mut_this =3D const_cast (this); > + tree callee_fndecl =3D 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 =3D get_known_function (callee_fndecl, cd= )) > + return kf->dyn_cast_builtin_kf (); > + > + return NULL; > +} > + [...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" > =20 > -#define NULL ((void *)0) > +#ifdef __cplusplus > + #if __cplusplus >=3D 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. [...snip...] > diff --git a/gcc/testsuite/c-c++-common/analyzer/analyzer-decls.h b/gcc/t= estsuite/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").=20 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. [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c b/gcc/testsuite/c-c= ++-common/analyzer/memcpy-2.c > similarity index 79% > rename from gcc/testsuite/gcc.dg/analyzer/memcpy-2.c > rename to gcc/testsuite/c-c++-common/analyzer/memcpy-2.c > index 51e4a694951..25b0a5e4ff4 100644 > --- a/gcc/testsuite/gcc.dg/analyzer/memcpy-2.c > +++ b/gcc/testsuite/c-c++-common/analyzer/memcpy-2.c > @@ -1,8 +1,9 @@ > /* { dg-additional-options "-Wno-stringop-overflow -Wno-analyzer-out-of-= bounds" } */ > =20 > -void > -main (int c, void *v) > +int > +test (int c, void *v) > { > static char a[] =3D ""; > __builtin_memcpy (v, a, -1); > + return 0; > } FWIW, the analyzer special-cases the handling of functions named "main", but I don't think it matters for this testcase, so the renaming is OK. [...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 p= oint" } */ > { > *mf =3D 0; > (*mf)[ny] =3D (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). [...snip...] > diff --git a/gcc/testsuite/c-c++-common/analyzer/test-setjmp.h b/gcc/test= suite/c-c++-common/analyzer/test-setjmp.h > new file mode 100644 > index 00000000000..52c57d02b70 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/analyzer/test-setjmp.h > @@ -0,0 +1,27 @@ > +/* Various integration tests for setjmp-handling expect a precise > + multiline output. > + =20 > + The outputs from -fdiagnostics-path-format=3Dinline-events for such > + setjmp tests are dependent on whether setjmp is a macro or a function > + (and whether that macro is defined in a system header). > + > + setjmp is a function on some systems and a macro on others. > + This header provides a SETJMP macro in a (fake) system header, > + along with precanned decls of setjmp, for consistency of output acros= s > + different systems. */ > + > +#pragma GCC system_header > + > +struct __jmp_buf_tag { > + char buf[1]; > +}; > +typedef struct __jmp_buf_tag jmp_buf[1]; > +typedef struct __jmp_buf_tag sigjmp_buf[1]; > + > +extern int setjmp(jmp_buf env); > +extern int sigsetjmp(sigjmp_buf env, int savesigs); > + > +extern void longjmp(jmp_buf env, int val); > +extern void siglongjmp(sigjmp_buf env, int val); > + > +#define SETJMP(E) setjmp(E) Is this a copy of gcc.dg/analyzer/test-setjmp.h ? If so, can the content of that file be #included from this one, rather than keeping a copy. > diff --git a/gcc/testsuite/c-c++-common/analyzer/test-uaccess.h b/gcc/testsuite/c-c++-common/analyzer/test-uaccess.h > new file mode 100644 > index 00000000000..70c9d6309ef > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/analyzer/test-uaccess.h > @@ -0,0 +1,15 @@ > +/* Shared header for testcases for copy_from_user/copy_to_user. */ > + > +/* Adapted from include/linux/compiler.h */ > + > +#define __user > + > +/* Adapted from include/asm-generic/uaccess.h */ > + > +extern int copy_from_user(void *to, const void __user *from, long n) > + __attribute__((access (write_only, 1, 3), > +=09=09 access (read_only, 2, 3))); > + > +extern long copy_to_user(void __user *to, const void *from, unsigned lon= g n) > + __attribute__((access (write_only, 1, 3), > +=09=09 access (read_only, 2, 3))); Similarly here, can this be a #include of the other file, rather than a copy? [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c b/= gcc/testsuite/c-c++-common/analyzer/write-to-string-literal-1bis.c > similarity index 86% > rename from gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c > rename to gcc/testsuite/c-c++-common/analyzer/write-to-string-literal-1bi= s.c > index 092500e066f..7d06febba77 100644 > --- a/gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c > +++ b/gcc/testsuite/c-c++-common/analyzer/write-to-string-literal-1bis.c > @@ -1,20 +1,21 @@ > #include > =20 > +#ifdef __cplusplus > +#define CONST_CAST(type) const_cast > +#else > +#define CONST_CAST(type) > +#endif > + > /* PR analyzer/95007. */ > =20 > void test_1 (void) > { > - char *s =3D "foo"; > + char *s =3D CONST_CAST(char *)("foo"); > s[0] =3D 'g'; /* { dg-warning "write to string literal" } */ > } > =20 > /* PR c/83347. */ > =20 > -void test_2 (void) > -{ > - memcpy ("abc", "def", 3); /* { dg-warning "write to string literal" } = */ > -} > - > static char * __attribute__((noinline)) > called_by_test_3 (void) > { Why the rename from -1.c to to -1bis.c ? [...snip...] > diff --git a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c b/gcc/testsuite/gc= c.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 */ > =20 > +/* { 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? > + > extern int > sprintf(char* dst, const char* fmt, ...) > __attribute__((__nothrow__)); > =20 > -#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 =3D 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 whe= re non-null expected" } */ > -} > =20 > -int > -test_null_fmt (char *dst) > -{ > - return sprintf (dst, NULL); /* { dg-warning "use of NULL where non-nu= ll expected" } */ > -} > - > -int > -test_uninit_dst (void) > -{ > - char *dst; > - return sprintf (dst, "hello world"); /* { dg-warning "use of uninitial= ized 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) > =20 > 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? [...snip...] Thanks again for the patch. This will be OK for trunk with the above issues fixed. Dave