[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.



	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" } }

More information about the Gcc-patches mailing list