diff mbox series

[PR100843] store by mult pieces: punt on max_len < min_len

Message ID orh7bh77ao.fsf@lxoliva.fsfla.org
State Committed
Commit c95a9f1ee7ebd461cbced455271a993bae3a42b6
Headers
Series [PR100843] store by mult pieces: punt on max_len < min_len |

Commit Message

Alexandre Oliva Dec. 9, 2021, 10:16 p.m. UTC 
  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.
---
 gcc/builtins.c                  |    3 ++-
 gcc/testsuite/gcc.dg/pr100843.c |    8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr100843.c

Comments

Jeff Law Dec. 10, 2021, 10:33 p.m. UTC | #1 
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
Alexandre Oliva Dec. 11, 2021, 5:18 a.m. UTC | #2 
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.
Jeff Law Dec. 14, 2021, 9:58 p.m. UTC | #3 
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 mbox series

Patch

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);
+}