Re: [Patch] PR 61692 - Fix for inline asm ICE

[David Wohlferd <dw at LimeGreenSocks dot com>]

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