From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by sourceware.org (Postfix) with ESMTPS id 5A7E53858C00 for ; Thu, 26 Oct 2023 15:51:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5A7E53858C00 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5A7E53858C00 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::52c ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698335510; cv=none; b=D50NOL2Rledbdeu8iDXh402r20ArwbOnXE9eALDskXydrbog5Y+0ygZSR7idaBbgWLcFESf3ZdyOHzFsnH5lmvYuZVr2nyyXl8jrqFE4PojKFRQeWJMtfVvhuP+pxQfrh8i50+RlQBu8yElDeAJrr6Ba6lsw+bCQYJ3awTxMsEU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698335510; c=relaxed/simple; bh=Qr6n6SDj4Wmz7V7fQHCzXuAymDzSPbYVvg6mfxnm7kU=; h=DKIM-Signature:From:Mime-Version:Subject:Date:Message-Id:To; b=ZccASE0Q4DbPrIMKQj3RS6nQ6joinaQU3PYFAJwRwhT/FTGFSDy8tbspvTVMJ8zyKX5IG4+kg837WBLmwW2h74ZZzqt1JYqTUpADmvLV3rixnCeNt5s4dWhFuRDAUgdlSCZetgOsxSWKenOdceK6OVtrgkVqPZzW9LBA+UQnosM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-53dd752685fso1653104a12.3 for ; Thu, 26 Oct 2023 08:51:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698335506; x=1698940306; darn=gcc.gnu.org; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:from:to:cc:subject:date:message-id :reply-to; bh=nessCJyHxZVDVXF3aGmZFyXDQ2jCLS/yCkpdST18hrc=; b=Tq3ApMJB0rlMyXcIvMhpDWTekRQVSxPGkzlXcYiMZTkIg+rWVWYnaXd2QCi5Bxq2EW /X5Hj+J7f3YlGkF8ChKnEC1cHDvnOcxu57QscRw5affmDWk0Hkm9bE3SV0xgCNAkoCaZ nKLY0WDBTRfAOBoNpTtZ+LBenl21lcv8f4q0afXkfijr4OPx0mxaJ3mPcRrj7qkP+omX D5tTiUaz3o6TXDGGxjUgY6kt+eiMnarPYyqGe/7VXYZRpO+quJU1ci/5JkNeabAhECbh TtDEXEQpMJ7hhushDb7TIW4yKqN66q5FWopkS/kNuwU9SPHAGWFnbyVM4xewhYfZAl2f nrnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698335506; x=1698940306; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nessCJyHxZVDVXF3aGmZFyXDQ2jCLS/yCkpdST18hrc=; b=r7K9bK5pMaLEqQuJfSF1YdMdSwDsz+HDsIwFhiNUS2QXK2AxUpt8wCHl3lCdiWl1Ng OfCyA6b3JkHsGXL/ACmWYhsZfEQpbb04X3xdp+84so3XLvWYSE0dWxSIj6tENo7axhjI fpdeDKq0uGMSrjaNOrD8wQDsbzntlqLL/LSKx/aOuqsLgQeJhjJGo1Qp60L9AZXxUQpR rF6GlkYAZhmjhOvJEdmE9kUhe9fCJZYeCrEryl6coIOSkzmXTUXtrfCTtwXnEyOd9YyC R62dPPe4Yi+fRXWkCHCEIlzdg0NICVovgaH2jQAh7r0dqRsBr4zR02X+ao28D/1x8gMh vtyw== X-Gm-Message-State: AOJu0YxHJi9N34UT7RljDMAkGcDMhJ4GSir1LzDmZ8oOiTRIp0vSwRH8 KAkkIhsJvifJM0LCkJIcg2EbRhEsrCw= X-Google-Smtp-Source: AGHT+IGfj5P4TaOmcJyVPY1sr+N9Mui/6ykpHz8kFCpc9WiXQGEdWzWHDCTW0D7CmAGrV4Y0r/NipQ== X-Received: by 2002:a17:907:2ce2:b0:9c3:1d7e:f5b5 with SMTP id hz2-20020a1709072ce200b009c31d7ef5b5mr52086ejc.20.1698335505643; Thu, 26 Oct 2023 08:51:45 -0700 (PDT) Received: from smtpclient.apple (dynamic-077-007-027-039.77.7.pool.telefonica.de. [77.7.27.39]) by smtp.gmail.com with ESMTPSA id gx13-20020a1709068a4d00b009ae6a6451fdsm11800061ejc.35.2023.10.26.08.51.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Oct 2023 08:51:45 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Richard Biener Mime-Version: 1.0 (1.0) Subject: Re: [PATCH] tree-optimization/109334: Improve computation for access attribute Date: Thu, 26 Oct 2023 17:51:34 +0200 Message-Id: <0B64F397-E3BE-42D2-9A36-4E3AE391BCF3@gmail.com> References: <0ebb3b58-e6dc-4147-aa24-d12c1516756f@gotplt.org> Cc: Martin Uecker , gcc-patches@gcc.gnu.org, Jakub Jelinek In-Reply-To: <0ebb3b58-e6dc-4147-aa24-d12c1516756f@gotplt.org> To: Siddhesh Poyarekar X-Mailer: iPhone Mail (20H115) X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: > Am 26.10.2023 um 13:51 schrieb Siddhesh Poyarekar : >=20 > =EF=BB=BFOn 2023-10-26 04:37, Martin Uecker wrote: >> Hi Sid and Jakub, >> here is the patch discussed in PR 109334. >=20 > I can't approve, but here's a review: Ok Thanks for the review, Richard=20 >> Martin >> tree-optimization/109334: Improve computation for access attribute >> The fix for PR104970 restricted size computations to the case >> where the access attribute was specified explicitly (no VLA). >> It also restricted it to void pointers or elements with constant >> sizes. The second restriction is enough to fix the original bug. >> Revert the first change to again allow size computations for VLA >> parameters and for VLA parameters together with an explicit access >> attribute. >> gcc/ChangeLog: >> PR tree-optimization/109334 >> * tree-object-size.cc (parm_object_size): Allow size >> computation for explicit access attributes. >> gcc/testsuite/ChangeLog: >> PR tree-optimization/109334 >> * gcc.dg/builtin-dynamic-object-size-20.c >> (test_parmsz_simple3): Supported again. >> (test_parmsz_external4): New test. >> * gcc.dg/builtin-dynamic-object-size-20.c: New test. >> * gcc.dg/pr104970.c: New test. >> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/t= estsuite/gcc.dg/builtin-dynamic-object-size-0.c >> index 6da04202ffe..07e3da6f254 100644 >> --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c >> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c >> @@ -455,7 +455,6 @@ test_parmsz_simple2 (size_t sz, char obj[]) >> return __builtin_dynamic_object_size (obj, 0); >> } >> -/* Implicitly constructed access attributes not supported yet. */ >> size_t >> __attribute__ ((noinline)) >> test_parmsz_simple3 (size_t sz, char obj[sz]) >> @@ -527,6 +526,13 @@ test_parmsz_internal3 (size_t sz1, size_t sz2, doubl= e obj[sz1][sz2]) >> return __builtin_dynamic_object_size (obj, 0); >> } >=20 > This test case now works. OK. >=20 >> +size_t >> +__attribute__ ((noinline)) >> +test_parmsz_internal4 (size_t sz1, size_t sz2, double obj[sz1 + 1][4]) >> +{ >> + return __builtin_dynamic_object_size (obj, 0); >> +} >> + >=20 > New test case that isn't supported yet. OK. >=20 >> /* Loops. */ >> size_t >> @@ -721,8 +727,8 @@ main (int argc, char **argv) >> if (test_parmsz_simple2 (__builtin_strlen (argv[0]) + 1, argv[0]) >> !=3D __builtin_strlen (argv[0]) + 1) >> FAIL (); >> - /* Only explicitly added access attributes are supported for now. */ >> - if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0]) !=3D= -1) >> + if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0]) >> + !=3D __builtin_strlen (argv[0]) + 1) >> FAIL (); >> int arr[42]; >> if (test_parmsz_scaled (arr, 42) !=3D sizeof (arr)) >> @@ -759,6 +765,8 @@ main (int argc, char **argv) >> FAIL (); >> if (test_parmsz_internal3 (4, 4, obj) !=3D -1) >> FAIL (); >> + if (test_parmsz_internal4 (3, 4, obj) !=3D -1) >> + FAIL (); >> if (test_loop (arr, 42, 0, 32, 1) !=3D 10 * sizeof (int)) >> FAIL (); >> if (test_loop (arr, 42, 32, -1, -1) !=3D 0) >> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c b/gcc/= testsuite/gcc.dg/builtin-dynamic-object-size-20.c >> new file mode 100644 >> index 00000000000..2c8e07dd98d >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c >> @@ -0,0 +1,49 @@ >> +/* PR 109334 >> + * { dg-do run } >> + * { dg-options "-O1" } */ >> + >> + >> +[[gnu::noinline,gnu::noipa]] >> +int f(int n, int buf[n]) >> + [[gnu::access(read_only, 2, 1)]] >> +{ >> + return __builtin_dynamic_object_size(buf, 0); >> +} >> + >> +[[gnu::noinline,gnu::noipa]] >> +int g(int n, int buf[]) >> + [[gnu::access(read_only, 2, 1)]] >> +{ >> + return __builtin_dynamic_object_size(buf, 0); >> +} >> + >> +[[gnu::noinline,gnu::noipa]] >> +int h(int n, int buf[n]) >> +{ >> + return __builtin_dynamic_object_size(buf, 0); >> +} >> + >> +int dummy(int x) { return x + 1; } >> + >> +[[gnu::noinline,gnu::noipa]] >> +int i(int n, int buf[dummy(n)]) >> +{ >> + return __builtin_dynamic_object_size(buf, 0); >> +} >> + >> +int main() >> +{ >> + int n =3D 10; >> + int buf[n]; >> + if (n * sizeof(int) !=3D f(n, buf)) >> + __builtin_abort(); >> + if (n * sizeof(int) !=3D g(n, buf)) >> + __builtin_abort(); >> + if (n * sizeof(int) !=3D h(n, buf)) >> + __builtin_abort(); >> + >> + (void)i(n, buf); >=20 > f(), g(), h() supported, but i() isn't. OK. >=20 >> + >> + return 0; >> +} >> + >> diff --git a/gcc/testsuite/gcc.dg/pr104970.c b/gcc/testsuite/gcc.dg/pr104= 970.c >> new file mode 100644 >> index 00000000000..e24a7f22dfb >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/pr104970.c >> @@ -0,0 +1,13 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O1 -D_FORTIFY_SOURCE=3D2" } */ >=20 > The -D_FORTIFY_SOURCE=3D2 shouldn't be necessary since it doesn't really d= o anything in the context of this test. >=20 >> + >> +__inline void >> +memset2(void *__dest, int __ch, long __len) { >> + long __trans_tmp_1 =3D __builtin_dynamic_object_size(__dest, 0); >> + __builtin___memset_chk(__dest, __ch, __len, __trans_tmp_1); >> +} >> + >> +void >> +mleye(int l, double E[][l]) { memset2(E, 0, sizeof(double)); } >=20 > New regression test for the ICE reported in pr104970. OK. >=20 >> + >> + >> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc >> index a62af050056..28f27adf9ca 100644 >> --- a/gcc/tree-object-size.cc >> +++ b/gcc/tree-object-size.cc >> @@ -1575,8 +1575,8 @@ parm_object_size (struct object_size_info *osi, tre= e var) >> tree typesize =3D TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm))); >> tree sz =3D NULL_TREE; >> - /* If we have an explicit access attribute with a usable size argumen= t... */ >> - if (access && access->sizarg !=3D UINT_MAX && !access->internal_p >> + /* If we have an access attribute with a usable size argument... */ >> + if (access && access->sizarg !=3D UINT_MAX >=20 > Dropped the unnecessary internal_p test. OK. >=20 >> /* ... and either PARM is void * or has a type that is complete an= d has a >> constant size... */ >> && ((typesize && poly_int_tree_p (typesize)) >> @@ -1587,10 +1587,14 @@ parm_object_size (struct object_size_info *osi, t= ree var) >> unsigned argpos =3D 0; >> /* ... then walk through the parameters to pick the size paramet= er and >> - safely scale it by the type size if needed. */ >> + safely scale it by the type size if needed. >> + >> + TODO: we could also compute the size of VLAs where the size is >> + given by a function parameter. */ >=20 > Isn't this testcase h() in builtin-dynamic-object-size-20.c? If you're re= ferring to testcase i(), then maybe "where the size is given by a non-trivia= l function of a function parameter, e.g. > fn (size_t n, char buf[dummy(n)])." >=20 >> for (arg =3D fnargs; arg; arg =3D TREE_CHAIN (arg), ++argpos) >> - if (argpos =3D=3D access->sizarg && INTEGRAL_TYPE_P (TREE_TYPE (arg)= )) >> + if (argpos =3D=3D access->sizarg) >> { >> + gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (arg))); >> sz =3D get_or_create_ssa_default_def (cfun, arg); >> if (sz !=3D NULL_TREE) >> { >=20 > We rely on the frontend to make sure that the arg at sizarg is an integral= type. OK. >=20 > Overall the change looks OK with a few nits I pointed out above. >=20 > Thanks, > Sid