[Bug sanitizer/101758] New: Inconsistent optimizations with UBSan

[Bug sanitizer/101758] New: Inconsistent optimizations with UBSan

[blubban at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

            Bug ID: 101758
           Summary: Inconsistent optimizations with UBSan
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: blubban at gmail dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

Input:

gcc -fsanitize=undefined -O2 -fno-strict-aliasing -Wall

float b(unsigned a) { return *(float*)&a; }
float c(unsigned a) { return *(float*)&a; }
float d(unsigned a) { return *(float*)&a; }


Output:

b:
        mov     DWORD PTR [rsp-4], edi
        movss   xmm0, DWORD PTR [rsp-4]
        ret
c:
        movd    xmm0, edi
        ret
d:
        movd    xmm0, edi
        ret

or, on ARM

b:
        sub     sp, sp, #8
        str     r0, [sp, #4]
        vldr.32 s0, [sp, #4]
        add     sp, sp, #8
        bx      lr
c:
        sub     sp, sp, #8
        vmov    s0, r0
        add     sp, sp, #8
        bx      lr
d:
        sub     sp, sp, #8
        vmov    s0, r0
        add     sp, sp, #8
        bx      lr


Expected output:

Same assembly for all three. And maybe don't set up an unused stack frame on
ARM (it's correctly optimized out without UBSan).


Compiler Explorer: https://godbolt.org/z/Tfs5qazqM


(Found it while trying to determine if that function is a strict aliasing
violation (it is), and whether it can be fixed with memcpy or a union (both
work).)

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
          Component|sanitizer                   |tree-optimization
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |missed-optimization
           Severity|normal                      |enhancement
   Last reconfirmed|                            |2021-08-03

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is ipa-icf going interesting.  We mark c and d as clones of b.  When we go
and do inlining, we fix up that a is no longer addressable because the check
for UBSAN_NULL is removed already.

So the real issue is after UBSAN_NULL is removed, there is no redoing
addressability of a.

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
You can see the difference here:
float b(unsigned a) { return *(float*)&a; }
float d1(unsigned a) { unsigned t = a; return *(float*)&t; }

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think the easiest way to fix this is inside instrument_mem_ref:ubsan.c check
TREE_OPERAND (base, 0) to see if it is ADDR_EXPR of a DECL_P then don't do the
check if non-weak.

Something like:
diff --git a/gcc/ubsan.c b/gcc/ubsan.c
index ba9adf0ad96..e66c430c7a2 100644
--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -1380,6 +1380,14 @@ instrument_mem_ref (tree mem, tree base,
gimple_stmt_iterator *iter,
   tree t = TREE_OPERAND (base, 0);
   if (!POINTER_TYPE_P (TREE_TYPE (t)))
     return;
+
+  /* For simple &decl don't instument this as it will be removed
+     later on. */
+  if (TREE_CODE (t) == ADDR_EXPR
+      && DECL_P (TREE_OPERAND (t, 0))
+      && DECL_WEAK (TREE_OPERAND (t, 0)))
+    return;
+
   if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (base)) && mem != base)
     ikind = UBSAN_MEMBER_ACCESS;
   tree kind = build_int_cst (build_pointer_type (TREE_TYPE (base)), ikind);

Mine.

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> +      && DECL_WEAK (TREE_OPERAND (t, 0)))

typo, should have been !DECL_WEAK :).

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is the fix which works:
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index ad7b140173f..f1fbdbd40c2 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -5587,6 +5587,16 @@ gimple_fold_call (gimple_stmt_iterator *gsi, bool
inplace)
                                        gimple_call_arg (stmt, 2),
                                        NULL_TREE);
          break;
+       case IFN_UBSAN_NULL:
+         {
+           tree addr = gimple_call_arg (stmt, 0);
+           if (TREE_CODE (addr) == ADDR_EXPR
+               && DECL_P (TREE_OPERAND (addr, 0))
+               && (!VAR_OR_FUNCTION_DECL_P (TREE_OPERAND (addr, 0))
+                   || !DECL_WEAK (TREE_OPERAND (addr, 0))))
+             replace_call_with_value (gsi, NULL_TREE);
+         }
+         break;
        case IFN_UBSAN_OBJECT_SIZE:
          {
            tree offset = gimple_call_arg (stmt, 1);

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
UBSAN_NULL doesn't check just whether it is non-NULL, but also the alignment.
So no, this doesn't look like the right fix.

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> UBSAN_NULL doesn't check just whether it is non-NULL, but also the alignment.
> So no, this doesn't look like the right fix.

Ok, I will add the check for align to be non-zero :).

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 51253
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51253&action=edit
Better patch

here is the better patch which also handles the alignment check :).

[Bug tree-optimization/101758] Inconsistent optimizations with UBSan

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101758

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|pinskia at gcc dot gnu.org         |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So my checks are not fully correct, I am not going to take my time to debug
this fully debug how we can remove the check.