Message ID | orh7bh77ao.fsf@lxoliva.fsfla.org |
---|---|
State | New |
Headers | show |
Series | [PR100843] store by mult pieces: punt on max_len < min_len | expand |
On 12/9/2021 3:16 PM, Alexandre Oliva via Gcc-patches wrote: > The testcase confuses the code that detects min and max len for the > memset, so max_len ends up less than min_len. That shouldn't be > possible, but the testcase requires us to handle this case. > > The store-by-mult-pieces algorithm actually relies on min and max > lengths, so if we find them to be inconsistent, the best we can do is > punting. > > Regstrapped on x86_64-linux-gnu. Ok to install? > > > for gcc/ChangeLog > > PR middle-end/100843 > * builtins.c (try_store_by_multiple_pieces): Fail if min_len > is greater than max_len. > > for gcc/testsuite/ChangeLog > > PR middle-end/100843 > * gcc.dg/pr100843.c: New. The patch is clearly safe. My question is should we have caught this earlier in the call chain? If so, I'd prefer to fix it there and leave the gcc_unreachable in place to catch other bogus cases that may get into try_store_by_multiple_pieces. Jeff
On Dec 10, 2021, Jeff Law <jeffreyalaw@gmail.com> wrote: > The patch is clearly safe. My question is should we have caught this > earlier in the call chain? Callers will call try_store_by_multiple_pieces if set_storage_via_setmem fails. setmem doesn't necessarily need min and max len to do its job, so if we were to modify callers, it would be just guarding the calls of try_store_by_multiple_pieces with max_len >= min_len: 3 callers in 2 files, which didn't seem appealing to me.
On 12/10/2021 10:18 PM, Alexandre Oliva wrote: > On Dec 10, 2021, Jeff Law <jeffreyalaw@gmail.com> wrote: > >> The patch is clearly safe. My question is should we have caught this >> earlier in the call chain? > Callers will call try_store_by_multiple_pieces if set_storage_via_setmem > fails. setmem doesn't necessarily need min and max len to do its job, > so if we were to modify callers, it would be just guarding the calls of > try_store_by_multiple_pieces with max_len >= min_len: 3 callers in 2 > files, which didn't seem appealing to me. Thanks for the additional info. OK for the trunk. Jeff
diff --git a/gcc/builtins.c b/gcc/builtins.c index 03829c03a5a11..304d87dafb750 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -3963,7 +3963,8 @@ try_store_by_multiple_pieces (rtx to, rtx len, unsigned int ctz_len, else if (max_len == min_len) blksize = max_len; else - gcc_unreachable (); + /* Huh, max_len < min_len? Punt. See pr100843.c. */ + return false; if (min_len >= blksize) { min_len -= blksize; diff --git a/gcc/testsuite/gcc.dg/pr100843.c b/gcc/testsuite/gcc.dg/pr100843.c new file mode 100644 index 0000000000000..695a2ec3f6818 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr100843.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -w" } */ + +char c; +void *memset(); +void test_integer_conversion_memset(void *d) { + memset(d, '\0', c); +}