diff mbox series

Fix ICE in categorize_decl_for_section with TLS decl (PR middle-end/82095)

Message ID 20170905211653.GH2323@tucnak
State New
Headers show
Series Fix ICE in categorize_decl_for_section with TLS decl (PR middle-end/82095) | expand

Commit Message

Jakub Jelinek Sept. 5, 2017, 9:16 p.m. UTC 
Hi!

If a DECL_THREAD_LOCAL_P decl has NULL DECL_INITIAL and
-fzero-initialized-in-bss (the default), we ICE starting with
r251602, which changed bss_initializer_p:
+  /* Do not put constants into the .bss section, they belong in a readonly
+     section.  */
+  return (!TREE_READONLY (decl)
+	  &&
to:
          (DECL_INITIAL (decl) == NULL
              /* In LTO we have no errors in program; error_mark_node is used
                 to mark offlined constructors.  */
              || (DECL_INITIAL (decl) == error_mark_node
                  && !in_lto_p)
              || (flag_zero_initialized_in_bss
                  && initializer_zerop (DECL_INITIAL (decl))))
Previously because bss_initializer_p for these returned true, ret was
SECCAT_BSS and therefore we set it to SECCAT_TBSS as intended, but now ret
is not SECCAT_BSS, but as TLS has only tbss and tdata possibilities, we
still want to use tbss.  DECL_INITIAL NULL for a decl means implicit zero
initialization.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2017-09-05  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/82095
	* varasm.c (categorize_decl_for_section): Use SECCAT_TBSS for TLS vars with
	NULL DECL_INITIAL.

	* gcc.dg/tls/pr82095.c: New test.


	Jakub

Comments

Richard Biener Sept. 6, 2017, 5:24 a.m. UTC | #1 
On September 5, 2017 11:16:53 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>If a DECL_THREAD_LOCAL_P decl has NULL DECL_INITIAL and
>-fzero-initialized-in-bss (the default), we ICE starting with
>r251602, which changed bss_initializer_p:
>+  /* Do not put constants into the .bss section, they belong in a
>readonly
>+     section.  */
>+  return (!TREE_READONLY (decl)
>+	  &&
>to:
>          (DECL_INITIAL (decl) == NULL
>        /* In LTO we have no errors in program; error_mark_node is used
>                 to mark offlined constructors.  */
>              || (DECL_INITIAL (decl) == error_mark_node
>                  && !in_lto_p)
>              || (flag_zero_initialized_in_bss
>                  && initializer_zerop (DECL_INITIAL (decl))))
>Previously because bss_initializer_p for these returned true, ret was
>SECCAT_BSS and therefore we set it to SECCAT_TBSS as intended, but now
>ret
>is not SECCAT_BSS, but as TLS has only tbss and tdata possibilities, we
>still want to use tbss.  DECL_INITIAL NULL for a decl means implicit
>zero
>initialization.
>
>Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok
>for
>trunk?

OK. 

Richard. 

>2017-09-05  Jakub Jelinek  <jakub@redhat.com>
>
>	PR middle-end/82095
>	* varasm.c (categorize_decl_for_section): Use SECCAT_TBSS for TLS vars
>with
>	NULL DECL_INITIAL.
>
>	* gcc.dg/tls/pr82095.c: New test.
>
>--- gcc/varasm.c.jj	2017-09-01 18:43:29.000000000 +0200
>+++ gcc/varasm.c	2017-09-04 12:29:10.166564776 +0200
>@@ -6562,8 +6562,9 @@ categorize_decl_for_section (const_tree
>      /* Note that this would be *just* SECCAT_BSS, except that there's
> 	 no concept of a read-only thread-local-data section.  */
>       if (ret == SECCAT_BSS
>-	       || (flag_zero_initialized_in_bss
>-		   && initializer_zerop (DECL_INITIAL (decl))))
>+	  || DECL_INITIAL (decl) == NULL
>+	  || (flag_zero_initialized_in_bss
>+	      && initializer_zerop (DECL_INITIAL (decl))))
> 	ret = SECCAT_TBSS;
>       else
> 	ret = SECCAT_TDATA;
>--- gcc/testsuite/gcc.dg/tls/pr82095.c.jj	2017-09-04 12:44:16.650538220
>+0200
>+++ gcc/testsuite/gcc.dg/tls/pr82095.c	2017-09-04 12:44:08.000000000
>+0200
>@@ -0,0 +1,16 @@
>+/* PR middle-end/82095 */
>+/* { dg-do compile } */
>+/* { dg-options "-Og -fno-tree-ccp" } */
>+/* { dg-require-effective-target tls } */
>+/* { dg-add-options tls } */
>+
>+static int b;
>+static __thread int c;
>+
>+void
>+foo (void)
>+{
>+  if (b)
>+    if (c)
>+      b = 1;
>+}
>
>	Jakub
Jeff Law Sept. 6, 2017, 3:29 p.m. UTC | #2 
On 09/05/2017 03:16 PM, Jakub Jelinek wrote:
> Hi!
> 
> If a DECL_THREAD_LOCAL_P decl has NULL DECL_INITIAL and
> -fzero-initialized-in-bss (the default), we ICE starting with
> r251602, which changed bss_initializer_p:
> +  /* Do not put constants into the .bss section, they belong in a readonly
> +     section.  */
> +  return (!TREE_READONLY (decl)
> +	  &&
> to:
>           (DECL_INITIAL (decl) == NULL
>               /* In LTO we have no errors in program; error_mark_node is used
>                  to mark offlined constructors.  */
>               || (DECL_INITIAL (decl) == error_mark_node
>                   && !in_lto_p)
>               || (flag_zero_initialized_in_bss
>                   && initializer_zerop (DECL_INITIAL (decl))))
> Previously because bss_initializer_p for these returned true, ret was
> SECCAT_BSS and therefore we set it to SECCAT_TBSS as intended, but now ret
> is not SECCAT_BSS, but as TLS has only tbss and tdata possibilities, we
> still want to use tbss.  DECL_INITIAL NULL for a decl means implicit zero
> initialization.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
> 
> 2017-09-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/82095
> 	* varasm.c (categorize_decl_for_section): Use SECCAT_TBSS for TLS vars with
> 	NULL DECL_INITIAL.
> 
> 	* gcc.dg/tls/pr82095.c: New test.
THanks.  Sorry about the breakage.  TLS didn't even cross my mind.
Presumably the TLS initialization sections are readonly and copied into
the actual thread specific locations.

Jeff
Jakub Jelinek Sept. 6, 2017, 3:39 p.m. UTC | #3 
On Wed, Sep 06, 2017 at 09:29:25AM -0600, Jeff Law wrote:
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> > 
> > 2017-09-05  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR middle-end/82095
> > 	* varasm.c (categorize_decl_for_section): Use SECCAT_TBSS for TLS vars with
> > 	NULL DECL_INITIAL.
> > 
> > 	* gcc.dg/tls/pr82095.c: New test.
> THanks.  Sorry about the breakage.  TLS didn't even cross my mind.
> Presumably the TLS initialization sections are readonly and copied into
> the actual thread specific locations.

.tbss section is just in headers (it implies zeroing the corresponding
thread private chunk) and .tdata is in relro part (the image; it might
contain relocations and we don't support separate images for parts without
and with relocations), copied to the thread private chunk.

	Jakub
diff mbox series

Patch

--- gcc/varasm.c.jj	2017-09-01 18:43:29.000000000 +0200
+++ gcc/varasm.c	2017-09-04 12:29:10.166564776 +0200
@@ -6562,8 +6562,9 @@  categorize_decl_for_section (const_tree
       /* Note that this would be *just* SECCAT_BSS, except that there's
 	 no concept of a read-only thread-local-data section.  */
       if (ret == SECCAT_BSS
-	       || (flag_zero_initialized_in_bss
-		   && initializer_zerop (DECL_INITIAL (decl))))
+	  || DECL_INITIAL (decl) == NULL
+	  || (flag_zero_initialized_in_bss
+	      && initializer_zerop (DECL_INITIAL (decl))))
 	ret = SECCAT_TBSS;
       else
 	ret = SECCAT_TDATA;
--- gcc/testsuite/gcc.dg/tls/pr82095.c.jj	2017-09-04 12:44:16.650538220 +0200
+++ gcc/testsuite/gcc.dg/tls/pr82095.c	2017-09-04 12:44:08.000000000 +0200
@@ -0,0 +1,16 @@ 
+/* PR middle-end/82095 */
+/* { dg-do compile } */
+/* { dg-options "-Og -fno-tree-ccp" } */
+/* { dg-require-effective-target tls } */
+/* { dg-add-options tls } */
+
+static int b;
+static __thread int c;
+
+void
+foo (void)
+{
+  if (b)
+    if (c)
+      b = 1;
+}