[Bug sanitizer/61591] New: Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[Bug sanitizer/61591] New: Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jamborm at gcc dot gnu.org]

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

            Bug ID: 61591
           Summary: Undefined behavior sanitizer does not catch
                    builtin_unreachable's from impossible devirtualization
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jamborm at gcc dot gnu.org
                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
              Host: x86_64-linux
            Target: x86_64-linux

Created attachment 32996
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32996&action=edit
Testcase

I believe that undefined behavior sanitizer does not fold
__builtin_unreachable into __builtin_trap as Jakub wrote it should in:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01823.html

When I tried it on the attached testcase I got no error:

$ ~/gcc/small/inst/bin/g++ modif.C -O3 -fsanitize=unreachable
-fsanitize-undefined-trap-on-error
$ LD_LIBRARY_PATH=/home/mjambor/gcc/mine/inst/lib64/ ./a.out 
$ echo $?
0

But when I applied the following patch:

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 33ff9b6..92a152a 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1649,7 +1649,7 @@ ipa_get_indirect_edge_target_1 (struct cgraph_edge *ie,
                 "Type inconsident devirtualization: %s/%i->%s\n",
                 ie->caller->name (), ie->caller->order,
                 IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (target)));
-      target = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
+      target = builtin_decl_implicit (BUILT_IN_TRAP);
       cgraph_get_create_node (target);
     }

and tried again, I got the expected behavior (all of this was tried on
recent trunk on x86_64-linux):

$ ~/gcc/small/inst/bin/g++ modif.C -O3
mjambor@virgil:~/gcc/small/tests/devirttrap$ ./a.out 
Illegal instruction

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #2 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I'd like to bring this to attention of UBSAN people one more time.  I
think that since 5.0 is much more aggressive turning virtual calls
which cannot happen because of type hierarchy into
__builtin_unreachable, being able to have the calls reported at
run time with UBSAN would be highly desirable.

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[mpolacek at gcc dot gnu.org]

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

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Recently I rewrote the implementation of -fsanitize=unreachable and now I get
an Illegal instruction on the testcase attached.  So is there anything else to
do?

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jamborm at gcc dot gnu.org]

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

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #3)
> Recently I rewrote the implementation of -fsanitize=unreachable and now I
> get an Illegal instruction on the testcase attached.  So is there anything
> else to do?

That is strange, because I don't (using trunk revision 218205 on
x86_64-linux).  I don't know what the difference could be, the
compiler is configured with

../src/configure --prefix=/home/mjambor/gcc/trunk/inst
--enable-languages=c,c++,fortran --enable-checking=yes --disable-bootstrap
--with-plugin-ld=/home/mjambor/binutils/obj/gold/ld-new

and I compile the testcase with:

~/gcc/trunk/inst/bin/g++ modif.C -O3 -fsanitize=unreachable
-fsanitize-undefined-trap-on-error -fdump-tree-ccp2 -fdump-tree-optimized

I can see in the two dumps that the only builtin called is
__builtin_unreachable.  Removing -fsanitize-undefined-trap-on-error
makes no difference.  The resultant a.out does not fail in any way.
Only when I change function ipa_impossible_devirt_target to return
BUILTIN_TRAP, it fails because of the illegal instruction.

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
   Target Milestone|---                         |5.0

--- Comment #5 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Alright, I don't get Illegal instruction on the testcase anymore, so perhaps I
goofed it up before.  Now I get what you get.

Let me take a look at it (most likely next week).

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[mpolacek at gcc dot gnu.org]

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

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
It appears that devirt changes
_4 = &this_1(D)->D.2680;
OBJ_TYPE_REF(_3;(struct top)_4->0) (_4);
into
_4 = &this_1(D)->D.2680;
__builtin_unreachable (_4);

but __builtin_unreachable shouldn't have any arguments!  Consequently, we fail
to sanitize this builtin call, because we call gimple_call_builtin_p on that
__builtin_unreachable (_4) stmt, but gimple_call_builtin_p checks
gimple_builtin_call_types_compatible_p and that is false, because the arguments
don't match.

So - is the __builtin_unreachable with an argument expected?

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[mpolacek at gcc dot gnu.org]

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

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
And if it is:

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index ce9fbcf..77b88f7 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -646,20 +646,21 @@ pass_sanopt::execute (function *fun)
           break;
         }
         }
-      else if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
+      else
         {
           tree callee = gimple_call_fndecl (stmt);
-          switch (DECL_FUNCTION_CODE (callee))
-        {
-        case BUILT_IN_UNREACHABLE:
-          if (flag_sanitize & SANITIZE_UNREACHABLE
-              && !lookup_attribute ("no_sanitize_undefined",
-                        DECL_ATTRIBUTES (fun->decl)))
-            no_next = ubsan_instrument_unreachable (&gsi);
-          break;
-        default:
-          break;
-        }
+          if (callee && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
+        switch (DECL_FUNCTION_CODE (callee))
+          {
+          case BUILT_IN_UNREACHABLE:
+            if (flag_sanitize & SANITIZE_UNREACHABLE
+            && !lookup_attribute ("no_sanitize_undefined",
+                          DECL_ATTRIBUTES (fun->decl)))
+              no_next = ubsan_instrument_unreachable (&gsi);
+            break;
+          default:
+            break;
+          }
         }

       if (dump_file && (dump_flags & TDF_DETAILS))

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think it is a gimple-fold/ipa-devirt etc. bug.
__builtin_unreachable doesn't have any arguments, so pretending it has is
broken and also a missed optimization, in the IL we think those arguments are
used when they aren't.  So IMNSHO we should just never generate such bogus
__builtin_unreachable calls.

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|mpolacek at gcc dot gnu.org        |jamborm at gcc dot gnu.org

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #6)
> 
> So - is the __builtin_unreachable with an argument expected?

I'd say that no, it is not and we should verify that somewhere.

I'll take over and will try to make sure that IPA does not create
__builtin_unreachable calls with arguments (and perhaps Honza might
help me with code in ipa-devirt?).

Anyway, thanks a lot, I would not have expected this problem.

[Bug sanitizer/61591] Undefined behavior sanitizer does not catch builtin_unreachable's from impossible devirtualization

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Martin Jambor <jamborm at gcc dot gnu.org> ---
This has been fixed by Honza's r220417.