From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dmalcolm@redhat.com>
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 E1E5D3858D1E
 for <gcc-patches@gcc.gnu.org>; Mon, 15 Aug 2022 18:16:07 +0000 (GMT)
DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E1E5D3858D1E
Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com
 [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id
 us-mta-675-tSd_g7llONeyaUPdiVXJkA-1; Mon, 15 Aug 2022 14:16:06 -0400
X-MC-Unique: tSd_g7llONeyaUPdiVXJkA-1
Received: by mail-qk1-f199.google.com with SMTP id
 s9-20020a05620a254900b006b54dd4d6deso7505153qko.3
 for <gcc-patches@gcc.gnu.org>; Mon, 15 Aug 2022 11:16:06 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 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;
 bh=dsoUTsI3Xe98Eko5DN8+q9fekQn9TTzsoNAgdW0vsMA=;
 b=ps9GjG5O/zLMIkXqF9vNfXOk1pIsbEnvnRVfHmvNwcTG1Q1xbvWbWm/B2B5PeWWQ36
 uAd//hMOdyaKT2q3CXWPkeOIUZUtTimgYmtLWSNGFK4kJuqIBpmsIcoGj3/BJlPh5zeO
 QxooT0sfmxXjlz+qWYsDG5U5AXtPV9Ns5IGEmKpm3yaCREbaboPDdaBgj9guBnrSVp7j
 MJGMlA0xtig5/frYou91mOpVdf2AxisN/rhYJ/ZZKzRfop/W5hZBIDOk/2+qGVNgxVXv
 5LQculNXGQ4hn0JEVVVCNNkOHO911pizY6Lg0QSUhKXIaPyyjHfo72PgZVkVD1M59LQc
 Eb6w==
X-Gm-Message-State: ACgBeo2KxtEZABu1BGXfirucmCfjE295uD4aHdGd7mpFeaL2qCCupjsX
 xAXp4WfhVyyBRLuBcFiUAMtuDM7Or7kGs+2F7Aw+NTZIJitxaBGVK5S3Mj/ew/SXXFCnul8nKvr
 nlgMPck4eBvadVHSHxA==
X-Received: by 2002:a0c:cc8f:0:b0:478:8ad6:c948 with SMTP id
 f15-20020a0ccc8f000000b004788ad6c948mr14376661qvl.23.1660587365214; 
 Mon, 15 Aug 2022 11:16:05 -0700 (PDT)
X-Google-Smtp-Source: AA6agR4xjTAYQzQQcI9IxhIBzpHhSmBiclLzeybZsAzJotoIAWPa30mcaAUhk4chYEdi2EEUZP9uLA==
X-Received: by 2002:a0c:cc8f:0:b0:478:8ad6:c948 with SMTP id
 f15-20020a0ccc8f000000b004788ad6c948mr14376631qvl.23.1660587364849; 
 Mon, 15 Aug 2022 11:16:04 -0700 (PDT)
Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net.
 [73.69.212.193]) by smtp.gmail.com with ESMTPSA id
 j17-20020ac86651000000b003436103df40sm8428316qtp.8.2022.08.15.11.16.03
 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
 Mon, 15 Aug 2022 11:16:03 -0700 (PDT)
Message-ID: <26fa8158e7fe8cb559250397b6494409c98effb6.camel@redhat.com>
Subject: Re: [PATCH] analyzer: warn on the use of floating points in the
 size argument [PR106181]
