diff mbox

substring_loc info needs default track-macro-expansion (PR preprocessor/78324)

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

Commit Message

David Malcolm Nov. 18, 2016, 10:57 p.m. UTC 
On Fri, 2016-11-18 at 09:51 -0700, Martin Sebor wrote:
> > Martin: are the changes to your test cases OK by you, or is there
> > a better way to rewrite them?
> 
> Thanks for looking into it!
> 
> Since the purpose of the test_sprintf_note function in the test is
> to verify the location of the caret within the warnings I think we
> should keep it if it's possible.  Would either removing the P macro
> or moving the function to a different file that doesn't use the
> -ftrack-macro-expansion=0 option work?

To get substring locations with the proposed patch, that code will need to
be in a file without -ftrack-macro-expansion=0.

builtin-sprintf-warn-4.c seems to fit the bill, and has caret-printing
enabled too, so here's another attempt at the patch, just covering the
affected test cases, which moves test_sprintf_note to that file, and
drops "P".

The carets/underlines from the warnings look sane, and the test case
verifies that via dg-begin/end-multiline-output directives.  The test
case also verifies the carets/underlins from the *notes*.

[FWIW, I'm less convinced by the carets/underlines from the notes:
they all underline the whole of the __builtin_sprintf expression,
though looking at gimple-ssa-sprintf.c, I see:

      location_t callloc = gimple_location (info.callstmt);

which is used for the "inform" calls in question.  Hence I think
it's faithfully printing what that code is asking it to.  I'd prefer
to not touch that location in this patch, since it seems
orthogonal to fixing the PR preprocessor/78324; perhaps something
to address as part of PR middle-end/77696 ?].

Martin: how does this look to you?  Any objections to this change
as part of my fix for PR preprocessor/78324?

gcc/testsuite/ChangeLog:
	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test_sprintf_note):
	Move to...
	* gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: ...here.  Drop
	-ftrack-macro-expansion=0.
	(test_sprintf_note): Remove "P" macro.  Add
	dg-begin/end-multiline-output directives.
	(LINE, buffer, ptr): Copy from builtin-sprintf-warn-1.c.

Comments

Martin Sebor Nov. 18, 2016, 10:47 p.m. UTC | #1 
On 11/18/2016 03:57 PM, David Malcolm wrote:
> On Fri, 2016-11-18 at 09:51 -0700, Martin Sebor wrote:
>>> Martin: are the changes to your test cases OK by you, or is there
>>> a better way to rewrite them?
>>
>> Thanks for looking into it!
>>
>> Since the purpose of the test_sprintf_note function in the test is
>> to verify the location of the caret within the warnings I think we
>> should keep it if it's possible.  Would either removing the P macro
>> or moving the function to a different file that doesn't use the
>> -ftrack-macro-expansion=0 option work?
>
> To get substring locations with the proposed patch, that code will need to
> be in a file without -ftrack-macro-expansion=0.
>
> builtin-sprintf-warn-4.c seems to fit the bill, and has caret-printing
> enabled too, so here's another attempt at the patch, just covering the
> affected test cases, which moves test_sprintf_note to that file, and
> drops "P".
>
> The carets/underlines from the warnings look sane, and the test case
> verifies that via dg-begin/end-multiline-output directives.  The test
> case also verifies the carets/underlins from the *notes*.
>
> [FWIW, I'm less convinced by the carets/underlines from the notes:
> they all underline the whole of the __builtin_sprintf expression,
> though looking at gimple-ssa-sprintf.c, I see:
>
>       location_t callloc = gimple_location (info.callstmt);
>
> which is used for the "inform" calls in question.  Hence I think
> it's faithfully printing what that code is asking it to.  I'd prefer
> to not touch that location in this patch, since it seems
> orthogonal to fixing the PR preprocessor/78324; perhaps something
> to address as part of PR middle-end/77696 ?].
>
> Martin: how does this look to you?  Any objections to this change
> as part of my fix for PR preprocessor/78324?

Not at all.  It looks great -- using the multiline output is even
better than the original.  You also noticed the comment about the
caret limitation being out of data and removed it.  Thanks for
that too!

I agree that the underlining in the notes could stand to be
improved at some point.  I'll see if I can get to it sometime
after I'm done with all my pending patches.

Thanks again!
Martin

PS If there's something I can help with while you're working on
the rest of the bug let me know.
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
index 5779a95..a24889b 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
@@ -170,35 +170,6 @@  void test_sprintf_zero_length_array (void *p, int i)
   __builtin_sprintf (buffer (1), "%s",  s [i].a);
 }
 
