assume built-in calls don't modify allocated objects (PR 101584)

Message ID 8b64dfc4-8ac1-7b30-62ec-8dc0087b6e04@gmail.com
State New
Headers show
Series
  • assume built-in calls don't modify allocated objects (PR 101584)
Related show

Commit Message

Aldy Hernandez via Gcc-patches July 22, 2021, 10:27 p.m.
Passing a pointer to a built-in function escapes it, which in
turn causes objects pointed to by other potentially aliased
pointers to be assumed to be modified by other built-in calls.
This leads to a class of false negative -Wuninitialized
warnings that can be avoided by the warning code and without
making changes to the aliasing machinery.

In addition, GCC conservatively assumes that an object whose
address is passed as an argument to any directive in a printf
call is modified by the call.  This is necessary if the directive
isn't known because it could be %n, but in such a case it's
reasonable to assume the pointed-to type wouldn't be const-
qualified.  This assumption makes it easy to detect a class
of uninitialized reads that are not detected today.

The attached patch implements both assumptions: i.e., that a call
to a built-in function declared to take only const pointer
arguments, or to a variadic function with only const pointers
as arguments, doesn't modify any objects.

The change detects certain uninitialized accesses slightly earlier
which causes uninit-38.c failures.  As the comment in the test
explains, that's expected.  I've simply removed the failed tests
and left the rest.  They exercise the same functionality (MEM_REF
formatting).

Tested on x86_64-linux.

Martin

Comments

Aldy Hernandez via Gcc-patches July 26, 2021, 2:35 p.m. | #1
On 7/22/2021 4:27 PM, Martin Sebor via Gcc-patches wrote:
> Passing a pointer to a built-in function escapes it, which in

> turn causes objects pointed to by other potentially aliased

> pointers to be assumed to be modified by other built-in calls.

> This leads to a class of false negative -Wuninitialized

> warnings that can be avoided by the warning code and without

> making changes to the aliasing machinery.

>

> In addition, GCC conservatively assumes that an object whose

> address is passed as an argument to any directive in a printf

> call is modified by the call.  This is necessary if the directive

> isn't known because it could be %n, but in such a case it's

> reasonable to assume the pointed-to type wouldn't be const-

> qualified.  This assumption makes it easy to detect a class

> of uninitialized reads that are not detected today.

>

> The attached patch implements both assumptions: i.e., that a call

> to a built-in function declared to take only const pointer

> arguments, or to a variadic function with only const pointers

> as arguments, doesn't modify any objects.

>

> The change detects certain uninitialized accesses slightly earlier

> which causes uninit-38.c failures.  As the comment in the test

> explains, that's expected.  I've simply removed the failed tests

> and left the rest.  They exercise the same functionality (MEM_REF

> formatting).

>

> Tested on x86_64-linux.

>

> Martin

>

>

> gcc-101584.diff

>

> PR tree-optimization/101584 - missing -Wuninitialized with an allocated object after a built-in call

>

> gcc/ChangeLog:

>

> 	PR tree-optimization/101584

> 	* tree-ssa-uninit.c (builtin_call_nomodifying_p): New function.

> 	(check_defs): Call it.

>

> gcc/testsuite/ChangeLog:

>

> 	PR tree-optimization/101584

> 	* gcc.dg/uninit-38.c: Remove assertions.

> 	* gcc.dg/uninit-41.c: New test.

OK
jeff

Patch

PR tree-optimization/101584 - missing -Wuninitialized with an allocated object after a built-in call

gcc/ChangeLog:

	PR tree-optimization/101584
	* tree-ssa-uninit.c (builtin_call_nomodifying_p): New function.
	(check_defs): Call it.

gcc/testsuite/ChangeLog:

	PR tree-optimization/101584
	* gcc.dg/uninit-38.c: Remove assertions.
	* gcc.dg/uninit-41.c: New test.