From: David Malcolm <dmalcolm@redhat.com>
To: Tim Lange <mail@tim-lange.me>, gcc-patches@gcc.gnu.org
Date: Mon, 15 Aug 2022 14:16:02 -0400
In-Reply-To: <20220815123525.49172-1-mail@tim-lange.me>
References: <20220815123525.49172-1-mail@tim-lange.me>
User-Agent: Evolution 3.44.4 (3.44.4-1.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=-10.7 required=5.0 tests=BAYES_00, BODY_8BITS,
 DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF,
 GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP,
 T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
 server2.sourceware.org
X-BeenThere: gcc-patches@gcc.gnu.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org>
List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>,
 <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe>
List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/>
List-Post: <mailto:gcc-patches@gcc.gnu.org>
List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help>
List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>,
 <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe>
X-List-Received-Date: Mon, 15 Aug 2022 18:16:10 -0000

On Mon, 2022-08-15 at 14:35 +0200, Tim Lange wrote:
> This patch fixes the ICE reported in PR106181 and adds a new warning
> to
> the analyzer complaining about the use of floating point operands.

Thanks for the patch.

Various comments inline...

>=20
> I decided to move the warning for floats inside the size argument out
> of
> the allocation size checker code and toward the allocation such that
> the
> warning only appears once.
> I'm not sure about the wording of the diagnostic, my current wording
> feels
> a bit bulky.=C2=A0

Agreed, and the warning itself is probably setting a new record for
option length: -Wanalyzer-imprecise-floating-point-arithmetic is 45
characters.  I'm not without sin here: I think the current record is -
Wanalyzer-unsafe-call-within-signal-handler, which is 43.

How about:
 -Wanalyzer-imprecise-float-arithmetic
 -Wanalyzer-imprecise-fp-arithmetic
instead?  (ideas welcome)


> Here is how the diagnostics look like:
>=20
> /path/to/main.c: In function =E2=80=98test_1=E2=80=99:
> /path/to/main.c:10:14: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results

https://gcc.gnu.org/codingconventions.html#Spelling
says we should use "floating-point" rather than "floating point".

How about just this:

"warning: use of floating-point arithmetic here might yield unexpected
results"

here...

> [-Wanalyzer-imprecise-floating-point-arithmetic]
> =C2=A0=C2=A0 10 |=C2=A0=C2=A0 int *ptr =3D malloc (sizeof (int) * n); /* =
{ dg-line test_1 }
> */
> =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 =E2=80=98test_1=E2=80=99: event 1
> =C2=A0=C2=A0=C2=A0 |
> =C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 10 |=C2=A0=C2=A0 int *ptr =3D malloc (si=
zeof (int) * n); /* { dg-line
> test_1 } */
> =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=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (1) operand =E2=
=80=98n=E2=80=99 is of type =E2=80=98float=E2=80=99
> =C2=A0=C2=A0=C2=A0 |
> /path/to/main.c:10:14: note: only use operands of a type that
> represents whole numbers inside the size argument

...and make the note say:

"only use operands of an integer type inside the size argument"

which tells the user that it's a size that we're complaining about.


> /path/to/main.c: In function =E2=80=98test_2=E2=80=99:
> /path/to/main.c:20:14: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results [-Wanalyzer-
> imprecise-floating-point-arithmetic]
> =C2=A0=C2=A0 20 |=C2=A0=C2=A0 int *ptr =3D malloc (n * 3.1); /* { dg-line=
 test_2 } */
> =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 =E2=80=98test_2=E2=80=99: event 1
> =C2=A0=C2=A0=C2=A0 |
> =C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 20 |=C2=A0=C2=A0 int *ptr =3D malloc (n =
* 3.1); /* { dg-line test_2 } */
> =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=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (1) operand =E2=
=80=983.1000000000000001e+0=E2=80=99 is of
> type =E2=80=98double=E2=80=99
> =C2=A0=C2=A0=C2=A0 |
> /path/to/main.c:20:14: note: only use operands of a type that
> represents whole numbers inside the size argument
>=20
> Also, another point to discuss is the event note in case the
> expression is
> wrapped in a variable, such as in test_3:
> /path/to/main.c:30:10: warning: use of floating point arithmetic
> inside the size argument might yield unexpected results [-Wanalyzer-
> imprecise-floating-point-arithmetic]
> =C2=A0=C2=A0 30 |=C2=A0=C2=A0 return malloc (size); /* { dg-line test_3 }=
 */
> =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 =E2=80=98test_3=E2=80=99: events 1-2
> =C2=A0=C2=A0=C2=A0 |
> =C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 37 | void test_3 (float f)
> =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=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 (1) entry to =E2=80=98test_3=E2=80=99
> =C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 38 | {
> =C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 39 |=C2=A0=C2=A0 void *ptr =3D alloc_me =
(f); /* { dg-message "calling
> 'alloc_me' from 'test_3'" } */
> =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=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=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (2) calling=
 =E2=80=98alloc_me=E2=80=99 from =E2=80=98test_3=E2=80=99
> =C2=A0=C2=A0=C2=A0 |
> =C2=A0=C2=A0=C2=A0 +--> =E2=80=98alloc_me=E2=80=99: events 3-4
> =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 28 | void *alloc_me (size_t size)
> =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=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=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (3) entry to =
=E2=80=98alloc_me=E2=80=99
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=
=A0 29 | {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=
=A0 30 |=C2=A0=C2=A0 return malloc (size); /* { dg-line test_3 } */
> =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=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=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 (4) operand =E2=80=98f=E2=80=99 is of type =E2=80=98float=E2=80=99
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |
>=20
> I'm not sure if it is easily discoverable that event (4) does refer
> to
> 'size'. I thought about also printing get_representative_tree
> (capacity)
> in the event but that would clutter the event message if the argument
> does hold the full expression. I don't have any strong feelings about
> the
> decision here but if I had to decide I'd leave it as is (especially
> because the warning is probably quite unusual).

Yeah; get_representative_tree tries gets a tree, but trees don't give
us a good way of referring to a local var within a particular stack
frame within a path.  So leaving it as is is OK.

> The index of the argument would also be a possibility, but that would
> get
> tricky for calloc.

[...snip...]

>=20
> diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
> index 61b58c575ff..bef15eae2c3 100644
> --- a/gcc/analyzer/analyzer.opt
> +++ b/gcc/analyzer/analyzer.opt
> @@ -98,6 +98,10 @@ Wanalyzer-free-of-non-heap
> =C2=A0Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning
> =C2=A0Warn about code paths in which a non-heap pointer is freed.
> =C2=A0
> +Wanalyzer-imprecise-floating-point-arithmetic
> +Common Var(warn_analyzer_imprecise_floating_point_arithmetic)
> Init(1) Warning
> +Warn about code paths in which floating point arithmetic is use in

"use" -> "used".

> locations where precise computation is needed.
> +
> =C2=A0Wanalyzer-jump-through-null
> =C2=A0Common Var(warn_analyzer_jump_through_null) Init(1) Warning
> =C2=A0Warn about code paths in which a NULL function pointer is called.
> diff --git a/gcc/analyzer/region-model-impl-calls.cc
> b/gcc/analyzer/region-model-impl-calls.cc
> index 8eebd122d42..c4d7e186963 100644
> --- a/gcc/analyzer/region-model-impl-calls.cc
> +++ b/gcc/analyzer/region-model-impl-calls.cc
> @@ -237,6 +237,7 @@ region_model::impl_call_alloca (const
> call_details &cd)
> =C2=A0=C2=A0 const region *new_reg =3D create_region_for_alloca (size_sva=
l,
> cd.get_ctxt ());
> =C2=A0=C2=A0 const svalue *ptr_sval
> =C2=A0=C2=A0=C2=A0=C2=A0 =3D m_mgr->get_ptr_svalue (cd.get_lhs_type (), n=
ew_reg);
> +=C2=A0 check_region_capacity_for_floats (ptr_sval, cd.get_ctxt ());
> =C2=A0=C2=A0 cd.maybe_set_lhs (ptr_sval);
> =C2=A0}
> =C2=A0
> @@ -393,6 +394,7 @@ region_model::impl_call_calloc (const
> call_details &cd)
> =C2=A0=C2=A0=C2=A0=C2=A0 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const svalue *ptr_sval
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=3D m_mgr->get_ptr_svalue=
 (cd.get_lhs_type (), new_reg);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 check_region_capacity_for_floats (ptr_sva=
l, cd.get_ctxt ());
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cd.maybe_set_lhs (ptr_sval);
> =C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0}
> @@ -497,6 +499,7 @@ region_model::impl_call_malloc (const
> call_details &cd)
> =C2=A0=C2=A0=C2=A0=C2=A0 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const svalue *ptr_sval
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=3D m_mgr->get_ptr_svalue=
 (cd.get_lhs_type (), new_reg);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 check_region_capacity_for_floats (ptr_sva=
l, cd.get_ctxt ());
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cd.maybe_set_lhs (ptr_sval);
> =C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0}

