| Message ID | 2fcc0cd7-be3e-8a5c-d742-589312a30eae@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Fix PR 92113 | expand |
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.
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
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; }
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.