Re: [Patch AArch64] Fix PR target/63874

[Richard Earnshaw (lists)]

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 = _weakref;
> + g = 
> + 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

[Ramana Radhakrishnan]

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

[Ramana Radhakrishnan]

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

[Ramana Radhakrishnan]

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

[James Greenhalgh]

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.