These checks would be simpler if they were consolidated into a single
call to check_region_capacity_for_floats in:
  region_model::set_dynamic_extents
akin to how we have check_dynamic_size_for_taint called there - though
that would make it harder to complain about specific arguments at
function calls.  Given that we're not doing the latter, please
consolidate them.

[...snip...]

> =C2=A0
> +/* Abstract class for diagnostics related touse of floating point

"touse" -> "to use"

> +=C2=A0=C2=A0 arithmetic where precision is needed.=C2=A0 */
> +
> +class imprecise_floating_point_arithmetic
> +: public
> pending_diagnostic_subclass<imprecise_floating_point_arithmetic>
> +{
> +public:
> +=C2=A0 imprecise_floating_point_arithmetic ()
> +=C2=A0 {}

Do we need thic ctor?  There aren't any fields to initialize.

> +
> +=C2=A0 const char *get_kind () const final override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 return "imprecise_floating_point_arithmetic";
> +=C2=A0 }

get_kind should probably only be implemented for concrete subclasses,
such as float_as_size_arg.

> +
> +=C2=A0 int get_controlling_option () const final override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 return OPT_Wanalyzer_imprecise_floating_point_arithme=
tic;
> +=C2=A0 }
> +
> +=C2=A0 bool
> +=C2=A0 operator=3D=3D (const imprecise_floating_point_arithmetic &other
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 ATTRIBUTE_UNUSED) const
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 return true;
> +=C2=A0 }

