On Mon, 2016-08-22 at 21:25 -0600, Martin Sebor wrote:
> > > Beyond that, the range normally works fine, except when macros
> > > are involved like they are in my tests. You can see the effect
> > > in the range.out file. (This works without your patch but it
> > > could very well be because I didn't set it up right.)
> >
> > Sadly I can't figure out what's going wrong - but the code's
> > changed a
> > lot at my end since then. Sorry.
>
> I have integrated the latest (already committed) version of your
> patch into my -Wformat-length patch. Everything (well almost)
> works and I get nice ranges for the format string and for (some)
> arguments.
>
> I was surprised at how long it took me to switch from the previous
> implementation (also copied from c-format.c) to this new API. As
> before, I had to copy bits and pieces of code from other parts of
> the compiler to get it to work. I was also surprised at how complex
> making use of it is. It added 130 lines of code to the pass, which
> is 40 more than what I had before. It seems that the
> format_warning_at_substring function from c-format.c (perhaps
> generalized and with some reasonable defaults hardcoded) should
> be defined where other parts of GCC (including the middle end)
> can reuse it.
I'm guessing that it was difficult because the most useful parts are
currently in c-format.c, whereas your code is in the middle-end.
Is the latest version of your patch posted somewhere where I can see
it?
The substring_loc class should probably be moved from c-family to gcc
also. We might need a langhook to support that though (not sure yet).
I'd be up for doing these moves (maybe moving
format_warning_at_substring to diagnostic.h/c), but I'd prefer to see
your patch first.
> I ran into a few minor glitches while testing it and I raised
> the following bugs for two of them:
>
> 77328 - incorrect caret location in -Wformat calling printf
> via a macro (this was pre-existing)
> 77331 - incorrect range location in -Wformat with a concatenated
> format literal (this is new)
>
> The third issue seems like a limitation that I should be able to
> overcome but I couldn't figure out how using the new API. The
> problem is that there doesn't seem to be a way to point the caret
> at the closing quote of a string, like for example in the following
> test. Even though by default the whole string is underlined (and
> the caret points to the opening quote), there doesn't seem to be
> a way to specify a range where the caret points to the other quote.
> It's no big deal and I only noticed it because one of my tests
> started failing, but it seems like it should be possible.
>
> $ cat t.c && /build/gcc-49905/gcc/xgcc -B /build/gcc-49905/gcc -S
> -Wformat t.c
> char d [2];
>
> void f (void)
> {
> __builtin_sprintf (d, "%sX", "1");
> }
> t.c: In function ‘f’:
> t.c:5:25: warning: writing a terminating nul past the end of the
> destination [-Wformat-length=]
> __builtin_sprintf (d, "%sX", "1");
> ^~~~~
> What I would like to see is similar to what I get when one of
> the format string characters is written past the end:
>
> t.c:5:30: warning: writing format character ‘Z’ at offset 4 past the
> end
> of the destination [-Wformat-length=]
> __builtin_sprintf (d, "%sXYZ", "");
> ^
>
So would you like the output to look like this:
Option (a): underline whole string, with caret at close-quote
t.c: In function ‘f’:
t.c:5:25: warning: writing a terminating nul past the end of the
destination [-Wformat-length=]
__builtin_sprintf (d, "%sX", "1");
~~~~^
or like this:
Option (b): just the close-quote
t.c: In function ‘f’:
t.c:5:25: warning: writing a terminating nul past
the end of the
destination [-Wformat-length=]
__builtin_sprintf (d,
"%sX", "1");
^
?
(do you also emit a note/inform showing the size of d?)
What API are you using to emit the warning? Given the location of the
string as a whole expressed as a location_t, you can probably do
something like this:
location_t
option_a (location_t string_as_a_whole_loc)
{
source_range src_range
= get_range_from_loc (line_table, string_as_a_whole_loc);
return make_location (src_range.m_finish, /* caret */
src_range.m_start, src_range.m_finish);
}
location_t
option_b (location_t string_as_a_whole_loc)
{
source_range
src_range
= get_range_from_loc (line_table, string_as_a_whole_loc);
return src_range.m_finish;
}
(these could be added to libcpp)
but I get the impression you want something like this integrated into
the format_warning or substring_loc APIs (and it's hard to tell without
seeing your patch).
Hope this is helpful
Dave
