This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: FDO usability patch -- correct insane profile data
> Adding assertion sounds good to me -- the only problem is that problem
> like this hard to be triggered during testing, thus making assertion
> less useful as it can be. Is it better to add the assertion with
> checking is enabled while still keeping the correction code?
Hi,
since we speak about code that has minimal to none testing coverage in our
automates testers (we don't really test anything multithreaded or with mismatching
profiles), I don't think gcc_checking_assert would make any difference.
Since we don't really want the future bug to stay around unnoticed (since it
would resolut in silent misupdates of profiles), we are only shooting to make
analysis easier so next time someone trips around the scenario he won't see
symptomatic message from cgraph verifier.
So I would go for normal assert and comment about nature of the bug.
Honza
>
> Thanks,
>
> David
>
> On Sun, Apr 17, 2011 at 11:53 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > hmm, looks we both run into same issue and developed different fixes.
> >>
> >> Good point. The change above was added based on 4.4.3 where the sum
> >> computation has no capping like above. Yes, with the current capping,
> >> the negative value won't result. However, it is still good to guard
> >> the scale independent of the implementation of ipcp_compute_node_scale
> >> -- it may change and break silently (the comment of the function says
> >> the implementation is wrong...)
> >
> > Well, if we want to add check in anticipation that we will introduce bug few
> > lines up, I think we could just add gcc_assert (scale < BASE); with an comment
> > explaining that ipcp_compute_node_scale must give results in range even when
> > profile is not flow consistent.
> >
> > Since we need to propagate the scales across callgraph (at least for the
> > correct implementation of the function), masking bug in ipcp_compute_node_scale
> > would make us to propagate the nonsenses further and silently producing
> > unnecesarily aplified insanity.
> >
> > I would pre-approve patch reverting the current change and adding the assert
> > with comment.
> >
> > Honza
> >>
> >> Thanks,
> >>
> >> David
> >>
> >>
> >> >
> >> > Honza
> >> >
> >