Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)

[Christophe Lyon <christophe.lyon@linaro.org>]

On Thu, 20 Dec 2018 at 01:04, Alexandre Oliva <aoliva@redhat.com> wrote:
>
> Christophe,
>
> Thanks again for the report.  This was quite an adventure to figure
> out ;-)  See below.
>

Glad I've helped. I wouldn't have been able to do the analysis :)


>
> [PR88146] avoid diagnostics diffs if cdtor_returns_this
>
> Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across
> platforms.  Specifically, on ARM, the diagnostics within the subtest
> derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did
> not match those displayed on other platforms, and the test failed.
>
> The difference seemed to have to do with locations assigned to ctors,
> but it was more subtle: on ARM, the instantiation of bor's template
> ctor was nested within the instantiation of bar's template ctor
> inherited from bor.  The reason turned out to be related with the
> internal return type of ctors: arm_cxx_cdtor_returns_this is enabled
> for because of AAPCS, while cxx.cdtor_returns_this is disabled on most
> other platforms.  While convert_to_void returns early with a VOID
> expr, the non-VOID return type of the base ctor CALL_EXPR causes
> convert_to_void to inspect the called decl for nodiscard attributes:
> maybe_warn_nodiscard -> cp_get_fndecl_from_callee ->
> maybe_constant_init -> cxx_eval_outermost_constant_expr ->
> instantiate_constexpr_fns -> nested instantiation.
>
> The internal return type assigned to a cdtor should not affect
> instantiation (constexpr or template) decisions, IMHO.  We know it
> affects diagnostics, but I have a hunch this might bring deeper issues
> with it, so I've arranged for the CALL_EXPR handler in convert_to_void
> to disregard cdtors, regardless of the ABI.
>
>
> The patch is awkward on purpose: it's meant to illustrate both
> portions of the affected code, to draw attention to a potential
> problem, and to get bootstrap-testing coverage for the path that will
> be taken on ARM.  I envision removing the first hunk, and the else
> from the second hunk, once testing is done.
>
> The first hunk is there to highlight where convert_to_void returns
> early on x86, instead of handling the CALL_EXPR.
>
> BTW (here's the potential problem), shouldn't we go into the CALL_EXPR
> case for the volatile void mentioned in comments next to the case, or
> won't that match VOID_TYPE_P?
>
> Finally, I shall mention the possibility of taking the opposite
> direction, and actually looking for nodiscard in cdtor calls so as to
> trigger the constexpr side effects that we've inadvertently triggered
> and observed with the inh-ctor32.C testcase.  It doesn't feel right to
> me, but I've been wrong many times before ;-)
>
> Would a rearranged version of the patch, dropping the redundant tests
> and retaining only the addition of the test for cdtor identifiers, be
> ok to install, provided that it passes regression testing?
>
>
> Note this patch does NOT carry a ChangeLog entry.  That's also on
> purpose, to indicate it's not meant to be included as is.
> ---
>  gcc/cp/cvt.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c
> index eb1687377c3e..1a15af8a6e99 100644
> --- a/gcc/cp/cvt.c
> +++ b/gcc/cp/cvt.c
> @@ -1112,7 +1112,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>          error_at (loc, "pseudo-destructor is not called");
>        return error_mark_node;
>      }
> -  if (VOID_TYPE_P (TREE_TYPE (expr)))
> +  if (VOID_TYPE_P (TREE_TYPE (expr))
> +      && TREE_CODE (expr) != CALL_EXPR)
>      return expr;
>    switch (TREE_CODE (expr))
>      {
> @@ -1169,6 +1170,24 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain)
>        break;
>
>      case CALL_EXPR:   /* We have a special meaning for volatile void fn().  */
> +      /* cdtors may return this or void, depending on
> +        targetm.cxx.cdtor_returns_this, but this shouldn't affect our
> +        decisions here: nodiscard cdtors are nonsensical, and we
> +        don't want to call maybe_warn_nodiscard because it may
> +        trigger constexpr or template instantiation in a way that
> +        changes their instantiaton nesting.  This changes the way
> +        contexts are printed in diagnostics, with bad consequences
> +        for the testsuite, but there may be other undesirable
> +        consequences of visiting referenced ctors too soon.  */
> +      if (DECL_P (TREE_OPERAND (expr, 0))
> +         && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (expr, 0))))
> +       return expr;
> +      /* FIXME: Move this test before the one above, after a round of
> +        testing as it is, to get coverage of the behavior we'd get on
> +        ARM.  */
> +      else if (VOID_TYPE_P (TREE_TYPE (expr)))
> +       return expr;
> +
>        maybe_warn_nodiscard (expr, implicit);
>        break;
>
>
>
> --
> Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
> Be the change, be Free!         FSF Latin America board member
> GNU Toolchain Engineer                Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe