| Message ID | 54E5C403.7050606@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > > > This patch adds handling of internal functions in operand_equal_p. > > I ran into a situation here in operand_equal_p where it segfaulted on the > internal function IFN_VA_ARG, because the CALL_EXPR_FN of an internal > function is NULL, and operand_equal_p does not expect NULL arguments: > ... > case CALL_EXPR: > /* If the CALL_EXPRs call different functions, then they > clearly can not be equal. */ > if (! operand_equal_p (CALL_EXPR_FN (arg0), CALL_EXPR_FN (arg1), > flags)) > return 0; > ... > > This patch fixes that by testing if CALL_EXPR_FN is NULL. > > OK for stage1? I'd call it a bug though, and we do have internal fns in generic already thus the issue is latent (with ubsan at least). Which means ok for trunk now. Thanks, Richard.
On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > I'd call it a bug though, and we do have internal fns in > generic already thus the issue is latent (with ubsan at least). > > Which means ok for trunk now. But the patch should better handle the internal calls right. I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, or if both are NULL and CALL_EXPR_IFN is different, or if call_expr_nargs is different. Jakub
On Thu, 19 Feb 2015, Jakub Jelinek wrote: > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > I'd call it a bug though, and we do have internal fns in > > generic already thus the issue is latent (with ubsan at least). > > > > Which means ok for trunk now. > > But the patch should better handle the internal calls right. > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > or if both are NULL and CALL_EXPR_IFN is different, or if > call_expr_nargs is different. The question is whether generic call handling works (esp. call_expr_flags works correctly - the argument compare should work fine already). Tom - care to update the patch? Thanks, Richard.
On 19-02-15 14:07, Richard Biener wrote: > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > >> On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: >>> I'd call it a bug though, and we do have internal fns in >>> generic already thus the issue is latent (with ubsan at least). >>> >>> Which means ok for trunk now. >> >> But the patch should better handle the internal calls right. >> I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, >> or if both are NULL and CALL_EXPR_IFN is different, or if >> call_expr_nargs is different. > > The question is whether generic call handling works (esp. call_expr_flags > works correctly - the argument compare should work fine already). > > Tom - care to update the patch? > I agree, the current patch is conservative and we can do better. But I think it's wiser to do that as a stage1 follow-up, and commit this conservative patch for stage4. Is that acceptable? Thanks, - Tom
On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 14:07, Richard Biener wrote: > > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > > > > > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > > > I'd call it a bug though, and we do have internal fns in > > > > generic already thus the issue is latent (with ubsan at least). > > > > > > > > Which means ok for trunk now. > > > > > > But the patch should better handle the internal calls right. > > > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > > > or if both are NULL and CALL_EXPR_IFN is different, or if > > > call_expr_nargs is different. > > > > The question is whether generic call handling works (esp. call_expr_flags > > works correctly - the argument compare should work fine already). > > > > Tom - care to update the patch? > > > > I agree, the current patch is conservative and we can do better. > But I think it's wiser to do that as a stage1 follow-up, and commit this > conservative patch for stage4. Is that acceptable? Then just defer it for stage1 completely. If a problem pops up with GCC 5 we can backport the proper patch together with a testcase. Richard.
2015-02-17 Tom de Vries <tom@codesourcery.com> * fold-const.c (operand_equal_p): Handle INTERNAL_FNs. --- gcc/fold-const.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 8377120..fbf76d0 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -3032,6 +3032,11 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags) switch (TREE_CODE (arg0)) { case CALL_EXPR: + /* Handle internal_fns conservatively. */ + if (CALL_EXPR_FN (arg0) == NULL_TREE + || CALL_EXPR_FN (arg1) == NULL_TREE) + return 0; + /* If the CALL_EXPRs call different functions, then they clearly can not be equal. */ if (! operand_equal_p (CALL_EXPR_FN (arg0), CALL_EXPR_FN (arg1), -- 1.9.1