I don't think it makes sense to have an operator=3D=3D here for an abstract
class with no fields.

> +};
> +
> +/* Concrete diagnostic to complain about uses of floating point
> arithmetic
> +=C2=A0=C2=A0 in the size argument of malloc etc.=C2=A0 */
> +
> +class float_as_size_arg : public imprecise_floating_point_arithmetic
> +{
> +public:
> +=C2=A0 float_as_size_arg (tree arg) : m_arg (arg)
> +=C2=A0 {}
> +
> +=C2=A0 bool operator=3D=3D (const float_as_size_arg &other) const
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 return pending_diagnostic::same_tree_p (m_arg, other.=
m_arg);
> +=C2=A0 }
> +
> +=C2=A0 bool emit (rich_location *rich_loc) final override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 diagnostic_metadata m;
> +=C2=A0=C2=A0=C2=A0 bool warned =3D warning_meta (rich_loc, m, get_contro=
lling_option
> (),
> +=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=C2=A0=C2=A0=C2=A0=C2=A0"use of floating point arithmetic
> inside the"
> +=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=C2=A0=C2=A0=C2=A0=C2=A0" size argument might yield
> unexpected"
> +=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=C2=A0=C2=A0=C2=A0=C2=A0" results");
> +=C2=A0=C2=A0=C2=A0 if (warned)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 inform (rich_loc->get_loc (), "only use o=
perands of a type
> that"
> +=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 " represents whol=
e numbers inside
> the"
> +=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 " size argument")=
;
> +=C2=A0=C2=A0=C2=A0 return warned;
> +=C2=A0 }
> +
> +=C2=A0 label_text describe_final_event (const evdesc::final_event &ev)
> final
> +=C2=A0 override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 if (m_arg)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ev.formatted_print ("operand %qE i=
s of type %qT",
> +=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=C2=A0=C2=A0=C2=A0=C2=A0 m_arg, TREE_TYPE (m_arg));
> +=C2=A0=C2=A0=C2=A0 return ev.formatted_print ("at least one operand of t=
he size
> argument is"
> +=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=C2=A0=C2=A0 " of a floating point type");
> +=C2=A0 }
> +
> +private:
> +=C2=A0 tree m_arg;
> +};
> +
> +/* Visitor to find uses of floating point variables/constants in an
> svalue.=C2=A0 */
> +
> +class contains_floating_point_visitor : public visitor
> +{
> +public:
> +=C2=A0 contains_floating_point_visitor (const svalue *root_sval) :
> m_result (NULL)
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 root_sval->accept (this);
> +=C2=A0 }
> +
> +=C2=A0 const svalue *get_svalue_to_report ()
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 return m_result;
> +=C2=A0 }
> +
> +=C2=A0 void visit_constant_svalue (const constant_svalue *sval) final
> override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 /* At the point the analyzer runs, constant integer o=
perands in
> a floating
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 point expression are already implic=
tly converted to floating
> points.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Thus, we do prefer to report non-co=
nstants such that the
> diagnostic
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 always reports a floating point ope=
rand.=C2=A0 */

If the constant is seen first, then the non-constant won't be favored
(though perhaps binary ops get canonicalized so that constants are on
the RHS?).

Maybe rework this so that the visitor accumulates an auto_vec<const
svalue *> of all floating point svalues it sees, and have
get_svalue_to_report do a qsort of them (if non-empty), and pick the
first, using a custom comparator to order them into preference of
reporting?  Though this is possibly overkill.

