Re: [PATCH 3/4] Use class substring_loc in c-format.c (PR c/52952)

[David Malcolm]

On Thu, 2016-08-04 at 12:08 -0600, Jeff Law wrote:
> On 08/03/2016 09:45 AM, David Malcolm wrote:
> > This patch updates c-format.c to use the new class substring_loc,
> > added
> > in the previous patch, replacing location_column_from_byte_offset.
> > Hence with this patch, Wformat can underline the precise erroneous
> > format string in many more cases.
> > 
> > The patch also introduces two new functions for emitting Wformat
> > warnings: format_warning_at_substring and format_warning_at_char,
> > providing an inform in the face of macros where the pertinent part
> > of
> > the format string may be separate from the function call.
> > 
> > Successfully bootstrapped in conjunction with the rest
> > of the
> > patch kit on x86_64-pc-linux-gnu.
> > 
> > (The v2 version of the patch had a successful selftest run for
> > stage 1 on
> > powerpc-ibm-aix7.1.3.0 (gcc111) in conjunction with the rest of the
> > patch
> > kit, and a successful build of stage1 for all targets via config
> > -list.mk;
> > the patch has only been rebased since)
> > 
> > OK for trunk if it passes individual testing? (on top of patches 1
> > -2)
> > 
> > gcc/c-family/ChangeLog:
> > PR c/52952
> > * c-format.c: Include "diagnostic.h".
> > (location_column_from_byte_offset): Delete.
> > (location_from_offset): Delete.
> > (format_warning_va): New function.
> > (format_warning_at_substring): New function.
> > (format_warning_at_char): New function.
> > (check_format_arg): Capture location of format_tree and pass to
> > check_format_info_main.
> > (check_format_info_main): Add params FMT_PARAM_LOC and
> > FORMAT_STRING_CST.  Convert calls to warning_at to calls to
> > format_warning_at_char.  Pass a substring_loc instance to
> > check_format_types.
> > (check_format_types): Convert first param from a location_t
> > to a const substring_loc & and rename to "fmt_loc".  Attempt
> > to extract the range of the relevant parameter and pass it
> > to format_type_warning.
> > (format_type_warning): Convert first param from a location_t
> > to a const substring_loc & and rename to "fmt_loc".  Add
> > params "param_range" and "type".  Replace calls to warning_at
> > with calls to format_warning_at_substring.
> > 
> > gcc/testsuite/ChangeLog:
> > PR c/52952
> > * gcc.dg/cpp/pr66415-1.c: Likewise.
> > * gcc.dg/format/asm_fprintf-1.c: Update column numbers.
> > * gcc.dg/format/c90-printf-1.c: Likewise.
> > * gcc.dg/format/diagnostic-ranges.c: New test case.
> > ---
> > 
> 
> > @@ -1758,6 +1859,7 @@ check_format_info_main (format_check_results
> > *res,
> >   ++format_chars;
> >   continue;
> > }
> > +  const char *start_of_this_format = format_chars;
> Do you realize that this isn't used for ~700 lines after this point? 
>  Is 
> there any sensible way to factor some code here to avoid the coding 
> disconnect.  I realize the function was huge before you got in here,
> but 
> if at all possible, I'd like to see a bit of cleanup.
> 
> I think this is OK after that cleanup.

Thanks.  The patch needed obvious fixups to apply after the cleanup,
given the breakup of check_format_info_main.

I'm attaching what I actually committed to trunk (r239253).

(Successfully bootstrapped on x86_64-pc-linux-gnu; 
successful selftest run for stage 1 on powerpc-ibm-aix7.1.3.0, on
gcc111)Index: gcc/c-family/ChangeLog
===
--- gcc/c-family/ChangeLog	(revision 239252)
+++ gcc/c-family/ChangeLog	(revision 239253)
@@ -1,5 +1,50 @@
 2016-08-08  David Malcolm  
 
