On Mon, 20 Jan 2025, Jakub Jelinek wrote:
> Hi!
>
> My October PR117259 fix to get_member_function_from_ptrfunc to use a
> TARGET_EXPR rather than SAVE_EXPR unfortunately caused some regressions as
> well as the following testcase shows.
> What happens is that
> get_member_function_from_ptrfunc -> build_base_path calls save_expr,
> so since the PR117259 change in mnay cases it will call save_expr on
> a TARGET_EXPR. And, for some strange reason a TARGET_EXPR is not considered
> an invariant, so we get a SAVE_EXPR wrapped around the TARGET_EXPR.
> That SAVE_EXPR <TARGET_EXPR <...>> gets initially added only to the second
> operand of ?:, so at that point it would still work fine during expansion.
> But unfortunately an expression with that subexpression is handed to the
> caller also through *instance_ptrptr = instance_ptr; and gets evaluated
> once again when computing the first argument to the method.
> So, essentially, we end up with
> (TARGET_EXPR <D.2907, ...>, (... ? ... SAVE_EXPR <TARGET_EXPR <D.2907, ...>
> ... : ...)) (... SAVE_EXPR <TARGET_EXPR <D.2907, ...> ..., ...);
> and while D.2907 is initialized during gimplification in the code dominating
> everything that uses it, the extra temporary created for the SAVE_EXPR
> is initialized only conditionally (if the ?: condition is true) but then
> used unconditionally, so we get
> pmf-4.C: In function ‘void foo(C, B*)’:
> pmf-4.C:12:11: warning: ‘<anonymous>’ may be used uninitialized
> [-Wmaybe-uninitialized]
> 12 | (y->*x) ();
> | ~~~~~~~~^~
> pmf-4.C:12:11: note: ‘<anonymous>’ was declared here
> 12 | (y->*x) ();
> | ~~~~~~~~^~
> diagnostic and wrong-code issue too.
>
> The following patch fixes it by considering a TARGET_EXPR invariant
> for SAVE_EXPR purposes the same as SAVE_EXPR is. Really creating another
> temporary for it is just a waste of the IL.
>
> Unfortunately I had to tweak the omp matching code to be able to accept
> TARGET_EXPR the same as SAVE_EXPR.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK (it really makes sense). I do wonder whether there's going to be
more fallout similar to the OMP one - did you try grepping for SAVE_EXPR
checks?
> Unfortunately, the PR117259 fix (and earlier fixes to that area) has been
> also backported to 14 branch, so it causes a P1 wrong-code there as well.
> And, I'm not sure I feel 100% confident to do the same thing on the branch.
> So wonder if on the branch something like
> --- gcc/cp/typeck.cc.jj 2025-01-18 21:49:40.736188461 +0100
> +++ gcc/cp/typeck.cc 2025-01-20 10:06:34.786837002 +0100
> @@ -4219,8 +4219,8 @@ get_member_function_from_ptrfunc (tree *
> && !DECL_P (instance_ptr)
> && !TREE_CONSTANT (instance_ptr)))
> instance_ptr = instance_save_expr
> - = force_target_expr (TREE_TYPE (instance_ptr), instance_ptr,
> - complain);
> + = save_expr (force_target_expr (TREE_TYPE (instance_ptr),
> + instance_ptr, complain));
>
> /* See above comment. */
> if (TREE_SIDE_EFFECTS (function)
> @@ -4228,7 +4228,8 @@ get_member_function_from_ptrfunc (tree *
> && !DECL_P (function)
> && !TREE_CONSTANT (function)))
> function
> - = force_target_expr (TREE_TYPE (function), function, complain);
> + = save_expr (force_target_expr (TREE_TYPE (function), function,
> + complain));
>
> /* Start by extracting all the information from the PMF itself. */
> e3 = pfn_from_ptrmemfunc (function);
> wouldn't be safer. Your thoughts on this?
I'm not really happy doing two different things - if we're not
comfortable with the trunk variant what about reverting the causing
change?
IMO backporting of the trunk fix after some soaking time should be
prefered.
Richard.
> 2025-01-20 Jakub Jelinek <ja...@redhat.com>
>
> PR c++/118509
> gcc/
> * tree.cc (tree_invariant_p_1): Return true for TARGET_EXPR too.
> gcc/c-family/
> * c-omp.cc (c_finish_omp_for): Handle TARGET_EXPR in first operand
> of COMPOUND_EXPR incr the same as SAVE_EXPR.
> gcc/testsuite/
> * g++.dg/expr/pmf-4.C: New test.
>
> --- gcc/tree.cc.jj 2025-01-02 11:23:23.443420160 +0100
> +++ gcc/tree.cc 2025-01-18 12:01:08.714037463 +0100
> @@ -3922,6 +3922,7 @@ tree_invariant_p_1 (tree t)
> switch (TREE_CODE (t))
> {
> case SAVE_EXPR:
> + case TARGET_EXPR:
> return true;
>
> case ADDR_EXPR:
> --- gcc/c-family/c-omp.cc.jj 2025-01-17 11:29:33.192695634 +0100
> +++ gcc/c-family/c-omp.cc 2025-01-19 22:32:51.295087212 +0100
> @@ -1195,7 +1195,8 @@ c_finish_omp_for (location_t locus, enum
> break;
>
> case COMPOUND_EXPR:
> - if (TREE_CODE (TREE_OPERAND (incr, 0)) != SAVE_EXPR
> + if ((TREE_CODE (TREE_OPERAND (incr, 0)) != SAVE_EXPR
> + && TREE_CODE (TREE_OPERAND (incr, 0)) != TARGET_EXPR)
> || TREE_CODE (TREE_OPERAND (incr, 1)) != MODIFY_EXPR)
> break;
> incr = TREE_OPERAND (incr, 1);
> --- gcc/testsuite/g++.dg/expr/pmf-4.C.jj 2025-01-18 12:04:44.838018784
> +0100
> +++ gcc/testsuite/g++.dg/expr/pmf-4.C 2025-01-18 12:03:14.505279524 +0100
> @@ -0,0 +1,22 @@
> +// PR c++/118509
> +// { dg-do run }
> +// { dg-options "-Wall -O2" }
> +
> +struct A { void foo () { a = 1; } int a; A () : a (0) {} };
> +struct B : virtual A {};
> +typedef void (A::*C) ();
> +
> +__attribute__((noipa)) void
> +foo (C x, B *y)
> +{
> + (y->*x) ();
> +}
> +
> +int
> +main ()
> +{
> + B b;
> + foo (&A::foo, &b);
> + if (b.a != 1)
> + __builtin_abort ();
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
