[fortran] Fix PR 92113
diff mbox series

Message ID 2fcc0cd7-be3e-8a5c-d742-589312a30eae@netcologne.de
State New
Headers show
Series
  • [fortran] Fix PR 92113
Related show

Commit Message

Thomas Koenig Nov. 2, 2019, 9:38 a.m. UTC
Hello world,

the attached patch fixes an 8/9/10 regression where, to fix PR 84487
by not putting the initializers and vtabs into the read-only section
(for reasons of size, which could grow enormously) led to a regression
on POWER9 and other non-x86 architectures, where the initializer was
sometimes optimized away, depending on optimization levels.

This was a strange beast to hunt down. This only showed up on
the testresults for gcc 8, so I tried to find out what commit
had fixed this on trunk, in order to backport.

However, bisecting this I found that the test case actually
segfaults all the way up to current trunk when run by hand.
By running the testsuite, I didn't see it.  This is strange,
and raises some issues about the testsuite and the possibility
of a latent issue, but I lack the knowledge to hunt this down.

In the meantime, here is this patch, which puts the vtabs and
the initializers where the user actually specified something
into the read-only section again.

Test case: Well, theoretically it is already there, so it makes
little sense to add a new one.

Regression-tested on powerpc64le-unknown-linux-gnu, also
verified by hand that pr51434.f90 now passes with -O2 there.

OK for trunk/9/8?

Regards

	Thomas

2019-11-02  Thomas Koenig  <tkoenig@gcc.gnu.org> 

 

         PR fortran/92133 

         * trans-decl.c (gfc_get_symbol_decl): If __def_init actually 

         contains a value, put it into  the read-only section.

Comments

Steve Kargl Nov. 3, 2019, 7:53 p.m. UTC | #1
On Sat, Nov 02, 2019 at 10:38:32AM +0100, Thomas Koenig wrote:
> 
> the attached patch fixes an 8/9/10 regression where, to fix PR 84487
> by not putting the initializers and vtabs into the read-only section
> (for reasons of size, which could grow enormously) led to a regression
> on POWER9 and other non-x86 architectures, where the initializer was
> sometimes optimized away, depending on optimization levels.
> 
> This was a strange beast to hunt down. This only showed up on
> the testresults for gcc 8, so I tried to find out what commit
> had fixed this on trunk, in order to backport.
> 
> However, bisecting this I found that the test case actually
> segfaults all the way up to current trunk when run by hand.
> By running the testsuite, I didn't see it.  This is strange,
> and raises some issues about the testsuite and the possibility
> of a latent issue, but I lack the knowledge to hunt this down.
> 
> In the meantime, here is this patch, which puts the vtabs and
> the initializers where the user actually specified something
> into the read-only section again.
> 
> Test case: Well, theoretically it is already there, so it makes
> little sense to add a new one.
> 
> Regression-tested on powerpc64le-unknown-linux-gnu, also
> verified by hand that pr51434.f90 now passes with -O2 there.
> 
> OK for trunk/9/8?

OK for all three.

It is, as you have indicated, troublesome that a segfaulting
testcase isn't caught by the testsuite.
Thomas Koenig Nov. 3, 2019, 10:38 p.m. UTC | #2
Hi Steve,

>> OK for trunk/9/8?
> 
> OK for all three.

Thanks, committed to trunk as r277760.

I'll be AFK for a few days, so I will have to wait before
committing this to gcc-9. Given the convoluted history of
this bug, this might not be a bad thing.

 > It is, as you have indicated, troublesome that a segfaulting
 > testcase isn't caught by the testsuite.

It certainly is, but I have no solution for this at the moment.

Thanks for the review!

Regards

	Thomas

Patch
diff mbox series

Index: trans-decl.c
===================================================================
--- trans-decl.c	(Revision 277486)
+++ trans-decl.c	(Arbeitskopie)
@@ -1911,14 +1911,19 @@  gfc_get_symbol_decl (gfc_symbol * sym)
   if (sym->attr.associate_var)
     GFC_DECL_ASSOCIATE_VAR_P (decl) = 1;
 
-  /* We no longer mark __def_init as read-only so it does not take up
-     space in the read-only section and dan go into the BSS instead,
-     see PR 84487.  Marking this as artificial means that OpenMP will
-     treat this as predetermined shared.  */
-  if (sym->attr.vtab
-      || (sym->name[0] == '_' && gfc_str_startswith (sym->name, "__def_init")))
-    DECL_ARTIFICIAL (decl) = 1;
+  /* We only mark __def_init as read-only if it actually has an
+     initializer so it does not needlessly take up space in the
+     read-only section and can go into the BSS instead, see PR 84487.
+     Marking this as artificial means that OpenMP will treat this as
+     predetermined shared.  */
 
+  if (sym->attr.vtab || gfc_str_startswith (sym->name, "__def_init"))
+    {
+      DECL_ARTIFICIAL (decl) = 1;
+      if (sym->attr.vtab || sym->value)
+	TREE_READONLY (decl) = 1;
+    }
+
   return decl;
 }