Hi, Jeff, thanks a lot for your review and comments.
I have addressed your comments,updated the patch, retested on both
aarch64 and x86.
The major changes in this version compared to the previous version are:
1. in routine “expand_builtin_memcmp”:
* move the inlining transformation AFTER the warning is issues for
-Wstringop-overflow;
* only apply inlining when there is No warning is issued.
2. in the testsuite, add a new testcase strcmpopt_6.c for this case.
3. update comments to:
* capitalize the first word.
* capitalize all the arguments.
NOTE, the routine ”expand_builtin_strcmp” and “expand_builtin_strncmp" are not
changed.
the reason is: there is NO overflow checking for these two routines currently.
if we need overflow checking for these two routines, I think that a separate
patch is needed.
if this is needed, let me know, I can work on this separate patch for issuing
warning for strcmp/strncmp when
-Wstringop-overflow is specified.
The new patch is as following, please take a look at it.
thanks.
Qing
gcc/ChangeLog
+2018-07-02 Qing Zhao <qing.z...@oracle.com>
+
+ PR middle-end/78809
+ * builtins.c (expand_builtin_memcmp): Inline the calls first
+ when result_eq is false.
+ (expand_builtin_strcmp): Inline the calls first.
+ (expand_builtin_strncmp): Likewise.
+ (inline_string_cmp): New routine. Expand a string compare
+ call by using a sequence of char comparison.
+ (inline_expand_builtin_string_cmp): New routine. Inline expansion
+ a call to str(n)cmp/memcmp.
+ * doc/invoke.texi (--param builtin-string-cmp-inline-length): New
option.
+ * params.def (BUILTIN_STRING_CMP_INLINE_LENGTH): New.
+
gcc/testsuite/ChangeLog
+2018-07-02 Qing Zhao <qing.z...@oracle.com>
+
+ PR middle-end/78809
+ * gcc.dg/strcmpopt_5.c: New test.
+ * gcc.dg/strcmpopt_6.c: New test.
+
0001-3nd-Patch-for-PR78009.patch
Description: Binary data
> On Jun 28, 2018, at 12:10 AM, Jeff Law <l...@redhat.com> wrote: > So I still need to dig into this patch. But I wanted to raise an > potential issue and get yours and Martin's thoughts on it. > > Martin (and others) have been working hard to improve GCC's ability to > give good diagnostics for various problems with calls to mem* and str* > functions (buffer overflows, restrict issues, etc). > > One of the problems Martin has identified is early conversion of these > calls into inlined direct operations. If those conversions happen prior > to the analysis for warnings we obviously can't issue any relevant warnings. > > Please capitalize the first word in sentences like this. This nit > appears in most of your comments. > > > So I believe you do inline expansion here prior to the checks for > Wstringop_overflow. I think you can safely move this code to after the > warn_stringop_overflow checks. Though you may want to make this code > conditional on both calls to check_access returning true and avoiding > your transformation if either or both calls return false. > > Alternately you'd need to verify that inline_expand_builtin_string_cmp > always returns false for cases which are going to generate a warning. > But that seems a bit tougher to maintain over time if we were to add > more warnings to this code. > > When referring to arguments in comments, please capitalize them. ie > VAR_STR, CONST_STR, etc. > > > But leave them as lower case in code fragments like this. > > > So this generally looks pretty good. THe biggest technical concern is > making sure we're doing the right thing WRT issuing warnings. You can > tackle that problem by deferring inlining to a later point after > warnings have been issued or by verifying that your routines do not > inline in cases where warnings will be issued. It may be worth adding > testcases for these issues. > > There's a large number of comments that need capitalization fixes. > > Given there was no measured runtime performance impact, but slight > improvements on codesize for values <= 3, let's go ahead with that as > the default. > > Can you address the issues above and repost for final review? > > Thanks, > jeff
