This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: fix -fcompare-debug OBJ_TYPE_REF dumps of linker plugin-less LTO builds


On Sat, Nov 11, 2017 at 12:57 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> There are two patches below.  Each one of them fixes the problem
> described below by itself, and both would also fix it together.
>
> The former preserves OBJ_TYPE_REF type info in all dumps that
> should/would have it, even after TYPE_BINFO is reset to release its
> memory.  To that end, it requires changes to the middle end, and a few
> changes to front-ends that set TYPE_BINFO.
>
> The latter creates a special case for -fcompare-debug to hide this
> irrelevant compilation artifact, but it's a far more localized change,
> with zero compile-time impact, and it paves the way to introduce other
> compare-debug-specific IR dumping changes to exclude other artifacts
> from leading to -fcompare-debug errors.
>
> I've regstrapped each of them on x86_64-linux-gnu and i686-linux-gnu.
>
> Ok to install the former?

Honza, can you have a look here?  You're knowing OBJ_TYPE_REF best.
I'm not sure what we require of TREE_TYPE (OBJ_TYPE_REF) and whether
explicitely recording the BINFO as a operand might be preferable (or sth
like that).

> Ok to install the latter?

That looks ok to me - another option would be to always dump the class.
Not sure if obj_type_ref_class would ICE if ! virtual_method_call_p - it
has lots of asserts and seems to return the type of '*this'.  As said above
it would sound more sustainable to put this explicitely somewhere on the
OBJ_TYPE_REF tree...

Thanks,
Richard.

