out of bounds access in insn-automata.c

On 03/23/2016 10:25 AM, Bernd Schmidt wrote:
> On 03/23/2016 07:32 AM, Aldy Hernandez wrote:
>>
>> int
>> maximal_insn_latency (rtx insn)
>> {
>>    int insn_code;
>>
>>    if (insn == 0)
>>      insn_code = DFA__ADVANCE_CYCLE;
>>
>>
>>    else
>>      {
>>        insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
>>        if (insn_code > DFA__ADVANCE_CYCLE)
>>          return 0;
>>      }
>>    return internal_maximal_insn_latency (insn_code, insn);
>> }
>>
>> In the case where insn==0, insn_code is set to the size of
>> default_latencies[] which will get accessed in the return.
>>
>> Does insn==0 never happen?
>
> I suspect it never happens in this function. I'd add a gcc_assert to
> that effect and try a bootstrap/test. Hmm, it seems to be a sel-sched
> only thing so a normal bootstrap would be meaningless, but from the
> context it looks fairly clearly like insn is always nonnull.

Vlad.  Bernd.  Thanks for your input.

I've added the assert on the caller (maximal_insn_latency), because as 
you mentioned, the helper function is used for other things in which 
insn==0 can happen.

Now we generate:


int
maximal_insn_latency (rtx insn)
{
   int insn_code;
   gcc_assert (insn != 0);	// <<<<--- Added code.

   if (insn == 0)
     insn_code = DFA__ADVANCE_CYCLE;


   else
     {
       insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
       if (insn_code > DFA__ADVANCE_CYCLE)
         return 0;
     }
   return internal_maximal_insn_latency (insn_code, insn);
}

>
> It looks like this block of code is written by a helper function that is
> really intended for other purposes than for maximal_insn_latency. Might
> be worth changing to
>   int insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
>   gcc_assert (insn_code <= DFA__ADVANCE_CYCLE);

dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't 
put that assert on the helper function.

While I was at it, I changed the helper function comment to reflect what 
it has been generating.  It was wrong.

First round of tests was ok, but test machine died.  OK pending tests?

Aldy
+
+	* genautomata.c (output_maximal_insn_latency_func): Assert that
+	insn is non-null.
+	(output_internal_insn_code_evaluation): Fix comment to match
+	generated code.

On 03/24/2016 11:17 AM, Aldy Hernandez wrote:
> On 03/23/2016 10:25 AM, Bernd Schmidt wrote:
>> It looks like this block of code is written by a helper function that is
>> really intended for other purposes than for maximal_insn_latency. Might
>> be worth changing to
>>   int insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
>>   gcc_assert (insn_code <= DFA__ADVANCE_CYCLE);
>
> dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't
> put that assert on the helper function.

So don't use the helper function? Just emit the block above directly.


Bernd

Hi,

On Thu, 24 Mar 2016, Bernd Schmidt wrote:
> On 03/24/2016 11:17 AM, Aldy Hernandez wrote:
> > On 03/23/2016 10:25 AM, Bernd Schmidt wrote:
> > > It looks like this block of code is written by a helper function that is
> > > really intended for other purposes than for maximal_insn_latency. Might
> > > be worth changing to
> > >   int insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
> > >   gcc_assert (insn_code <= DFA__ADVANCE_CYCLE);
> >
> > dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't
> > put that assert on the helper function.
> 
> So don't use the helper function? Just emit the block above directly.

Let me chime in :)  The function under scrutiny, maximal_insn_latency, was
added as part of selective scheduling merge; at the same time,
output_default_latencies was factored out of
output_internal_insn_latency_func, and the pair of new functions
output_internal_maximal_insn_latency_func/output_maximal_insn_latency_func
tried to mirror existing pair of
output_internal_insn_latency_func/output_insn_latency_func.

In particular, output_insn_latency_func also invokes
output_internal_insn_code_evaluation (twice, for each argument).  This means
that generated 'insn_latency' can also call 'internal_insn_latency' with
DFA__ADVANCE_CYCLE in arguments.  However, 'internal_insn_latency' then has a
specially emitted 'if' statement that checks if either of the arguments is 
' >= DFA__ADVANCE_CYCLE', and returns 0 in that case.

