| Message ID | b592eea8-9699-e706-196b-36f40d5d53b5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fix PR 103143 | expand |
On 12/6/2021 10:31 AM, Martin Sebor wrote: > I have broken up the patch into a series of six. Attached is > part (1), the fix for the typo that causes PR 103143. > > On 12/3/21 5:00 PM, Jeff Law wrote: >> >> >> On 11/8/2021 7:34 PM, Martin Sebor via Gcc-patches wrote: >>> The pointer-query code that implements compute_objsize() that's >>> in turn used by most middle end access warnings now has a few >>> warts in it and (at least) one bug. With the exception of >>> the bug the warts aren't behind any user-visible bugs that >>> I know of but they do cause problems in new code I've been >>> implementing on top of it. Besides fixing the one bug (just >>> a typo) the attached patch cleans up these latent issues: >>> >>> 1) It moves the bndrng member from the access_ref class to >>> access_data. As a FIXME in the code notes, the member never >>> did belong in the former and only takes up space in the cache. >>> >>> 2) The compute_objsize_r() function is big, unwieldy, and tedious >>> to step through because of all the if statements that are better >>> coded as one switch statement. This change factors out more >>> of its code into smaller handler functions as has been suggested >>> and done a few times before. >>> >>> 3) (2) exposed a few places where I fail to pass the current >>> GIMPLE statement down to ranger. This leads to worse quality >>> range info, including possible false positives and negatives. >>> I just spotted these problems in code review but I haven't >>> taken the time to come up with test cases. This change fixes >>> these oversights as well. >>> >>> 4) The handling of PHI statements is also in one big, hard-to- >>> follow function. This change moves the handling of each PHI >>> argument into its own handler which merges it into the previous >>> argument. This makes the code easier to work with and opens it >>> to reuse also for MIN_EXPR and MAX_EXPR. (This is primarily >>> used to print informational notes after warnings.) >>> >>> 5) Finally, the patch factors code to dump each access_ref >>> cached by the pointer_query cache out of pointer_query::dump >>> and into access_ref::dump. This helps with debugging. >>> >>> These changes should have no user-visible effect and other than >>> a regression test for the typo (PR 103143) come with no tests. >>> They've been tested on x86_64-linux. >> Sigh. You've identified 6 distinct changes above. The 5 you've >> enumerated plus a typo fix somewhere. There's no reason why they >> need to be a single patch and many reasons why they should be a >> series of independent patches. Combining them into a single patch >> isn't how we do things and it hides the actual bugfix in here. >> >> Please send a fix for the typo first since that should be able to >> trivially go forward. Then a patch for item #1. That should be >> trivial to review when it's pulled out from teh rest of the patch. >> Beyond that, your choice on ordering, but you need to break this down. >> >> >> >> >> Jeff >> > > > gcc-103413.diff > > commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9 > Author: Martin Sebor<msebor@redhat.com> > Date: Sat Dec 4 16:22:07 2021 -0700 > > Use the recursive form of compute_objsize [PR 103143]. > > gcc/ChangeLog: > > PR middle-end/103143 > * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r. > > gcc/testsuite/ChangeLog: > PR middle-end/103143 > * gcc.dg/Wstringop-overflow-83.c: New test. Thanks. Presumably by going through the public API, qry->depth got reset to 0 and/or we'd get a re-initialized snlim at each call, leading to the infinite recursion? OK for the trunk. Thanks for breaking this out. jeff
On 12/6/21 1:14 PM, Jeff Law wrote: > > > On 12/6/2021 10:31 AM, Martin Sebor wrote: >> I have broken up the patch into a series of six. Attached is >> part (1), the fix for the typo that causes PR 103143. >> >> On 12/3/21 5:00 PM, Jeff Law wrote: >>> >>> >>> On 11/8/2021 7:34 PM, Martin Sebor via Gcc-patches wrote: >>>> The pointer-query code that implements compute_objsize() that's >>>> in turn used by most middle end access warnings now has a few >>>> warts in it and (at least) one bug. With the exception of >>>> the bug the warts aren't behind any user-visible bugs that >>>> I know of but they do cause problems in new code I've been >>>> implementing on top of it. Besides fixing the one bug (just >>>> a typo) the attached patch cleans up these latent issues: >>>> >>>> 1) It moves the bndrng member from the access_ref class to >>>> access_data. As a FIXME in the code notes, the member never >>>> did belong in the former and only takes up space in the cache. >>>> >>>> 2) The compute_objsize_r() function is big, unwieldy, and tedious >>>> to step through because of all the if statements that are better >>>> coded as one switch statement. This change factors out more >>>> of its code into smaller handler functions as has been suggested >>>> and done a few times before. >>>> >>>> 3) (2) exposed a few places where I fail to pass the current >>>> GIMPLE statement down to ranger. This leads to worse quality >>>> range info, including possible false positives and negatives. >>>> I just spotted these problems in code review but I haven't >>>> taken the time to come up with test cases. This change fixes >>>> these oversights as well. >>>> >>>> 4) The handling of PHI statements is also in one big, hard-to- >>>> follow function. This change moves the handling of each PHI >>>> argument into its own handler which merges it into the previous >>>> argument. This makes the code easier to work with and opens it >>>> to reuse also for MIN_EXPR and MAX_EXPR. (This is primarily >>>> used to print informational notes after warnings.) >>>> >>>> 5) Finally, the patch factors code to dump each access_ref >>>> cached by the pointer_query cache out of pointer_query::dump >>>> and into access_ref::dump. This helps with debugging. >>>> >>>> These changes should have no user-visible effect and other than >>>> a regression test for the typo (PR 103143) come with no tests. >>>> They've been tested on x86_64-linux. >>> Sigh. You've identified 6 distinct changes above. The 5 you've >>> enumerated plus a typo fix somewhere. There's no reason why they >>> need to be a single patch and many reasons why they should be a >>> series of independent patches. Combining them into a single patch >>> isn't how we do things and it hides the actual bugfix in here. >>> >>> Please send a fix for the typo first since that should be able to >>> trivially go forward. Then a patch for item #1. That should be >>> trivial to review when it's pulled out from teh rest of the patch. >>> Beyond that, your choice on ordering, but you need to break this down. >>> >>> >>> >>> >>> Jeff >>> >> >> >> gcc-103413.diff >> >> commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9 >> Author: Martin Sebor<msebor@redhat.com> >> Date: Sat Dec 4 16:22:07 2021 -0700 >> >> Use the recursive form of compute_objsize [PR 103143]. >> >> gcc/ChangeLog: >> >> PR middle-end/103143 >> * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r. >> >> gcc/testsuite/ChangeLog: >> PR middle-end/103143 >> * gcc.dg/Wstringop-overflow-83.c: New test. > Thanks. Presumably by going through the public API, qry->depth got > reset to 0 and/or we'd get a re-initialized snlim at each call, leading > to the infinite recursion? Yes, each call creates a new ssa_name_limit_t::visited bitmap. It might be better to make compute_objsize_r() a member of the same class as the bitmap and also rename it, to make it harder to make this mistake again. Martin > > OK for the trunk. Thanks for breaking this out. > > jeff >
commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9
Author: Martin Sebor <msebor@redhat.com>
Date: Sat Dec 4 16:22:07 2021 -0700
Use the recursive form of compute_objsize [PR 103143].
gcc/ChangeLog:
PR middle-end/103143
* pointer-query.cc (gimple_call_return_array): Call compute_objsize_r.
gcc/testsuite/ChangeLog:
PR middle-end/103143
* gcc.dg/Wstringop-overflow-83.c: New test.
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 2ead0271617..25ce4303849 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -199,7 +199,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize (src, stmt, 1, &aref, qry)
+ if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
new file mode 100644
index 00000000000..6928ee4d559
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
@@ -0,0 +1,19 @@
+/* PR middle-end/103143 - ICE due to infinite recursion in pointer-query.cc
+ { dg-do compile }
+ { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+void foo (size_t x)
+{
+ struct T { char buf[64]; char buf2[64]; } t;
+ char *p = &t.buf[8];
+ char *r = t.buf2;
+ size_t i;
+
+ for (i = 0; i < x; i++)
+ {
+ r = __builtin_mempcpy (r, p, i);
+ p = r + 1;
+ }
+}