On Fri, 13 Jul 2018, Jakub Jelinek wrote: > On Fri, Jul 13, 2018 at 01:49:38PM +0200, Richard Biener wrote: > > The testcase in the PR, while previously ICEing because the C++ FE doesn't > > properly capture VLA size fields, now ICEs because gimplification > > introduces SSA uses that appear in a different function than its > > definition. This happens because there is tree sharing between > > the functions. For nested functions (which the C++ lambdas are not) > > such tree sharing ended up being harmless before GCC7 because unnesting > > resolves all locals with wrong origin to the static chain (and > > gimplification ordering made sure definitions always appear in the > > outer function). > > > > The following resolves this by unsharing size expressions in c-common.c > > > > I realize that this may end up pessimizing early code since 1:1 > > tree-sharing with what is gimplified from a DECL_EXPR doesn't > > share the gimplification result. > > I think the unsharing is just wrong, we never want to unshare those, > the SAVE_EXPR expression needs to be evaluated at the DECL_EXPR point and > never anywhere else, never twice (it could have side-effects etc.). > The C++ FE must be fixed to handle the lambda cases.
Well, the SAVE_EXPR isn't the issue - it is the expression wrapping it, in this case ((sizetype) (SAVE_EXPR <(ssizetype) arg + -1>) + 1) * 4) which is in TYPE_SIZE_UNIT. The unsharing we do makes the sizepos get a decl but all other references to the TYPE_SIZE_UNIT expression get gimplified again. And folding may cause the tree sharing to happen not at the TYPE_SIZE_UNIT point but at a sub-expression of that. > > Another option might be to force gimplification to not generate > > SSA temporaries when gimplifying size positions but gimplify_one_sizepos > > oddly enough unshares trees before gimplifying ...(!?) This would > > need to be removed (see patch after the tested patch below). > > I like that gimplify_one_sizepos change much more (I guess we need a > non-lambda testcase). It breaks Ada bootstrap. I guess Ada has variable-size types in non-function scope (not sure how TYPE_SIZES_GIMPLIFIED works then though). That said, r92495 moved the unshare_expr from layout_type to gimplify_one_sizepos. Without removing the unshare_expr the patch is a no-op because gimplifying SAVE_EXPRs already works correctly. > Lambdas were broken even before GCC7, while we might not ICE, we certainly > didn't generate correct code. Well, we did ICE but only during RTL expansion (no GIMPLE verifier checks that automatic vars in a function are in the proper function). But yes, we probably need a non-C++, nested function testcase. Richard.
