| Message ID | 5465B973.40007@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod <tejas.belagod@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.belagod@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.
On 14/11/14 08:19, Andrew Pinski wrote: > On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod <tejas.belagod@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.
On 14 November 2014 08:19, Andrew Pinski <pinskia@gmail.com> wrote: > On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod <tejas.belagod@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
> On Nov 14, 2014, at 12:54 AM, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > >> On 14 November 2014 08:19, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod <tejas.belagod@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
On Fri, Nov 14, 2014 at 12:50 AM, Tejas Belagod <tejas.belagod@arm.com> wrote: > On 14/11/14 08:19, Andrew Pinski wrote: >> >> On Fri, Nov 14, 2014 at 12:12 AM, Tejas Belagod <tejas.belagod@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. > >
On 14 November 2014 08:12, Tejas Belagod <tejas.belagod@arm.com> wrote: > 2014-11-14 Tejas Belagod <tejas.belagod@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
On 14/11/14 17:33, Marcus Shawcroft wrote: > On 14 November 2014 08:12, Tejas Belagod <tejas.belagod@arm.com> wrote: > >> 2014-11-14 Tejas Belagod <tejas.belagod@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.
On 20 November 2014 14:33, Tejas Belagod <tejas.belagod@arm.com> wrote: > The same patch applies cleanly to 4.9. OK to commit? > > Thanks, > Tejas. Provided it regresses ok, yes. /Marcus
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 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) + || INTVAL (offset) < -1048575 || INTVAL (offset) > 1048575) return SYMBOL_FORCE_TO_MEM; return SYMBOL_TINY_ABSOLUTE; case AARCH64_CMODEL_SMALL: - if (SYMBOL_REF_WEAK (x)) + /* Same reasoning as the tiny code model, but the offset cap here is + 4G. */ + if (SYMBOL_REF_WEAK (x) + || INTVAL (offset) < (HOST_WIDE_INT) -4294967263 + || INTVAL (offset) > (HOST_WIDE_INT) 4294967264) return SYMBOL_FORCE_TO_MEM; return SYMBOL_SMALL_ABSOLUTE; diff --git a/gcc/testsuite/gcc.target/aarch64/symbol-range-tiny.c b/gcc/testsuite/gcc.target/aarch64/symbol-range-tiny.c new file mode 100644 index 0000000..d7d2039 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/symbol-range-tiny.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -save-temps -mcmodel=tiny" } */ + +int fixed_regs[0x00200000]; + +int +foo() +{ + return fixed_regs[0x00080000]; +} + +/* { dg-final { scan-assembler-not "adr\tx\[0-9\]+, fixed_regs\\\+" } } */ +/* { dg-final {cleanup-saved-temps } } */ diff --git a/gcc/testsuite/gcc.target/aarch64/symbol-range.c b/gcc/testsuite/gcc.target/aarch64/symbol-range.c new file mode 100644 index 0000000..f999bb3 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/symbol-range.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -save-temps -mcmodel=small" } */ + +int fixed_regs[0x200000000ULL]; + +int +foo() +{ + return fixed_regs[0x100000000ULL]; +} + +/* { dg-final { scan-assembler-not "adrp\tx\[0-9\]+, fixed_regs\\\+" } } */ +/* { dg-final {cleanup-saved-temps } } */
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.belagod@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.