So ultimately pre-existing code was checking ' > DFA__ADVANCE_CYCLE' first and
' >= DFA_ADVANCE_CYCLE' second (for no good reason as far as I can see), and
when the new '_maximal_' functions were introduced, the second check was not
duplicated in the new copy.

So as long we are not looking for hacking it up further, I'd like to clean up
both functions at the same time.  If calling the 'internal_' variants with
DFA__ADVANCE_CYCLE is rare, extending 'default_insn_latencies' by 1 zero
element corresponding to DFA__ADVANCE_CYCLE is a simple suitable fix. If
either DFA__ADVANCE_CYCLE is not guaranteed to be rare, or extending the table
in that style is undesired, I suggest creating a variant of
'output_internal_insn_code_evaluation' that performs a '>=' rather than '>'
test in the first place, and use it in both output_insn_latency_func and
output_maximal_insn_latency_func.  If acknowledged, I volunteer to regstrap on
x86_64 and submit that in stage1.

Thoughts?

Thanks.
Alexander

On 03/24/2016 10:02 AM, Alexander Monakov wrote:
> Hi,
>
> On Thu, 24 Mar 2016, Bernd Schmidt wrote:
>> On 03/24/2016 11:17 AM, Aldy Hernandez wrote:
>>> On 03/23/2016 10:25 AM, Bernd Schmidt wrote:
>>>> It looks like this block of code is written by a helper function that is
>>>> really intended for other purposes than for maximal_insn_latency. Might
>>>> be worth changing to
>>>>    int insn_code = dfa_insn_code (as_a <rtx_insn *> (insn));
>>>>    gcc_assert (insn_code <= DFA__ADVANCE_CYCLE);
>>>
>>> dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't
>>> put that assert on the helper function.
>>
>> So don't use the helper function? Just emit the block above directly.
>
> Let me chime in :)  The function under scrutiny, maximal_insn_latency, was
> added as part of selective scheduling merge; at the same time,
> output_default_latencies was factored out of
> output_internal_insn_latency_func, and the pair of new functions
> output_internal_maximal_insn_latency_func/output_maximal_insn_latency_func
> tried to mirror existing pair of
> output_internal_insn_latency_func/output_insn_latency_func.
>
> In particular, output_insn_latency_func also invokes
> output_internal_insn_code_evaluation (twice, for each argument).  This means
> that generated 'insn_latency' can also call 'internal_insn_latency' with
> DFA__ADVANCE_CYCLE in arguments.  However, 'internal_insn_latency' then has a
> specially emitted 'if' statement that checks if either of the arguments is
> ' >= DFA__ADVANCE_CYCLE', and returns 0 in that case.
>
> So ultimately pre-existing code was checking ' > DFA__ADVANCE_CYCLE' first and
> ' >= DFA_ADVANCE_CYCLE' second (for no good reason as far as I can see), and
> when the new '_maximal_' functions were introduced, the second check was not
> duplicated in the new copy.
>
> So as long we are not looking for hacking it up further, I'd like to clean up
> both functions at the same time.  If calling the 'internal_' variants with
> DFA__ADVANCE_CYCLE is rare, extending 'default_insn_latencies' by 1 zero
> element corresponding to DFA__ADVANCE_CYCLE is a simple suitable fix. If
> either DFA__ADVANCE_CYCLE is not guaranteed to be rare, or extending the table
> in that style is undesired, I suggest creating a variant of
> 'output_internal_insn_code_evaluation' that performs a '>=' rather than '>'
> test in the first place, and use it in both output_insn_latency_func and
> output_maximal_insn_latency_func.  If acknowledged, I volunteer to regstrap on
> x86_64 and submit that in stage1.
>
> Thoughts?

If Bernd is fine with this, I'm happy to retract my patch and any 
possible followups.  I'm just interested in having no path causing a 
possible out of bounds access.  If your patch will do that, I'm cool.

Aldy

On 03/25/2016 04:43 AM, Aldy Hernandez wrote:
> If Bernd is fine with this, I'm happy to retract my patch and any
> possible followups.  I'm just interested in having no path causing a
> possible out of bounds access.  If your patch will do that, I'm cool.

I'll need to see that patch first to comment :-)


Bernd