Re: RFA: MIPS: Fix race condition causing PR 69129

[Nick Clifton]

Hi Matthias,


https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69129

this fixes the bootstrap errors for me, seen in both libgnat and libgfortran.


Great - I have gone ahead and checked the patch in.

Cheers
  Nick

Re: RFA: MIPS: Fix race condition causing PR 69129

[Matthias Klose]

On 19.01.2016 14:52, Nick Clifton wrote:

Hi Catherine, Hi Eric, Hi Matthew,

   GCC PR 69129 reports a problem with the MIPS backend:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69129

   I traced the problem down to a race condition in
   mips_compute_frame_info.  This calls mips_global_pointer, which
   through a torturous chain of inferior calls can end up with
   mips_get_cprestore_base_and_offset trying to use the information in
   the frame structure which has yet to be computed...

   The attached patch fixes the problem by moving the initialisation of
   the global_pointer field in the frame structure to after the args_size
   and hard_frame_pointer_offset fields have been initialised.

   Tested with no regressions on a mipsisa32-elf toolchain.  (I know that
   there are lots of different possible mips configurations.  I was not
   sure which one(s) I should test, so I chose one at random).


this fixes the bootstrap errors for me, seen in both libgnat and libgfortran.

RFA: MIPS: Fix race condition causing PR 69129

[Nick Clifton]

Hi Catherine, Hi Eric, Hi Matthew,

  GCC PR 69129 reports a problem with the MIPS backend:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69129

  I traced the problem down to a race condition in
  mips_compute_frame_info.  This calls mips_global_pointer, which
  through a torturous chain of inferior calls can end up with
  mips_get_cprestore_base_and_offset trying to use the information in
  the frame structure which has yet to be computed...

  The attached patch fixes the problem by moving the initialisation of
  the global_pointer field in the frame structure to after the args_size
  and hard_frame_pointer_offset fields have been initialised.

  Tested with no regressions on a mipsisa32-elf toolchain.  (I know that
  there are lots of different possible mips configurations.  I was not
  sure which one(s) I should test, so I chose one at random).

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2016-01-19  Nick Clifton  

PR target/69129
* config/mips/mips.c (mips_compute_frame_info): Move the
initialisation of the global_pointer field to after the
initialisation of the hard_frame_pointer_offset and args_size
fields.

gcc/testsuite/ChangeLog
2016-01-19  Nick Clifton  

PR target/69129
* gcc.target/mips/pr69129.c: New testcase.

Index: gcc/config/mips/mips.c
===
--- gcc/config/mips/mips.c	(revision 232560)
+++ gcc/config/mips/mips.c	(working copy)
@@ -10347,8 +10347,6 @@
   memset (frame, 0, sizeof (*frame));
   size = get_frame_size ();
 
-  cfun->machine->global_pointer = mips_global_pointer ();
-
   /* The first two blocks contain the outgoing argument area and the $gp save
  slot.  This area isn't needed in leaf functions.  We can also skip it
  if we know that none of the called functions will use this space.
@@ -10375,6 +10373,26 @@
   frame->args_size = crtl->outgoing_args_size;
   frame->cprestore_size = MIPS_GP_SAVE_AREA_SIZE;
 }
+
+  /* MIPS16 code offsets the frame pointer by the size of the outgoing
+ arguments.  This tends to increase the chances of using unextended
+ instructions for local variables and incoming arguments.  */
+  if (TARGET_MIPS16)
+frame->hard_frame_pointer_offset = frame->args_size;
+
+  /* PR 69129: Beware of a possible race condition.  mips_global_pointer
+ might call mips_cfun_has_inflexible_gp_ref_p which in turn can call
+ mips_find_gp_ref which will iterate over the current insn sequence.
+ If any of these insns use the cprestore_save_slot_operand or
+ cprestore_load_slot_operand predicates in order to be recognised then
+ they will call mips_cprestore_address_p which calls
+ mips_get_cprestore_base_and_offset which expects the frame information
+ to be filled in...  In fact mips_get_cprestore_base_and_offset only
+ needs the args_size and hard_frame_pointer_offset fields to be filled
+ in, which is why the global_pointer field is initialised here and not
+ earlier.  */
+  cfun->machine->global_pointer = mips_global_pointer ();
+
   offset = frame->args_size + frame->cprestore_size;
 
   /* Move above the local variables.  */
@@ -10520,12 +10538,6 @@
 frame->acc_save_offset = frame->acc_sp_offset - offset;
   if (frame->num_cop0_regs > 0)
 frame->cop0_save_offset = frame->cop0_sp_offset - offset;
-
-  /* MIPS16 code offsets the frame pointer by the size of the outgoing
- arguments.  This tends to increase the chances of using unextended
- instructions for local variables and incoming arguments.  */
-  if (TARGET_MIPS16)
-frame->hard_frame_pointer_offset = frame->args_size;
 }
 
 /* Return the style of GP load sequence that is being used for the
--- /dev/null	2016-01-19 08:08:42.474962807 +
+++ gcc/testsuite/gcc.target/mips/pr69129.c	2016-01-19 13:18:58.554155529 +
@@ -0,0 +1,29 @@
+_Noreturn void fn1 (int) __attribute__((__visibility__("hidden")));
+
+void
+fn2 (void *p1)
+{
+  int a[7];
+  float *b;
+  int c, n;
+
+  if (c != p1) /* { dg-warning "comparison between pointer and integer" } */
+fn1 (1);
+
+  n = 0;
+  for (; c; n++)
+{
+  int d;
+  if (a[n] != d)
+	fn1(n);
+}
+
+  b = p1;
+
+  while (1)
+{
+  *b = 3.40282347e38f;
+  if (a[0])
+	return;
+}
+}