Re: [PATCH] Move class substring_loc from c-family into gcc
On 09/03/2016 03:13 AM, Manuel López-Ibáñez wrote: On 02/09/16 23:55, Martin Sebor wrote: diff --git a/gcc/substring-locations.h b/gcc/substring-locations.h index f839c74..bb0de4f 100644 --- a/gcc/substring-locations.h +++ b/gcc/substring-locations.h @@ -20,6 +20,73 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_SUBSTRING_LOCATIONS_H #define GCC_SUBSTRING_LOCATIONS_H +#include + Is this header file going to be used in the middle-end? If so, then it is suspicious that it includes cpplib.h. Otherwise, perhaps it should live in c-family/ I'm not sure if you meant this question for me or for David but I believe the main (only?) reason for this patch is let to make use, in the -Wformat-length middle-end pass, of the same range location API as in c-family/c-format in the C/C++ front ends. David, please correct me if I mischaracterized it. Martin I'm not complaining about substring-locations.c because libcpp is already linked with everything else even for other non-C languages, like Ada, but the above is leaking all cpplib.h into the rest of the compiler, which defeats the purpose of this in coretypes.h /* Provide forward struct declaration so that we don't have to include all of cpplib.h whenever a random prototype includes a pointer. Note that the cpp_reader and cpp_token typedefs remain part of cpplib.h. */ struct cpp_reader; struct cpp_token; Cheers, Manuel.
Re: [PATCH] Move class substring_loc from c-family into gcc
On 02/09/16 23:55, Martin Sebor wrote: diff --git a/gcc/substring-locations.h b/gcc/substring-locations.h index f839c74..bb0de4f 100644 --- a/gcc/substring-locations.h +++ b/gcc/substring-locations.h @@ -20,6 +20,73 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_SUBSTRING_LOCATIONS_H #define GCC_SUBSTRING_LOCATIONS_H +#include + Is this header file going to be used in the middle-end? If so, then it is suspicious that it includes cpplib.h. Otherwise, perhaps it should live in c-family/ I'm not complaining about substring-locations.c because libcpp is already linked with everything else even for other non-C languages, like Ada, but the above is leaking all cpplib.h into the rest of the compiler, which defeats the purpose of this in coretypes.h /* Provide forward struct declaration so that we don't have to include all of cpplib.h whenever a random prototype includes a pointer. Note that the cpp_reader and cpp_token typedefs remain part of cpplib.h. */ struct cpp_reader; struct cpp_token; Cheers, Manuel.
Re: [PATCH] Move class substring_loc from c-family into gcc
On Fri, 2016-09-02 at 16:55 -0600, Martin Sebor wrote: > I've successfully tested the patch below by incorporating it into > the -Wformat-length pass I've been working on. I'd like to submit > the latest and hopefully close to final version of my work for > review without duplicating this code and it might be helpful if > it was possible to build my latest patch without having to find > and install this one first. Could someone review and approve > David's patch? I'm not quite sure what the boundaries of my "diagnostics"/"libcpp" maintainer rights are, and on whether I could self-approve this one - the addition of the langhook gave me pause. But https://gcc.gnu.org/ml/gcc-patches/2016-08/msg02161.html suggests that maybe I'm being too reticent, along with your positive feedback on the patch; it is all about diagnostics infrastructure, after all. So if no-one complains I'll commit it early next week. > Thanks > Martin > > On 08/25/2016 10:09 AM, David Malcolm wrote: > > This patch is intended to help Martin's new middle-end sprintf > > format > > warning. > > > > It moves most of the on-demand locations-within-strings > > code in c-family into gcc, into a new substring-locations.c file to > > go with substring-locations.h: class substring_loc, representing > > a source caret and source range within a STRING_CST, along with > > format_warning for handling emitting a warning relating to it. > > > > The actual work of reconstructing the source locations within > > a string seems inherently frontend-specific, so the patch > > introduces a > > langhook to do this, implementing it for C using the existing code > > (and thus hiding the details of string-concatenation as being > > specific to the c-family). Attempts to get substring location > > from other frontends will fail, and the format_warning_* calls > > handle such failures gracefully by simply using the location of > > the string as a whole for the warning. > > > > I've tested it with Martin's gimple-ssa-sprintf patch, and I'm able > > to > > emit carets and underlines in the correct places in C code from the > > middle end with this approach (patch to follow). > > > > Successfully bootstrapped on x86_64-pc-linux-gnu. > > > > OK for trunk? > > > > gcc/ChangeLog: > > * Makefile.in (OBJS): Add substring-locations.o. > > * langhooks-def.h (class substring_loc): New forward decl. > > (lhd_get_substring_location): New decl. > > (LANG_HOOKS_GET_SUBSTRING_LOCATION): New macro. > > (LANG_HOOKS_INITIALIZER): Add > > LANG_HOOKS_GET_SUBSTRING_LOCATION. > > * langhooks.c (lhd_get_substring_location): New function. > > * langhooks.h (class substring_loc): New forward decl. > > (struct lang_hooks): Add field get_substring_location. > > * substring-locations.c: New file, taking definition of > > format_warning_va and format_warning_at_substring from > > c-family/c-format.c, making them non-static. > > * substring-locations.h: Include . > > (class substring_loc): Move class here from c-family/c > > -common.h. > > Add comments. > > (format_warning_va): New decl. > > (format_warning_at_substring): New decl. > > (get_source_location_for_substring): Add comment. > > > > gcc/c-family/ChangeLog: > > * c-common.c (get_cpp_ttype_from_string_type): Handle being > > passed > > a POINTER_TYPE. > > (substring_loc::get_location): Move to substring-locations.c, > > keeping implementation as... > > (c_get_substring_location): New function, from the above, > > reworked > > to use accessors rather than member lookup. > > * c-common.h (class substring_loc): Move to substring > > -locations.h, > > replacing with a forward decl. > > (c_get_substring_location): New decl. > > * c-format.c: Include "substring-locations.h". > > (format_warning_va): Move to substring-locations.c. > > (format_warning_at_substring): Likewise. > > > > gcc/c/ChangeLog: > > * c-lang.c (LANG_HOOKS_GET_SUBSTRING_LOCATION): Use > > c_get_substring_location for this new langhook. > > > > gcc/testsuite/ChangeLog: > > * gcc.dg/plugin/diagnostic_plugin_test_string_literals.c: > > Include > > "substring-locations.h". > > --- > > gcc/Makefile.in| 1 + > > gcc/c-family/c-common.c| 23 +-- > > gcc/c-family/c-common.h| 32 +--- > > gcc/c-family/c-format.c| 157 +--- > > - > > gcc/c/c-lang.c | 3 + > > gcc/langhooks-def.h| 8 +- > > gcc/langhooks.c| 8 + > > gcc/langhooks.h| 9 + > > gcc/substring-locations.c | 195 > > + > > gcc/substring-locations.h | 67 +++ > > .../diagnostic_plugin_test_string_literals.c |
Re: [PATCH] Move class substring_loc from c-family into gcc
I've successfully tested the patch below by incorporating it into
the -Wformat-length pass I've been working on. I'd like to submit
the latest and hopefully close to final version of my work for
review without duplicating this code and it might be helpful if
it was possible to build my latest patch without having to find
and install this one first. Could someone review and approve
David's patch?
Thanks
Martin
On 08/25/2016 10:09 AM, David Malcolm wrote:
This patch is intended to help Martin's new middle-end sprintf format
warning.
It moves most of the on-demand locations-within-strings
code in c-family into gcc, into a new substring-locations.c file to
go with substring-locations.h: class substring_loc, representing
a source caret and source range within a STRING_CST, along with
format_warning for handling emitting a warning relating to it.
The actual work of reconstructing the source locations within
a string seems inherently frontend-specific, so the patch introduces a
langhook to do this, implementing it for C using the existing code
(and thus hiding the details of string-concatenation as being
specific to the c-family). Attempts to get substring location
from other frontends will fail, and the format_warning_* calls
handle such failures gracefully by simply using the location of
the string as a whole for the warning.
I've tested it with Martin's gimple-ssa-sprintf patch, and I'm able to
emit carets and underlines in the correct places in C code from the
middle end with this approach (patch to follow).
Successfully bootstrapped on x86_64-pc-linux-gnu.
OK for trunk?
gcc/ChangeLog:
* Makefile.in (OBJS): Add substring-locations.o.
* langhooks-def.h (class substring_loc): New forward decl.
(lhd_get_substring_location): New decl.
(LANG_HOOKS_GET_SUBSTRING_LOCATION): New macro.
(LANG_HOOKS_INITIALIZER): Add LANG_HOOKS_GET_SUBSTRING_LOCATION.
* langhooks.c (lhd_get_substring_location): New function.
* langhooks.h (class substring_loc): New forward decl.
(struct lang_hooks): Add field get_substring_location.
* substring-locations.c: New file, taking definition of
format_warning_va and format_warning_at_substring from
c-family/c-format.c, making them non-static.
* substring-locations.h: Include .
(class substring_loc): Move class here from c-family/c-common.h.
Add comments.
(format_warning_va): New decl.
(format_warning_at_substring): New decl.
(get_source_location_for_substring): Add comment.
gcc/c-family/ChangeLog:
* c-common.c (get_cpp_ttype_from_string_type): Handle being passed
a POINTER_TYPE.
(substring_loc::get_location): Move to substring-locations.c,
keeping implementation as...
(c_get_substring_location): New function, from the above, reworked
to use accessors rather than member lookup.
* c-common.h (class substring_loc): Move to substring-locations.h,
replacing with a forward decl.
(c_get_substring_location): New decl.
* c-format.c: Include "substring-locations.h".
(format_warning_va): Move to substring-locations.c.
(format_warning_at_substring): Likewise.
gcc/c/ChangeLog:
* c-lang.c (LANG_HOOKS_GET_SUBSTRING_LOCATION): Use
c_get_substring_location for this new langhook.
gcc/testsuite/ChangeLog:
* gcc.dg/plugin/diagnostic_plugin_test_string_literals.c: Include
"substring-locations.h".
---
gcc/Makefile.in| 1 +
gcc/c-family/c-common.c| 23 +--
gcc/c-family/c-common.h| 32 +---
gcc/c-family/c-format.c| 157 +
gcc/c/c-lang.c | 3 +
gcc/langhooks-def.h| 8 +-
gcc/langhooks.c| 8 +
gcc/langhooks.h| 9 +
gcc/substring-locations.c | 195 +
gcc/substring-locations.h | 67 +++
.../diagnostic_plugin_test_string_literals.c | 1 +
11 files changed, 308 insertions(+), 196 deletions(-)
create mode 100644 gcc/substring-locations.c
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 1b7464b..769efcf 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1444,6 +1444,7 @@ OBJS = \
store-motion.o \
streamer-hooks.o \
stringpool.o \
+ substring-locations.o \
target-globals.o \
targhooks.o \
timevar.o \
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..f3e44c2 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -1122,6 +1122,9 @@ static enum cpp_ttype
get_cpp_ttype_from_string_type (tree string_type)
{
gcc_assert (string_type);
+ if (TREE_CODE (string_type)