+	PR c/52952
+	* c-format.c: Include "diagnostic.h".
+	(location_column_from_byte_offset): Delete.
+	(location_from_offset): Delete.
+	(format_warning_va): New function.
+	(format_warning_at_substring): New function.
+	(format_warning_at_char): New function.
+	(check_format_arg): Capture location of format_tree and pass to
+	check_format_info_main.
+	(argument_parser): Add fields "start_of_this_format" and
+	"format_string_cst".
+	(flag_chars_t::validate): Add param "format_string_cst".  Convert
+	warning_at call using location_from_offset to call to
+	format_warning_at_char.
+	(argument_parser::argument_parser): Add param "format_string_cst_"
+	and use use it to initialize field "format_string_cst".
+	Initialize new field "start_of_this_format".
+	(argument_parser::read_format_flags): Convert warning_at call
+	using location_from_offset to a call to format_warning_at_char.
+	(argument_parser::read_any_format_left_precision): Likewise.
+	(argument_parser::read_any_format_precision): Likewise.
+	(argument_parser::read_any_other_modifier): Likewise.
+	(argument_parser::find_format_char_info): Likewise, in three places.
+	(argument_parser::parse_any_scan_set): Likewise, in one place.
+	(argument_parser::handle_conversions): Likewise, in two places.
+	(argument_parser::check_argument_type): Add param "fmt_param_loc"
+	and 

Re: [PATCH 3/4] Use class substring_loc in c-format.c (PR c/52952)

[Jeff Law]

On 08/04/2016 01:24 PM, David Malcolm wrote:


Do you realize that this isn't used for ~700 lines after this point?
 Is
there any sensible way to factor some code here to avoid the coding
disconnect.  I realize the function was huge before you got in here,
but
if at all possible, I'd like to see a bit of cleanup.

I think this is OK after that cleanup.


format_chars can get modified in numerous places in the intervening
lines, which is why I stash the value there.
Yea, I figured that was the case.  I first noticed the stashed value, 
but didn't see where it was used for far longer than I expected.




I can do some kind of cleanup of check_format_info_main, maybe
splitting out the things in the body of loop, moving them to support
functions.

That's essentially what I was thinking.



That said, I note that Martin's sprintf patch:
  https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00056.html
also touches those ~700 lines in check_format_info_main in over a dozen
places.  Given that, would you prefer I do the cleanup before or after
the substring_loc patch?
I think you should go first with the cleanup.  It'll cause Martin some 
heartburn, but that happens sometimes.


And FWIW, if you hadn't needed to stash away that value I probably 
wouldn't have noticed how badly that function (and the loop in 
particular) needed some refactoring.


jeff

Re: [PATCH 3/4] Use class substring_loc in c-format.c (PR c/52952)

[David Malcolm]

On Thu, 2016-08-04 at 12:08 -0600, Jeff Law wrote:
> On 08/03/2016 09:45 AM, David Malcolm wrote:
> > This patch updates c-format.c to use the new class substring_loc,
> > added
> > in the previous patch, replacing location_column_from_byte_offset.
> > Hence with this patch, Wformat can underline the precise erroneous
> > format string in many more cases.
> > 
> > The patch also introduces two new functions for emitting Wformat
> > warnings: format_warning_at_substring and format_warning_at_char,
> > providing an inform in the face of macros where the pertinent part
> > of
> > the format string may be separate from the function call.
> > 
> > Successfully bootstrapped in conjunction with the rest
> > of the
> > patch kit on x86_64-pc-linux-gnu.
> > 
> > (The v2 version of the patch had a successful selftest run for
> > stage 1 on
> > powerpc-ibm-aix7.1.3.0 (gcc111) in conjunction with the rest of the
> > patch
> > kit, and a successful build of stage1 for all targets via config
> > -list.mk;
> > the patch has only been rebased since)
> > 
> > OK for trunk if it passes individual testing? (on top of patches 1
> > -2)
> > 
> > gcc/c-family/ChangeLog:
> > PR c/52952
> > * c-format.c: Include "diagnostic.h".
> > (location_column_from_byte_offset): Delete.
> > (location_from_offset): Delete.
> > (format_warning_va): New function.
> > (format_warning_at_substring): New function.
> > (format_warning_at_char): New function.
> > (check_format_arg): Capture location of format_tree and pass to
> > check_format_info_main.
> > (check_format_info_main): Add params FMT_PARAM_LOC and
> > FORMAT_STRING_CST.  Convert calls to warning_at to calls to
> > format_warning_at_char.  Pass a substring_loc instance to
> > check_format_types.
> > (check_format_types): Convert first param from a location_t
> > to a const substring_loc & and rename to "fmt_loc".  Attempt
> > to extract the range of the relevant parameter and pass it
> > to format_type_warning.
> > (format_type_warning): Convert first param from a location_t
> > to a const substring_loc & and rename to "fmt_loc".  Add
> > params "param_range" and "type".  Replace calls to warning_at
> > with calls to format_warning_at_substring.
> > 
> > gcc/testsuite/ChangeLog:
> > PR c/52952
> > * gcc.dg/cpp/pr66415-1.c: Likewise.
> > * gcc.dg/format/asm_fprintf-1.c: Update column numbers.
> > * gcc.dg/format/c90-printf-1.c: Likewise.
> > * gcc.dg/format/diagnostic-ranges.c: New test case.
> > ---
> > 
> 
> > @@ -1758,6 +1859,7 @@ check_format_info_main (format_check_results
> > *res,
> >   ++format_chars;
> >   continue;
> > }
> > +  const char *start_of_this_format = format_chars;
> Do you realize that this isn't used for ~700 lines after this point? 
>  Is 
> there any sensible way to factor some code here to avoid the coding 
> disconnect.  I realize the function was huge before you got in here,
> but 
> if at all possible, I'd like to see a bit of cleanup.
> 
> I think this is OK after that cleanup.