diff --git a/gcc/testsuite/gcc.dg/uninit-38.c b/gcc/testsuite/gcc.dg/uninit-38.c
index 8dacc8c63a6..0d70bcd8e98 100644
--- a/gcc/testsuite/gcc.dg/uninit-38.c
+++ b/gcc/testsuite/gcc.dg/uninit-38.c
@@ -1,5 +1,5 @@ 
-/* Verify that dereferencing uninitialized allocated objects and VLAs
-   correctly reflects offsets into the objects.
+/* Verify that dereferencing uninitialized VLAs correctly reflects
+   offsets into the objects.
    The test's main purpose is to exercise the formatting of MEM_REFs.
    If -Wuninitialized gets smarter and detects uninitialized accesses
    before they're turned into MEM_REFs the test will likely need to
@@ -18,41 +18,6 @@  extern void* malloc (size_t);
 
 void sink (void*, ...);
 
-#undef T
-#define T(Type, idx, off)			\
-  __attribute__ ((noipa))			\
-  void UNIQ (test_)(int n)			\
-  {						\
-    void *p = malloc (n);			\
-    Type *q = (Type*)((char*)p + off);		\
-    sink (p, q[idx]);				\
-  }						\
-  typedef void dummy_type
-
-T (int, 0, 0);      // { dg-warning "'\\*\\(int \\*\\)p' is used uninitialized" }
-T (int, 0, 1);      // { dg-warning "'\\*\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)'" }
-T (int, 0, 2);      // { dg-warning "'\\*\\(int \\*\\)\\(\\(char \\*\\)p \\+ 2\\)'" }
-T (int, 0, 3);      // { dg-warning "'\\*\\(int \\*\\)\\(\\(char \\*\\)p \\+ 3\\)'" }
-T (int, 0, 4);      // { dg-warning "'\\(\\(int \\*\\)p\\)\\\[1]'" }
-T (int, 0, 5);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)\\)\\\[1]'" }
-T (int, 0, 6);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 2\\)\\)\\\[1]'" }
-T (int, 0, 7);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 3\\)\\)\\\[1]'" }
-T (int, 0, 8);      // { dg-warning "'\\(\\(int \\*\\)p\\)\\\[2]'" }
-T (int, 0, 9);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)\\)\\\[2]'" }
-
-
-T (int, 1, 0);      // { dg-warning "'\\(\\(int \\*\\)p\\)\\\[1]' is used uninitialized" }
-T (int, 1, 1);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)\\)\\\[1]'" }
-T (int, 1, 2);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 2\\)\\)\\\[1]'" }
-T (int, 1, 3);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 3\\)\\)\\\[1]'" }
-T (int, 1, 4);      // { dg-warning "'\\(\\(int \\*\\)p\\)\\\[2]'" }
-T (int, 1, 5);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)\\)\\\[2]'" }
-T (int, 1, 6);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 2\\)\\)\\\[2]'" }
-T (int, 1, 7);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 3\\)\\)\\\[2]'" }
-T (int, 1, 8);      // { dg-warning "'\\(\\(int \\*\\)p\\)\\\[3]'" }
-T (int, 1, 9);      // { dg-warning "'\\(\\(int \\*\\)\\(\\(char \\*\\)p \\+ 1\\)\\)\\\[3]'" }
-
-#undef T
 #define T(Type, idx, off)			\
   __attribute__ ((noipa))			\
   void UNIQ (test_)(int n)			\
diff --git a/gcc/testsuite/gcc.dg/uninit-41.c b/gcc/testsuite/gcc.dg/uninit-41.c
new file mode 100644
index 00000000000..b485611e994
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-41.c
@@ -0,0 +1,121 @@ 
+/* Verify that calls to non-modifying built-ins aren't considered
+   potentially modifying.
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+void* alloca (size_t);
+void* calloc (size_t, size_t);
+void* malloc (size_t);
+int printf (const char *, ...);
+int scanf (const char *, ...);
+int sprintf (char *, const char *, ...);
+int snprintf (char *, size_t, const char *, ...);
+int puts (const char *);
+char* strcpy (char*, const char*);
+size_t strlen (const char*);
+
+void noproto ();
+
+void sink (int, ...);
+
+extern char a[];
+
+void nowarn_noproto (const char *fmt)
+{
+  int i;
+  noproto (&i);
+  sink (i);
+}
+
+void nowarn_scanf (const char *fmt)
+{
+  int i;
+  scanf ("%i", &i);
+  sink (i);
+}
+
+void test_puts_sprintf_alloca (const char *fmt)
+{
+  char *p;
+  {
+    p = alloca (8);
+    sprintf (a, fmt, p);                // fmt might contain %n
+    puts (p);
+  }
+
+  {
+    p = alloca (8);
+    snprintf (0, 0, fmt, p);            // same as above
+    puts (p);
+  }
+}
+
+void test_puts_alloca (const char *s)
+{
+  char *p = alloca (8);
+
+  {
+    char a[] = "foo";
+    puts (a);
+  }
+
+  puts (p);                             // { dg-warning "-Wuninitialized" }
+
+  {
+    p = alloca (strlen (s) + 1);
+    strcpy (p, s);
+    puts (p);
+  }
+
+  {
+    /* Verify that the puts() calls above isn't considered to have
+       potentially modified *P, and same for the one below.  */
+    p = alloca (strlen (s));
+    puts (p);                           // { dg-warning "-Wuninitialized" }
+    puts (p + 1);                       // { dg-warning "-Wuninitialized" }
+  }
+}
+
+
+void test_puts_malloc (const char *s, const char *t)
+{
+  char *p;
+
+  {
+    p = malloc (strlen (s) + 1);
+    strcpy (p, s);
+    puts (p);
+  }
+
+  {
+    p = malloc (strlen (t));
+    puts (p);                           // { dg-warning "-Wuninitialized" }
+  }
+}
+
+
+void test_puts_vla (const char *s, const char *t)
+{
+  {
+    char a[strlen (s) + 1];
+    strcpy (a, s);
+    puts (a);
+  }
+
+  {
+    char b[strlen (t)];
+    puts (b);                           // { dg-warning "-Wuninitialized" }
+  }
+}
+
+
+void test_printf_puts (const char *s)
+{
+  char *p = __builtin_malloc (1);
+
+  printf ("%s", s);
+
+  puts (p);                             // { dg-warning "-Wuninitialized" }
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 148f3c2b31d..0c4b0257f0d 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -219,6 +219,70 @@  struct check_defs_data
   bool found_may_defs;
 };
 
