analyzer: fixes to __atomic_{exchange,load,store} [PR114286]

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.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
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 <dmalcolm@redhat.com>

diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index ed48ffbcba254e0d78daec7543996ef1cbfd3282..d197ccbd0f051c92855185a860355834ce671510 100644 (file)
--- 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<kf_atomic_load_n> ());
   kfm.add (BUILT_IN_ATOMIC_LOAD_8, make_unique<kf_atomic_load_n> ());
   kfm.add (BUILT_IN_ATOMIC_LOAD_16, make_unique<kf_atomic_load_n> ());
+  kfm.add (BUILT_IN_ATOMIC_STORE, make_unique<kf_atomic_store> ());
   kfm.add (BUILT_IN_ATOMIC_STORE_N, make_unique<kf_atomic_store_n> ());
   kfm.add (BUILT_IN_ATOMIC_STORE_1, make_unique<kf_atomic_store_n> ());
   kfm.add (BUILT_IN_ATOMIC_STORE_2, make_unique<kf_atomic_store_n> ());

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 (file)
index 0000000..1ff47ff
--- /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);
+}