public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Malcolm <dmalcolm@redhat.com>
To: Martin Sebor <msebor@gmail.com>
Cc: gcc-patches@gcc.gnu.org, David Malcolm <dmalcolm@redhat.com>
Subject: [PATCH] Fixups for Martin's gimple-ssa-sprintf.c patch
Date: Thu, 25 Aug 2016 15:54:00 -0000	[thread overview]
Message-ID: <1472142198-12113-1-git-send-email-dmalcolm@redhat.com> (raw)
In-Reply-To: <1472141352-10884-1-git-send-email-dmalcolm@redhat.com>

Martin: here are the fixups for your patch I needed to apply to make
it work with mine.  I couldn't actually get any of your existing test
cases to emit locations within the string literals, due to them all
being embedded in macro expansions (possibly relating to PR c/77328),
so I added a simple testcase using -fdiagnostics-show-caret, which
does successfully show a range within the string.

Posting in the hope that it's helpful; I haven't attempted a bootstrap
with it.

gcc/ChangeLog:
	* gimple-ssa-sprintf.c (class substring_loc): Delete.
	(g_string_concat_db): Delete.
	(substring_loc::get_location): Delete.
	(format_warning_va): Delete.
	(format_directive): Update use of substring_loc ctor to supply
	type of format string.
	(add_bytes): Likewise.

gcc/testsuite/ChangeLog:
	* gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: New test case.
---
 gcc/gimple-ssa-sprintf.c                           | 129 +--------------------
 .../gcc.dg/tree-ssa/builtin-sprintf-warn-4.c       |  17 +++
 2 files changed, 21 insertions(+), 125 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index ff8697c..fa63047 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -349,129 +349,6 @@ get_format_string (tree format, location_t *ploc)
   return fmtstr;
 }
 
