out of bounds access in insn-automata.c

[Aldy Hernandez]

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