On 08/23/2016 07:59 AM, David Malcolm wrote:
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?
I'm planning/hoping to post it this week. It was only difficult
in that I had to gather bits from different parts of the compiler
and figure out how to make them work together. I.e., it wasn't
a simple matter of replacing one function call with another. In
the end, it boiled down to replacing the get_location function
with this:
const char *
substring_loc::get_location (location_t *out_loc) const
{
gcc_assert (out_loc);
if (!g_string_concat_db)
g_string_concat_db
= new (ggc_alloc <string_concat_db> ()) string_concat_db ();
static struct cpp_reader* parse_in;
if (!parse_in)
{
/* Create and initialize a preprocessing reader. */
parse_in = cpp_create_reader (CLK_GNUC99, ident_hash, line_table);
cpp_init_iconv (parse_in);
}
return get_source_location_for_substring (parse_in, g_string_concat_db,
m_fmt_string_loc, CPP_STRING,
m_caret_idx, m_start_idx, m_end_idx,
out_loc);
}
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.
Sure.
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");
^
?
Either one would work for me. If this were to become a general
purpose interface then I think it would be nice to let the caller
decide which end/quote (if any) to point the caret at.
(do you also emit a note/inform showing the size of d?)
Yes. The full diagnostic looks like this (with the care at
the wrong end as we're discussing):
t.c:5:25: warning: writing a terminating nul past the end of the
destination [-Wformat-length=]
__builtin_sprintf (d, "%sX", "A");
^~~~~
t.c:5:3: note: format output 3 bytes into a destination of size 2
__builtin_sprintf (d, "%sX", "A");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
What API are you using to emit the warning?
I copied the format_warning_va and format_warning_at subscript
functions from c-format.c and I'm calling the latter. I'm not
using the hint for anything yet (not sure if there's
an opportunity to make use of it).
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).
I guess I was hoping for a simple high level interface to warning_at
where I could control the offset of the caret and the underlining
within some bounds. Completely off the cuff, say if warning_at were
overloaded to take another argument with the offsets, then something
like this (with offsets in characters):
void my_warning (location_t loc)
{
int caret = /* offset of caret from the beginning of loc */;
int begin = /* optional offset of the start of underlining */;
int end = /* optional offset of the end of underlining */;
warning_at (range (loc, caret, begin, end), ...);
}
Maybe I should prototype it to understand if it can be done and what
the tradeoffs might be.
The most recently posted patch uses the location_from_offset function
that c-format.c used before your changes:
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00986.html
In my latest patch I replaced the function and calls to warning_at
with its result with format_warning_at_substring. I mainly did it
because I thought that was going to be new/recommended API for this
sort of thing. I wasn't having any problems with previous approach
or looking for enhancements (though the split location for the
format directive and its argument is nice). Did I misunderstand
what the intent of your changes was? (I.e., did you not expect
me to make that switch?)
Hope this is helpful
Dave
Yes, thanks.
Martin
