On Sat, Nov 11, 2017 at 12:57 AM, Alexandre Oliva <aol...@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
