From f6fadb756918af8f3021ca78e0df483b02232822 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Sat, 22 Jun 2019 17:12:26 -0400
Subject: [PATCH 32/51] FIXME: more precision-of-wording

---
 gcc/analyzer/sm-file.cc                        | 101 +++++++++++++++++++++----
 gcc/analyzer/sm-malloc.cc                      |  28 +++++--
 gcc/analyzer/sm.h                              |   6 +-
 gcc/testsuite/gcc.dg/analyzer/file-1.c         |  12 +--
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-1.c |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-4.c |   4 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-5.c |   4 +-
 7 files changed, 124 insertions(+), 33 deletions(-)

diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 78a86e2..adbfe59 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -40,17 +40,49 @@ static const bool DEBUG = false;
 
 ////////////////////////////////////////////////////////////////////////////
 
-class double_fclose : public pending_diagnostic_subclass<double_fclose>
+class file_diagnostic : public pending_diagnostic
 {
 public:
-  double_fclose (tree arg) : m_arg (arg) {}
+  file_diagnostic (const fileptr_state_machine &sm, tree arg)
+  : m_sm (sm), m_arg (arg)
+  {}
 
-  const char *get_kind () const FINAL OVERRIDE { return "double_fclose"; }
-  bool operator== (const double_fclose &other) const
+  bool subclass_equal_p (const pending_diagnostic &base_other) const OVERRIDE
+  {
+    return m_arg == ((const file_diagnostic &)base_other).m_arg;
+  }
+
+  label_text describe_state_change (const evdesc::state_change &change)
+    OVERRIDE
   {
-    return m_arg == other.m_arg;
+    if (change.m_old_state == m_sm.m_start
+	&& change.m_new_state == m_sm.m_unchecked)
+      // TODO: verify that it's the fopen stmt, not a copy
+      return label_text::borrow ("opened here");
+    if (change.m_old_state == m_sm.m_unchecked
+	&& change.m_new_state == m_sm.m_nonnull)
+      return formatted_print ("assuming %qE is non-NULL",
+			      change.m_expr);
+    if (change.m_new_state == m_sm.m_null)
+      return formatted_print ("assuming %qE is NULL",
+			      change.m_expr);
+    return label_text ();
   }
 
+protected:
+  const fileptr_state_machine &m_sm;
+  tree m_arg;
+};
+
+class double_fclose : public file_diagnostic
+{
+public:
+  double_fclose (const fileptr_state_machine &sm, tree arg)
+    : file_diagnostic (sm, arg)
+  {}
+
+  const char *get_kind () const FINAL OVERRIDE { return "double_fclose"; }
+
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
     warning_at (rich_loc, OPT_Wanalyzer_double_fclose,
@@ -58,20 +90,37 @@ public:
 		m_arg);
   }
 
+  label_text describe_state_change (const evdesc::state_change &change)
+    OVERRIDE
+  {
+    if (change.m_new_state == m_sm.m_closed)
+      {
+	m_first_fclose_event = change.m_event_id;
+	return formatted_print ("first %qs here", "fclose");
+      }
+    return file_diagnostic::describe_state_change (change);
+  }
+
+  label_text describe_final_event (const evdesc::final_event &) FINAL OVERRIDE
+  {
+    if (m_first_fclose_event.known_p ())
+      return formatted_print ("second %qs here; first %qs was at (%i)",
+			      "fclose", "fclose", m_first_fclose_event.one_based ());
+    return formatted_print ("second %qs here", "fclose");
+  }
+
 private:
-  tree m_arg;
+  event_id_t m_first_fclose_event;
 };
 
-class file_leak : public pending_diagnostic_subclass<file_leak>
+class file_leak : public file_diagnostic
 {
 public:
-  file_leak (tree arg) : m_arg (arg) {}
+  file_leak (const fileptr_state_machine &sm, tree arg)
+    : file_diagnostic (sm, arg)
+  {}
 
   const char *get_kind () const FINAL OVERRIDE { return "file_leak"; }
-  bool operator== (const file_leak &other) const
-  {
-    return m_arg == other.m_arg;
-  }
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -84,8 +133,28 @@ public:
 		m_arg);
   }
 
+  label_text describe_state_change (const evdesc::state_change &change)
+    FINAL OVERRIDE
+  {
+    if (change.m_new_state == m_sm.m_unchecked)
+      {
+	m_fopen_event = change.m_event_id;
+	return label_text::borrow ("opened here");
+      }
+    return file_diagnostic::describe_state_change (change);
+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
+  {
+    if (m_fopen_event.known_p ())
+      return formatted_print ("%qE leaks here; was opened at (%i)",
+			      ev.m_expr, m_fopen_event.one_based ());
+    else
+      return formatted_print ("%qE leaks here", ev.m_expr);
+  }
+
 private:
-  tree m_arg;
+  event_id_t m_fopen_event;
 };
 
 ////////////////////////////////////////////////////////////////////////////
