This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] PR92090: Fix testcase failures by r276469


on 2019/11/5 上午6:57, Joseph Myers wrote:
> On Mon, 4 Nov 2019, luoxhu wrote:
> 
>> -finline-functions is enabled by default for O2 since r276469, update the
>> test cases with -fno-inline-functions.
>>
>> v2: disable inlining for the failed cases.  Add two more failed cases
>> not listed in BZ.  Tested on P8LE, P8BE and P9LE.
> 
> If inlining (or other interprocedural analysis) invalidates a test's 
> intent (e.g. all the code gets optimized away), our normal approach is to 
> use noinline etc. function attributes to prevent that inlining.
> 
> If you're adding such options to work around an ICE, which certainly 
> appears to be the case in the architecture-independent testcases here, you 
> should (a) have comments in the tests saying explicitly that the options 
> are there temporarily to work around the ICE in a bug whose number is 
> given in the comment, and (b) a remark in the open regression bug for the 
> ICE saying that those options have been added as a temporary workaround 
> and that a patch fixing the ICE should remove them again.  The commit 
> message also needs to make very clear that the commit is *not* a fix for 
> that bug and so it must *not* be closed as fixed until there is an actual 
> fix for the ICE.
> 

Hi Joseph,

Very good point!  Since gcc doesn't pursue 100% testsuite pass rate, I noticed
there are a few failures exposed/caused by some PRs all the time.  Could we
just leave the test case there without any pre workaround till the PR get fixed?
It seems more like what we did often.  Or the good thing with pre workaround
here is to make this case sensitive again for being used for other testing?

BR,
Kewen

> So I don't think this patch is OK without having such comments in the 
> tests to explain the issue and a carefully written commit message warning 
> that the patch is a workaround, not a fix and the bug in question must not 
> be closed simply because of the commit mentioning it.
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]