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.
Aldy
Andrew
PS. and might as well create and test the follow up patch so its ready
to go. I'd leave this as-is with the asserts for a week or so.