@@ -145,7 +214,7 @@ fileptr_state_machine::on_stmt (sm_context *sm_ctxt,
 	  sm_ctxt->on_transition (node, stmt , arg, m_nonnull, m_closed);
 
 	  sm_ctxt->warn_for_state (node, stmt, arg, m_closed,
-				   new double_fclose (arg));
+				   new double_fclose (*this, arg));
 	  sm_ctxt->on_transition (node, stmt, arg, m_closed, m_stop);
 	  return true;
 	}
@@ -209,9 +278,9 @@ fileptr_state_machine::on_leak (sm_context *sm_ctxt,
   const
 {
   sm_ctxt->warn_for_state (node, stmt, var, m_unchecked,
-			   new file_leak (var));
+			   new file_leak (*this, var));
   sm_ctxt->warn_for_state (node, stmt, var, m_nonnull,
-			   new file_leak (var));
+			   new file_leak (*this, var));
 }
 
 bool
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index d2b4536..8be83e1 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -52,6 +52,23 @@ public:
     return m_arg == ((const malloc_diagnostic &)base_other).m_arg;
   }
 
+  label_text describe_state_change (const evdesc::state_change &change)
+    OVERRIDE
+  {
+    if (change.m_old_state == m_sm.m_start
+	&& change.m_new_state == m_sm.m_unchecked)
+      // TODO: verify that it's the allocation stmt, not a copy
+      return label_text::borrow ("allocated here");
+    if (change.m_old_state == m_sm.m_unchecked
+	&& change.m_new_state == m_sm.m_nonnull)
+      return formatted_print ("assuming %qE is non-NULL",
+			      change.m_expr);
+    if (change.m_new_state == m_sm.m_null)
+      return formatted_print ("assuming %qE is NULL",
+			      change.m_expr);
+    return label_text ();
+  }
+
 protected:
   const malloc_state_machine &m_sm;
   tree m_arg;
@@ -98,7 +115,7 @@ public:
 	m_first_free_event = change.m_event_id;
 	return formatted_print ("first %qs here", "free");
       }
-    return label_text ();
+    return malloc_diagnostic::describe_state_change (change);
   }
 
   label_text describe_call_with_state (const evdesc::call_with_state &info)
@@ -148,7 +165,7 @@ public:
     if (change.m_old_state == m_sm.m_start
 	&& change.m_new_state == m_sm.m_unchecked)
       return label_text::borrow ("this call could return NULL");
-    return label_text ();
+    return malloc_diagnostic::describe_state_change (change);
   }
 
   label_text describe_return_of_state (const evdesc::return_of_state &info)
@@ -347,7 +364,7 @@ public:
   {
     if (change.m_new_state == m_sm.m_free)
       return label_text::borrow ("freed here");
-    return label_text ();
+    return malloc_diagnostic::describe_state_change (change);
   }
 };
 
@@ -375,7 +392,7 @@ public:
 	m_malloc_event = change.m_event_id;
 	return label_text::borrow ("allocated here");
       }
-    return label_text ();
+    return malloc_diagnostic::describe_state_change (change);
   }
 
   label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
@@ -383,7 +400,8 @@ public:
     if (m_malloc_event.known_p ())
       return formatted_print ("%qE leaks here; was allocated at (%i)",
 			      ev.m_expr, m_malloc_event.one_based ());
-    return formatted_print ("%qE leaks here", ev.m_expr);
+    else
+      return formatted_print ("%qE leaks here", ev.m_expr);
   }
 
 private:
diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h
index 300441d..dfcaa90 100644
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -232,7 +232,8 @@ class pending_diagnostic
  public:
   virtual ~pending_diagnostic () {}
 
-  /* Vfunc for emitting the diagnostic.  */
+  /* Vfunc for emitting the diagnostic.  The rich_location will have been
+     populated with a diagnostic_path.  */
   virtual void emit (rich_location *) = 0;
 
   /* Hand-coded RTTI: get an ID for the subclass.  */
@@ -327,6 +328,8 @@ class pending_diagnostic
 
   /* End of precision-of-wording vfuncs.  */
 
+  /* Utility function for implementing the above vfuncs.  */
+
   static label_text formatted_print (const char *fmt, ...)
     ATTRIBUTE_GCC_DIAG(1,2);
 };
@@ -469,7 +472,6 @@ public:
 		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
 
-private:
   state_t m_start;
   state_t m_unchecked;
   state_t m_null;
diff --git a/gcc/testsuite/gcc.dg/analyzer/file-1.c b/gcc/testsuite/gcc.dg/analyzer/file-1.c
index af5bcc7..55f6977 100644
--- a/gcc/testsuite/gcc.dg/analyzer/file-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/file-1.c
@@ -3,24 +3,26 @@
 void
 test_1 (const char *path)
 {
-  FILE *f = fopen (path, "r");
+  FILE *f = fopen (path, "r"); /* { dg-message "opened here" } */
   if (!f)
     return;
 
-  fclose (f);
+  fclose (f); /* { dg-message "\\(4\\) first 'fclose' here" } */
   fclose (f); /* { dg-warning "double 'fclose' of FILE 'f'" } */ 
+  /* { dg-message "second 'fclose' here; first 'fclose' was at \\(4\\)" "" { target *-*-* } .-1 } */
 }
 
 void
 test_2 (const char *src, const char *dst)
 {
-  FILE *f_in = fopen (src, "r");
+  FILE *f_in = fopen (src, "r"); /* { dg-message "\\(1\\) opened here" } */
   if (!f_in)
     return;
 
   FILE *f_out = fopen (src, "w");
   if (!f_out)
-    return; /* { dg-warning "leak of FILE 'f_in'" } */ 
+    return; /* { dg-warning "leak of FILE 'f_in'" } */
+/* { dg-message "\\(5\\) 'f_in' leaks here; was opened at \\(1\\)" "" { target *-*-* } .-1 } */
 
   fclose (f_out);
   fclose (f_in);
@@ -29,6 +31,6 @@ test_2 (const char *src, const char *dst)
 void
 test_3 (const char *path)
 {
-  FILE *f = fopen (path, "r");
+  FILE *f = fopen (path, "r"); /* { dg-message "opened here" } */
   return; /* { dg-warning "leak of FILE 'f'" } */ 
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-1.c
index 7d2f4cf..39092b4 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-1.c
@@ -9,7 +9,7 @@ void test_1 (void)
   free (ptr); /* { dg-line second_free } */
 
   /* { dg-warning "double-free of 'ptr'"  "" { target *-*-* } second_free } */
-  /* { dg-message "\\(1\\) state of 'ptr': 'start' -> 'unchecked'" "" { target *-*-* } malloc } */
+  /* { dg-message "\\(1\\) allocated here" "" { target *-*-* } malloc } */
   /* { dg-message "\\(2\\) first 'free' here" "" { target *-*-* } first_free } */
   /* { dg-message "\\(3\\) second 'free' here; first 'free' was at \\(2\\)" "" { target *-*-* } second_free } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-4.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-4.c
index 1efea6e..f8a2686 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-4.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-4.c
@@ -12,8 +12,8 @@ int *test_4 (void)
   return ptr;
 
   /* { dg-warning "use of NULL 'ptr'" "" { target *-*-* } on_null_ptr } */
-  /* { dg-message "\\(1\\) state of 'ptr': 'start' -> 'unchecked'" "" { target *-*-* } malloc } */
+  /* { dg-message "\\(1\\) allocated here" "" { target *-*-* } malloc } */
   /* { dg-message "\\(2\\) following 'false' branch" "" { target *-*-* } cond } */
-  /* { dg-message "\\(3\\) state of 'ptr': 'unchecked' -> 'null'" "" { target *-*-* } cond } */
+  /* { dg-message "\\(3\\) assuming 'ptr' is NULL" "" { target *-*-* } cond } */
   /* { dg-message "\\(4\\) here \\('ptr' is in state 'null'\\)" "" { target *-*-* } on_null_ptr } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-5.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-5.c
index d2d4452..3ac3735 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-5.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-5.c
@@ -30,9 +30,9 @@ int test (const char *filename, int flag)
   return 0;
 
   /* { dg-warning "double-free of 'p'" "" { target *-*-* } second_free_of_p } */
-  /* { dg-message "\\(1\\) state of 'p': 'start' -> 'unchecked'" "" { target *-*-* } malloc_of_p } */
+  /* { dg-message "\\(1\\) allocated here" "" { target *-*-* } malloc_of_p } */
   /* { dg-message "\\(2\\) following 'false' branch" "" { target *-*-* } test_of_p } */
-  /* { dg-message "\\(3\\) state of 'p': 'unchecked' -> 'nonnull'" "" { target *-*-* } test_of_p } */
+  /* { dg-message "\\(3\\) assuming 'p' is non-NULL" "" { target *-*-* } test_of_p } */
   /* { dg-message "\\(4\\) following 'true' branch" "" { target *-*-* } test_of_q } */
   /* { dg-message "\\(5\\) first 'free' here" "" { target *-*-* } first_free_of_p } */
   /* { dg-message "\\(6\\) second 'free' here; first 'free' was at \\(5\\)" "" { target *-*-* } second_free_of_p } */
-- 
1.8.5.3

