From 9f096d394a21f3ff492054c1b4b706c01400bb75 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 1 Sep 2015 09:10:32 -0400 Subject: [PATCH 27/42] Use rich locations in c-family/c-format.c This is a proof-of-concept of (a) using the string-literal location info to show locations of errors in format strings, and (b) underlining the corresponding argument where applicable (no more having to guess what the compiler means by "argument 2") In particular, this can handle format strings built using concatentation, potentially split over multiple lines etc. Screenshot: https://dmalcolm.fedorapeople.org/gcc/2015-09-04/ranges-in-format-string-diagnostics.html I also attempted to add captions to the underlines, but they looked too "busy", so I removed them for now. In theory this could replace some of the work done by Manu on PR c/52952 in e.g. r223470, since it adds handling of string concatentation, but I didn't go as far as rewriting all of them yet. gcc/c-family/ChangeLog: * c-format.c: Include gcc-rich-location.h. (check_format_arg): Pass in "format_tree" to check_format_info_main. (check_format_info_main): Add param "format_string_cst". Generate a source_range pass it to the call to check_format_types, (check_format_types): Replace location_t param with a source_range. Generate param_range and pars it to format_type_warning where applicable. (format_type_warning): Convert first param from location_t to source_range, as the range of the format string, and add a "param_range" parameter. Use them in the warnings. gcc/ChangeLog: * gcc-rich-location.c (gcc_rich_location::set_caption): New method. * gcc-rich-location.h (gcc_rich_location::set_caption): New method. gcc/testsuite/ChangeLog: * gcc.dg/format/diagnostic-ranges.c: New file. --- gcc/c-family/c-format.c | 115 +++++++++++++++--------- gcc/testsuite/gcc.dg/format/diagnostic-ranges.c | 101 +++++++++++++++++++++ 2 files changed, 173 insertions(+), 43 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/format/diagnostic-ranges.c diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index ab58076..5d8de29 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see #include "c-format.h" #include "alloc-pool.h" #include "c-target.h" +#include "gcc-rich-location.h" /* Handle attributes associated with format checking. */ @@ -1023,6 +1024,7 @@ static void check_format_info (function_format_info *, tree); static void check_format_arg (void *, tree, unsigned HOST_WIDE_INT); static void check_format_info_main (format_check_results *, function_format_info *, + tree, const char *, int, tree, unsigned HOST_WIDE_INT, object_allocator &); @@ -1036,8 +1038,12 @@ static void finish_dollar_format_checking (format_check_results *, int); static const format_flag_spec *get_flag_spec (const format_flag_spec *, int, const char *); -static void check_format_types (location_t, format_wanted_type *); -static void format_type_warning (location_t, format_wanted_type *, tree, tree); +static void check_format_types (source_range fmt_source_range, + format_wanted_type *); +static void format_type_warning (source_range fmt_source_range, + source_range *param_range, + format_wanted_type *, tree, + tree); /* Decode a format type from a string, returning the type, or format_type_error if not valid, in which case the caller should print an @@ -1689,12 +1695,13 @@ check_format_arg (void *ctx, tree format_tree, res->number_other++; object_allocator fwt_pool ("format_wanted_type pool", 10); - check_format_info_main (res, info, format_chars, format_length, + check_format_info_main (res, info, format_tree, format_chars, format_length, params, arg_num, fwt_pool); } -/* Do the main part of checking a call to a format function. FORMAT_CHARS +/* Do the main part of checking a call to a format function. + FORMAT_STRING_CST is the STRING_CST format string. FORMAT_CHARS is the NUL-terminated format string (which at this point may contain internal NUL characters); FORMAT_LENGTH is its length (excluding the terminating NUL character). ARG_NUM is one less than the number of @@ -1703,7 +1710,9 @@ check_format_arg (void *ctx, tree format_tree, static void check_format_info_main (format_check_results *res, - function_format_info *info, const char *format_chars, + function_format_info *info, + tree format_string_cst, + const char *format_chars, int format_length, tree params, unsigned HOST_WIDE_INT arg_num, object_allocator &fwt_pool) @@ -1763,6 +1772,7 @@ check_format_info_main (format_check_results *res, ++format_chars; continue; } + const char *start_of_this_format = format_chars; flag_chars[0] = 0; if ((fki->flags & (int) FMT_FLAG_USE_DOLLAR) && has_operand_number != 0) @@ -2426,7 +2436,17 @@ check_format_info_main (format_check_results *res, } if (first_wanted_type != 0) - check_format_types (format_string_loc, first_wanted_type); + { + ptrdiff_t offset_to_format_start = (start_of_this_format - 1) - orig_format_chars; + ptrdiff_t offset_to_format_end = (format_chars - 1) - orig_format_chars; + cpp_string_location *strloc + = TREE_STRING_LOCATION (format_string_cst); + gcc_assert (strloc); + source_range fmt_source_range + = strloc->get_range_between_indices (offset_to_format_start, + offset_to_format_end); + check_format_types (fmt_source_range, first_wanted_type); + } } if (format_chars - orig_format_chars != format_length) @@ -2446,10 +2466,11 @@ check_format_info_main (format_check_results *res, /* Check the argument types from a single format conversion (possibly - including width and precision arguments). LOC is the location of - the format string. */ + including width and precision arguments). FMT_SOURCE_RANGE is the + location of the format string. */ static void -check_format_types (location_t loc, format_wanted_type *types) +check_format_types (source_range fmt_source_range, + format_wanted_type *types) { for (; types != 0; types = types->next) { @@ -2476,7 +2497,7 @@ check_format_types (location_t loc, format_wanted_type *types) cur_param = types->param; if (!cur_param) { - format_type_warning (loc, types, wanted_type, NULL); + format_type_warning (fmt_source_range, NULL, types, wanted_type, NULL); continue; } @@ -2486,6 +2507,7 @@ check_format_types (location_t loc, format_wanted_type *types) orig_cur_type = cur_type; char_type_flag = 0; + source_range param_range = EXPR_LOCATION_RANGE (cur_param); STRIP_NOPS (cur_param); /* Check the types of any additional pointer arguments @@ -2550,7 +2572,8 @@ check_format_types (location_t loc, format_wanted_type *types) } else { - format_type_warning (loc, types, wanted_type, orig_cur_type); + format_type_warning (fmt_source_range, ¶m_range, types, + wanted_type, orig_cur_type); break; } } @@ -2618,20 +2641,24 @@ check_format_types (location_t loc, format_wanted_type *types) && TYPE_PRECISION (cur_type) == TYPE_PRECISION (wanted_type)) continue; /* Now we have a type mismatch. */ - format_type_warning (loc, types, wanted_type, orig_cur_type); + format_type_warning (fmt_source_range, ¶m_range, types, wanted_type, + orig_cur_type); } } -/* Give a warning at LOC about a format argument of different type from that - expected. WANTED_TYPE is the type the argument should have, possibly - stripped of pointer dereferences. The description (such as "field +/* Give a warning at FMT_SOURCE_RANGE about a format argument of different type + from that expected. If non-NULL, PARAM_RANGE is the source range of the + relevant argument. WANTED_TYPE is the type the argument should have, + possibly stripped of pointer dereferences. The description (such as "field precision"), the placement in the format string, a possibly more friendly name of WANTED_TYPE, and the number of pointer dereferences are taken from TYPE. ARG_TYPE is the type of the actual argument, or NULL if it is missing. */ static void -format_type_warning (location_t loc, format_wanted_type *type, +format_type_warning (source_range fmt_source_range, + source_range *param_range, + format_wanted_type *type, tree wanted_type, tree arg_type) { int kind = type->kind; @@ -2640,7 +2667,6 @@ format_type_warning (location_t loc, format_wanted_type *type, int format_length = type->format_length; int pointer_count = type->pointer_count; int arg_num = type->arg_num; - unsigned int offset_loc = type->offset_loc; char *p; /* If ARG_TYPE is a typedef with a misleading name (for example, @@ -2674,41 +2700,44 @@ format_type_warning (location_t loc, format_wanted_type *type, p[pointer_count + 1] = 0; } - loc = location_from_offset (loc, offset_loc); - + gcc_rich_location richloc (fmt_source_range); + + if (param_range) + richloc.add_range (*param_range); + if (wanted_type_name) { if (arg_type) - warning_at (loc, OPT_Wformat_, - "%s %<%s%.*s%> expects argument of type %<%s%s%>, " - "but argument %d has type %qT", - gettext (kind_descriptions[kind]), - (kind == CF_KIND_FORMAT ? "%" : ""), - format_length, format_start, - wanted_type_name, p, arg_num, arg_type); + warning_at_rich_loc (&richloc, OPT_Wformat_, + "%s %<%s%.*s%> expects argument of type %<%s%s%>, " + "but argument %d has type %qT", + gettext (kind_descriptions[kind]), + (kind == CF_KIND_FORMAT ? "%" : ""), + format_length, format_start, + wanted_type_name, p, arg_num, arg_type); else - warning_at (loc, OPT_Wformat_, - "%s %<%s%.*s%> expects a matching %<%s%s%> argument", - gettext (kind_descriptions[kind]), - (kind == CF_KIND_FORMAT ? "%" : ""), - format_length, format_start, wanted_type_name, p); + warning_at_rich_loc (&richloc, OPT_Wformat_, + "%s %<%s%.*s%> expects a matching %<%s%s%> argument", + gettext (kind_descriptions[kind]), + (kind == CF_KIND_FORMAT ? "%" : ""), + format_length, format_start, wanted_type_name, p); } else { if (arg_type) - warning_at (loc, OPT_Wformat_, - "%s %<%s%.*s%> expects argument of type %<%T%s%>, " - "but argument %d has type %qT", - gettext (kind_descriptions[kind]), - (kind == CF_KIND_FORMAT ? "%" : ""), - format_length, format_start, - wanted_type, p, arg_num, arg_type); + warning_at_rich_loc (&richloc, OPT_Wformat_, + "%s %<%s%.*s%> expects argument of type %<%T%s%>, " + "but argument %d has type %qT", + gettext (kind_descriptions[kind]), + (kind == CF_KIND_FORMAT ? "%" : ""), + format_length, format_start, + wanted_type, p, arg_num, arg_type); else - warning_at (loc, OPT_Wformat_, - "%s %<%s%.*s%> expects a matching %<%T%s%> argument", - gettext (kind_descriptions[kind]), - (kind == CF_KIND_FORMAT ? "%" : ""), - format_length, format_start, wanted_type, p); + warning_at_rich_loc (&richloc, OPT_Wformat_, + "%s %<%s%.*s%> expects a matching %<%T%s%> argument", + gettext (kind_descriptions[kind]), + (kind == CF_KIND_FORMAT ? "%" : ""), + format_length, format_start, wanted_type, p); } } diff --git a/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c new file mode 100644 index 0000000..f890f6a --- /dev/null +++ b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c @@ -0,0 +1,101 @@ +/* { dg-options "-Wformat -fdiagnostics-show-caret" } */ + +/* See PR 52952. */ + +extern int printf (__const char *__restrict __format, ...); + +void test_mismatching_types (const char *msg) +{ + printf("hello %i", msg); /* { dg-warning "format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */ + +/* { dg-begin-multiline-output "" } + printf("hello %i", msg); + ^~ ~~~ + { dg-end-multiline-output "" } */ +} + +void test_multiple_arguments (void) +{ + printf ("arg0: %i arg1: %s arg 2: %i", /* { dg-warning "format '%s'" } */ + 100, 101, 102); +/* { dg-begin-multiline-output "" } + printf ("arg0: %i arg1: %s arg 2: %i", + ^~ + 100, 101, 102); + ~~~ + { dg-end-multiline-output "" } */ +/* FIXME: why the trailing whitespace in the line above? */ +} + +void multiline_format_string (void) { + printf ("before the fmt specifier" + "%" /* { dg-warning "format '%d' expects a matching 'int' argument" } */ + "d" + "after the fmt specifier"); + +/* { dg-begin-multiline-output "" } + "%" + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + "d" + ~~ + { dg-end-multiline-output "" } */ +} + +void test_hex (const char *msg) +{ + /* "%" is \x25 + "i" is \x69 */ + printf("hello \x25\x69", msg); /* { dg-warning "format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */ + +/* { dg-begin-multiline-output "" } + printf("hello \x25\x69", msg); + ^~~~~~~~ ~~~ + { dg-end-multiline-output "" } */ +} + +void test_oct (const char *msg) +{ + /* "%" is octal 045 + "i" is octal 151. */ + printf("hello \045\151", msg); /* { dg-warning "format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */ + +/* { dg-begin-multiline-output "" } + printf("hello \045\151", msg); + ^~~~~~~~ ~~~ + { dg-end-multiline-output "" } */ +} + +void test_multiple (const char *msg) +{ + /* "%" is \x25 in hex + "i" is \151 in octal. */ + printf("prefix" "\x25" "\151" "suffix", /* { dg-warning "format '%i'" } */ + msg); + +/* { dg-begin-multiline-output "" } + printf("prefix" "\x25" "\151" "suffix", + ^~~~~~~~~~~~ + msg); + ~~~ + { dg-end-multiline-output "" } */ +/* FIXME: why the trailing whitespace in the line above? */ +} + +void test_u8 (const char *msg) +{ + printf(u8"hello %i", msg);/* { dg-warning "format '%i' expects argument of type 'int', but argument 2 has type 'const char \\*' " } */ +/* { dg-begin-multiline-output "" } + printf(u8"hello %i", msg); + ^~ ~~~ + { dg-end-multiline-output "" } */ +} + +void test_spurious_percent (void) +{ + printf("hello world %"); /* { dg-warning "spurious trailing" } */ + +/* { dg-begin-multiline-output "" } + printf("hello world %"); + ^ + { dg-end-multiline-output "" } */ +} -- 1.8.5.3