PR gcc/68256 Defining TARGET_USE_CONSTANT_BLOCKS_P causes go bootstrap failure on aarch64.

Message ID 1518801506-27652-1-git-send-email-vladimir.mezentsev@oracle.com
State New
Headers show
Series
  • PR gcc/68256 Defining TARGET_USE_CONSTANT_BLOCKS_P causes go bootstrap failure on aarch64.
Related show

Commit Message

vladimir.mezentsev@oracle.com Feb. 16, 2018, 5:18 p.m.
From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>


Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve
bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).
The real bug is in gcc/varasm.c.
hash_section() returns an unstable value.
As result, two blocks are created in get_block_for_section() for one unnamed section.
A list of objects in these blocks depends on the -gtoggle option.
I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and
I fixed hash_section() in gcc/varasm.c

Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).
Testing finished ok.

ChangeLog:
2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

PR gcc/68256
* varasm.c (hash_section): Return an unchangeble hash value
* config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):
Return !aarch64_can_use_per_function_literal_pools_p ();
---
 gcc/config/aarch64/aarch64.c | 8 +++-----
 gcc/varasm.c                 | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

-- 
1.8.3.1

Comments

Richard Biener Feb. 26, 2018, 11:07 a.m. | #1
On Fri, Feb 16, 2018 at 6:18 PM,  <vladimir.mezentsev@oracle.com> wrote:
> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>

>

> Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve

> bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).

> The real bug is in gcc/varasm.c.

> hash_section() returns an unstable value.

> As result, two blocks are created in get_block_for_section() for one unnamed section.

> A list of objects in these blocks depends on the -gtoggle option.

> I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and

> I fixed hash_section() in gcc/varasm.c

>

> Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).

> Testing finished ok.


Ok.

Thanks,
Richard.

> ChangeLog:

> 2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>

> PR gcc/68256

> * varasm.c (hash_section): Return an unchangeble hash value

> * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

> Return !aarch64_can_use_per_function_literal_pools_p ();

> ---

>  gcc/config/aarch64/aarch64.c | 8 +++-----

>  gcc/varasm.c                 | 2 +-

>  2 files changed, 4 insertions(+), 6 deletions(-)

>

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> index 174310c..a0a495d 100644

> --- a/gcc/config/aarch64/aarch64.c

> +++ b/gcc/config/aarch64/aarch64.c

> @@ -7596,11 +7596,9 @@ aarch64_can_use_per_function_literal_pools_p (void)

>  static bool

>  aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)

>  {

> -  /* Fixme:: In an ideal world this would work similar

> -     to the logic in aarch64_select_rtx_section but this

> -     breaks bootstrap in gcc go.  For now we workaround

> -     this by returning false here.  */

> -  return false;

> +  /* We can't use blocks for constants when we're using a per-function

> +     constant pool.  */

> +  return !aarch64_can_use_per_function_literal_pools_p ();

>  }

>

>  /* Select appropriate section for constants depending

> diff --git a/gcc/varasm.c b/gcc/varasm.c

> index b045efa..5aae5b4 100644

> --- a/gcc/varasm.c

> +++ b/gcc/varasm.c

> @@ -225,7 +225,7 @@ hash_section (section *sect)

>  {

>    if (sect->common.flags & SECTION_NAMED)

>      return htab_hash_string (sect->named.name);

> -  return sect->common.flags;

> +  return sect->common.flags & ~SECTION_DECLARED;

>  }

>

>  /* Helper routines for maintaining object_block_htab.  */

> --

> 1.8.3.1

>
Richard Biener March 15, 2018, 8:55 a.m. | #2
On Mon, Feb 26, 2018 at 12:07 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Feb 16, 2018 at 6:18 PM,  <vladimir.mezentsev@oracle.com> wrote:

>> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>

>>

>> Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve

>> bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).

>> The real bug is in gcc/varasm.c.

>> hash_section() returns an unstable value.

>> As result, two blocks are created in get_block_for_section() for one unnamed section.

