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 76C673858D32 for ; Fri, 10 Mar 2023 18:07:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 76C673858D32 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=1678471662; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=J0l3mZhSyEpxV1tIWQKg8URuaVWpyXsFs/YKk9o23PM=; b=cP3h/7gI/IHATBMPgXBUfqRDii22K14K+ypx4P9BSlVAEYIe4ruOTCW4e37OgZA5Vv5YXn OxsL50iBrFvblR90LN1HwxIpw16U8ehFhdPUoqcXK+hMth714Jx9gGunCmR+A2owTkRVbm Heet65jIfMQy7i8v4I9qbVDKs7C/2yI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-363-c13Cq6x6PqqiToWHz2S_QA-1; Fri, 10 Mar 2023 13:07:41 -0500 X-MC-Unique: c13Cq6x6PqqiToWHz2S_QA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BAE2D18E0A62; Fri, 10 Mar 2023 18:07:40 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7AACDC15BA0; Fri, 10 Mar 2023 18:07:40 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 32AI7b4X896564 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 10 Mar 2023 19:07:38 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 32AI7bQp896563; Fri, 10 Mar 2023 19:07:37 +0100 Date: Fri, 10 Mar 2023 19:07:36 +0100 From: Jakub Jelinek To: Marek Polacek Cc: Richard Biener , GCC Patches Subject: Re: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060] Message-ID: Reply-To: Jakub Jelinek References: <20230308210930.128620-1-polacek@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 Thu, Mar 09, 2023 at 07:44:53PM -0500, Marek Polacek wrote: > On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote: > > On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote: > > > I think this is a reasonable way to address the regression, so OK. > > > > It is true that both C and C++ (including c++14_down and c++17 and later > > where the latter have different ordering rules) evaluate the lhs of > > MODIFY_EXPR after rhs, so conceptually this patch makes sense. > > Thank you both for taking a look. > > > But I wonder why we do in ubsan_maybe_instrument_array_ref: > > if (e != NULL_TREE) > > { > > tree t = copy_node (*expr_p); > > TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1), > > e, op1); > > *expr_p = t; > > } > > rather than modification of the ARRAY_REF's operand in place. If we > > did that, we wouldn't really care about the order, shared tree would > > be instrumented once, with SAVE_EXPR in there making sure we don't > > compute that multiple times. Is that because the 2 copies could > > have side-effects and we do want to evaluate those multiple times? > > I'd assumed that that was the point of the copy_node. But now that > I'm actually experimenting with it, I can't trigger any problems > without the copy_node. So maybe we can use the following patch, which > also adds a new test, bounds-21.c, to check that side-effects are > evaluated correctly. I didn't bother writing a description for this > patch yet because I sort of think we should apply both patches at the > same time. Perhaps it would be safer to apply for GCC 13 just your first patch and maybe the testsuite coverage from this one and defer this change for GCC 14? > Regtested on x86_64-pc-linux-gnu. > > -- >8 -- > PR sanitizer/108060 > PR sanitizer/109050 > > gcc/c-family/ChangeLog: > > * c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node. > > gcc/testsuite/ChangeLog: > > * c-c++-common/ubsan/bounds-17.c: New test. > * c-c++-common/ubsan/bounds-18.c: New test. > * c-c++-common/ubsan/bounds-19.c: New test. > * c-c++-common/ubsan/bounds-20.c: New test. > * c-c++-common/ubsan/bounds-21.c: New test. Jakub