+/* Return true if STMT is a call to built-in function all of whose
+   by-reference arguments are const-qualified (i.e., the function can
+   be assumed not to modify them).  */
+
+static bool
+builtin_call_nomodifying_p (gimple *stmt)
+{
+  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
+    return false;
+
+  tree fndecl = gimple_call_fndecl (stmt);
+  if (!fndecl)
+    return false;
+
+  tree fntype = TREE_TYPE (fndecl);
+  if (!fntype)
+    return false;
+
+  /* Check the called function's signature for non-constc pointers.
+     If one is found, return false.  */
+  unsigned argno = 0;
+  tree argtype;
+  function_args_iterator it;
+  FOREACH_FUNCTION_ARGS (fntype, argtype, it)
+    {
+      if (VOID_TYPE_P (argtype))
+	return true;
+
+      ++argno;
+
+      if (!POINTER_TYPE_P (argtype))
+	continue;
+
+      if (TYPE_READONLY (TREE_TYPE (argtype)))
+	continue;
+
+      return false;
+    }
+
+  /* If the number of actual arguments to the call is less than or
+     equal to the number of parameters, return false.  */
+  unsigned nargs = gimple_call_num_args (stmt);
+  if (nargs <= argno)
+    return false;
+
+  /* Check arguments passed through the ellipsis in calls to variadic
+     functions for pointers.  If one is found that's a non-constant
+     pointer, return false.  */
+  for (; argno < nargs; ++argno)
+    {
+      tree arg = gimple_call_arg (stmt, argno);
+      argtype = TREE_TYPE (arg);
+      if (!POINTER_TYPE_P (argtype))
+	continue;
+
+      if (TYPE_READONLY (TREE_TYPE (argtype)))
+	continue;
+
+      return false;
+    }
+
+  return true;
+}
+
 /* Callback for walk_aliased_vdefs.  */
 
 static bool
@@ -261,6 +325,9 @@  check_defs (ao_ref *ref, tree vdef, void *data_)
       return false;
     }
 
+  if (builtin_call_nomodifying_p (def_stmt))
+    return false;
+
   /* Found a may-def on this path.  */
   data->found_may_defs = true;
   return true;