>> A list of objects in these blocks depends on the -gtoggle option.

>> I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and

>> I fixed hash_section() in gcc/varasm.c

>>

>> Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).

>> Testing finished ok.

>

> Ok.


Committed on behalf of Vladimir.  r258553.

Richard.

> Thanks,

> Richard.

>

>> ChangeLog:

>> 2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>>

>> PR gcc/68256

>> * varasm.c (hash_section): Return an unchangeble hash value

>> * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

>> Return !aarch64_can_use_per_function_literal_pools_p ();

>> ---

>>  gcc/config/aarch64/aarch64.c | 8 +++-----

>>  gcc/varasm.c                 | 2 +-

>>  2 files changed, 4 insertions(+), 6 deletions(-)

>>

>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>> index 174310c..a0a495d 100644

>> --- a/gcc/config/aarch64/aarch64.c

>> +++ b/gcc/config/aarch64/aarch64.c

>> @@ -7596,11 +7596,9 @@ aarch64_can_use_per_function_literal_pools_p (void)

>>  static bool

>>  aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)

>>  {

>> -  /* Fixme:: In an ideal world this would work similar

>> -     to the logic in aarch64_select_rtx_section but this

>> -     breaks bootstrap in gcc go.  For now we workaround

>> -     this by returning false here.  */

>> -  return false;

>> +  /* We can't use blocks for constants when we're using a per-function

>> +     constant pool.  */

>> +  return !aarch64_can_use_per_function_literal_pools_p ();

>>  }

>>

>>  /* Select appropriate section for constants depending

>> diff --git a/gcc/varasm.c b/gcc/varasm.c

>> index b045efa..5aae5b4 100644

>> --- a/gcc/varasm.c

>> +++ b/gcc/varasm.c

>> @@ -225,7 +225,7 @@ hash_section (section *sect)

>>  {

>>    if (sect->common.flags & SECTION_NAMED)

>>      return htab_hash_string (sect->named.name);

>> -  return sect->common.flags;

>> +  return sect->common.flags & ~SECTION_DECLARED;

>>  }

>>

>>  /* Helper routines for maintaining object_block_htab.  */

>> --

>> 1.8.3.1

>>
Bin.Cheng March 15, 2018, 2:07 p.m. | #3
On Fri, Feb 16, 2018 at 5:18 PM,  <vladimir.mezentsev@oracle.com> wrote:
> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>

>

> Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve

> bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).

> The real bug is in gcc/varasm.c.

> hash_section() returns an unstable value.

> As result, two blocks are created in get_block_for_section() for one unnamed section.

> A list of objects in these blocks depends on the -gtoggle option.

> I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and

> I fixed hash_section() in gcc/varasm.c

>

> Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).

> Testing finished ok.

Hi Vladimir,
Thanks for fixing the long standing issue, but this change causes below failure,
could you have a look?  Thanks

Failures:
        gcc.dg/attr-weakref-1.c

Bisected to:


commit 536728c16d6a0173930ecfe370302baa471c299e
Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Mar 15 08:55:04 2018 +0000

    2018-03-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

            PR target/68256
            * varasm.c (hash_section): Return an unchangeble hash value
            * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):
            Return !aarch64_can_use_per_function_literal_pools_p ().


Thanks,
bin
>

> ChangeLog:

> 2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>

> PR gcc/68256

> * varasm.c (hash_section): Return an unchangeble hash value

> * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

> Return !aarch64_can_use_per_function_literal_pools_p ();

> ---

>  gcc/config/aarch64/aarch64.c | 8 +++-----

>  gcc/varasm.c                 | 2 +-

>  2 files changed, 4 insertions(+), 6 deletions(-)

>

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> index 174310c..a0a495d 100644

> --- a/gcc/config/aarch64/aarch64.c

> +++ b/gcc/config/aarch64/aarch64.c

> @@ -7596,11 +7596,9 @@ aarch64_can_use_per_function_literal_pools_p (void)

>  static bool

>  aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)

