From patchwork Tue Mar 19 13:09:12 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 1913565 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=NPOV94TO; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4TzXBP0C8Fz1yWn for ; Wed, 20 Mar 2024 00:09:49 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D644B3858282 for ; Tue, 19 Mar 2024 13:09:46 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 470643858402 for ; Tue, 19 Mar 2024 13:09:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 470643858402 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 470643858402 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710853765; cv=none; b=Gr9lwNPaXRvmHfc6k9d7OqYrag2thgJwuBLHaDPDhGCZQHQyAJDV7DF2+cNTdgXzZPM58J45SajmtmipVVGWVK6KTXBgaYaxDy4FVdBvvPHjo+9UwVqIG9XYCCbiFwDyz44hBye0xDlTw07gme9QeN8mq9Xcn1r7Z/J32PdjFRU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710853765; c=relaxed/simple; bh=ITej2fPCIV/K90w8eYiLTvmwjSaqImpLbwDvgQYSWAY=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=bjQMD/ZgInoFF8jw/k5NOPNaEeRZxsBEdi3KPN+v/+YQsxrDyzrD4xPyPKxHZNAARsIW+WwbWmo95lUCBjBF8e+U/6wpowvJ0AmJHgNGveSbFKQ3Ial4YfA1SZCx0rm3vUS5LcZIOQv96ZRAQDI8nyYKH4uH8TgnwBc3LU81jvA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1710853759; 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; bh=Ep56ShfZcibxC7G9W0gBlTYediS12lcJ83ELEo9LsgU=; b=NPOV94TOZxeoeA9tQ5NjH6wRymeau6EKR8oygIbC/Sqnnl9ongFK+Rcm5dqKB1Zt+09JUH em5uK5jnSs7k8ggJKwdpPKJjm4ontVhRqN4kJqRZFTCPV7gRheqrY6QZKao0XDDigbQZly /T8hZlUbAJ52Yjy2NQjUoWijMVysMDI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-52-A_x9gGx_MKq_tGTX2kQiew-1; Tue, 19 Mar 2024 09:09:18 -0400 X-MC-Unique: A_x9gGx_MKq_tGTX2kQiew-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 125E1101A586 for ; Tue, 19 Mar 2024 13:09:18 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.22.32.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id D895B10F53; Tue, 19 Mar 2024 13:09:17 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [pushed] analyzer: fixes to __atomic_{exchange, load, store} [PR114286] Date: Tue, 19 Mar 2024 09:09:12 -0400 Message-Id: <20240319130912.693919-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, 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, 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org In r14-1497-gef768035ae8090 I added some support to the analyzer for __atomic_ builtins (enough to fix false positives I was seeing in my integration tests). Unfortunately I messed up the implementation of __atomic_{exchange,load,store}, leading to ICEs seen in PR analyzer/114286. Fixed thusly, fixing the ICEs. Given that we're in stage 4, the patch doesn't add support for any of the various __atomic_compare_exchange builtins, so that these continue to fall back to the analyzer's "anything could happen" handling of unknown functions. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Successful run of analyzer integration tests on x86_64-pc-linux-gnu. Pushed to trunk as r14-9544-gc7a774edbf802d. gcc/analyzer/ChangeLog: PR analyzer/114286 * kf.cc (class kf_atomic_exchange): Reimplement based on signature seen in gimple, rather than user-facing signature. (class kf_atomic_load): Likewise. (class kf_atomic_store): New. (register_atomic_builtins): Register kf_atomic_store. gcc/testsuite/ChangeLog: PR analyzer/114286 * c-c++-common/analyzer/atomic-builtins-pr114286.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/kf.cc | 135 +++++++++++++----- .../analyzer/atomic-builtins-pr114286.c | 48 +++++++ 2 files changed, 150 insertions(+), 33 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/analyzer/atomic-builtins-pr114286.c diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc index ed48ffbcba2..d197ccbd0f0 100644 --- a/gcc/analyzer/kf.cc +++ b/gcc/analyzer/kf.cc @@ -116,39 +116,54 @@ kf_alloca::impl_call_pre (const call_details &cd) const cd.maybe_set_lhs (ptr_sval); } -/* Handler for: - void __atomic_exchange (type *ptr, type *val, type *ret, int memorder). */ +/* Handler for __atomic_exchange. + Although the user-facing documentation specifies it as having this + signature: + void __atomic_exchange (type *ptr, type *val, type *ret, int memorder) + + by the time the C/C++ frontends have acted on it, any calls that + can't be mapped to a _N variation end up with this signature: + + void + __atomic_exchange (size_t sz, void *ptr, void *val, void *ret, + int memorder) + + as seen in the gimple seen by the analyzer, and as specified + in sync-builtins.def. */ class kf_atomic_exchange : public internal_known_function { public: /* This is effectively: - *RET = *PTR; - *PTR = *VAL; + tmpA = *PTR; + tmpB = *VAL; + *PTR = tmpB; + *RET = tmpA; */ void impl_call_pre (const call_details &cd) const final override { - const svalue *ptr_ptr_sval = cd.get_arg_svalue (0); - tree ptr_ptr_tree = cd.get_arg_tree (0); - const svalue *val_ptr_sval = cd.get_arg_svalue (1); - tree val_ptr_tree = cd.get_arg_tree (1); - const svalue *ret_ptr_sval = cd.get_arg_svalue (2); - tree ret_ptr_tree = cd.get_arg_tree (2); + const svalue *num_bytes_sval = cd.get_arg_svalue (0); + const svalue *ptr_sval = cd.get_arg_svalue (1); + tree ptr_tree = cd.get_arg_tree (1); + const svalue *val_sval = cd.get_arg_svalue (2); + tree val_tree = cd.get_arg_tree (2); + const svalue *ret_sval = cd.get_arg_svalue (3); + tree ret_tree = cd.get_arg_tree (3); /* Ignore the memorder param. */ region_model *model = cd.get_model (); region_model_context *ctxt = cd.get_ctxt (); - const region *val_region - = model->deref_rvalue (val_ptr_sval, val_ptr_tree, ctxt); - const svalue *star_val_sval = model->get_store_value (val_region, ctxt); - const region *ptr_region - = model->deref_rvalue (ptr_ptr_sval, ptr_ptr_tree, ctxt); - const svalue *star_ptr_sval = model->get_store_value (ptr_region, ctxt); - const region *ret_region - = model->deref_rvalue (ret_ptr_sval, ret_ptr_tree, ctxt); - model->set_value (ptr_region, star_val_sval, ctxt); - model->set_value (ret_region, star_ptr_sval, ctxt); + const region *ptr_reg = model->deref_rvalue (ptr_sval, ptr_tree, ctxt); + const region *val_reg = model->deref_rvalue (val_sval, val_tree, ctxt); + const region *ret_reg = model->deref_rvalue (ret_sval, ret_tree, ctxt); + + const svalue *tmp_a_sval + = model->read_bytes (ptr_reg, ptr_tree, num_bytes_sval, ctxt); + const svalue *tmp_b_sval + = model->read_bytes (val_reg, val_tree, num_bytes_sval, ctxt); + model->write_bytes (ptr_reg, num_bytes_sval, tmp_b_sval, ctxt); + model->write_bytes (ret_reg, num_bytes_sval, tmp_a_sval, ctxt); } }; @@ -265,32 +280,85 @@ private: enum tree_code m_op; }; -/* Handler for: - void __atomic_load (type *ptr, type *ret, int memorder). */ +/* Handler for __atomic_load. + Although the user-facing documentation specifies it as having this + signature: + + void __atomic_load (type *ptr, type *ret, int memorder) + + by the time the C/C++ frontends have acted on it, any calls that + can't be mapped to a _N variation end up with this signature: + + void __atomic_load (size_t sz, const void *src, void *dst, int memorder); + + as seen in the gimple seen by the analyzer, and as specified + in sync-builtins.def. */ class kf_atomic_load : public internal_known_function { public: /* This is effectively: - *RET = *PTR; + memmove (dst, src, sz); */ void impl_call_pre (const call_details &cd) const final override { - const svalue *ptr_ptr_sval = cd.get_arg_svalue (0); - tree ptr_ptr_tree = cd.get_arg_tree (0); - const svalue *ret_ptr_sval = cd.get_arg_svalue (1); - tree ret_ptr_tree = cd.get_arg_tree (1); + const svalue *num_bytes_sval = cd.get_arg_svalue (0); + const svalue *src_sval = cd.get_arg_svalue (1); + tree src_tree = cd.get_arg_tree (1); + const svalue *dst_sval = cd.get_arg_svalue (2); + tree dst_tree = cd.get_arg_tree (2); /* Ignore the memorder param. */ region_model *model = cd.get_model (); region_model_context *ctxt = cd.get_ctxt (); - const region *ptr_region - = model->deref_rvalue (ptr_ptr_sval, ptr_ptr_tree, ctxt); - const svalue *star_ptr_sval = model->get_store_value (ptr_region, ctxt); - const region *ret_region - = model->deref_rvalue (ret_ptr_sval, ret_ptr_tree, ctxt); - model->set_value (ret_region, star_ptr_sval, ctxt); + const region *dst_reg = model->deref_rvalue (dst_sval, dst_tree, ctxt); + const region *src_reg = model->deref_rvalue (src_sval, src_tree, ctxt); + + const svalue *data_sval + = model->read_bytes (src_reg, src_tree, num_bytes_sval, ctxt); + model->write_bytes (dst_reg, num_bytes_sval, data_sval, ctxt); + } +}; + +/* Handler for __atomic_store. + Although the user-facing documentation specifies it as having this + signature: + + void __atomic_store (type *ptr, type *val, int memorder) + + by the time the C/C++ frontends have acted on it, any calls that + can't be mapped to a _N variation end up with this signature: + + void __atomic_store (size_t sz, type *dst, type *src, int memorder) + + as seen in the gimple seen by the analyzer, and as specified + in sync-builtins.def. */ + +class kf_atomic_store : public internal_known_function +{ +public: + /* This is effectively: + memmove (dst, src, sz); + */ + void impl_call_pre (const call_details &cd) const final override + { + const svalue *num_bytes_sval = cd.get_arg_svalue (0); + const svalue *dst_sval = cd.get_arg_svalue (1); + tree dst_tree = cd.get_arg_tree (1); + const svalue *src_sval = cd.get_arg_svalue (2); + tree src_tree = cd.get_arg_tree (2); + /* Ignore the memorder param. */ + + region_model *model = cd.get_model (); + region_model_context *ctxt = cd.get_ctxt (); + + const region *dst_reg = model->deref_rvalue (dst_sval, dst_tree, ctxt); + const region *src_reg = model->deref_rvalue (src_sval, src_tree, ctxt); + + const svalue *data_sval + = model->read_bytes (src_reg, src_tree, num_bytes_sval, ctxt); + model->write_bytes (dst_reg, num_bytes_sval, data_sval, ctxt); } }; @@ -2021,6 +2089,7 @@ register_atomic_builtins (known_function_manager &kfm) kfm.add (BUILT_IN_ATOMIC_LOAD_4, make_unique ()); kfm.add (BUILT_IN_ATOMIC_LOAD_8, make_unique ()); kfm.add (BUILT_IN_ATOMIC_LOAD_16, make_unique ()); + kfm.add (BUILT_IN_ATOMIC_STORE, make_unique ()); kfm.add (BUILT_IN_ATOMIC_STORE_N, make_unique ()); kfm.add (BUILT_IN_ATOMIC_STORE_1, make_unique ()); kfm.add (BUILT_IN_ATOMIC_STORE_2, make_unique ()); diff --git a/gcc/testsuite/c-c++-common/analyzer/atomic-builtins-pr114286.c b/gcc/testsuite/c-c++-common/analyzer/atomic-builtins-pr114286.c new file mode 100644 index 00000000000..1ff47ffaec8 --- /dev/null +++ b/gcc/testsuite/c-c++-common/analyzer/atomic-builtins-pr114286.c @@ -0,0 +1,48 @@ +#include "analyzer-decls.h" + +struct S { long long a[16]; } s; + +struct S +test_atomic_load (void) +{ + struct S r; + __atomic_load (&s, &r, __ATOMIC_RELAXED); + __analyzer_eval (r.a[0] == s.a[0]); /* { dg-warning "TRUE" } */ + __analyzer_eval (r.a[15] == s.a[15]); /* { dg-warning "TRUE" } */ + return r; +} + +void +test_atomic_store (struct S x) +{ + __atomic_store (&s, &x, __ATOMIC_RELAXED); + __analyzer_eval (s.a[0] == x.a[0]); /* { dg-warning "TRUE" } */ + __analyzer_eval (s.a[15] == x.a[15]); /* { dg-warning "TRUE" } */ +} + +struct S +test_atomic_exchange (struct S x) +{ + struct S init_x, init_s; + struct S r; + + /* Capture initial values of x and s for comparison below. */ + __atomic_load (&x, &init_x, __ATOMIC_RELAXED); + __atomic_load (&s, &init_s, __ATOMIC_RELAXED); + + __atomic_exchange (&s, &x, &r, __ATOMIC_RELAXED); + + __analyzer_eval (s.a[0] == init_x.a[0]); /* { dg-warning "TRUE" } */ + __analyzer_eval (s.a[15] == init_x.a[15]); /* { dg-warning "TRUE" } */ + __analyzer_eval (r.a[0] == init_s.a[0]); /* { dg-warning "TRUE" } */ + __analyzer_eval (r.a[15] == init_s.a[15]); /* { dg-warning "TRUE" } */ + + return r; +} + +int +test_atomic_compare_exchange (struct S *e, struct S *d) +{ + return __atomic_compare_exchange (&s, e, d, 0, + __ATOMIC_RELAXED, __ATOMIC_RELAXED); +}