On Thu, 20 Dec 2018 at 01:04, Alexandre Oliva <aol...@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