>  {

> -  /* Fixme:: In an ideal world this would work similar

> -     to the logic in aarch64_select_rtx_section but this

> -     breaks bootstrap in gcc go.  For now we workaround

> -     this by returning false here.  */

> -  return false;

> +  /* We can't use blocks for constants when we're using a per-function

> +     constant pool.  */

> +  return !aarch64_can_use_per_function_literal_pools_p ();

>  }

>

>  /* Select appropriate section for constants depending

> diff --git a/gcc/varasm.c b/gcc/varasm.c

> index b045efa..5aae5b4 100644

> --- a/gcc/varasm.c

> +++ b/gcc/varasm.c

> @@ -225,7 +225,7 @@ hash_section (section *sect)

>  {

>    if (sect->common.flags & SECTION_NAMED)

>      return htab_hash_string (sect->named.name);

> -  return sect->common.flags;

> +  return sect->common.flags & ~SECTION_DECLARED;

>  }

>

>  /* Helper routines for maintaining object_block_htab.  */

> --

> 1.8.3.1

>
Christophe Lyon March 15, 2018, 2:10 p.m. | #4
On 15 March 2018 at 15:07, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Fri, Feb 16, 2018 at 5:18 PM,  <vladimir.mezentsev@oracle.com> wrote:

>> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>

>>

>> Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve

>> bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).

>> The real bug is in gcc/varasm.c.

>> hash_section() returns an unstable value.

>> As result, two blocks are created in get_block_for_section() for one unnamed section.

>> A list of objects in these blocks depends on the -gtoggle option.

>> I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and

>> I fixed hash_section() in gcc/varasm.c

>>

>> Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).

>> Testing finished ok.

> Hi Vladimir,

> Thanks for fixing the long standing issue, but this change causes below failure,

> could you have a look?  Thanks

>

> Failures:

>         gcc.dg/attr-weakref-1.c

>

> Bisected to:

>

>

> commit 536728c16d6a0173930ecfe370302baa471c299e

> Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>

> Date:   Thu Mar 15 08:55:04 2018 +0000

>

>     2018-03-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>

>             PR target/68256

>             * varasm.c (hash_section): Return an unchangeble hash value

>             * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

>             Return !aarch64_can_use_per_function_literal_pools_p ().

>


I see this too:
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/gcc3/gcc/xgcc
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/gcc3/gcc/
/gcc/testsuite/gcc.dg/attr-weakref-1.c -fno-diagnostics-show-caret
-fdiagnostics-color=never -O2 /gcc/testsuite/gcc.dg/attr-weakref-1a.c
-lm -o ./attr-weakref-1.exe
/ccLqgn8f.o:(.rodata.cst8+0x30): undefined reference to `wv12'
/ccLqgn8f.o:(.rodata.cst8+0x38): undefined reference to `wv12'
/ccLqgn8f.o:(.rodata.cst8+0x60): undefined reference to `wf12'
/ccLqgn8f.o:(.rodata.cst8+0x68): undefined reference to `wf12'
collect2: error: ld returned 1 exit status
compiler exited with status 1
FAIL: gcc.dg/attr-weakref-1.c (test for excess errors)

Christophe

>

> Thanks,

> bin

>>

>> ChangeLog:

>> 2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>>

>> PR gcc/68256

>> * varasm.c (hash_section): Return an unchangeble hash value

>> * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

>> Return !aarch64_can_use_per_function_literal_pools_p ();

>> ---

>>  gcc/config/aarch64/aarch64.c | 8 +++-----

>>  gcc/varasm.c                 | 2 +-

>>  2 files changed, 4 insertions(+), 6 deletions(-)

>>

>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>> index 174310c..a0a495d 100644

>> --- a/gcc/config/aarch64/aarch64.c

>> +++ b/gcc/config/aarch64/aarch64.c

>> @@ -7596,11 +7596,9 @@ aarch64_can_use_per_function_literal_pools_p (void)

>>  static bool

>>  aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)