-/* Describes a location range outlining a substring within a string
-   literal.  */
-
-class substring_loc
-{
- public:
-  substring_loc (location_t fmt_string_loc,
-		 int caret_idx, int start_idx, int end_idx)
-    : m_fmt_string_loc (fmt_string_loc),
-    m_caret_idx (caret_idx), m_start_idx (start_idx), m_end_idx (end_idx)
-  { }
-
-  const char *get_location (location_t *out_loc) const;
-
-  location_t get_fmt_string_loc () const { return m_fmt_string_loc; }
-
- private:
-  location_t m_fmt_string_loc;
-  int m_caret_idx;
-  int m_start_idx;
-  int m_end_idx;
-};
-
-/* The global record of string concatentations, for use in extracting
-   locations within string literals.  */
-
-GTY(()) string_concat_db *g_string_concat_db;
-
-/* Attempt to determine the source location of the substring.
-   If successful, return NULL and write the source location to *OUT_LOC.
-   Otherwise return an error message.  Error messages are intended
-   for GCC developers (to help debugging) rather than for end-users.  */
-
-const char *
-substring_loc::get_location (location_t *out_loc) const
-{
-  gcc_assert (out_loc);
-
-  if (!g_string_concat_db)
-    g_string_concat_db
-      = new (ggc_alloc <string_concat_db> ()) string_concat_db ();
-
-  static struct cpp_reader* parse_in;
-  if (!parse_in)
-    {
-      /* Create and initialize a preprocessing reader.  */
-      parse_in = cpp_create_reader (CLK_GNUC99, ident_hash, line_table);
-      cpp_init_iconv (parse_in);
-    }
-
-  return get_source_location_for_substring (parse_in, g_string_concat_db,
-					    m_fmt_string_loc, CPP_STRING,
-					    m_caret_idx, m_start_idx, m_end_idx,
-					    out_loc);
-}
-
-static ATTRIBUTE_GCC_DIAG (5,0) bool
-format_warning_va (const substring_loc &fmt_loc,
-		   const source_range *param_range,
-		   const char *corrected_substring,
-		   int opt, const char *gmsgid, va_list *ap)
-{
-  bool substring_within_range = false;
-  location_t primary_loc;
-  location_t fmt_substring_loc = UNKNOWN_LOCATION;
-  source_range fmt_loc_range
-    = get_range_from_loc (line_table, fmt_loc.get_fmt_string_loc ());
-  const char *err = fmt_loc.get_location (&fmt_substring_loc);
-  source_range fmt_substring_range
-    = get_range_from_loc (line_table, fmt_substring_loc);
-  if (err)
-    /* Case 3: unable to get substring location.  */
-    primary_loc = fmt_loc.get_fmt_string_loc ();
-  else
-    {
-      if (fmt_substring_range.m_start >= fmt_loc_range.m_start
-	  && fmt_substring_range.m_finish <= fmt_loc_range.m_finish)
-	/* Case 1.  */
-	{
-	  substring_within_range = true;
-	  primary_loc = fmt_substring_loc;
-	}
-      else
-	/* Case 2.  */
-	{
-	  substring_within_range = false;
-	  primary_loc = fmt_loc.get_fmt_string_loc ();
-	}
-    }
-
-  rich_location richloc (line_table, primary_loc);
-
-  if (param_range)
-    {
-      location_t param_loc = make_location (param_range->m_start,
-					    param_range->m_start,
-					    param_range->m_finish);
-      richloc.add_range (param_loc, false);
-    }
-
-  if (!err && corrected_substring && substring_within_range)
-    richloc.add_fixit_replace (fmt_substring_range, corrected_substring);
-
-  diagnostic_info diagnostic;
-  diagnostic_set_info (&diagnostic, gmsgid, ap, &richloc, DK_WARNING);
-  diagnostic.option_index = opt;
-  bool warned = report_diagnostic (&diagnostic);
-
-  if (!err && fmt_substring_loc && !substring_within_range)
-    /* Case 2.  */
-    if (warned)
-      {
-	rich_location substring_richloc (line_table, fmt_substring_loc);
-	if (corrected_substring)
-	  substring_richloc.add_fixit_replace (fmt_substring_range,
-					       corrected_substring);
-	inform_at_rich_loc (&substring_richloc,
-			    "format string is defined here");
-      }
-
-  return warned;
-}
-
 static ATTRIBUTE_GCC_DIAG (5,0) bool
 fmtwarn (const substring_loc &fmt_loc,
 	 const source_range *param_range,
@@ -1695,7 +1572,8 @@ format_directive (const pass_sprintf_length::call_info &info,
 
   /* Create a location for the whole directive from the % to the format
      specifier.  */
-  substring_loc dirloc (info.fmtloc, offset, offset, offset + cvtlen - 1);
+  substring_loc dirloc (info.fmtloc, TREE_TYPE (info.format), offset, offset,
+			offset + cvtlen - 1);
 
   /* Also create a location range for the argument if possible.
      This doesn't work for integer literals or function calls.  */
@@ -1920,7 +1798,8 @@ add_bytes (const pass_sprintf_length::call_info &info,
       size_t len = strlen (info.fmtstr + off);
 
       substring_loc loc
-	(info.fmtloc, off - !len, len ? off : 0, off + len - !!len);
+	(info.fmtloc, TREE_TYPE (info.format), off - !len, len ? off : 0,
+	 off + len - !!len);
 
       /* Is the output of the last directive the result of the argument
 	 being within a range whose lower bound would fit in the buffer
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
new file mode 100644
index 0000000..b10a9cd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -fdiagnostics-show-caret" } */
+
+void test (void)
+{
+  char a[4];
+  __builtin_sprintf (a, "abc%sghi", "def"); /* { dg-warning "29: .%s. directive writing 3 bytes into a region of size 1" } */
+  /* { dg-begin-multiline-output "" }
+   __builtin_sprintf (a, "abc%sghi", "def");
+                             ^~      ~~~~~
+     { dg-end-multiline-output "" } */
+
+  /* { dg-begin-multiline-output "" }
+   __builtin_sprintf (a, "abc%sghi", "def");
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+}
-- 
1.8.5.3

  reply	other threads:[~2016-08-25 15:54 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-25 15:40 [PATCH] Move class substring_loc from c-family into gcc David Malcolm
2016-08-25 15:54 ` David Malcolm [this message]
2016-08-25 16:30   ` [PATCH] Fixups for Martin's gimple-ssa-sprintf.c patch Martin Sebor
2016-08-31 16:23     ` Martin Sebor
2016-08-31 16:27       ` David Malcolm
2016-08-31 16:48         ` Martin Sebor
2016-09-02 23:57 ` [PATCH] Move class substring_loc from c-family into gcc Martin Sebor
2016-09-03  3:03   ` David Malcolm
2016-09-03  9:22   ` Manuel López-Ibáñez
2016-09-06 22:07     ` Martin Sebor
2016-09-07 17:19     ` [committed] " David Malcolm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1472142198-12113-1-git-send-email-dmalcolm@redhat.com \
    --to=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=msebor@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).