[PATCH] Selectively trap if ranger and vr-values disagree on range builtins.
Aldy Hernandez
aldyh@redhat.com
Mon Nov 2 10:44:42 GMT 2020
On 10/29/20 3:39 PM, Aldy Hernandez wrote:
>
>
> On 10/29/20 2:53 PM, Andrew MacLeod wrote:
>> On 10/27/20 11:29 AM, Aldy Hernandez wrote:
>>> The UBSAN builtins degrade into PLUS/MINUS/MULT and call
>>> extract_range_from_binary_expr, which as the PR shows, can special
>>> case some symbolics which the ranger doesn't currently handle.
>>>
>>> Looking at vr_values::extract_range_builtin(), I see that every single
>>> place where we ask for a range, we bail on non-integers (symbolics,
>>> etc). That is, with the exception of the UBSAN builtins.
>>>
>>> Since this seems to be particular to UBSAN, we could still go with the
>>> original plan of removing the duplicity in ranger vs vr-values, but
>>> leave in the UBSAN builtin handling. This isn't ideal, as we'd like
>>> to remove all the common code, but I'd be willing to put up with UBSAN
>>> duplication for the time being.
>>>
>>> This patch disables the assert on the UBSAN builtins, while still
>>> trapping if any other differences are found between the vr_values and
>>> the ranger versions of builtin range handling.
>>>
>>> As a follow-up, once Fedora can test this approach, I'll remove all
>>> the builtin code from extract_range_builtin, with the exception of the
>>> UBSAN stuff (renaming it to extract_range_ubsan_builtin).
>>>
>>> Since the builtin code has proven fickle across architectures, I've
>>> tested this with {-m32,-m64,-fsanitize=signed-integer-overflow} on
>>> x86, ppc64le, and aarch64. I think this should be enough. If it
>>> isn't, we can revert the patch, and leave the duplicate code until
>>> the next release cycle when hopefully vr_values, evrp, and friends
>>> will all be overhauled.
>>>
>>> Andrew, do you have any thoughts on this?
>>>
>>>
>> OK.
>>
>> I think we want to remove as much duplication as possible, which will
>> then give us confidence that that ranger versions are correct.
>> THe UBSAN versions will have to get tighter ranger integration with
>> relationals when they are available in order to handle things like this.
>>
>> I dont suppose you can create a testcase for this? Otherwise we'll
>> have to tag it somehow so we dont forget to come back to it when we
>> start handling these ubsan builtins differently.
>
> Ughh...not easily. It's deep in a Fortran testcase, and AFAICT the
> range that is determined (~[0,0]) does not affect the code generated.
>
> I'll post as is, while I ponder this. Perhaps I can hand craft a gimple
> FE test that will trigger different code out of evrp that we can somehow
> test. If/when I do, I'll push the test.
This took some work, but I was finally able to distill a C testcase
where the behavior is visible in the generated code. It's a small
testcase that will serve as a simple test for relationals in the world
of built-ins.
Basically the Y - X is turned into a .UBSAN_CHECK_SUB call that must
play nice with the (x < y) relationship.
Pushed.
gcc/testsuite/ChangeLog:
PR tree-optimization/97505
* gcc.dg/pr97505.c: New test.
---
gcc/testsuite/gcc.dg/pr97505.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/pr97505.c
diff --git a/gcc/testsuite/gcc.dg/pr97505.c b/gcc/testsuite/gcc.dg/pr97505.c
new file mode 100644
index 00000000000..f01d912067a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr97505.c
@@ -0,0 +1,23 @@
+// { dg-do compile }
+// { dg-options "-Os -fsanitize=signed-integer-overflow -fdump-tree-evrp" }
+
+// Test that .UBSAN_CHECK_SUB(y, x) is treated as y-x for range
+// purposes, where X and Y are related to each other.
+//
+// This effectively checks that range relationals work with builtins.
+
+void unreachable();
+
+int foobar(int x, int y)
+{
+ if (x < y)
+ {
+ int z = y - x;
+ if (z == 0)
+ unreachable();
+ return z;
+ }
+ return 5;
+}
+
+// { dg-final { scan-tree-dump-not "unreachable" "evrp" } }
--
2.26.2
More information about the Gcc-patches
mailing list