>
> -fcompare-debug OBJ_TYPE_REF: introduce TYPE_BINFO_EVER_SET et al
>
> In LTO compilation of e.g. libstdc++-prettyprint/shared_ptr, we reset
> TYPE_BINFO of certain types, to release memory, depending on whether
> we are to generate debug info.
>
> Alas, TYPE_BINFO is tested by OBJ_TYPE_REF dumpers, to decide whether
> or not to dump its class.  This causes -fcompare-debug to fail,
> because the two different rounds of compilation will have opposite
> debug information generation requests, and thus will decide
> differently whether to reset TYPE_BINFO long before the
> -fcompare-debug dumps.
>
> What really matters for the virtual_method_call_p test is whether
> TYPE_BINFO is set, or was ever set, and this is the function that
> decides whether to dump the type of the OBJ_TYPE_REF.  So, I introduce
> memory in TYPE_BINFO of whether it was ever set, in a way that doesn't
> take up more space, but slows down former uses of TYPE_BINFO a bit:
> its (new) setter macro won't let it go back to NULL, and a (new) reset
> macro will make it point back to the node itself, so that we know it's
> neither NULL nor a TREE_BINFO, and won't stop any garbage from being
> collected.
>
> With this patch, the aforementioned libstdc++ test passes even with
> -fcompare-debug.
>
>
> I suppose there might be remaining uses of TYPE_BINFO that could
> benefit from testing TYPE_BINFO_EVER_SET instead, but I haven't
> investigated them all, and those I have only guarded uses of BINFO_*,
> so they can't change.
>
>
> for  gcc/ChangeLog
>
>         * tree.h (TYPE_BINFO): Return NULL if it's not a TREE_BINFO
>         object.
>         (TYPE_BINFO_EVER_SET): Test that it's not NULL.
>         (SET_TYPE_BINFO): Don't change it from non-NULL to NULL.
>         (RESET_TYPE_BINFO): Set it to a non-TREE_BINFO, not NULL.
>         * ipa-devirt.c (set_type_binfo): Use SET_TYPE_BINFO.
>         * tree.c (free_lang_data_in_type): Use RESET_TYPE_BINFO.
>         (virtual_method_call_p): Test TYPE_BINFO_EVER_SET.
>
> for  gcc/cp/ChangeLog
>
>         * class.c (fixup_type_variants): Use SET_TYPE_BINFO.
>         * decl.c (xref_basetypes): Likewise.
>
> for gcc/objc/ChangeLog
>
>         * objc-act.c (objc_xref_basetypes): Use SET_TYPE_BINFO.
> ---
>  gcc/cp/class.c      |    2 +-
>  gcc/cp/decl.c       |    2 +-
>  gcc/ipa-devirt.c    |    2 +-
>  gcc/objc/objc-act.c |    4 ++--
>  gcc/tree.c          |    4 ++--
>  gcc/tree.h          |   24 +++++++++++++++++++++++-
>  6 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/cp/class.c b/gcc/cp/class.c
> index 98e62c6ad450..4f92204ba504 100644
> --- a/gcc/cp/class.c
> +++ b/gcc/cp/class.c
> @@ -1927,7 +1927,7 @@ fixup_type_variants (tree t)
>
>        TYPE_POLYMORPHIC_P (variants) = TYPE_POLYMORPHIC_P (t);
>
> -      TYPE_BINFO (variants) = TYPE_BINFO (t);
> +      SET_TYPE_BINFO (variants, TYPE_BINFO (t));
>
>        /* Copy whatever these are holding today.  */
>        TYPE_VFIELD (variants) = TYPE_VFIELD (t);
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 0ce8f2d34358..8cd39375cf3c 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -13695,7 +13695,7 @@ xref_basetypes (tree ref, tree base_list)
>
>    binfo = make_tree_binfo (max_bases);
>
> -  TYPE_BINFO (ref) = binfo;
> +  SET_TYPE_BINFO (ref, binfo);
>    BINFO_OFFSET (binfo) = size_zero_node;
>    BINFO_TYPE (binfo) = ref;
>
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index f03c7f099f73..6120c362dc74 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -651,7 +651,7 @@ set_type_binfo (tree type, tree binfo)
>  {
>    for (; type; type = TYPE_NEXT_VARIANT (type))
>      if (COMPLETE_TYPE_P (type))
> -      TYPE_BINFO (type) = binfo;
> +      SET_TYPE_BINFO (type, binfo);
>      else
>        gcc_assert (!TYPE_BINFO (type));
>  }
> diff --git a/gcc/objc/objc-act.c b/gcc/objc/objc-act.c
> index 765192c82aaa..f017435dd681 100644
> --- a/gcc/objc/objc-act.c
> +++ b/gcc/objc/objc-act.c
> @@ -2694,13 +2694,13 @@ objc_xref_basetypes (tree ref, tree basetype)
>  {
>    tree variant;
>    tree binfo = make_tree_binfo (basetype ? 1 : 0);
> -  TYPE_BINFO (ref) = binfo;
> +  SET_TYPE_BINFO (ref, binfo);
>    BINFO_OFFSET (binfo) = size_zero_node;
>    BINFO_TYPE (binfo) = ref;
>
>    gcc_assert (TYPE_MAIN_VARIANT (ref) == ref);
>    for (variant = ref; variant; variant = TYPE_NEXT_VARIANT (variant))
> -    TYPE_BINFO (variant) = binfo;
> +    SET_TYPE_BINFO (variant, binfo);
>
>    if (basetype)
>      {
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 28e157f5fd28..9634f0b72171 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -4966,7 +4966,7 @@ free_lang_data_in_type (tree type)
>               && ((!BINFO_N_BASE_BINFOS (TYPE_BINFO (type))
>                    && !BINFO_VTABLE (TYPE_BINFO (type)))
>                   || debug_info_level != DINFO_LEVEL_NONE))
> -           TYPE_BINFO (type) = NULL;
> +           RESET_TYPE_BINFO (type);
>         }
>      }
>    else if (INTEGRAL_TYPE_P (type)
> @@ -12006,7 +12006,7 @@ virtual_method_call_p (const_tree target)
>    /* If we do not have BINFO associated, it means that type was built
>       without devirtualization enabled.  Do not consider this a virtual
>       call.  */
> -  if (!TYPE_BINFO (obj_type_ref_class (target)))
> +  if (!TYPE_BINFO_EVER_SET (obj_type_ref_class (target)))
>      return false;
>    return true;
>  }
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 277aa919780e..c2c130e00dbc 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -2120,7 +2120,29 @@ extern machine_mode vector_type_mode (const_tree);
>  #define TYPE_MAX_VALUE_RAW(NODE) (TYPE_CHECK (NODE)->type_non_common.maxval)
>  /* For record and union types, information about this type, as a base type
>     for itself.  */
> -#define TYPE_BINFO(NODE) (RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval)
> +#define TYPE_BINFO(NODE) \
> +  (RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval                        \
> +   && (TREE_CODE (RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval)        \
> +       == TREE_BINFO)                                                  \
> +   ? RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval              \
> +   : NULL_TREE)
> +/* We want TYPE_BINFO to remember, with TYPE_BINFO_EVER_SET, whether
> +   it was ever set, so don't allow it to be set to NULL_TREE once it
> +   gets an actual value.  If it was ever set and it's to be cleared,
> +   use RESET_TYPE_BINFO, to make it point to the type itself (or
> +   anything, really, but pointing to the type itself doesn't stop
> +   garbage collection).  */
> +#define TYPE_BINFO_EVER_SET(NODE) \
> +  (!!RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval)
> +#define SET_TYPE_BINFO(NODE,BINFO) \
> +  (!RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval || (BINFO)    \
> +   ? (NODE)->type_non_common.maxval = (BINFO)                          \
> +   : (NODE)->type_non_common.maxval == (BINFO)                         \
> +   ? (BINFO) : (gcc_unreachable (), (BINFO)))
> +#define RESET_TYPE_BINFO(NODE) \
> +  (RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval                        \
> +   ? RECORD_OR_UNION_CHECK (NODE)->type_non_common.maxval = (NODE)     \
> +   : NULL_TREE)
>
>  /* For types, used in a language-dependent way.  */
>  #define TYPE_LANG_SLOT_1(NODE) \
>
>
>
> introduce TDF_compare_debug, omit OBJ_TYPE_REF casts with it
>
> for  gcc/ChangeLog
>
>         * dumpfile.h (TDF_COMPARE_DEBUG): New.
>         * final.c (rest_of_clean_state): Set it for the
>         -fcompare-debug dump.
>         * tree-pretty-print.c (dump_generic_node): Omit OBJ_TYPE_REF
>         class when TDF_COMPARE_DEBUG is set.
> ---
>  gcc/dumpfile.h          |    1 +
>  gcc/final.c             |    2 +-
>  gcc/tree-pretty-print.c |   10 +++++++++-
>  3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
> index 4d9f6b3656a6..1b4d7e7dab71 100644
> --- a/gcc/dumpfile.h
> +++ b/gcc/dumpfile.h
> @@ -93,6 +93,7 @@ enum dump_kind
>  #define MSG_NOTE                (1 << 24)  /* general optimization info */
>  #define MSG_ALL                (MSG_OPTIMIZED_LOCATIONS | MSG_MISSED_OPTIMIZATION \
>                          | MSG_NOTE)
> +#define TDF_COMPARE_DEBUG (1 << 25)    /* Dumping for -fcompare-debug.  */
>
>
>  /* Value of TDF_NONE is used just for bits filtered by TDF_KIND_MASK.  */
> diff --git a/gcc/final.c b/gcc/final.c
> index 039c37a31352..fe35a36dbbf2 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -4629,7 +4629,7 @@ rest_of_clean_state (void)
>         {
>           flag_dump_noaddr = flag_dump_unnumbered = 1;
>           if (flag_compare_debug_opt || flag_compare_debug)
> -           dump_flags |= TDF_NOUID;
> +           dump_flags |= TDF_NOUID | TDF_COMPARE_DEBUG;
>           dump_function_header (final_output, current_function_decl,
>                                 dump_flags);
>           final_insns_dump_p = true;
> diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
> index 61a28c6757fb..80d45f96d67c 100644
> --- a/gcc/tree-pretty-print.c
> +++ b/gcc/tree-pretty-print.c
> @@ -2760,7 +2760,15 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags,
>        pp_string (pp, "OBJ_TYPE_REF(");
>        dump_generic_node (pp, OBJ_TYPE_REF_EXPR (node), spc, flags, false);
>        pp_semicolon (pp);
> -      if (!(flags & TDF_SLIM) && virtual_method_call_p (node))
> +      /* We omit the class type for -fcompare-debug because we may
> +        drop TYPE_BINFO early depending on debug info, and then
> +        virtual_method_call_p would return false, whereas when
> +        TYPE_BINFO is preserved it may still return true and then
> +        we'd print the class type.  Compare tree and rtl dumps for
> +        libstdc++-prettyprinters/shared_ptr.cc with and without -g,
> +        for example, at occurrences of OBJ_TYPE_REF.  */
> +      if (!(flags & (TDF_SLIM | TDF_COMPARE_DEBUG))
> +         && virtual_method_call_p (node))
>         {
>           pp_string (pp, "(");
>           dump_generic_node (pp, obj_type_ref_class (node), spc, flags, false);
>
>
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]