>>  {

>> -  /* Fixme:: In an ideal world this would work similar

>> -     to the logic in aarch64_select_rtx_section but this

>> -     breaks bootstrap in gcc go.  For now we workaround

>> -     this by returning false here.  */

>> -  return false;

>> +  /* We can't use blocks for constants when we're using a per-function

>> +     constant pool.  */

>> +  return !aarch64_can_use_per_function_literal_pools_p ();

>>  }

>>

>>  /* Select appropriate section for constants depending

>> diff --git a/gcc/varasm.c b/gcc/varasm.c

>> index b045efa..5aae5b4 100644

>> --- a/gcc/varasm.c

>> +++ b/gcc/varasm.c

>> @@ -225,7 +225,7 @@ hash_section (section *sect)

>>  {

>>    if (sect->common.flags & SECTION_NAMED)

>>      return htab_hash_string (sect->named.name);

>> -  return sect->common.flags;

>> +  return sect->common.flags & ~SECTION_DECLARED;

>>  }

>>

>>  /* Helper routines for maintaining object_block_htab.  */

>> --

>> 1.8.3.1

>>
vladimir.mezentsev@oracle.com March 16, 2018, 6:16 a.m. | #5
On 03/15/2018 07:07 AM, Bin.Cheng wrote:
> On Fri, Feb 16, 2018 at 5:18 PM,  <vladimir.mezentsev@oracle.com> wrote:

>> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>

>>

>> Ramana Radhakrishnan made a workaround in gcc/config/aarch64/aarch64.c to resolve

>> bootstrap comparison failure (2015-11-10, commit bc443a71dafa2e707bae4b2fa74f83b05dea37ab).

>> The real bug is in gcc/varasm.c.

>> hash_section() returns an unstable value.

>> As result, two blocks are created in get_block_for_section() for one unnamed section.

>> A list of objects in these blocks depends on the -gtoggle option.

>> I removed Ramana's workaround in gcc/config/aarch64/aarch64.c and  

>> I fixed hash_section() in gcc/varasm.c

>>

>> Bootstrapped on aarch64-unknown-linux-gnu including (c,c++ and go).

>> Testing finished ok.

> Hi Vladimir,

> Thanks for fixing the long standing issue, but this change causes below failure,

> could you have a look?  Thanks

I did not see this failure.
I cannot tell why.
I use one CFARM machine (gcc116) and when my fix was approved I clean up
my directories
on this machine (including binaries and log files).

ramana.radhakrishnan@arm.com implemented (2015-11-06, commit
cd4fcdb8ff16ec2dad56f9e736ac4552f8f52001)
a new feature ('Switch constant pools to separate rodata sections').
An additional fix is needed for this feature.

The test below demonstrates problem:
% cat t.c
extern void abort(void);
typedef int vtype;
static vtype Wv12 __attribute__((weakref ("wv12")));
extern vtype wv12 __attribute__((weak));

#define chk(p) do { if (!p) abort (); } while (0)

int main () {
  chk (!&Wv12);
  chk (!&wv12);
  return (0);
}

% gcc  t.c
/tmp/cciZgRxK.o:(.rodata+0x0): undefined reference to `wv12'
/tmp/cciZgRxK.o:(.rodata+0x8): undefined reference to `wv12'

% gcc  t.c -S
% cat t.s
....
        .size   main, .-main
*        .weakref        Wv12,wv12   *<<<<<<   Not here. This should be
after definitions of Wv12 and wv12.
Wv12        .section        .rodata
        .align  3
.LC0:
        .xword  Wv12
        .align  3
.LC1:
        .xword  wv12


