Re: 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
+
+       * 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",