Re: [Patch AArch64] Fix PR target/63874
On 31/03/16 14:11, Ramana Radhakrishnan wrote:
> Hi,
>
> In this PR we have a situation where we aren't really detecting
> weak references vs weak definitions. If one has a weak definition
> that binds locally there's no reason not to put out PC relative
> relocations.
>
> However if you have a genuine weak reference that is
> known not to bind locally it makes very little sense
> to put out an entry into the literal pool which doesn't always
> work with DSOs and shared objects.
>
> Tested aarch64-none-linux-gnu bootstrap and regression test with no
> regressions
>
> This is not a regression and given what we've seen recently with protected
> symbols and binds_locally_p I'd rather this were queued for GCC 7.
>
> Ok ?
>
Yes, this looks fine. Sorry for the delay replying.
R.
> regards
> Ramana
>
> gcc/
>
> * config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed.
> Only force to memory if it is a weak external reference.
>
>
> gcc/testsuite
>
> * gcc.target/aarch64/pr63874.c: New test.
>
>
> pr63874.txt
>
>
> commit e41d4bd6abbee99628909d4af612504844dee640
> Author: Ramana Radhakrishnan
> Date: Thu Mar 31 13:47:33 2016 +0100
>
> fix PR63874
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index cf1239d..6782316 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -9387,15 +9387,18 @@ aarch64_classify_symbol (rtx x, rtx offset)
>switch (aarch64_cmodel)
> {
> case AARCH64_CMODEL_TINY:
> - /* When we retreive symbol + offset address, we have to make sure
> + /* When we retrieve symbol + offset address, we have to make sure
>the offset does not cause overflow of the final address. But
>we have no way of knowing the address of symbol at compile time
>so we can't accurately say if the distance between the PC and
>symbol + offset is outside the addressible range of +/-1M in the
>TINY code model. So we rely on images not being greater than
>1M and cap the offset at 1M and anything beyond 1M will have to
> - be loaded using an alternative mechanism. */
> - if (SYMBOL_REF_WEAK (x)
> + be loaded using an alternative mechanism. Furthermore if the
> + symbol is a weak reference to something that isn't known to
> + resolve to a symbol in this module, then force to memory. */
> + if ((SYMBOL_REF_WEAK (x)
> +&& !aarch64_symbol_binds_local_p (x))
> || INTVAL (offset) < -1048575 || INTVAL (offset) > 1048575)
> return SYMBOL_FORCE_TO_MEM;
> return SYMBOL_TINY_ABSOLUTE;
> @@ -9403,7 +9406,8 @@ aarch64_classify_symbol (rtx x, rtx offset)
> case AARCH64_CMODEL_SMALL:
> /* Same reasoning as the tiny code model, but the offset cap here is
>4G. */
> - if (SYMBOL_REF_WEAK (x)
> + if ((SYMBOL_REF_WEAK (x)
> +&& !aarch64_symbol_binds_local_p (x))
> || !IN_RANGE (INTVAL (offset), HOST_WIDE_INT_C (-4294967263),
> HOST_WIDE_INT_C (4294967264)))
> return SYMBOL_FORCE_TO_MEM;
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr63874.c
> b/gcc/testsuite/gcc.target/aarch64/pr63874.c
> new file mode 100644
> index 000..1a745a0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr63874.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-skip-if "Not applicable for mcmodel=large" { aarch64*-*-* } {
> "-mcmodel=large" } { "" } } */
> +
> +extern void __attribute__((weak)) foo_weakref (void);
> +void __attribute__((weak, noinline)) bar (void)
> +{
> + return;
> +}
> +void (*f) (void);
> +void (*g) (void);
> +
> +int
> +main (void)
> +{
> + f = &foo_weakref;
> + g = &bar;
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler-not "adr*foo_weakref" } } */
> +/* { dg-final { scan-assembler-not "\\.(word|xword)\tbar" } } */
>
Re: [Patch AArch64] Fix PR target/63874
On Thu, May 5, 2016 at 8:45 AM, Ramana Radhakrishnan wrote: > On Thu, Mar 31, 2016 at 2:11 PM, Ramana Radhakrishnan > wrote: >> Hi, >> >> In this PR we have a situation where we aren't really detecting >> weak references vs weak definitions. If one has a weak definition >> that binds locally there's no reason not to put out PC relative >> relocations. >> >> However if you have a genuine weak reference that is >> known not to bind locally it makes very little sense >> to put out an entry into the literal pool which doesn't always >> work with DSOs and shared objects. >> >> Tested aarch64-none-linux-gnu bootstrap and regression test with no >> regressions >> >> This is not a regression and given what we've seen recently with protected >> symbols and binds_locally_p I'd rather this were queued for GCC 7. >> >> Ok ? > > Ping ^ 2. > > https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01680.html Ping ^3 https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01680.html Ramana > > regards > Ramana >> >> regards >> Ramana >> >> gcc/ >> >> * config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed. >> Only force to memory if it is a weak external reference. >> >> >> gcc/testsuite >> >> * gcc.target/aarch64/pr63874.c: New test.
Re: [Patch AArch64] Fix PR target/63874
On Thu, Mar 31, 2016 at 2:11 PM, Ramana Radhakrishnan wrote: > Hi, > > In this PR we have a situation where we aren't really detecting > weak references vs weak definitions. If one has a weak definition > that binds locally there's no reason not to put out PC relative > relocations. > > However if you have a genuine weak reference that is > known not to bind locally it makes very little sense > to put out an entry into the literal pool which doesn't always > work with DSOs and shared objects. > > Tested aarch64-none-linux-gnu bootstrap and regression test with no > regressions > > This is not a regression and given what we've seen recently with protected > symbols and binds_locally_p I'd rather this were queued for GCC 7. > > Ok ? Ping ^ 2. https://gcc.gnu.org/ml/gcc-patches/2016-03/msg01680.html regards Ramana > > regards > Ramana > > gcc/ > > * config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed. > Only force to memory if it is a weak external reference. > > > gcc/testsuite > > * gcc.target/aarch64/pr63874.c: New test.
Re: [Patch AArch64] Fix PR target/63874
On Thu, Mar 31, 2016 at 5:30 PM, James Greenhalgh wrote: > On Thu, Mar 31, 2016 at 02:11:49PM +0100, Ramana Radhakrishnan wrote: >> Hi, >> >> In this PR we have a situation where we aren't really detecting >> weak references vs weak definitions. If one has a weak definition >> that binds locally there's no reason not to put out PC relative >> relocations. >> >> However if you have a genuine weak reference that is >> known not to bind locally it makes very little sense >> to put out an entry into the literal pool which doesn't always >> work with DSOs and shared objects. >> >> Tested aarch64-none-linux-gnu bootstrap and regression test with no >> regressions >> >> This is not a regression and given what we've seen recently with protected >> symbols and binds_locally_p I'd rather this were queued for GCC 7. >> >> Ok ? > > Based on the bugzilla report, this looks OK for GCC 7 to me. But I don't > know the dark corners of the elf specification, so I'd rather leave the > final review to Richard or Marcus. Richard / Marcus, ping ? Ramana > > Thanks, > James > >> gcc/ >> >> * config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed. >> Only force to memory if it is a weak external reference. >> >> gcc/testsuite >> >> * gcc.target/aarch64/pr63874.c: New test. >
Re: [Patch AArch64] Fix PR target/63874
On Thu, Mar 31, 2016 at 02:11:49PM +0100, Ramana Radhakrishnan wrote: > Hi, > > In this PR we have a situation where we aren't really detecting > weak references vs weak definitions. If one has a weak definition > that binds locally there's no reason not to put out PC relative > relocations. > > However if you have a genuine weak reference that is > known not to bind locally it makes very little sense > to put out an entry into the literal pool which doesn't always > work with DSOs and shared objects. > > Tested aarch64-none-linux-gnu bootstrap and regression test with no > regressions > > This is not a regression and given what we've seen recently with protected > symbols and binds_locally_p I'd rather this were queued for GCC 7. > > Ok ? Based on the bugzilla report, this looks OK for GCC 7 to me. But I don't know the dark corners of the elf specification, so I'd rather leave the final review to Richard or Marcus. Thanks, James > gcc/ > > * config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed. > Only force to memory if it is a weak external reference. > > gcc/testsuite > > * gcc.target/aarch64/pr63874.c: New test.
[Patch AArch64] Fix PR target/63874
Hi,
In this PR we have a situation where we aren't really detecting
weak references vs weak definitions. If one has a weak definition
that binds locally there's no reason not to put out PC relative
relocations.
However if you have a genuine weak reference that is
known not to bind locally it makes very little sense
to put out an entry into the literal pool which doesn't always
work with DSOs and shared objects.
Tested aarch64-none-linux-gnu bootstrap and regression test with no regressions
This is not a regression and given what we've seen recently with protected
symbols and binds_locally_p I'd rather this were queued for GCC 7.
Ok ?
regards
Ramana
gcc/
* config/aarch64/aarch64.c (aarch64_classify_symbol): Typo in comment fixed.
Only force to memory if it is a weak external reference.
gcc/testsuite
* gcc.target/aarch64/pr63874.c: New test.
commit e41d4bd6abbee99628909d4af612504844dee640
Author: Ramana Radhakrishnan
Date: Thu Mar 31 13:47:33 2016 +0100
fix PR63874
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index cf1239d..6782316 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -9387,15 +9387,18 @@ aarch64_classify_symbol (rtx x, rtx offset)
switch (aarch64_cmodel)
{
case AARCH64_CMODEL_TINY:
- /* When we retreive symbol + offset address, we have to make sure
+ /* When we retrieve symbol + offset address, we have to make sure
the offset does not cause overflow of the final address. But
we have no way of knowing the address of symbol at compile time
so we can't accurately say if the distance between the PC and
symbol + offset is outside the addressible range of +/-1M in the
TINY code model. So we rely on images not being greater than
1M and cap the offset at 1M and anything beyond 1M will have to
-be loaded using an alternative mechanism. */
- if (SYMBOL_REF_WEAK (x)
+be loaded using an alternative mechanism. Furthermore if the
+symbol is a weak reference to something that isn't known to
+resolve to a symbol in this module, then force to memory. */
+ if ((SYMBOL_REF_WEAK (x)
+ && !aarch64_symbol_binds_local_p (x))
|| INTVAL (offset) < -1048575 || INTVAL (offset) > 1048575)
return SYMBOL_FORCE_TO_MEM;
return SYMBOL_TINY_ABSOLUTE;
@@ -9403,7 +9406,8 @@ aarch64_classify_symbol (rtx x, rtx offset)
case AARCH64_CMODEL_SMALL:
/* Same reasoning as the tiny code model, but the offset cap here is
4G. */
- if (SYMBOL_REF_WEAK (x)
+ if ((SYMBOL_REF_WEAK (x)
+ && !aarch64_symbol_binds_local_p (x))
|| !IN_RANGE (INTVAL (offset), HOST_WIDE_INT_C (-4294967263),
HOST_WIDE_INT_C (4294967264)))
return SYMBOL_FORCE_TO_MEM;
diff --git a/gcc/testsuite/gcc.target/aarch64/pr63874.c
b/gcc/testsuite/gcc.target/aarch64/pr63874.c
new file mode 100644
index 000..1a745a0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr63874.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-skip-if "Not applicable for mcmodel=large" { aarch64*-*-* } {
"-mcmodel=large" } { "" } } */
+
+extern void __attribute__((weak)) foo_weakref (void);
+void __attribute__((weak, noinline)) bar (void)
+{
+ return;
+}
+void (*f) (void);
+void (*g) (void);
+
+int
+main (void)
+{
+ f = &foo_weakref;
+ g = &bar;
+ return 0;
+}
+
+/* { dg-final { scan-assembler-not "adr*foo_weakref" } } */
+/* { dg-final { scan-assembler-not "\\.(word|xword)\tbar" } } */