format_chars can get modified in numerous places in the intervening
lines, which is why I stash the value there.

I can do some kind of cleanup of check_format_info_main, maybe
splitting out the things in the body of loop, moving them to support
functions.

That said, I note that Martin's sprintf patch:
  https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00056.html
also touches those ~700 lines in check_format_info_main in over a dozen
places.  Given that, would you prefer I do the cleanup before or after
the substring_loc patch?

[CCing Martin]

Re: [PATCH 3/4] Use class substring_loc in c-format.c (PR c/52952)

[Jeff Law]

On 08/03/2016 09:45 AM, David Malcolm wrote:

This patch updates c-format.c to use the new class substring_loc, added
in the previous patch, replacing location_column_from_byte_offset.
Hence with this patch, Wformat can underline the precise erroneous
format string in many more cases.

The patch also introduces two new functions for emitting Wformat
warnings: format_warning_at_substring and format_warning_at_char,
providing an inform in the face of macros where the pertinent part of
the format string may be separate from the function call.

Successfully bootstrapped in conjunction with the rest of the
patch kit on x86_64-pc-linux-gnu.

(The v2 version of the patch had a successful selftest run for stage 1 on
powerpc-ibm-aix7.1.3.0 (gcc111) in conjunction with the rest of the patch
kit, and a successful build of stage1 for all targets via config-list.mk;
the patch has only been rebased since)

OK for trunk if it passes individual testing? (on top of patches 1-2)

gcc/c-family/ChangeLog:
PR c/52952
* c-format.c: Include "diagnostic.h".
(location_column_from_byte_offset): Delete.
(location_from_offset): Delete.
(format_warning_va): New function.
(format_warning_at_substring): New function.
(format_warning_at_char): New function.
(check_format_arg): Capture location of format_tree and pass to
check_format_info_main.
(check_format_info_main): Add params FMT_PARAM_LOC and
FORMAT_STRING_CST.  Convert calls to warning_at to calls to
format_warning_at_char.  Pass a substring_loc instance to
check_format_types.
(check_format_types): Convert first param from a location_t
to a const substring_loc & and rename to "fmt_loc".  Attempt
to extract the range of the relevant parameter and pass it
to format_type_warning.
(format_type_warning): Convert first param from a location_t
to a const substring_loc & and rename to "fmt_loc".  Add
params "param_range" and "type".  Replace calls to warning_at
with calls to format_warning_at_substring.

gcc/testsuite/ChangeLog:
PR c/52952
* gcc.dg/cpp/pr66415-1.c: Likewise.
* gcc.dg/format/asm_fprintf-1.c: Update column numbers.
* gcc.dg/format/c90-printf-1.c: Likewise.
* gcc.dg/format/diagnostic-ranges.c: New test case.
---




@@ -1758,6 +1859,7 @@ check_format_info_main (format_check_results *res,
  ++format_chars;
  continue;
}
+  const char *start_of_this_format = format_chars;
Do you realize that this isn't used for ~700 lines after this point?  Is 
there any sensible way to factor some code here to avoid the coding 
disconnect.  I realize the function was huge before you got in here, but 
if at all possible, I'd like to see a bit of cleanup.


I think this is OK after that cleanup.

jeff