[Patch] PR 61692 - Fix for inline asm ICE

[David Wohlferd]

On 7/30/2014 9:58 PM, Jeff Law wrote:
> On 07/28/14 16:39, David Wohlferd wrote:
>>
>> On 7/28/2014 12:42 PM, Jeff Law wrote:
>>> On 07/27/14 01:26, David Wohlferd wrote:
>>>> I'm not sure which maintainer to cc for inline asm stuff?
>>>>
>>>> I have a release on file with the FSF, but don't have SVN write 
>>>> access.
>>>>
>>>> Problem:
>>>> extract_insn() in recog.c will ICE if (noperands > 
>>>> MAX_RECOG_OPERANDS).
>>>> Normally this isn't a problem since expand_asm_operands() in 
>>>> cfgexpand.c
>>>> catches and reports a proper error for this condition. However,
>>>> expand_asm_operands() only checks (ninputs + noutputs) instead of
>>>> (ninputs + noutputs + nlabels), so you can get the ICE when using "asm
>>>> goto."  See the bugzilla entry for sample code.
>>>>
>>>> ChangeLog:
>>>> 2014-07-27  David Wohlferd  <dw@LimeGreenSocks.com>
>>>>
>>>>          PR target/61692
>>>>          * cfgexpand.c (expand_asm_operands): Count all inline asm
>>>> parameters.
>>> You should also include 'nclobbers'.
>>
>> Reading thru asm_noperands (which is what extract_insn uses to count
>> operands), I would have thought you were right.  But while making this
>> fail with nLabels was easy, I wasn't able to get this to ICE at all
>> using clobbers (30 labels + 11 clobbers still didn't ICE).
>>
>> And I'm reluctant to propose that change unless I can see it fail.
> I understand, but I'm still quite confident it's the right thing to 
> do.  Running that 30 label + 11 clobber testcase under valgrind might 
> show the problem, if you can stand waiting that long...

I'd love to.  Unfortunately, my platform doesn't support valgrind.

> Also, please include the testcase you had nlabels part.

I have created the testcase for the 31 labels problem.  However, not so 
much for the nclobbers part.  And if I'm going to patch both, I should 
have testcases for both.

I also worry about potentially breaking existing code.  While the 31 
labels thing I proposed won't break existing code (it would have been 
ICE-ing), adding nclobbers to the count of parameters could do so.

When Jeff Law says that something is so about gcc, I tend to believe 
that it is so.  But, unless I can see an example of the actual problem 
here, I have no idea how to create a test case for it.  So I spent the 
last several hours hunting for it.

If valgrind was going to show the problem, presumably we were expecting 
some type of array overrun.  And there are a LOT of places that do 
foo[MAX_RECOG_OPERANDS].  Absent valgrind, I resorted to using printfs 
sprinkled throughout the code to check indices. However, despite my best 
efforts, I was unable to see any out of range accesses, gcc_asserts, 
segment faults or any other indications of a problem.  All the places I 
tried seem to treat clobbers separately.  That doesn't prove anything: I 
could easily have missed something.  But I did make a sincere effort.

Given how confident you are that there's a problem, could you point out 
a place+condition I should focus on?  That way I can create the 
additional test case, satisfy my curiosity, and sleep soundly at night 
knowing I haven't (unnecessarily) broken people's code.

Thanks,
dw