out of bounds access in insn-automata.c

[Aldy Hernandez]

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
-------------- next part --------------
+
+	* genautomata.c (output_maximal_insn_latency_func): Assert that
+	insn is non-null.
+	(output_internal_insn_code_evaluation): Fix comment to match
+	generated code.

diff --git a/gcc/genautomata.c b/gcc/genautomata.c
index e3a6c59..91abd2e 100644
--- a/gcc/genautomata.c
+++ b/gcc/genautomata.c
@@ -8113,14 +8113,14 @@ output_internal_trans_func (void)
 
 /* Output code
 
-  if (insn != 0)
+  if (insn == 0)
+    insn_code = DFA__ADVANCE_CYCLE;
+  else
     {
       insn_code = dfa_insn_code (insn);
       if (insn_code > DFA__ADVANCE_CYCLE)
         return code;
     }
-  else
-    insn_code = DFA__ADVANCE_CYCLE;
 
   where insn denotes INSN_NAME, insn_code denotes INSN_CODE_NAME, and
   code denotes CODE.  */
@@ -8527,6 +8527,7 @@ output_maximal_insn_latency_func (void)
 	   "maximal_insn_latency", INSN_PARAMETER_NAME);
   fprintf (output_file, "{\n  int %s;\n",
 	   INTERNAL_INSN_CODE_NAME);
+  fprintf (output_file, "  gcc_assert (%s != 0);\n", INSN_PARAMETER_NAME);
   output_internal_insn_code_evaluation (INSN_PARAMETER_NAME,
 					INTERNAL_INSN_CODE_NAME, 0);
   fprintf (output_file, "  return %s (%s, %s);\n}\n\n",