Re: [PATCH] substring_loc info needs default track-macro-expansion (PR preprocessor/78324)
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.
Re: [PATCH] substring_loc info needs default track-macro-expansion (PR preprocessor/78324)
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.
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
Re: [PATCH] substring_loc info needs default track-macro-expansion (PR preprocessor/78324)
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?
Martin
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..b6a6011 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
@@ -181,13 +181,13 @@ void test_sprintf_note (void)
/* 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" } */
+ "%c%s%i", '1', "2", 3);/* { dg-warning ".%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" } */
+ "%c%s%i", '1', "23", 45); /* { dg-warning ".%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" } */
+ "%c%s%i", '1', "2", 345); /* { dg-warning ".%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
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..b587d00 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*, ...);
