Re: [PATCH] Make some asan builtins tm_pure (PR sanitizer/55508)

[Dmitry Vyukov <dvyukov at google dot com>]

On Wed, Dec 12, 2012 at 11:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Various TM tests ICE when built with -fgnu-tm -fsanitizer=address.
> The problem is that asan.c pass adds calls to builtins that weren't there
> before and TM is upset about it.  The __asan_report* are all like
> abort, in correctly written program they shouldn't have a user visible
> effect, in bad program they will terminate the process, but in any case
> it doesn't matter how many times they are retried as part of a transaction,
> there is no state to roll back on transaction cancellation.
> __asan_handle_no_return, while not being noreturn, just marks the stack as
> unprotected, so again in correctly written application no effect, in bad app
> might result in some issues being undetected, but still, it can be done many
> times and isn't irreversible.

Hi!

I was only loosely following tm-languages discussions. Does gcc tm
model guarantees strong consistency for all memory accesses? I mean
there are tm implementations that allow transient inconsistencies,
than are detected later and trx is restarted. Can't asan trigger false
positives in this case?
Also, what is the order of instrumentation in tm+asan setting? I mean
that neither tm must instrument asan instrumentation, nor asan must
instrument tm instrumentation. Is it the case? There also can be
conflicts related to ordering of instrumentation in the following
case:
asan_check();
speculative_load();
tm_check();
In this case tm hopes to detect inconsistent speculative load
afterwards, but asan will crash the program before tm has a chance to
abort and retry (it is related to the first point).

Sorry, I don't know how gcc tm works. But I just suspect that it can
be very tricky.


> The following patch fixes the ICEs by making all of these transaction pure.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> As for TSAN, no idea what to do, TSAN diagnostics could be in a similar
> category, there is nothing to reverse, but as the library doesn't understand
> transactions, perhaps better would be not to tsan instrument anything inside
> of transactions, until the library is made TM aware.
>
> 2012-12-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR sanitizer/55508
>         * builtin-attrs.def (ATTR_TMPURE_NOTHROW_LEAF_LIST,
>         ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST): New.
>         * asan.c (ATTR_TMPURE_NOTHROW_LEAF_LIST,
>         ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST): Define.
>         * sanitizer.def: Make __asan_report_* and __asan_handle_no_return
>         builtins tm pure.
>
> --- gcc/builtin-attrs.def.jj    2012-10-22 08:42:23.000000000 +0200
> +++ gcc/builtin-attrs.def       2012-12-12 11:56:54.942938879 +0100
> @@ -263,6 +263,11 @@ DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_N
>  DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LIST,
>                    ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LIST)
>
> +DEF_ATTR_TREE_LIST (ATTR_TMPURE_NOTHROW_LEAF_LIST,
> +                   ATTR_TM_TMPURE, ATTR_NULL, ATTR_NOTHROW_LEAF_LIST)
> +DEF_ATTR_TREE_LIST (ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST,
> +                   ATTR_TM_TMPURE, ATTR_NULL, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +
>  /* Construct a tree for a format_arg attribute.  */
>  #define DEF_FORMAT_ARG_ATTRIBUTE(FA)                                   \
>    DEF_ATTR_TREE_LIST (ATTR_FORMAT_ARG_##FA, ATTR_FORMAT_ARG,           \
> --- gcc/asan.c.jj       2012-12-11 13:05:36.000000000 +0100
> +++ gcc/asan.c  2012-12-12 11:57:59.626550534 +0100
> @@ -1611,8 +1611,13 @@ initialize_sanitizer_builtins (void)
>  #define BT_FN_VOID_VPTR_I16_INT BT_FN_VOID_VPTR_IX_INT[4]
>  #undef ATTR_NOTHROW_LEAF_LIST
>  #define ATTR_NOTHROW_LEAF_LIST ECF_NOTHROW | ECF_LEAF
> +#undef ATTR_TMPURE_NOTHROW_LEAF_LIST
> +#define ATTR_TMPURE_NOTHROW_LEAF_LIST ECF_TM_PURE | ATTR_NOTHROW_LEAF_LIST
>  #undef ATTR_NORETURN_NOTHROW_LEAF_LIST
>  #define ATTR_NORETURN_NOTHROW_LEAF_LIST ECF_NORETURN | ATTR_NOTHROW_LEAF_LIST
> +#undef ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST
> +#define ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST \
> +  ECF_TM_PURE | ATTR_NORETURN_NOTHROW_LEAF_LIST
>  #undef DEF_SANITIZER_BUILTIN
>  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
>    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,          \
> --- gcc/sanitizer.def.jj        2012-12-11 11:28:10.000000000 +0100
> +++ gcc/sanitizer.def   2012-12-12 11:57:12.714833945 +0100
> @@ -32,25 +32,25 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_INIT
>  /* Do not reorder the BUILT_IN_ASAN_REPORT* builtins, e.g. cfgcleanup.c
>     relies on this order.  */
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD1, "__asan_report_load1",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD2, "__asan_report_load2",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD4, "__asan_report_load4",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD8, "__asan_report_load8",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD16, "__asan_report_load16",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE1, "__asan_report_store1",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE2, "__asan_report_store2",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE4, "__asan_report_store4",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE8, "__asan_report_store8",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE16, "__asan_report_store16",
> -                     BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID_PTR, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REGISTER_GLOBALS,
>                       "__asan_register_globals",
>                       BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
> @@ -59,7 +59,7 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_UNRE
>                       BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_HANDLE_NO_RETURN,
>                       "__asan_handle_no_return",
> -                     BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST)
> +                     BT_FN_VOID, ATTR_TMPURE_NOTHROW_LEAF_LIST)
>
>  /* Thread Sanitizer */
>  DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_INIT, "__tsan_init",
>
>         Jakub