-/* Verify that the note printed along with the diagnostic mentions
-   the correct sizes and refers to the location corresponding to
-   the affected directive.  */
-
-void test_sprintf_note (void)
-{
-#define P __builtin_sprintf
-
-  /* Diagnostic column numbers are 1-based.  */
-
-  P (buffer (0),                /* { dg-message "format output 4 bytes into a destination of size 0" } */
-     "%c%s%i", '1', "2", 3);    /* { dg-warning "7:.%c. directive writing 1 byte into a region of size 0" } */
-
-  P (buffer (1),                /* { dg-message "format output 6 bytes into a destination of size 1" } */
-     "%c%s%i", '1', "23", 45);  /* { dg-warning "9:.%s. directive writing 2 bytes into a region of size 0" } */
-
-  P (buffer (2),                /* { dg-message "format output 6 bytes into a destination of size 2" } */
-     "%c%s%i", '1', "2", 345);  /* { dg-warning "11:.%i. directive writing 3 bytes into a region of size 0" } */
-
-  /* It would be nice if the caret in the location range for the format
-     string below could be made to point at the closing quote of the format
-     string, like so:
-       sprintf (d, "%c%s%i", '1', "2", 3456);
-	            ~~~~~~^
-     Unfortunately, that doesn't work with the current setup.  */
-  P (buffer (6),                /* { dg-message "format output 7 bytes into a destination of size 6" } */
-     "%c%s%i", '1', "2", 3456); /* { dg-warning "writing a terminating nul past the end of the destination" } */
-}
-
 #undef T
 #define T(size, fmt, ...)					  \
   __builtin___sprintf_chk (buffer (size), 0, objsize (size), fmt, \
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
index faa5806..3b3fb68 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
@@ -1,5 +1,5 @@ 
 /* { dg-do compile } */
-/* { dg-options "-Wformat -Wformat-length=1 -fdiagnostics-show-caret -ftrack-macro-expansion=0" } */
+/* { dg-options "-Wformat -Wformat-length=1 -fdiagnostics-show-caret" } */
 
 extern int sprintf (char*, const char*, ...);
 
@@ -91,3 +91,81 @@  void test (void)
 }
 
 /* { dg-prune-output "too many arguments for format" } */
+
+/* When debugging, define LINE to the line number of the test case to exercise
+   and avoid exercising any of the others.  The buffer macro
+   below makes use of LINE to avoid warnings for other lines.  */
+#ifndef LINE
+# define LINE 0
+#endif
+
+char buffer [256];
+extern char *ptr;
+
+/* Evaluate to an array of SIZE characters when non-negative and LINE
+   is not set or set to the line the macro is on, or to a pointer to
+   an unknown object otherwise.  */
+#define buffer(size)							\
+  (0 <= size && (!LINE || __LINE__ == LINE)				\
+   ? buffer + sizeof buffer - size : ptr)
+
+/* Verify that the note printed along with the diagnostic mentions
+   the correct sizes and refers to the location corresponding to
+   the affected directive.  */
+
+void test_sprintf_note (void)
+{
+  /* Diagnostic column numbers are 1-based.  */
+
+  __builtin_sprintf (buffer (0), "%c%s%i", '1', "2", 3);
+  /* { dg-warning "35: .%c. directive writing 1 byte into a region of size 0" "" { target *-*-* } .-1 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (0), "%c%s%i", '1', "2", 3);
+                                   ^~
+     { dg-end-multiline-output "" }
+
+     { dg-message "format output 4 bytes into a destination of size 0" "" { target *-*-* } .-7 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (0), "%c%s%i", '1', "2", 3);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+
+  __builtin_sprintf (buffer (1), "%c%s%i", '1', "23", 45);
+  /* { dg-warning "37: .%s. directive writing 2 bytes into a region of size 0" "" { target *-*-* } .-1 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (1), "%c%s%i", '1', "23", 45);
+                                     ^~          ~~~~
+     { dg-end-multiline-output "" }
+
+     { dg-message "format output 6 bytes into a destination of size 1" "" { target *-*-* } .-7 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (1), "%c%s%i", '1', "23", 45);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+
+  __builtin_sprintf (buffer (2), "%c%s%i", '1', "2", 345);
+  /* { dg-warning "39: .%i. directive writing 3 bytes into a region of size 0" "" { target *-*-* } .-1 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (2), "%c%s%i", '1', "2", 345);
+                                       ^~
+     { dg-end-multiline-output "" }
+
+     { dg-message "format output 6 bytes into a destination of size 2" "" { target *-*-* } .-7 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (2), "%c%s%i", '1', "2", 345);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+
+  __builtin_sprintf (buffer (6), "%c%s%i", '1', "2", 3456);
+  /* { dg-warning "41: writing a terminating nul past the end of the destination" "" { target *-*-* } .-1 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (6), "%c%s%i", '1', "2", 3456);
+                                   ~~~~~~^
+     { dg-end-multiline-output "" }
+
+     { dg-message "format output 7 bytes into a destination of size 6" "" { target *-*-* } .-7 }
+     { dg-begin-multiline-output "" }
+   __builtin_sprintf (buffer (6), "%c%s%i", '1', "2", 3456);
+   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+     { dg-end-multiline-output "" } */
+}