Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/25/19 1:42 PM, Martin Sebor wrote:
> On 7/24/19 8:39 PM, Jeff Law wrote:
>> On 7/24/19 8:17 PM, Martin Sebor wrote:
Committed in r273783 after retesting and including a test for
the warning that I had left out of the patch I posted here.
Martin
PS I suspect some of the tests I added might need tweaking on
big-endian systems. I'll deal with them tomorrow.
>>>
>>> And maybe also strictly aligned targets. A sparc-solaris2.11 cross
>>> shows these failures. It looks like it doesn't like something about
>>> some of the 64 bit stores in the tests.
>>>
>>> FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0
>>> FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized
>>> "_not_eliminated_" 0
>>> FAIL: gcc.dg/strlenopt-71.c (test for excess errors)
>>> FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0
>>> FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized
>>> "_not_eliminated_" 0
>>
>>
>> msp430-elf failures:
>>
>>
>>> New tests that FAIL (5 tests):
>>>
>>> msp430-sim: gcc.dg/strlenopt-70.c (test for excess errors)
>>> msp430-sim: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized
>>> "_not_eliminated_" 0
>
> This one is due to bugs in the endian macros the test defines
> to try to handle both big and little-endian. I've fixed those/
>
>>> msp430-sim: gcc.dg/strlenopt-71.c (test for excess errors)
>
> Same here, though this is a runtime test so it will also fail
> to link outside of an emulator. (Changing the test harness to
> report these tests as UNSUPPORTED instead would avoid this sort
> of ambiguity.)
Well, the "test for excess errors" should be an indicator that something
went wrong before trying to execute. Typically these are compile-time
warnings/errors or occasionally a link time error due to an undefined
symbol.
I don't have access to the tester, so I can't really look at it until I
return though.
>
>>> msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized
>>> "_not_eliminated_" 0
>>> msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized
>>> "strlen" 0
>
> This failure is due to to a combination of the absence of early
> store merging in pr83821. The former prevents the stores below
>
> char a[4];
> a[0] = 'X';
> a[1] = 0;
> a[2] = a[3] = 0;
>
> from turning into:
>
> MEM [(char * {ref-all})] = 49;
>
> pr83821 causes the strlen pass to invalidate strlen information
> when it sees the assignment to a[2] (or beyond). I need to dust
> off and resubmit my patch for that from last year.
>
> Until that patch is approved I have disabled the test on strictly
> aligned targets.
>
> The failure in gcc.dg/Wstringop-overflow-14.c on visium-elf was
> because of a difference between strictly aligned targets and
> the rest, which triggers different warnings between the two sets.
> I disabled the unexpected warning and the test passes.
>
> I've just committed r273812 and r273814 with these changes.
No problem. When I'm back from PTO I'll review state in the tester and
pass along anything else related.
Jeff
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/24/19 8:39 PM, Jeff Law wrote:
On 7/24/19 8:17 PM, Martin Sebor wrote:
Committed in r273783 after retesting and including a test for
the warning that I had left out of the patch I posted here.
Martin
PS I suspect some of the tests I added might need tweaking on
big-endian systems. I'll deal with them tomorrow.
And maybe also strictly aligned targets. A sparc-solaris2.11 cross
shows these failures. It looks like it doesn't like something about
some of the 64 bit stores in the tests.
FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0
FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized
"_not_eliminated_" 0
FAIL: gcc.dg/strlenopt-71.c (test for excess errors)
FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0
FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized
"_not_eliminated_" 0
msp430-elf failures:
New tests that FAIL (5 tests):
msp430-sim: gcc.dg/strlenopt-70.c (test for excess errors)
msp430-sim: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized
"_not_eliminated_" 0
This one is due to bugs in the endian macros the test defines
to try to handle both big and little-endian. I've fixed those/
msp430-sim: gcc.dg/strlenopt-71.c (test for excess errors)
Same here, though this is a runtime test so it will also fail
to link outside of an emulator. (Changing the test harness to
report these tests as UNSUPPORTED instead would avoid this sort
of ambiguity.)
msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized
"_not_eliminated_" 0
msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0
This failure is due to to a combination of the absence of early
store merging in pr83821. The former prevents the stores below
char a[4];
a[0] = 'X';
a[1] = 0;
a[2] = a[3] = 0;
from turning into:
MEM [(char * {ref-all})] = 49;
pr83821 causes the strlen pass to invalidate strlen information
when it sees the assignment to a[2] (or beyond). I need to dust
off and resubmit my patch for that from last year.
Until that patch is approved I have disabled the test on strictly
aligned targets.
The failure in gcc.dg/Wstringop-overflow-14.c on visium-elf was
because of a difference between strictly aligned targets and
the rest, which triggers different warnings between the two sets.
I disabled the unexpected warning and the test passes.
I've just committed r273812 and r273814 with these changes.
Martin
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/24/19 8:17 PM, Martin Sebor wrote: >> Committed in r273783 after retesting and including a test for >> the warning that I had left out of the patch I posted here. >> >> Martin >> >> PS I suspect some of the tests I added might need tweaking on >> big-endian systems. I'll deal with them tomorrow. > > And maybe also strictly aligned targets. A sparc-solaris2.11 cross > shows these failures. It looks like it doesn't like something about > some of the 64 bit stores in the tests. > > FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0 > FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > FAIL: gcc.dg/strlenopt-71.c (test for excess errors) > FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0 > FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized > "_not_eliminated_" 0 visium-elf: > New tests that FAIL (16 tests): > > visium-sim: gcc.dg/Wstringop-overflow-14.c (test for warnings, line 24) > visium-sim: gcc.dg/Wstringop-overflow-14.c (test for excess errors) > visium-sim: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > visium-sim: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0 > visium-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > visium-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/24/19 8:17 PM, Martin Sebor wrote: >> Committed in r273783 after retesting and including a test for >> the warning that I had left out of the patch I posted here. >> >> Martin >> >> PS I suspect some of the tests I added might need tweaking on >> big-endian systems. I'll deal with them tomorrow. > > And maybe also strictly aligned targets. A sparc-solaris2.11 cross > shows these failures. It looks like it doesn't like something about > some of the 64 bit stores in the tests. > > FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0 > FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > FAIL: gcc.dg/strlenopt-71.c (test for excess errors) > FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0 > FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized > "_not_eliminated_" 0 msp430-elf failures: > New tests that FAIL (5 tests): > > msp430-sim: gcc.dg/strlenopt-70.c (test for excess errors) > msp430-sim: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > msp430-sim: gcc.dg/strlenopt-71.c (test for excess errors) > msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized > "_not_eliminated_" 0 > msp430-sim: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
Committed in r273783 after retesting and including a test for the warning that I had left out of the patch I posted here. Martin PS I suspect some of the tests I added might need tweaking on big-endian systems. I'll deal with them tomorrow. And maybe also strictly aligned targets. A sparc-solaris2.11 cross shows these failures. It looks like it doesn't like something about some of the 64 bit stores in the tests. FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "strlen" 0 FAIL: gcc.dg/strlenopt-70.c scan-tree-dump-times optimized "_not_eliminated_" 0 FAIL: gcc.dg/strlenopt-71.c (test for excess errors) FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "strlen" 0 FAIL: gcc.dg/strlenopt-72.c scan-tree-dump-times optimized "_not_eliminated_" 0
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/24/19 11:06 AM, Jeff Law wrote: On 7/22/19 5:17 PM, Martin Sebor wrote: Umm "store_b4_nul"? Are you really trying to save 3 characters in the name by writing it this way? :-) I'm actually saving four characters over "store_before_nul" ;-) :-) It's just a name I picked because I like it. Would you prefer to go back to the original 'cmp' instead? It doesn't get much shorter than that, or less descriptive, especially when there is no comment explaining what it's for. (FWIW, I renamed it because I found myself going back to the description of compare_nonzero_chars to remember what cmp actually meant.) Fair enough. Though "b4" reads like it belongs in a text message to me. I don't want to get nitty over this. Will s/b4/before/ work for you? If it's distracting I'll change it. If you wanted to add a comment before the variable, that would be good as well, particularly since we don't have a good name. You've got two entries in the array, but the comment reads as if there's just one element. What is the difference between the two array elements? Since handle_store deals with sequences of one or more bytes some of the optimizations it implements(*) need to consider both where the first byte of the sequence is stored and where the last one is. The first element of this array is for the first byte and the second one is for the last character. The comment a few lines down is meant to make the distinction clear but we can expand the comment above the variable even more. I think my brain locked up with the "b4". Maybe it went into a mode where I'm trying to parse texts from my teenager which is clearly incompatible with reading code. :-) That's a good enough argument to change it :) /* The offset of the last stored byte. */ unsigned HOST_WIDE_INT endoff = offset + lenrange[2] - 1; (lenrange[2] is the size of the store.) [*] E.g., the one we discussed not so long ago (PR90989) or the one that removes a store of '\0' over the terminating '\0'. I didn't see anything terribly concerning so far, but I do want to look at handle_store again after the comment is fixed so that I'm sure I'm interpreting things correctly. Does this help? /* The corresponding element is set to 1 if the first and last element, respectively, of the sequence of characters being written over the string described by SI ends before the terminating nul (if it has one), to zero if the nul is being overwritten but not beyond, or negative otherwise. */ Yea. I also note a /* */ empty comment in handle_store, you probably wanted to write something there :-) I did initially want to write something there but then I wasn't sure the conditional was quite right. I went to investigate and forgot to fix the conditional. There was an unnecessarily test there so I removed both it and the comment placeholder. OK with the nit fixes noted earlier, variable name fix and comment fixes. Committed in r273783 after retesting and including a test for the warning that I had left out of the patch I posted here. Martin PS I suspect some of the tests I added might need tweaking on big-endian systems. I'll deal with them tomorrow.
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/22/19 5:17 PM, Martin Sebor wrote: >> Umm "store_b4_nul"? Are you really trying to save 3 characters in the >> name by writing it this way? :-) > > I'm actually saving four characters over "store_before_nul" ;-) :-) > > It's just a name I picked because I like it. Would you prefer to > go back to the original 'cmp' instead? It doesn't get much shorter > than that, or less descriptive, especially when there is no comment > explaining what it's for. (FWIW, I renamed it because I found myself > going back to the description of compare_nonzero_chars to remember > what cmp actually meant.) Fair enough. Though "b4" reads like it belongs in a text message to me. I don't want to get nitty over this. Will s/b4/before/ work for you? If you wanted to add a comment before the variable, that would be good as well, particularly since we don't have a good name. > >> You've got two entries in the array, but the comment reads as if there's >> just one element. What is the difference between the two array elements? > > Since handle_store deals with sequences of one or more bytes some > of the optimizations it implements(*) need to consider both where > the first byte of the sequence is stored and where the last one is. > The first element of this array is for the first byte and the second > one is for the last character. The comment a few lines down is meant > to make the distinction clear but we can expand the comment above > the variable even more. I think my brain locked up with the "b4". Maybe it went into a mode where I'm trying to parse texts from my teenager which is clearly incompatible with reading code. :-) > > /* The offset of the last stored byte. */ > unsigned HOST_WIDE_INT endoff = offset + lenrange[2] - 1; > > (lenrange[2] is the size of the store.) > > [*] E.g., the one we discussed not so long ago (PR90989) or the one > that removes a store of '\0' over the terminating '\0'. > >> >> I didn't see anything terribly concerning so far, but I do want to look >> at handle_store again after the comment is fixed so that I'm sure I'm >> interpreting things correctly. > > Does this help? > > /* The corresponding element is set to 1 if the first and last > element, respectively, of the sequence of characters being > written over the string described by SI ends before > the terminating nul (if it has one), to zero if the nul is > being overwritten but not beyond, or negative otherwise. */ Yea. I also note a /* */ empty comment in handle_store, you probably wanted to write something there :-) OK with the nit fixes noted earlier, variable name fix and comment fixes. jeff
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/22/19 3:33 PM, Jeff Law wrote:
On 7/19/19 4:04 PM, Martin Sebor wrote:
On targets with permissive alignment requirements GCC sometimes
lowers stores of short (between two and 16 bytes), power-of-two
char sequences to single integer stores of the corresponding
width. This happens for sequences of ordinary character stores
as well as for some calls to memcpy.
However, the strlen pass is only prepared to handle character
stores and not those of wider integers. As a result, the strlen
optimization tends to get defeated in cases when it could benefit
the most: very short strings. I counted 1544 instances where
the strlen optimization was disabled in a GCC build on x86_64
due to this sort of early store merging, and over two thousand
in a build of the Linux kernel.
In addition, -Wstringop-overflow only considers calls to string
functions and is ineffective against past-the-end accesses by
these merged multibyte stores.
To improve the effectiveness of both the optimization as well
as the warning the attached patch enhances the strlen pass to
consider these wide accesses. Since the infrastructure for doing
this is already in place (strlen can compute multibyte accesses
via MEM_REFs of character arrays), the enhancement isn't very
intrusive. It relies on the native_encode_expr function to
determine the encoding of an expression and its "length".
I tested the patch on x86_64. I expect the tests the patch
adds will need some adjustment for big-endian and strictly
aligned targets.
Martin
gcc-91183.diff
PR tree-optimization/91183 - strlen of a strcpy result with a conditional
source not folded
PR tree-optimization/86688 - missing -Wstringop-overflow using a non-string
local array in strnlen with excessive bound
gcc/ChangeLog:
PR tree-optimization/91183
PR tree-optimization/86688
* builtins.c (compute_objsize): Handle MEM_REF.
* tree-ssa-strlen.c (class ssa_name_limit_t): New.
(get_min_string_length): Remove.
(count_nonzero_bytes): New function.
(handle_char_store): Rename...
(handle_store): to this. Handle multibyte stores via integer types.
(strlen_check_and_optimize_stmt): Adjust conditional and the called
function name.
gcc/testsuite/ChangeLog:
PR tree-optimization/91183
PR tree-optimization/86688
* gcc.dg/attr-nonstring-2.c: Remove xfails.
* gcc.dg/strlenopt-70.c: New test.
* gcc.dg/strlenopt-71.c: New test.
* gcc.dg/strlenopt-72.c: New test.
* gcc.dg/strlenopt-8.c: Remove xfails.
+
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 88b6bd7869e..ca1aca3ed9e 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
+
+/* If the SSA_NAME has already been "seen" return a positive value.
+ Otherwise add it to VISITED. If the SSA_NAME limit has been
+ reached, return a negative value. Otherwise return zero. */
+
+int ssa_name_limit_t::next_ssa_name (tree ssa_name)
Nit. Return type on a different line than the function's name. The
point behind that convention is to make it easier to search for a
function's definition.
+/* Determine the minimum and maximum number of leading non-zero bytes
+ in the representation of EXP and set LENRANGE[0] and LENRANGE[1]
+ to each. Set LENRANGE[2] to the total number of bytes in
+ the representation. Set *NULTREM if the representation contains
+ a zero byte, and set *ALLNUL if all the bytes are zero. Avoid
+ recursing deeper than the limits in SNLIM allow. Return true
+ on success and false otherwise. */
S/NULTREM/NULTERM/
if (si != NULL)
{
- int cmp = compare_nonzero_chars (si, offset);
- gcc_assert (offset == 0 || cmp >= 0);
- if (storing_all_zeros_p && cmp == 0 && si->full_string_p)
+ /* Set to 1 if the string described by SI is being written into
+before the terminating nul (if it has one), to zero if the nul
+is being overwritten but not beyond, or negative otherwise. */
+ int store_b4_nul[2];
Umm "store_b4_nul"? Are you really trying to save 3 characters in the
name by writing it this way? :-)
I'm actually saving four characters over "store_before_nul" ;-)
It's just a name I picked because I like it. Would you prefer to
go back to the original 'cmp' instead? It doesn't get much shorter
than that, or less descriptive, especially when there is no comment
explaining what it's for. (FWIW, I renamed it because I found myself
going back to the description of compare_nonzero_chars to remember
what cmp actually meant.)
You've got two entries in the array, but the comment reads as if there's
just one element. What is the difference between the two array elements?
Since handle_store deals with sequences of one or more bytes some
of the optimizations it implements(*) need to consider both where
the first byte of the sequence is
Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On 7/19/19 4:04 PM, Martin Sebor wrote:
> On targets with permissive alignment requirements GCC sometimes
> lowers stores of short (between two and 16 bytes), power-of-two
> char sequences to single integer stores of the corresponding
> width. This happens for sequences of ordinary character stores
> as well as for some calls to memcpy.
>
> However, the strlen pass is only prepared to handle character
> stores and not those of wider integers. As a result, the strlen
> optimization tends to get defeated in cases when it could benefit
> the most: very short strings. I counted 1544 instances where
> the strlen optimization was disabled in a GCC build on x86_64
> due to this sort of early store merging, and over two thousand
> in a build of the Linux kernel.
>
> In addition, -Wstringop-overflow only considers calls to string
> functions and is ineffective against past-the-end accesses by
> these merged multibyte stores.
>
> To improve the effectiveness of both the optimization as well
> as the warning the attached patch enhances the strlen pass to
> consider these wide accesses. Since the infrastructure for doing
> this is already in place (strlen can compute multibyte accesses
> via MEM_REFs of character arrays), the enhancement isn't very
> intrusive. It relies on the native_encode_expr function to
> determine the encoding of an expression and its "length".
>
> I tested the patch on x86_64. I expect the tests the patch
> adds will need some adjustment for big-endian and strictly
> aligned targets.
>
> Martin
>
> gcc-91183.diff
>
> PR tree-optimization/91183 - strlen of a strcpy result with a conditional
> source not folded
> PR tree-optimization/86688 - missing -Wstringop-overflow using a non-string
> local array in strnlen with excessive bound
>
> gcc/ChangeLog:
>
> PR tree-optimization/91183
> PR tree-optimization/86688
> * builtins.c (compute_objsize): Handle MEM_REF.
> * tree-ssa-strlen.c (class ssa_name_limit_t): New.
> (get_min_string_length): Remove.
> (count_nonzero_bytes): New function.
> (handle_char_store): Rename...
> (handle_store): to this. Handle multibyte stores via integer types.
> (strlen_check_and_optimize_stmt): Adjust conditional and the called
> function name.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/91183
> PR tree-optimization/86688
> * gcc.dg/attr-nonstring-2.c: Remove xfails.
> * gcc.dg/strlenopt-70.c: New test.
> * gcc.dg/strlenopt-71.c: New test.
> * gcc.dg/strlenopt-72.c: New test.
> * gcc.dg/strlenopt-8.c: Remove xfails.
>
> +
>if (TREE_CODE (dest) != ADDR_EXPR)
> return NULL_TREE;
>
> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
> index 88b6bd7869e..ca1aca3ed9e 100644
> --- a/gcc/tree-ssa-strlen.c
> +++ b/gcc/tree-ssa-strlen.c
> +
> +/* If the SSA_NAME has already been "seen" return a positive value.
> + Otherwise add it to VISITED. If the SSA_NAME limit has been
> + reached, return a negative value. Otherwise return zero. */
> +
> +int ssa_name_limit_t::next_ssa_name (tree ssa_name)
Nit. Return type on a different line than the function's name. The
point behind that convention is to make it easier to search for a
function's definition.
> +/* Determine the minimum and maximum number of leading non-zero bytes
> + in the representation of EXP and set LENRANGE[0] and LENRANGE[1]
> + to each. Set LENRANGE[2] to the total number of bytes in
> + the representation. Set *NULTREM if the representation contains
> + a zero byte, and set *ALLNUL if all the bytes are zero. Avoid
> + recursing deeper than the limits in SNLIM allow. Return true
> + on success and false otherwise. */
S/NULTREM/NULTERM/
>
>if (si != NULL)
> {
> - int cmp = compare_nonzero_chars (si, offset);
> - gcc_assert (offset == 0 || cmp >= 0);
> - if (storing_all_zeros_p && cmp == 0 && si->full_string_p)
> + /* Set to 1 if the string described by SI is being written into
> + before the terminating nul (if it has one), to zero if the nul
> + is being overwritten but not beyond, or negative otherwise. */
> + int store_b4_nul[2];
Umm "store_b4_nul"? Are you really trying to save 3 characters in the
name by writing it this way? :-)
You've got two entries in the array, but the comment reads as if there's
just one element. What is the difference between the two array elements?
I didn't see anything terribly concerning so far, but I do want to look
at handle_store again after the comment is fixed so that I'm sure I'm
interpreting things correctly.
Jeff
[PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)
On targets with permissive alignment requirements GCC sometimes
lowers stores of short (between two and 16 bytes), power-of-two
char sequences to single integer stores of the corresponding
width. This happens for sequences of ordinary character stores
as well as for some calls to memcpy.
However, the strlen pass is only prepared to handle character
stores and not those of wider integers. As a result, the strlen
optimization tends to get defeated in cases when it could benefit
the most: very short strings. I counted 1544 instances where
the strlen optimization was disabled in a GCC build on x86_64
due to this sort of early store merging, and over two thousand
in a build of the Linux kernel.
In addition, -Wstringop-overflow only considers calls to string
functions and is ineffective against past-the-end accesses by
these merged multibyte stores.
To improve the effectiveness of both the optimization as well
as the warning the attached patch enhances the strlen pass to
consider these wide accesses. Since the infrastructure for doing
this is already in place (strlen can compute multibyte accesses
via MEM_REFs of character arrays), the enhancement isn't very
intrusive. It relies on the native_encode_expr function to
determine the encoding of an expression and its "length".
I tested the patch on x86_64. I expect the tests the patch
adds will need some adjustment for big-endian and strictly
aligned targets.
Martin
PR tree-optimization/91183 - strlen of a strcpy result with a conditional source not folded
PR tree-optimization/86688 - missing -Wstringop-overflow using a non-string local array in strnlen with excessive bound
gcc/ChangeLog:
PR tree-optimization/91183
PR tree-optimization/86688
* builtins.c (compute_objsize): Handle MEM_REF.
* tree-ssa-strlen.c (class ssa_name_limit_t): New.
(get_min_string_length): Remove.
(count_nonzero_bytes): New function.
(handle_char_store): Rename...
(handle_store): to this. Handle multibyte stores via integer types.
(strlen_check_and_optimize_stmt): Adjust conditional and the called
function name.
gcc/testsuite/ChangeLog:
PR tree-optimization/91183
PR tree-optimization/86688
* gcc.dg/attr-nonstring-2.c: Remove xfails.
* gcc.dg/strlenopt-70.c: New test.
* gcc.dg/strlenopt-71.c: New test.
* gcc.dg/strlenopt-72.c: New test.
* gcc.dg/strlenopt-8.c: Remove xfails.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index e5a9261e84c..695a9d191af 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3652,6 +3652,20 @@ compute_objsize (tree dest, int ostype)
if (!ostype)
return NULL_TREE;
+ if (TREE_CODE (dest) == MEM_REF)
+{
+ tree ref = TREE_OPERAND (dest, 0);
+ tree off = TREE_OPERAND (dest, 1);
+ if (tree size = compute_objsize (ref, ostype))
+ {
+ if (tree_int_cst_lt (off, size))
+ return fold_build2 (MINUS_EXPR, size_type_node, size, off);
+ return integer_zero_node;
+ }
+
+ return NULL_TREE;
+}
+
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
diff --git a/gcc/testsuite/gcc.dg/attr-nonstring-2.c b/gcc/testsuite/gcc.dg/attr-nonstring-2.c
index 246a3729a2a..ef2144d6207 100644
--- a/gcc/testsuite/gcc.dg/attr-nonstring-2.c
+++ b/gcc/testsuite/gcc.dg/attr-nonstring-2.c
@@ -73,8 +73,8 @@ void test_strnlen_string_cst (void)
T (3, "12", 3, 1);
T (3, "12", 3, 9);
T (3, "123", 3, 1);
- T (3, "123", 3, 4); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 4" "bug 86688" { xfail *-*-* } } */
- T (3, "123", 3, 9); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 9" "bug 86688" { xfail *-*-* } } */
+ T (3, "123", 3, 4); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 4" } */
+ T (3, "123", 3, 9); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound 9" } */
T (5, "1", 2, 1);
T (5, "1", 2, 2);
@@ -110,6 +110,6 @@ void test_strnlen_string_range (void)
{
T (3, "1", 2, UR (0, 1));
T (3, "1", 2, UR (3, 9));
- T (3, "123", 3, UR (4, 5)); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \\\[4, 5]" "bug 86688" { xfail *-*-* } } */
- T (3, "123", 3, UR (5, 9)); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \\\[5, 9]" "bug 86688" { xfail *-*-* } } */
+ T (3, "123", 3, UR (4, 5)); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \\\[4, 5]" } */
+ T (3, "123", 3, UR (5, 9)); /* { dg-warning "argument 1 declared attribute .nonstring. is smaller than the specified bound \\\[5, 9]" } */
}
diff --git a/gcc/testsuite/gcc.dg/strlenopt-70.c b/gcc/testsuite/gcc.dg/strlenopt-70.c
new file mode 100644
index 000..59e1081c9b5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/strlenopt-70.c
