Re: [PATCH] enhance -Wrestrict for sprintf %s arguments
On 07/19/2017 10:10 AM, Martin Sebor wrote: > On 07/19/2017 12:42 AM, Jeff Law wrote: >> On 07/02/2017 02:00 PM, Martin Sebor wrote: >>> The attached patch enhances the -Wrestrict warning to detect more >>> than just trivial instances of overlapping copying by sprintf and >>> related functions. >>> >>> The meat of the patch is relatively simple but because it introduces >>> dependencies between existing classes in the sprintf pass I had to >>> move the class definitions around. That makes the changes look more >>> extensive than they really are. >>> >>> The enhancement works by first determining the base object (or >>> pointer) from the destination of the sprintf call, the constant >>> offset into the object, and its size. For each %s argument, it >>> then computes same information. If it determines that overlap >>> between the two is possible it stores the information for the >>> directive argument (including the size of the argument) for later >>> processing. After the whole call/format string has been processed, >>> the patch then iterates over the stored directives and their >>> arguments and compares the size/length of the argument against >>> the function's overall output. If they overlap it issues >>> a warning. >>> >>> Tested on x86_64-linux. >>> >>> -Wrestrict is not currently included in either -Wextra or -Wall >>> and this patch doesn't change it even though there have been >>> requests to add it to one of these two options. I'd like to do >>> that as a separate step. >> Yea, I think separate step is wise. >> >>> >>> As the next step I'd also like to extend a limited form of the >>> -Wrestrict enhancement to other restrict-qualified built-ins (like >>> strcpy), and ultimately also to user-defined functions that make >>> use of restrict. I think this might perhaps best be done in >>> a separate pass where the computed pointer information can be >>> cached to avoid recomputing it for each call, but I don't expect >>> to be able to have the new pass (or whatever form the enhancement >>> might end up taking) to also handle sprintf (at least not with >>> the same accuracy it does now) because the sprintf data for each >>> format directive are not available outside the sprintf pass. >> Seems reasonable. Actual implementation will tell us for sure :-) >> >> >>> >>> Martin >>> >>> gcc-35503.diff >>> >>> >>> PR tree-optimization/35503 - Warning about restricted pointers? >>> >>> gcc/c-family/ChangeLog: >>> >>> PR tree-optimization/35503 >>> * gcc/c-family/c-common.c (check_function_restrict): Avoid >>> diagnosing >>> sprintf et al. unless both -Wformat-overflow and -Wformat-truncation >>> are disabled. >>> >>> gcc/ChangeLog: >>> >>> PR tree-optimization/35503 >>> * gimple-ssa-sprintf.c (format_result::alias_info): New struct. >>> (directive::argno): New member. >>> (format_result::aliases, format_result::alias_count): New data >>> members. >>> (format_result::append_alias): New member function. >>> (fmtresult::dst_offset): New data member. >>> (pass_sprintf_length::call_info::dst_origin): New data member. >>> (pass_sprintf_length::call_info::dst_field, dst_offset): Same. >>> (char_type_p, array_elt_at_offset, field_at_offset): New functions. >>> (get_origin_and_offset): Same. >>> (format_string): Call it. >>> (format_directive): Call append_alias and set directive argument >>> number. >>> (pass_sprintf_length::compute_format_length): Diagnose arguments >>> that overlap the destination buffer. >>> (pass_sprintf_length::handle_gimple_call): Initialize new members. >>> gcc/testsuite/ChangeLog: >>> >>> PR tree-optimization/35503 >>> * gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test. >> I'm OK with the general concept of enhancing the warning. The big >> question I have is whether or not we'd be better off using the alias >> oracle here rather than what appears to be rolling our own data >> structures and analysis routines to describe memory objects and their >> potential alias relationship. >> >> See tree-ssa-alias.h. In particular you're looking for ao_ref. You may >> also be intersted in the points-to solutions. Would using that >> infrastructure make sense? > > This patch make only limited use of the alias analysis which > is in the current form overly broad (i.e., it answers the "may > alias" question, not the "must alias" one). That makes it > suitable for throttling optimization but not so much to trigger > warnings. The other challenge is that the information the > sprintf pass needs to detect overlaps is being computed in > stages as each directive is being processed, and then again > when the whole format string has been processed and the size > of the full output is known. I thought we had a must-alias query as well. Though it may not be properly named :-)Hmm, perhaps not, DSE uses stmt_kills_ref_p which seems to do most of the necessary checks inline. > > That said, it certainly
Re: [PATCH] enhance -Wrestrict for sprintf %s arguments
Ping: Jeff, please see my reply below. On 07/19/2017 10:10 AM, Martin Sebor wrote: On 07/19/2017 12:42 AM, Jeff Law wrote: On 07/02/2017 02:00 PM, Martin Sebor wrote: The attached patch enhances the -Wrestrict warning to detect more than just trivial instances of overlapping copying by sprintf and related functions. The meat of the patch is relatively simple but because it introduces dependencies between existing classes in the sprintf pass I had to move the class definitions around. That makes the changes look more extensive than they really are. The enhancement works by first determining the base object (or pointer) from the destination of the sprintf call, the constant offset into the object, and its size. For each %s argument, it then computes same information. If it determines that overlap between the two is possible it stores the information for the directive argument (including the size of the argument) for later processing. After the whole call/format string has been processed, the patch then iterates over the stored directives and their arguments and compares the size/length of the argument against the function's overall output. If they overlap it issues a warning. Tested on x86_64-linux. -Wrestrict is not currently included in either -Wextra or -Wall and this patch doesn't change it even though there have been requests to add it to one of these two options. I'd like to do that as a separate step. Yea, I think separate step is wise. As the next step I'd also like to extend a limited form of the -Wrestrict enhancement to other restrict-qualified built-ins (like strcpy), and ultimately also to user-defined functions that make use of restrict. I think this might perhaps best be done in a separate pass where the computed pointer information can be cached to avoid recomputing it for each call, but I don't expect to be able to have the new pass (or whatever form the enhancement might end up taking) to also handle sprintf (at least not with the same accuracy it does now) because the sprintf data for each format directive are not available outside the sprintf pass. Seems reasonable. Actual implementation will tell us for sure :-) Martin gcc-35503.diff PR tree-optimization/35503 - Warning about restricted pointers? gcc/c-family/ChangeLog: PR tree-optimization/35503 * gcc/c-family/c-common.c (check_function_restrict): Avoid diagnosing sprintf et al. unless both -Wformat-overflow and -Wformat-truncation are disabled. gcc/ChangeLog: PR tree-optimization/35503 * gimple-ssa-sprintf.c (format_result::alias_info): New struct. (directive::argno): New member. (format_result::aliases, format_result::alias_count): New data members. (format_result::append_alias): New member function. (fmtresult::dst_offset): New data member. (pass_sprintf_length::call_info::dst_origin): New data member. (pass_sprintf_length::call_info::dst_field, dst_offset): Same. (char_type_p, array_elt_at_offset, field_at_offset): New functions. (get_origin_and_offset): Same. (format_string): Call it. (format_directive): Call append_alias and set directive argument number. (pass_sprintf_length::compute_format_length): Diagnose arguments that overlap the destination buffer. (pass_sprintf_length::handle_gimple_call): Initialize new members. gcc/testsuite/ChangeLog: PR tree-optimization/35503 * gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test. I'm OK with the general concept of enhancing the warning. The big question I have is whether or not we'd be better off using the alias oracle here rather than what appears to be rolling our own data structures and analysis routines to describe memory objects and their potential alias relationship. See tree-ssa-alias.h. In particular you're looking for ao_ref. You may also be intersted in the points-to solutions. Would using that infrastructure make sense? This patch make only limited use of the alias analysis which is in the current form overly broad (i.e., it answers the "may alias" question, not the "must alias" one). That makes it suitable for throttling optimization but not so much to trigger warnings. The other challenge is that the information the sprintf pass needs to detect overlaps is being computed in stages as each directive is being processed, and then again when the whole format string has been processed and the size of the full output is known. That said, it certainly makes sense to use the alias analysis to its full potential when possible. In the next step (posted for review a few days ago: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00917.html) I take full advantage of it in addition to enhancing it to also answer the "must alias" question. I've been thinking I should revisit the sprintf solution with this enhancement (and some others I'm still working on(*)) is in place. To give the feature broader exposure I propose to commit the initial patch as is,
Re: [PATCH] enhance -Wrestrict for sprintf %s arguments
On 07/19/2017 12:42 AM, Jeff Law wrote: On 07/02/2017 02:00 PM, Martin Sebor wrote: The attached patch enhances the -Wrestrict warning to detect more than just trivial instances of overlapping copying by sprintf and related functions. The meat of the patch is relatively simple but because it introduces dependencies between existing classes in the sprintf pass I had to move the class definitions around. That makes the changes look more extensive than they really are. The enhancement works by first determining the base object (or pointer) from the destination of the sprintf call, the constant offset into the object, and its size. For each %s argument, it then computes same information. If it determines that overlap between the two is possible it stores the information for the directive argument (including the size of the argument) for later processing. After the whole call/format string has been processed, the patch then iterates over the stored directives and their arguments and compares the size/length of the argument against the function's overall output. If they overlap it issues a warning. Tested on x86_64-linux. -Wrestrict is not currently included in either -Wextra or -Wall and this patch doesn't change it even though there have been requests to add it to one of these two options. I'd like to do that as a separate step. Yea, I think separate step is wise. As the next step I'd also like to extend a limited form of the -Wrestrict enhancement to other restrict-qualified built-ins (like strcpy), and ultimately also to user-defined functions that make use of restrict. I think this might perhaps best be done in a separate pass where the computed pointer information can be cached to avoid recomputing it for each call, but I don't expect to be able to have the new pass (or whatever form the enhancement might end up taking) to also handle sprintf (at least not with the same accuracy it does now) because the sprintf data for each format directive are not available outside the sprintf pass. Seems reasonable. Actual implementation will tell us for sure :-) Martin gcc-35503.diff PR tree-optimization/35503 - Warning about restricted pointers? gcc/c-family/ChangeLog: PR tree-optimization/35503 * gcc/c-family/c-common.c (check_function_restrict): Avoid diagnosing sprintf et al. unless both -Wformat-overflow and -Wformat-truncation are disabled. gcc/ChangeLog: PR tree-optimization/35503 * gimple-ssa-sprintf.c (format_result::alias_info): New struct. (directive::argno): New member. (format_result::aliases, format_result::alias_count): New data members. (format_result::append_alias): New member function. (fmtresult::dst_offset): New data member. (pass_sprintf_length::call_info::dst_origin): New data member. (pass_sprintf_length::call_info::dst_field, dst_offset): Same. (char_type_p, array_elt_at_offset, field_at_offset): New functions. (get_origin_and_offset): Same. (format_string): Call it. (format_directive): Call append_alias and set directive argument number. (pass_sprintf_length::compute_format_length): Diagnose arguments that overlap the destination buffer. (pass_sprintf_length::handle_gimple_call): Initialize new members. gcc/testsuite/ChangeLog: PR tree-optimization/35503 * gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test. I'm OK with the general concept of enhancing the warning. The big question I have is whether or not we'd be better off using the alias oracle here rather than what appears to be rolling our own data structures and analysis routines to describe memory objects and their potential alias relationship. See tree-ssa-alias.h. In particular you're looking for ao_ref. You may also be intersted in the points-to solutions. Would using that infrastructure make sense? This patch make only limited use of the alias analysis which is in the current form overly broad (i.e., it answers the "may alias" question, not the "must alias" one). That makes it suitable for throttling optimization but not so much to trigger warnings. The other challenge is that the information the sprintf pass needs to detect overlaps is being computed in stages as each directive is being processed, and then again when the whole format string has been processed and the size of the full output is known. That said, it certainly makes sense to use the alias analysis to its full potential when possible. In the next step (posted for review a few days ago: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00917.html) I take full advantage of it in addition to enhancing it to also answer the "must alias" question. I've been thinking I should revisit the sprintf solution with this enhancement (and some others I'm still working on(*)) is in place. To give the feature broader exposure I propose to commit the initial patch as
Re: [PATCH] enhance -Wrestrict for sprintf %s arguments
On 07/02/2017 02:00 PM, Martin Sebor wrote: > The attached patch enhances the -Wrestrict warning to detect more > than just trivial instances of overlapping copying by sprintf and > related functions. > > The meat of the patch is relatively simple but because it introduces > dependencies between existing classes in the sprintf pass I had to > move the class definitions around. That makes the changes look more > extensive than they really are. > > The enhancement works by first determining the base object (or > pointer) from the destination of the sprintf call, the constant > offset into the object, and its size. For each %s argument, it > then computes same information. If it determines that overlap > between the two is possible it stores the information for the > directive argument (including the size of the argument) for later > processing. After the whole call/format string has been processed, > the patch then iterates over the stored directives and their > arguments and compares the size/length of the argument against > the function's overall output. If they overlap it issues > a warning. > > Tested on x86_64-linux. > > -Wrestrict is not currently included in either -Wextra or -Wall > and this patch doesn't change it even though there have been > requests to add it to one of these two options. I'd like to do > that as a separate step. Yea, I think separate step is wise. > > As the next step I'd also like to extend a limited form of the > -Wrestrict enhancement to other restrict-qualified built-ins (like > strcpy), and ultimately also to user-defined functions that make > use of restrict. I think this might perhaps best be done in > a separate pass where the computed pointer information can be > cached to avoid recomputing it for each call, but I don't expect > to be able to have the new pass (or whatever form the enhancement > might end up taking) to also handle sprintf (at least not with > the same accuracy it does now) because the sprintf data for each > format directive are not available outside the sprintf pass. Seems reasonable. Actual implementation will tell us for sure :-) > > Martin > > gcc-35503.diff > > > PR tree-optimization/35503 - Warning about restricted pointers? > > gcc/c-family/ChangeLog: > > PR tree-optimization/35503 > * gcc/c-family/c-common.c (check_function_restrict): Avoid diagnosing > sprintf et al. unless both -Wformat-overflow and -Wformat-truncation > are disabled. > > gcc/ChangeLog: > > PR tree-optimization/35503 > * gimple-ssa-sprintf.c (format_result::alias_info): New struct. > (directive::argno): New member. > (format_result::aliases, format_result::alias_count): New data members. > (format_result::append_alias): New member function. > (fmtresult::dst_offset): New data member. > (pass_sprintf_length::call_info::dst_origin): New data member. > (pass_sprintf_length::call_info::dst_field, dst_offset): Same. > (char_type_p, array_elt_at_offset, field_at_offset): New functions. > (get_origin_and_offset): Same. > (format_string): Call it. > (format_directive): Call append_alias and set directive argument > number. > (pass_sprintf_length::compute_format_length): Diagnose arguments > that overlap the destination buffer. > (pass_sprintf_length::handle_gimple_call): Initialize new members. > gcc/testsuite/ChangeLog: > > PR tree-optimization/35503 > * gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test. I'm OK with the general concept of enhancing the warning. The big question I have is whether or not we'd be better off using the alias oracle here rather than what appears to be rolling our own data structures and analysis routines to describe memory objects and their potential alias relationship. See tree-ssa-alias.h. In particular you're looking for ao_ref. You may also be intersted in the points-to solutions. Would using that infrastructure make sense? jeff
[PING #2] [PATCH] enhance -Wrestrict for sprintf %s arguments
Ping #2: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00036.html On 07/02/2017 02:00 PM, Martin Sebor wrote: The attached patch enhances the -Wrestrict warning to detect more than just trivial instances of overlapping copying by sprintf and related functions. The meat of the patch is relatively simple but because it introduces dependencies between existing classes in the sprintf pass I had to move the class definitions around. That makes the changes look more extensive than they really are. The enhancement works by first determining the base object (or pointer) from the destination of the sprintf call, the constant offset into the object, and its size. For each %s argument, it then computes same information. If it determines that overlap between the two is possible it stores the information for the directive argument (including the size of the argument) for later processing. After the whole call/format string has been processed, the patch then iterates over the stored directives and their arguments and compares the size/length of the argument against the function's overall output. If they overlap it issues a warning. Tested on x86_64-linux. -Wrestrict is not currently included in either -Wextra or -Wall and this patch doesn't change it even though there have been requests to add it to one of these two options. I'd like to do that as a separate step. As the next step I'd also like to extend a limited form of the -Wrestrict enhancement to other restrict-qualified built-ins (like strcpy), and ultimately also to user-defined functions that make use of restrict. I think this might perhaps best be done in a separate pass where the computed pointer information can be cached to avoid recomputing it for each call, but I don't expect to be able to have the new pass (or whatever form the enhancement might end up taking) to also handle sprintf (at least not with the same accuracy it does now) because the sprintf data for each format directive are not available outside the sprintf pass. Martin
[PING] [PATCH] enhance -Wrestrict for sprintf %s arguments
Ping: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00036.html On 07/02/2017 02:00 PM, Martin Sebor wrote: The attached patch enhances the -Wrestrict warning to detect more than just trivial instances of overlapping copying by sprintf and related functions. The meat of the patch is relatively simple but because it introduces dependencies between existing classes in the sprintf pass I had to move the class definitions around. That makes the changes look more extensive than they really are. The enhancement works by first determining the base object (or pointer) from the destination of the sprintf call, the constant offset into the object, and its size. For each %s argument, it then computes same information. If it determines that overlap between the two is possible it stores the information for the directive argument (including the size of the argument) for later processing. After the whole call/format string has been processed, the patch then iterates over the stored directives and their arguments and compares the size/length of the argument against the function's overall output. If they overlap it issues a warning. Tested on x86_64-linux. -Wrestrict is not currently included in either -Wextra or -Wall and this patch doesn't change it even though there have been requests to add it to one of these two options. I'd like to do that as a separate step. As the next step I'd also like to extend a limited form of the -Wrestrict enhancement to other restrict-qualified built-ins (like strcpy), and ultimately also to user-defined functions that make use of restrict. I think this might perhaps best be done in a separate pass where the computed pointer information can be cached to avoid recomputing it for each call, but I don't expect to be able to have the new pass (or whatever form the enhancement might end up taking) to also handle sprintf (at least not with the same accuracy it does now) because the sprintf data for each format directive are not available outside the sprintf pass. Martin
[PATCH] enhance -Wrestrict for sprintf %s arguments
The attached patch enhances the -Wrestrict warning to detect more
than just trivial instances of overlapping copying by sprintf and
related functions.
The meat of the patch is relatively simple but because it introduces
dependencies between existing classes in the sprintf pass I had to
move the class definitions around. That makes the changes look more
extensive than they really are.
The enhancement works by first determining the base object (or
pointer) from the destination of the sprintf call, the constant
offset into the object, and its size. For each %s argument, it
then computes same information. If it determines that overlap
between the two is possible it stores the information for the
directive argument (including the size of the argument) for later
processing. After the whole call/format string has been processed,
the patch then iterates over the stored directives and their
arguments and compares the size/length of the argument against
the function's overall output. If they overlap it issues
a warning.
Tested on x86_64-linux.
-Wrestrict is not currently included in either -Wextra or -Wall
and this patch doesn't change it even though there have been
requests to add it to one of these two options. I'd like to do
that as a separate step.
As the next step I'd also like to extend a limited form of the
-Wrestrict enhancement to other restrict-qualified built-ins (like
strcpy), and ultimately also to user-defined functions that make
use of restrict. I think this might perhaps best be done in
a separate pass where the computed pointer information can be
cached to avoid recomputing it for each call, but I don't expect
to be able to have the new pass (or whatever form the enhancement
might end up taking) to also handle sprintf (at least not with
the same accuracy it does now) because the sprintf data for each
format directive are not available outside the sprintf pass.
Martin
PR tree-optimization/35503 - Warning about restricted pointers?
gcc/c-family/ChangeLog:
PR tree-optimization/35503
* gcc/c-family/c-common.c (check_function_restrict): Avoid diagnosing
sprintf et al. unless both -Wformat-overflow and -Wformat-truncation
are disabled.
gcc/ChangeLog:
PR tree-optimization/35503
* gimple-ssa-sprintf.c (format_result::alias_info): New struct.
(directive::argno): New member.
(format_result::aliases, format_result::alias_count): New data members.
(format_result::append_alias): New member function.
(fmtresult::dst_offset): New data member.
(pass_sprintf_length::call_info::dst_origin): New data member.
(pass_sprintf_length::call_info::dst_field, dst_offset): Same.
(char_type_p, array_elt_at_offset, field_at_offset): New functions.
(get_origin_and_offset): Same.
(format_string): Call it.
(format_directive): Call append_alias and set directive argument
number.
(pass_sprintf_length::compute_format_length): Diagnose arguments
that overlap the destination buffer.
(pass_sprintf_length::handle_gimple_call): Initialize new members.
gcc/testsuite/ChangeLog:
PR tree-optimization/35503
* gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 4395e51..406ebc4 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5266,6 +5266,24 @@ check_function_restrict (const_tree fndecl, const_tree fntype,
else
parms = TYPE_ARG_TYPES (fntype);
+ if (DECL_BUILT_IN (fndecl)
+ && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+{
+ switch (DECL_FUNCTION_CODE (fndecl))
+ {
+ case BUILT_IN_SPRINTF:
+ case BUILT_IN_SPRINTF_CHK:
+ case BUILT_IN_SNPRINTF:
+ case BUILT_IN_SNPRINTF_CHK:
+ /* These are handled in gimple-ssa-sprintf.c. */
+ if (warn_format_overflow || warn_format_trunc)
+ return;
+
+ default:
+ break;
+ }
+}
+
for (i = 0; i < nargs; i++)
TREE_VISITED (argarray[i]) = 0;
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index f43778b..8113353 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -67,6 +67,7 @@ along with GCC; see the file COPYING3. If not see
#include "cfgloop.h"
#include "intl.h"
#include "langhooks.h"
+#include "tree-ssa-alias.h"
#include "builtins.h"
#include "stor-layout.h"
@@ -78,6 +79,7 @@ along with GCC; see the file COPYING3. If not see
#include "input.h"
#include "toplev.h"
#include "substring-locations.h"
+#include "gcc-rich-location.h"
#include "diagnostic.h"
/* The likely worst case value of MB_LEN_MAX for the target, large enough
@@ -93,6 +95,30 @@ along with GCC; see the file COPYING3. If not see
namespace {
+/* Return the value of INT_MIN for the target. */
+
+static inline HOST_WIDE_INT
+target_int_min ()
+{
+ return tree_to_shwi (TYPE_MIN_VALUE (integer_type_node));
+}
+
+/* Return the value of INT_MAX for the target. */
+
+static inline unsigned HOST_WIDE_INT
+target_int_max ()
+{
+ return tree_to_uhwi (TYPE_MAX_VALUE (integer_type_node));
+}
+
+/* Return the value of
