I reverted the original patch and added the assertion instead. Thanks,
David On Sun, Apr 17, 2011 at 2:31 PM, Jan Hubicka <hubi...@ucw.cz> wrote: >> 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 <hubi...@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 >> >> > >> > >
