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?
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?
2025-01-20 Jakub Jelinek <[email protected]>
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