> +=C2=A0=C2=A0=C2=A0 tree type =3D sval->get_type ();
> +=C2=A0=C2=A0=C2=A0 if (type && FLOAT_TYPE_P (type) && !m_result)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 m_result =3D sval;
> +=C2=A0 }
> +
> +=C2=A0 void visit_conjured_svalue (const conjured_svalue *sval) final
> override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 tree type =3D sval->get_type ();
> +=C2=A0=C2=A0=C2=A0 if (type && SCALAR_FLOAT_TYPE_P (type))
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 m_result =3D sval;
> +=C2=A0 }
> +
> +=C2=A0 void visit_initial_svalue (const initial_svalue *sval) final
> override
> +=C2=A0 {
> +=C2=A0=C2=A0=C2=A0 tree type =3D sval->get_type ();
> +=C2=A0=C2=A0=C2=A0 if (type && SCALAR_FLOAT_TYPE_P (type))
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 m_result =3D sval;
> +=C2=A0 }
> +
> +private:
> +=C2=A0 /* Non-null if at least one floating point operand was found.=C2=
=A0 */
> +=C2=A0 const svalue *m_result;
> +};
> +
> +/* May complain about uses of floating point operands in the
> capacity of SVAL.
> +
> +=C2=A0=C2=A0 SVAL is expected to be a region_svalue.=C2=A0 */
> +
> +void
> +region_model::check_region_capacity_for_floats (const svalue *sval,
> +=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=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=A0region_model_co=
ntext
> *ctxt)
> +const
> +{
> +=C2=A0 if (!ctxt)
> +=C2=A0=C2=A0=C2=A0 return;
> +
> +=C2=A0 if (const region_svalue *reg_sval =3D dyn_cast <const region_sval=
ue
> *> (sval))
> +=C2=A0=C2=A0=C2=A0 {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const svalue *capacity =3D get_capacity (=
reg_sval->get_pointee
> ());
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 contains_floating_point_visitor v (capaci=
ty);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (const svalue *float_sval =3D v.get_sv=
alue_to_report ())
> +=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 tree diag_arg =3D get_r=
epresentative_tree (float_sval);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ctxt->warn (new float_a=
s_size_arg (diag_arg));
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> +=C2=A0=C2=A0=C2=A0 }
> +}
> +
> =C2=A0/* Set the value of the region given by LHS_REG to the value given
> =C2=A0=C2=A0=C2=A0 by RHS_SVAL.
> =C2=A0=C2=A0=C2=A0 Use CTXT to report any warnings associated with writin=
g to
> LHS_REG.=C2=A0 */

[...snip...]

> @@ -9758,6 +9759,7 @@ Enabling this option effectively enables the
> following warnings:
> =C2=A0-Wanalyzer-fd-use-without-check @gol
> =C2=A0-Wanalyzer-file-leak @gol
> =C2=A0-Wanalyzer-free-of-non-heap @gol
> +-Wno-analyzer-imprecise-floating-point-arithmetic @gol

Lose the "no-" here.


> =C2=A0-Wanalyzer-jump-through-null @gol
> =C2=A0-Wanalyzer-malloc-leak @gol
> =C2=A0-Wanalyzer-mismatching-deallocation @gol
> @@ -9946,6 +9948,18 @@ is called on a non-heap pointer (e.g. an on-
> stack buffer, or a global).
> =C2=A0
> =C2=A0See @uref{https://cwe.mitre.org/data/definitions/590.html, CWE-590:
> Free of Memory not on the Heap}.
> =C2=A0
> +@item -Wno-analyzer-imprecise-floating-point-arithmetic
> +@opindex Wanalyzer-imprecise-floating-point-arithmetic
> +@opindex Wno-analyzer-imprecise-floating-point-arithmetic
> +This warning requires @option{-fanalyzer}, which enables it; use
> +@option{-Wno-analyzer-imprecise-floating-point-arithmetic}
> +to disable it.
> +
> +This diagnostic warns for paths through the code in which floating
> point
> +arithmetic is used in locations where precise computation is
> needed.=C2=A0 This
> +diagnostic only warns on use of floating points operands inside the
> +allocation size at the moment.

"inside the allocation size " -> "inside the calculation of an
allocation size".

[...snip...]

Other than the above, the patch looks good; thanks.

Dave