On Sat, Jul 11, 2020 at 10:21 AM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Jan Hubicka <hubi...@ucw.cz> writes:
> >> Jan Hubicka <hubi...@ucw.cz> writes:
> >> >> The testcase has failed since r9-5035, because obj_type_ref_class
> >> >> tries to look up an ODR type when no ODR type information is
> >> >> available. (The information was available earlier in the
> >> >> compilation, but was freed during pass_ipa_free_lang_data.)
> >> >> We then crash dereferencing the null get_odr_type result.
> >> >>
> >> >> The test passes with -O2. However, it fails again if -fdump-tree-all
> >> >> is used, since obj_type_ref_class is called indirectly from the
> >> >> dump routines.
> >> >>
> >> >> Other code seems to create ODR type entries on the fly by passing
> >> >> "true" as the insert parameter. But since obj_type_ref_class is
> >> >> used by dump routines, I guess it should have no side-effects and
> >> >> should simply punt in this case.
> >> > Thanks for looking into this!
> >> >
> >> > The ODR type hash is indeed supposed to be populated lazilly except for
> >> > LTO when we populate it at stream in time. I think just making function
> >> > return NULL in case it should not is fragile, because we may hit missed
> >> > optimizations and other surprised elsewhere.
> >> >
> >> > What about adding new "for_dump" parameter set implicitly to false that
> >> > will make the function return ret in case get_odr_type is NULL?
> >> > For dumping it probably does not matter what ODR variant you see, since
> >> > we end up with a type name in the dump anyway.
> >>
> >> I think there are two problematic cases (at least as far as this
> >> testcase goes). One is the direct call to obj_type_ref_class in
> >> tree-pretty-print.c. The other is an indirect call via
> >> virtual_method_call_p, which is also called by tree-pretty-print.c.
> >> So I guess both of them would need a "for_dump" parameter.
> >>
> >> TBH doing it that way feels fragile too. Even if we get it right now,
> >> it's going to be hard to ensure that all indirect calls to
> >> obj_type_ref_class
> >> in future make the correct choice between lazy initialisation and remaining
> >> side-effect free.
> >
> > Yep, it is not very pretty :(. In general the output of optimizations
> > does not depend much on what is inserted into the type inheritance graph
> > (because we must assume that types are derived in other units anyway)
> > except for anonymous types. However speculative devirt is an exception
> > here, so I guess we want to go woth for_dump markers (it is useful to
> > have obj type ref pretty printed in a meaningful way).
>
> OK, this patch (belatedly) does that.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK for trunk and
> GCC 9+?
OK.
Richard.
> Thanks,
> Richard
>
>
> gcc/
> PR middle-end/95114
> * tree.h (virtual_method_call_p): Add a default-false parameter
> that indicates whether the function is being called from dump
> routines.
> (obj_type_ref_class): Likewise.
> * tree.c (virtual_method_call_p): Likewise.
> * ipa-devirt.c (obj_type_ref_class): Likewise. Lazily add ODR
> type information for the type when the parameter is false.
> * tree-pretty-print.c (dump_generic_node): Update calls to
> virtual_method_call_p and obj_type_ref_class accordingly.
>
> gcc/testsuite/
> PR middle-end/95114
> * g++.target/aarch64/pr95114.C: New test.
> ---
> gcc/ipa-devirt.c | 9 ++++++---
> gcc/testsuite/g++.target/aarch64/pr95114.C | 3 +++
> gcc/tree-pretty-print.c | 5 +++--
> gcc/tree.c | 7 ++++---
> gcc/tree.h | 4 ++--
> 5 files changed, 18 insertions(+), 10 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/aarch64/pr95114.C
>
> diff --git a/gcc/tree.h b/gcc/tree.h
> index cf546ed9491..866d9ba8fbc 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -5241,8 +5241,8 @@ extern location_t *block_nonartificial_location (tree);
> extern location_t tree_nonartificial_location (tree);
> extern tree block_ultimate_origin (const_tree);
> extern tree get_binfo_at_offset (tree, poly_int64, tree);
> -extern bool virtual_method_call_p (const_tree);
> -extern tree obj_type_ref_class (const_tree ref);
> +extern bool virtual_method_call_p (const_tree, bool = false);
> +extern tree obj_type_ref_class (const_tree ref, bool = false);
> extern bool types_same_for_odr (const_tree type1, const_tree type2);
> extern bool contains_bitfld_component_ref_p (const_tree);
> extern bool block_may_fallthru (const_tree);
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 342da55bba7..3d9968fd7a0 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -12810,10 +12810,11 @@ lhd_gcc_personality (void)
> OBJ_TYPE_REF representing an virtual call of C++ method.
> (As opposed to OBJ_TYPE_REF representing objc calls
> through a cast where middle-end devirtualization machinery
> - can't apply.) */
> + can't apply.) FOR_DUMP_P is true when being called from
> + the dump routines. */
>
> bool
> -virtual_method_call_p (const_tree target)
> +virtual_method_call_p (const_tree target, bool for_dump_p)
> {
> if (TREE_CODE (target) != OBJ_TYPE_REF)
> return false;
> @@ -12826,7 +12827,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 (obj_type_ref_class (target, for_dump_p)))
> return false;
> return true;
> }
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 0340decba9b..3ab7049734f 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -1883,10 +1883,11 @@ add_type_duplicate (odr_type val, tree type)
> return build_bases;
> }
>
> -/* REF is OBJ_TYPE_REF, return the class the ref corresponds to. */
> +/* REF is OBJ_TYPE_REF, return the class the ref corresponds to.
> + FOR_DUMP_P is true when being called from the dump routines. */
>
> tree
> -obj_type_ref_class (const_tree ref)
> +obj_type_ref_class (const_tree ref, bool for_dump_p)
> {
> gcc_checking_assert (TREE_CODE (ref) == OBJ_TYPE_REF);
> ref = TREE_TYPE (ref);
> @@ -1902,8 +1903,10 @@ obj_type_ref_class (const_tree ref)
> tree ret = TREE_TYPE (ref);
> if (!in_lto_p && !TYPE_STRUCTURAL_EQUALITY_P (ret))
> ret = TYPE_CANONICAL (ret);
> + else if (odr_type ot = get_odr_type (ret, !for_dump_p))
> + ret = ot->type;
> else
> - ret = get_odr_type (ret)->type;
> + gcc_assert (for_dump_p);
> return ret;
> }
>
> diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
> index 4f50f37b0da..be1ed906c1d 100644
> --- a/gcc/tree-pretty-print.c
> +++ b/gcc/tree-pretty-print.c
> @@ -3167,10 +3167,11 @@ dump_generic_node (pretty_printer *pp, tree node, int
> spc, dump_flags_t flags,
> 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))
> + && virtual_method_call_p (node, true))
> {
> pp_string (pp, "(");
> - dump_generic_node (pp, obj_type_ref_class (node), spc, flags,
> false);
> + dump_generic_node (pp, obj_type_ref_class (node, true),
> + spc, flags, false);
> pp_string (pp, ")");
> }
> dump_generic_node (pp, OBJ_TYPE_REF_OBJECT (node), spc, flags, false);
> diff --git a/gcc/testsuite/g++.target/aarch64/pr95114.C
> b/gcc/testsuite/g++.target/aarch64/pr95114.C
> new file mode 100644
> index 00000000000..1689159e47c
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/aarch64/pr95114.C
> @@ -0,0 +1,3 @@
> +template<typename T> struct foo { virtual void f() = 0; };
> +extern foo<__Int8x8_t> &x;
> +void f() { x.f(); }
