Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On 14/11/14 17:33, Marcus Shawcroft wrote: On 14 November 2014 08:12, Tejas Belagod tejas.bela...@arm.com wrote: 2014-11-14 Tejas Belagod tejas.bela...@arm.com gcc/ * config/aarch64/aarch64-protos.h (aarch64_classify_symbol): Fixup prototype. * config/aarch64/aarch64.c (aarch64_expand_mov_immediate, aarch64_cannot_force_const_mem, aarch64_classify_address, aarch64_classify_symbolic_expression): Fixup call to aarch64_classify_symbol. (aarch64_classify_symbol): Add range-checking for symbol + offset addressing for tiny and small models. testsuite/ * gcc.target/aarch64/symbol-range.c: New. * gcc.target/aarch64/symbol-range-tiny.c: New. OK. Could you rustle up a back port ? The same patch applies cleanly to 4.9. OK to commit? Thanks, Tejas.
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On 20 November 2014 14:33, Tejas Belagod tejas.bela...@arm.com wrote: The same patch applies cleanly to 4.9. OK to commit? Thanks, Tejas. Provided it regresses ok, yes. /Marcus
[AArch64, Patch] Add range-check for Symbol + offset addressing.
Hi,
Following the discussion here
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been
tracked down to a range-checking bug with symbol + offset style
addressing with adrp where we allowed any arbitrary offset and this
would cause truncation of final addresses when relocations were being
resolved by ld.
When we retreive 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 alternate
mechanism. Similarly for the SMALL code model the offset has been capped
at 4G.
The cap value for the offset in each code model is open to debate.
All testing done with Alan's workaround
patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed.
bootstrapped aarch64-linux.
OK for trunk?
Thanks,
Tejas.
2014-11-14 Tejas Belagod tejas.bela...@arm.com
gcc/
* config/aarch64/aarch64-protos.h (aarch64_classify_symbol):
Fixup prototype.
* config/aarch64/aarch64.c (aarch64_expand_mov_immediate,
aarch64_cannot_force_const_mem, aarch64_classify_address,
aarch64_classify_symbolic_expression): Fixup call to
aarch64_classify_symbol.
(aarch64_classify_symbol): Add range-checking for
symbol + offset addressing for tiny and small models.
testsuite/
* gcc.target/aarch64/symbol-range.c: New.
* gcc.target/aarch64/symbol-range-tiny.c: New.diff --git a/gcc/config/aarch64/aarch64-protos.h
b/gcc/config/aarch64/aarch64-protos.h
index 470b9eb..2cf4292 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -218,7 +218,7 @@ const char *aarch64_mangle_builtin_type (const_tree);
const char *aarch64_output_casesi (rtx *);
const char *aarch64_rewrite_selected_cpu (const char *name);
-enum aarch64_symbol_type aarch64_classify_symbol (rtx,
+enum aarch64_symbol_type aarch64_classify_symbol (rtx, rtx,
enum aarch64_symbol_context);
enum aarch64_symbol_type aarch64_classify_tls_symbol (rtx);
enum reg_class aarch64_regno_regclass (unsigned);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 736ad90..d6ecf6c 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1072,7 +1072,7 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm)
before we start classifying the symbol. */
split_const (imm, base, offset);
- sty = aarch64_classify_symbol (base, SYMBOL_CONTEXT_ADR);
+ sty = aarch64_classify_symbol (base, offset, SYMBOL_CONTEXT_ADR);
switch (sty)
{
case SYMBOL_FORCE_TO_MEM:
@@ -2963,7 +2963,7 @@ aarch64_cannot_force_const_mem (machine_mode mode
ATTRIBUTE_UNUSED, rtx x)
split_const (x, base, offset);
if (GET_CODE (base) == SYMBOL_REF || GET_CODE (base) == LABEL_REF)
{
- if (aarch64_classify_symbol (base, SYMBOL_CONTEXT_ADR)
+ if (aarch64_classify_symbol (base, offset, SYMBOL_CONTEXT_ADR)
!= SYMBOL_FORCE_TO_MEM)
return true;
else
@@ -3377,7 +3377,7 @@ aarch64_classify_address (struct aarch64_address_info
*info,
rtx sym, offs;
split_const (info-offset, sym, offs);
if (GET_CODE (sym) == SYMBOL_REF
- (aarch64_classify_symbol (sym, SYMBOL_CONTEXT_MEM)
+ (aarch64_classify_symbol (sym, offs, SYMBOL_CONTEXT_MEM)
== SYMBOL_SMALL_ABSOLUTE))
{
/* The symbol and offset must be aligned to the access size. */
@@ -3434,7 +3434,7 @@ aarch64_classify_symbolic_expression (rtx x,
rtx offset;
split_const (x, x, offset);
- return aarch64_classify_symbol (x, context);
+ return aarch64_classify_symbol (x, offset, context);
}
@@ -6594,7 +6594,7 @@ aarch64_classify_tls_symbol (rtx x)
LABEL_REF X in context CONTEXT. */
enum aarch64_symbol_type
-aarch64_classify_symbol (rtx x,
+aarch64_classify_symbol (rtx x, rtx offset,
enum aarch64_symbol_context context ATTRIBUTE_UNUSED)
{
if (GET_CODE (x) == LABEL_REF)
@@ -6628,12 +6628,25 @@ aarch64_classify_symbol (rtx x,
switch (aarch64_cmodel)
{
case AARCH64_CMODEL_TINY:
- if (SYMBOL_REF_WEAK (x))
+ /* When we retreive 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
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod tejas.bela...@arm.com wrote: Hi, Following the discussion here https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been tracked down to a range-checking bug with symbol + offset style addressing with adrp where we allowed any arbitrary offset and this would cause truncation of final addresses when relocations were being resolved by ld. When we retreive 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 alternate mechanism. Similarly for the SMALL code model the offset has been capped at 4G. The cap value for the offset in each code model is open to debate. All testing done with Alan's workaround patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed. bootstrapped aarch64-linux. OK for trunk? This looks like a better fix than I would have came up with. Since you are touching this area you might want to look into this issue: I notice SYMBOL_REF_WEAK (x) is true for references(decls) which are comdat's which are declared in the translation unit. So force them to memory when really we know they are declared and don't have a value of zero so they will fit in the medium code model. This happens with vtables and we lose some performance because of this. Thanks, Andrew Pinski Thanks, Tejas. 2014-11-14 Tejas Belagod tejas.bela...@arm.com gcc/ * config/aarch64/aarch64-protos.h (aarch64_classify_symbol): Fixup prototype. * config/aarch64/aarch64.c (aarch64_expand_mov_immediate, aarch64_cannot_force_const_mem, aarch64_classify_address, aarch64_classify_symbolic_expression): Fixup call to aarch64_classify_symbol. (aarch64_classify_symbol): Add range-checking for symbol + offset addressing for tiny and small models. testsuite/ * gcc.target/aarch64/symbol-range.c: New. * gcc.target/aarch64/symbol-range-tiny.c: New.
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On 14/11/14 08:19, Andrew Pinski wrote: On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod tejas.bela...@arm.com wrote: Hi, Following the discussion here https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been tracked down to a range-checking bug with symbol + offset style addressing with adrp where we allowed any arbitrary offset and this would cause truncation of final addresses when relocations were being resolved by ld. When we retreive 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 alternate mechanism. Similarly for the SMALL code model the offset has been capped at 4G. The cap value for the offset in each code model is open to debate. All testing done with Alan's workaround patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed. bootstrapped aarch64-linux. OK for trunk? This looks like a better fix than I would have came up with. Since you are touching this area you might want to look into this issue: I notice SYMBOL_REF_WEAK (x) is true for references(decls) which are comdat's which are declared in the translation unit. So force them to memory when really we know they are declared and don't have a value of zero so they will fit in the medium code model. This happens with vtables and we lose some performance because of this. Do you have a bugzilla ticket open for this? Thanks, Tejas.
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On 14 November 2014 08:19, Andrew Pinski pins...@gmail.com wrote: On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod tejas.bela...@arm.com wrote: Hi, Following the discussion here https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been tracked down to a range-checking bug with symbol + offset style addressing with adrp where we allowed any arbitrary offset and this would cause truncation of final addresses when relocations were being resolved by ld. When we retreive 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 alternate mechanism. Similarly for the SMALL code model the offset has been capped at 4G. The cap value for the offset in each code model is open to debate. All testing done with Alan's workaround patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed. bootstrapped aarch64-linux. OK for trunk? This looks like a better fix than I would have came up with. Since you are touching this area you might want to look into this issue: I notice SYMBOL_REF_WEAK (x) is true for references(decls) which are comdat's which are declared in the translation unit. So force them to memory when really we know they are declared and don't have a value of zero so they will fit in the medium code model. This happens with vtables and we lose some performance because of this. Andrew, do you mind if we take that as a separate patch, I'd like to take Tejas' patch sooner rather than later since it gates building a variety of stuff some folks care about. Cheers /Marcus
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On Nov 14, 2014, at 12:54 AM, Marcus Shawcroft marcus.shawcr...@gmail.com wrote: On 14 November 2014 08:19, Andrew Pinski pins...@gmail.com wrote: On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod tejas.bela...@arm.com wrote: Hi, Following the discussion here https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been tracked down to a range-checking bug with symbol + offset style addressing with adrp where we allowed any arbitrary offset and this would cause truncation of final addresses when relocations were being resolved by ld. When we retreive 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 alternate mechanism. Similarly for the SMALL code model the offset has been capped at 4G. The cap value for the offset in each code model is open to debate. All testing done with Alan's workaround patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed. bootstrapped aarch64-linux. OK for trunk? This looks like a better fix than I would have came up with. Since you are touching this area you might want to look into this issue: I notice SYMBOL_REF_WEAK (x) is true for references(decls) which are comdat's which are declared in the translation unit. So force them to memory when really we know they are declared and don't have a value of zero so they will fit in the medium code model. This happens with vtables and we lose some performance because of this. Andrew, do you mind if we take that as a separate patch, I'd like to take Tejas' patch sooner rather than later since it gates building a variety of stuff some folks care about. Yes that is ok. This was more of since you were looking into this area kind of thing but it can wait until later. Thanks, Andrew Cheers /Marcus
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On Fri, Nov 14, 2014 at 12:50 AM, Tejas Belagod tejas.bela...@arm.com wrote: On 14/11/14 08:19, Andrew Pinski wrote: On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod tejas.bela...@arm.com wrote: Hi, Following the discussion here https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02237.html, this has been tracked down to a range-checking bug with symbol + offset style addressing with adrp where we allowed any arbitrary offset and this would cause truncation of final addresses when relocations were being resolved by ld. When we retreive 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 alternate mechanism. Similarly for the SMALL code model the offset has been capped at 4G. The cap value for the offset in each code model is open to debate. All testing done with Alan's workaround patch(https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01509.html) reversed. bootstrapped aarch64-linux. OK for trunk? This looks like a better fix than I would have came up with. Since you are touching this area you might want to look into this issue: I notice SYMBOL_REF_WEAK (x) is true for references(decls) which are comdat's which are declared in the translation unit. So force them to memory when really we know they are declared and don't have a value of zero so they will fit in the medium code model. This happens with vtables and we lose some performance because of this. Do you have a bugzilla ticket open for this? I do now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63874 . Thanks, Andrew Thanks, Tejas.
Re: [AArch64, Patch] Add range-check for Symbol + offset addressing.
On 14 November 2014 08:12, Tejas Belagod tejas.bela...@arm.com wrote: 2014-11-14 Tejas Belagod tejas.bela...@arm.com gcc/ * config/aarch64/aarch64-protos.h (aarch64_classify_symbol): Fixup prototype. * config/aarch64/aarch64.c (aarch64_expand_mov_immediate, aarch64_cannot_force_const_mem, aarch64_classify_address, aarch64_classify_symbolic_expression): Fixup call to aarch64_classify_symbol. (aarch64_classify_symbol): Add range-checking for symbol + offset addressing for tiny and small models. testsuite/ * gcc.target/aarch64/symbol-range.c: New. * gcc.target/aarch64/symbol-range-tiny.c: New. OK. Could you rustle up a back port ? /Marcus
