diff mbox

Fixups for Martin's gimple-ssa-sprintf.c patch

Message ID 1472142198-12113-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Aug. 25, 2016, 4:23 p.m. UTC
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

Comments

Martin Sebor Aug. 25, 2016, 4:30 p.m. UTC | #1
On 08/25/2016 10:23 AM, David Malcolm wrote:
> 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.

Thanks!  Let me go through it, test it and get back to you when I'm
done (sometime next week after I get back from PTO).

Martin
Martin Sebor Aug. 31, 2016, 4:23 p.m. UTC | #2
On 08/25/2016 10:30 AM, Martin Sebor wrote:
> On 08/25/2016 10:23 AM, David Malcolm wrote:
>> 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.

I've tried the patch but the changes don't compile because
substring_loc is not declared.  I see the class defined in
c-family/c-common.h which I can't include here in the middle
end.  Am I missing some another patch?

Thanks
Martin
David Malcolm Aug. 31, 2016, 4:26 p.m. UTC | #3
On Wed, 2016-08-31 at 10:23 -0600, Martin Sebor wrote:
> On 08/25/2016 10:30 AM, Martin Sebor wrote:
> > On 08/25/2016 10:23 AM, David Malcolm wrote:
> > > 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.
> 
> I've tried the patch but the changes don't compile because
> substring_loc is not declared.  I see the class defined in
> c-family/c-common.h which I can't include here in the middle
> end.  Am I missing some another patch?

The fixup patch is on top of
  https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01811.html
which moves class substring_loc to gcc/substring-locations.h.
Martin Sebor Aug. 31, 2016, 4:48 p.m. UTC | #4
On 08/31/2016 10:26 AM, David Malcolm wrote:
> On Wed, 2016-08-31 at 10:23 -0600, Martin Sebor wrote:
>> On 08/25/2016 10:30 AM, Martin Sebor wrote:
>>> On 08/25/2016 10:23 AM, David Malcolm wrote:
>>>> 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.
>>
>> I've tried the patch but the changes don't compile because
>> substring_loc is not declared.  I see the class defined in
>> c-family/c-common.h which I can't include here in the middle
>> end.  Am I missing some another patch?
>
> The fixup patch is on top of
>    https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01811.html
> which moves class substring_loc to gcc/substring-locations.h.

Great!  With that patch my pass builds fine, all my tests pass,
and based on a quick comparison the output of the diagnostics
looks unchanged.

Thanks
Martin
diff mbox

Patch

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 "" } */
+}