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.129.124]) by sourceware.org (Postfix) with ESMTPS id 92E153858D32 for ; Fri, 25 Aug 2023 20:10:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 92E153858D32 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=1692994255; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zm89tmV01gWdGEtwRjhqFg9E9dsmgQo/7VkBEZJra6Q=; b=QTnJY5Ce3ksdNdft8IXnC3q3wtvFg0/vyQBD6Cx05e9oRxaMoiSjYsRIsUINek2cvdRxDB GQ1ti5LNtAVv1pT+WqZVNsoM/zI2omXG+kN3YmTUcBQjUTUz4ezBNLPUopC50EdXygaTDd 5wAqDx9xAttLj1fMZ256q30GgKod7TU= Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-541-mJrTZbSkMAyBNKyeTC_SkA-1; Fri, 25 Aug 2023 16:10:53 -0400 X-MC-Unique: mJrTZbSkMAyBNKyeTC_SkA-1 Received: by mail-oo1-f69.google.com with SMTP id 006d021491bc7-570b373618fso1290007eaf.0 for ; Fri, 25 Aug 2023 13:10:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692994253; x=1693599053; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=zm89tmV01gWdGEtwRjhqFg9E9dsmgQo/7VkBEZJra6Q=; b=lIH2tCRPW/TWJCUGlNZl7ZcQFtrYlAEUArbwQPFx7By7jIF2CtpPK37R6Tb+5lH7AF GGtuJTVVE2RaJxpgDZcyRO2Iiz4NZ3YKd0FLJKBugE7pbMdL0wkafa9AqBRB72SkYYYN CotKZ5CRZvK3aLc7hsjFDVIOqeM3MxAHUPCBe22zWK75PXXzYsK1Rz4OxXbsdLyWad23 7uTxbO5Fb3jG+m/NfuI15vdPC7FAKV4vPHhrkUWypIfPulkd5CYkxd6oJ3d+g5Cup7Db Qd0ADHZAY+c6IugcoQYPHSCmRFStKWYSJWlmaMQTOmgu07TcbG6TysbiqJddN8NcQnYs Zx2A== X-Gm-Message-State: AOJu0YwFtnwQDimznPVdoVE9B8Gt4yqSM/bZlwG+6aTaQyzvl6LaExgu oMuUmIYV3wodd6q28GXwsbAIDHWbZvhGqmLkYVTFPWOCOXXz87UTT1/FLOzsZ5NGxDx2U2QYjn1 IgdRn5lOMNhToPdErrQ== X-Received: by 2002:a05:6870:d10f:b0:1be:f46d:a26c with SMTP id e15-20020a056870d10f00b001bef46da26cmr4398464oac.27.1692994253081; Fri, 25 Aug 2023 13:10:53 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFaaejIhmfxYRwra/Wncq7KibOyBCbKKEos+qZfGdYBmqHCIwuFAflhIJq10rqY4LLVU08YvA== X-Received: by 2002:a05:6870:d10f:b0:1be:f46d:a26c with SMTP id e15-20020a056870d10f00b001bef46da26cmr4398441oac.27.1692994252636; Fri, 25 Aug 2023 13:10:52 -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 w6-20020a0c8e46000000b0063d038df3f3sm780134qvb.52.2023.08.25.13.10.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Aug 2023 13:10:52 -0700 (PDT) Message-ID: <86149a0ec44a6d8d12d5dfa35c3561eb3efe14e5.camel@redhat.com> Subject: Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (1). From: David Malcolm To: Benjamin Priour Cc: gcc-patches@gcc.gnu.org Date: Fri, 25 Aug 2023 16:10:51 -0400 In-Reply-To: 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=-9.9 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: On Fri, 2023-08-25 at 14:48 +0200, Benjamin Priour wrote: > Hi David, >=20 > Thanks for the review. >=20 > On Fri, Aug 25, 2023 at 2:12=E2=80=AFAM David Malcolm > wrote: >=20 > > > 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. > >=20 > > Hi Benjamin. > >=20 > > Many thanks for putting this together, it looks like it was a lot > > of > > work. > >=20 > > > Successfully regstrapped on x86_64-linux-gnu off trunk > > > 18befd6f050e70f11ecca1dd58624f0ee3c68cc7. > >=20 > > Did you compare the before/after results from DejaGnu somehow? > >=20 > > 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. > >=20 > >=20 [...snip...] >=20 > > I confess I'm still a little hazy as to the whole builtin_kf logic, > > but > > I trust you that this is needed. > >=20 > > Please can you add a paragraph to this comment to explain the > > motivation here (perhaps giving examples?) > >=20 > > > + > > > +const builtin_known_function * > > > +region_model::get_builtin_kf (const gcall *call, > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 region_model_context *ctxt /* =3D NULL > > > */) > > const > > > +{ > > > +=C2=A0 region_model *mut_this =3D const_cast (this)= ; > > > +=C2=A0 tree callee_fndecl =3D mut_this->get_fndecl_for_call (call, > > > ctxt); > > > +=C2=A0 if (! callee_fndecl) > > > +=C2=A0=C2=A0=C2=A0 return NULL; > > > + > > > +=C2=A0 call_details cd (call, mut_this, ctxt); > > > +=C2=A0 if (const known_function *kf =3D get_known_function > > > (callee_fndecl, cd)) > > > +=C2=A0=C2=A0=C2=A0 return kf->dyn_cast_builtin_kf (); > > > + > > > +=C2=A0 return NULL; > > > +} > > > + > >=20 > >=20 > The new comment is as follow: >=20 > /* Get any builtin_known_function for CALL and emit any warning to > CTXT > =C2=A0=C2=A0 if not NULL. >=20 > =C2=A0=C2=A0 The call must match all assumptions made by the known_functi= on > (such as > =C2=A0=C2=A0 e.g. "argument 1's type must be a pointer type"). >=20 > =C2=A0=C2=A0 Return NULL if no builtin_known_function is found, or it doe= s > =C2=A0=C2=A0 not match the assumption(s). >=20 > =C2=A0=C2=A0 Internally calls get_known_function to find a known_function= and > cast it > =C2=A0=C2=A0 to a builtin_known_function. >=20 > =C2=A0=C2=A0 For instance, calloc is a C builtin, defined in gcc/builtins= .def > =C2=A0=C2=A0 by the DEF_LIB_BUILTIN macro. Such builtins are recognized b= y the > =C2=A0=C2=A0 analyzer by their name, so that even in C++ or if the user > redeclares > =C2=A0=C2=A0 them but mismatch their signature, they are still recognized= as > builtins. >=20 > =C2=A0=C2=A0 Cases when a supposed builtin is not flagged as one by the F= E: >=20 > =C2=A0=C2=A0=C2=A0 The C++ FE does not recognize calloc as a builtin if i= t has not > been > =C2=A0=C2=A0=C2=A0 included from a standard header, but the C FE does. He= nce in C++ > if > =C2=A0=C2=A0=C2=A0 CALL comes from a calloc and stdlib is not included, > =C2=A0=C2=A0=C2=A0 gcc/tree.h:fndecl_built_in_p (CALL) would be false. >=20 > =C2=A0=C2=A0=C2=A0 In C code, a __SIZE_TYPE__ calloc (__SIZE_TYPE__, __SI= ZE_TYPE__) > user > =C2=A0=C2=A0=C2=A0 declaration has obviously a mismatching signature from= the > standard, and > =C2=A0=C2=A0=C2=A0 its function_decl tree won't be unified by > =C2=A0=C2=A0=C2=A0 gcc/c-decl.cc:match_builtin_function_types. >=20 > =C2=A0=C2=A0 Yet in both cases the analyzer should treat the calls as a b= uiltin > calloc > =C2=A0=C2=A0 so that extra attributes unspecified by the standard but add= ed by > GCC > =C2=A0=C2=A0 (e.g. sprintf attributes in gcc/builtins.def), useful for th= e > detection > of > =C2=A0=C2=A0 dangerous behavior, are indeed processed. >=20 > =C2=A0=C2=A0 Therefore for those cases when a "builtin flag" is not added= by > the FE, > =C2=A0=C2=A0 builtins' kf are derived from builtin_known_function, whose = method > =C2=A0=C2=A0 builtin_known_function::builtin_decl returns the builtin's > =C2=A0=C2=A0 function_decl tree as defined in gcc/builtins.def, with all = the > extra > =C2=A0=C2=A0 attributes.=C2=A0 */ >=20 > I hope it clarifies the new kf subclass's purpose. Thanks! [...snip...] > >=20 >=20 > > > 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 @@ > > > =C2=A0/* See e.g. https://en.cppreference.com/w/c/io/fprintf > > > =C2=A0=C2=A0=C2=A0 and > > > https://www.man7.org/linux/man-pages/man3/sprintf.3.html=C2=A0*/ > > >=20 > > > +/* { dg-skip-if "C++ fpermissive already throws an error" { c++ > > > } } */ > >=20 > > Given that this is in the gcc.dg directory, this directive > > presumably > > never skips. > >=20 > > 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? > >=20 > > If so, could it just be a regular comment for humans? > >=20 > >=20 > 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 >=20 > /* This test needs not be moved to c-c++-common/analyzer as C++ > =C2=A0=C2=A0 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...] > >=20 > > Looks like you split this out into sprintf-2.c, but note that I > > recently touched sprintf-1.c in > > 3b691e0190c6e7291f8a52e1e14d8293a28ff4ce and in > > 5ef89c5c2f52a2c47fd26845d1f73e20b9081fc9.=C2=A0 I think those changes > > affect > > the rest of the file below this hunk, but does the result still > > work > > after rebasing?=C2=A0 Should any of those changes have been moved to c- > > c++- > > common? > >=20 > >=20 > Your new change with strlen has been added to the C++-friendly bit. >=20 >=20 > > [...snip...] > >=20 > > Thanks again for the patch. > >=20 > > This will be OK for trunk with the above issues fixed. > >=20 > > Dave > >=20 > >=20 > 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