I can file a new PR and fix it by Tuesday/Wednesday
or we can  temporary restore Ramana's workaround in 
aarch64_use_blocks_for_constant_p:
% git diff
gcc/config/aarch64/aarch64.c                                                                               

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 4b5183b..222ea33 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7735,7 +7735,11 @@ aarch64_use_blocks_for_constant_p (machine_mode,
const_rtx)
 {
   /* We can't use blocks for constants when we're using a per-function
      constant pool.  */
-  return !aarch64_can_use_per_function_literal_pools_p ();
+  /* Fixme::
+     return !aarch64_can_use_per_function_literal_pools_p ();
+     This return statement breaks gcc.dg/attr-weakref-1.c test.
+     For now we workaround this by returning false here.  */
+  return false;
 }
 
 /* Select appropriate section for constants depending


-Vladimir

>

> Failures:

>         gcc.dg/attr-weakref-1.c

>

> Bisected to:

>

>

> commit 536728c16d6a0173930ecfe370302baa471c299e

> Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>

> Date:   Thu Mar 15 08:55:04 2018 +0000

>

>     2018-03-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>

>             PR target/68256

>             * varasm.c (hash_section): Return an unchangeble hash value

>             * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

>             Return !aarch64_can_use_per_function_literal_pools_p ().

>

>

> Thanks,

> bin

>> ChangeLog:

>> 2018-02-15  Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>

>>

>> PR gcc/68256

>> * varasm.c (hash_section): Return an unchangeble hash value

>> * config/aarch64/aarch64.c (aarch64_use_blocks_for_constant_p):

>> Return !aarch64_can_use_per_function_literal_pools_p ();

>> ---

>>  gcc/config/aarch64/aarch64.c | 8 +++-----

>>  gcc/varasm.c                 | 2 +-

>>  2 files changed, 4 insertions(+), 6 deletions(-)

>>

>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>> index 174310c..a0a495d 100644

>> --- a/gcc/config/aarch64/aarch64.c

>> +++ b/gcc/config/aarch64/aarch64.c

>> @@ -7596,11 +7596,9 @@ aarch64_can_use_per_function_literal_pools_p (void)

>>  static bool

>>  aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)

>>  {

>> -  /* Fixme:: In an ideal world this would work similar

>> -     to the logic in aarch64_select_rtx_section but this

>> -     breaks bootstrap in gcc go.  For now we workaround

>> -     this by returning false here.  */

>> -  return false;

>> +  /* We can't use blocks for constants when we're using a per-function

>> +     constant pool.  */

>> +  return !aarch64_can_use_per_function_literal_pools_p ();

>>  }

>>

>>  /* Select appropriate section for constants depending

>> diff --git a/gcc/varasm.c b/gcc/varasm.c

>> index b045efa..5aae5b4 100644

>> --- a/gcc/varasm.c

>> +++ b/gcc/varasm.c

>> @@ -225,7 +225,7 @@ hash_section (section *sect)

>>  {

>>    if (sect->common.flags & SECTION_NAMED)

>>      return htab_hash_string (sect->named.name);

>> -  return sect->common.flags;

>> +  return sect->common.flags & ~SECTION_DECLARED;

>>  }

>>

>>  /* Helper routines for maintaining object_block_htab.  */

>> --

>> 1.8.3.1

>>

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 174310c..a0a495d 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7596,11 +7596,9 @@  aarch64_can_use_per_function_literal_pools_p (void)
 static bool
 aarch64_use_blocks_for_constant_p (machine_mode, const_rtx)
 {
-  /* Fixme:: In an ideal world this would work similar
-     to the logic in aarch64_select_rtx_section but this
-     breaks bootstrap in gcc go.  For now we workaround
-     this by returning false here.  */
-  return false;
+  /* We can't use blocks for constants when we're using a per-function
+     constant pool.  */
+  return !aarch64_can_use_per_function_literal_pools_p ();
 }
 
 /* Select appropriate section for constants depending
diff --git a/gcc/varasm.c b/gcc/varasm.c
index b045efa..5aae5b4 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -225,7 +225,7 @@  hash_section (section *sect)
 {
   if (sect->common.flags & SECTION_NAMED)
     return htab_hash_string (sect->named.name);
-  return sect->common.flags;
+  return sect->common.flags & ~SECTION_DECLARED;
 }
 
 /* Helper routines for maintaining object_block_htab.  */