On Fri, 2016-11-18 at 15:47 -0700, Martin Sebor wrote:
> 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.
Thanks.
The updated patch successfully bootstrapped®rtested
(on x86_64-pc-linux-gnu), so I've committed it to trunk (r242667).
I also verified the problematic test case under valgrind, and
confirmed that it's fixed (gcc.dg/tree-ssa/builtin-sprintf-2.c),
so I'll close out the bug.
Patch follows, for reference.
gcc/ChangeLog:
PR preprocessor/78324
* input.c (get_substring_ranges_for_loc): Fail gracefully if
-ftrack-macro-expansion has a value other than 2.
gcc/testsuite/ChangeLog:
PR preprocessor/78324
* gcc.dg/plugin/diagnostic-test-string-literals-1.c
(test_multitoken_macro): New function.
* gcc.dg/plugin/diagnostic-test-string-literals-3.c: New test
case.
* gcc.dg/plugin/diagnostic-test-string-literals-4.c: New test
case.
* gcc.dg/plugin/plugin.exp (plugin_test_list): Add the new test
cases.
* 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.
---
gcc/input.c | 9 +++
.../plugin/diagnostic-test-string-literals-1.c | 16 +++++
.../plugin/diagnostic-test-string-literals-3.c | 43 ++++++++++++
.../plugin/diagnostic-test-string-literals-4.c | 43 ++++++++++++
gcc/testsuite/gcc.dg/plugin/plugin.exp | 4 +-
.../gcc.dg/tree-ssa/builtin-sprintf-warn-1.c | 29 --------
.../gcc.dg/tree-ssa/builtin-sprintf-warn-4.c | 80 +++++++++++++++++++++-
7 files changed, 193 insertions(+), 31 deletions(-)
create mode 100644
gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-3.c
create mode 100644
gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-4.c
diff --git a/gcc/input.c b/gcc/input.c
index 728f4dd..611e18b 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -1322,6 +1322,15 @@ get_substring_ranges_for_loc (cpp_reader *pfile,
if (strloc == UNKNOWN_LOCATION)
return "unknown location";
+ /* Reparsing the strings requires accurate location information.
+ If -ftrack-macro-expansion has been overridden from its default
+ of 2, then we might have a location of a macro expansion point,
+ rather than the location of the literal itself.
+ Avoid this by requiring that we have full macro expansion tracking
+ for substring locations to be available. */
+ if (cpp_get_options (pfile)->track_macro_expansion != 2)
+ return "track_macro_expansion != 2";
+
/* If string concatenation has occurred at STRLOC, get the locations
of all of the literal tokens making up the compound string.
Otherwise, just use STRLOC. */
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-1.c
b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-1.c
index 3e44936..76a085e 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-1.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-1.c
@@ -243,6 +243,22 @@ test_macro (void)
{ dg-end-multiline-output "" } */
}
+void
+test_multitoken_macro (void)
+{
+#define RANGE ("0123456789") /* { dg-error "unable to read substring
location: macro expansion" } */
+ __emit_string_literal_range (RANGE, 4, 3, 6);
+/* { dg-begin-multiline-output "" }
+ #define RANGE ("0123456789")
+ ^
+ { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ __emit_string_literal_range (RANGE, 4, 3, 6);
+ ^~~~~
+ { dg-end-multiline-output "" } */
+#undef RANGE
+}
+
/* Verify that the location of the closing quote is used
for the location of the null terminating character. */
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-3.c
b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-3.c
new file mode 100644
index 0000000..95b78bc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-3.c
@@ -0,0 +1,43 @@
+/* Similar to diagnostic-test-string-literals-1.c, but with
+ -ftrack-macro-expansion=0. */
+
+/* { dg-do compile } */
+/* { dg-options "-O -ftrack-macro-expansion=0" } */
+
+extern void __emit_string_literal_range (const void *literal, int caret_idx,
+ int start_idx, int end_idx);
+
+void
+test_simple_string_literal (void)
+{
+ __emit_string_literal_range ("0123456789", /* { dg-error "unable to read
substring location: track_macro_expansion != 2" } */
+ 6, 6, 7);
+}
+
+void
+test_concatenated_string_literal (void)
+{
+ __emit_string_literal_range ("01234" "56789", /* { dg-error "unable to read
substring location: track_macro_expansion != 2" } */
+ 4, 3, 6);
+}
+
+/* To reproduce PR preprocessor/78324, the macro name should start
+ with the letter 'R'. */
+
+void
+test_macro (void)
+{
+#define RANGE "01234"
+ __emit_string_literal_range (RANGE /* { dg-error "unable to read substring
location: track_macro_expansion != 2" } */
+ "56789",
+ 4, 3, 6);
+#undef RANGE
+}
+
+void
+test_multitoken_macro (void)
+{
+#define RANGE ("0123456789")
+ __emit_string_literal_range (RANGE, 4, 3, 6); /* { dg-error "unable to read
substring location: track_macro_expansion != 2" } */
+#undef RANGE
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-4.c
b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-4.c
new file mode 100644
index 0000000..d47818a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-string-literals-4.c
@@ -0,0 +1,43 @@
+/* Similar to diagnostic-test-string-literals-1.c, but with
+ -ftrack-macro-expansion=1. */
+
+/* { dg-do compile } */
+/* { dg-options "-O -ftrack-macro-expansion=1" } */
+
+extern void __emit_string_literal_range (const void *literal, int caret_idx,
+ int start_idx, int end_idx);
+
+void
+test_simple_string_literal (void)
+{
+ __emit_string_literal_range ("0123456789", /* { dg-error "unable to read
substring location: track_macro_expansion != 2" } */
+ 6, 6, 7);
+}
+
+void
+test_concatenated_string_literal (void)
+{
+ __emit_string_literal_range ("01234" "56789", /* { dg-error "unable to read
substring location: track_macro_expansion != 2" } */
+ 4, 3, 6);
+}
+
+/* To reproduce PR preprocessor/78324, the macro name should start
+ with the letter 'R'. */
+
+void
+test_macro (void)
+{
+#define RANGE "01234" /* { dg-error "unable to read substring location:
track_macro_expansion != 2" } */
+ __emit_string_literal_range (RANGE
+ "56789",
+ 4, 3, 6);
+#undef RANGE
+}
+
+void
+test_multitoken_macro (void)
+{
+#define RANGE ("0123456789") /* { dg-error "unable to read substring
location: track_macro_expansion != 2" } */
+ __emit_string_literal_range (RANGE, 4, 3, 6);
+#undef RANGE
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/plugin.exp
b/gcc/testsuite/gcc.dg/plugin/plugin.exp
index 32ca748..eb15a66 100644
--- a/gcc/testsuite/gcc.dg/plugin/plugin.exp
+++ b/gcc/testsuite/gcc.dg/plugin/plugin.exp
@@ -73,7 +73,9 @@ set plugin_test_list [list \
diagnostic-test-show-trees-1.c } \
{ diagnostic_plugin_test_string_literals.c \
diagnostic-test-string-literals-1.c \
- diagnostic-test-string-literals-2.c } \
+ diagnostic-test-string-literals-2.c \
+ diagnostic-test-string-literals-3.c \
+ diagnostic-test-string-literals-4.c } \
{ location_overflow_plugin.c \
location-overflow-test-1.c \
location-overflow-test-2.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 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 "" } */
+}
--
1.8.5.3
