[v3,AArch64] Fix symbol offset limit

ping

From: Wilco Dijkstra
Sent: 17 January 2017 15:14
To: Richard Earnshaw; GCC Patches; James Greenhalgh
Cc: nd
Subject: Re: [PATCH v3][AArch64] Fix symbol offset limit
    
Here is v3 of the patch - tree_fits_uhwi_p was necessary to ensure the size of a
declaration is an integer. So the question is whether we should allow
largish offsets outside of the bounds of symbols (v1), no offsets (this version), or
small offsets (small negative and positive offsets just outside a symbol are common).
The only thing we can't allow is any offset like we currently do...

In aarch64_classify_symbol symbols are allowed full-range offsets on relocations.
This means the offset can use all of the +/-4GB offset, leaving no offset available
for the symbol itself.  This results in relocation overflow and link-time errors
for simple expressions like &global_char + 0xffffff00.

To avoid this, limit the offset to +/-1GB so that the symbol needs to be within a
3GB offset from its references.  For the tiny code model use a 64KB offset, allowing
most of the 1MB range for code/data between the symbol and its references.
For symbols with a defined size, limit the offset to be within the size of the symbol.


ChangeLog:
2017-01-17  Wilco Dijkstra  <wdijkstr@arm.com>

    gcc/
        * config/aarch64/aarch64.c (aarch64_classify_symbol):
        Apply reasonable limit to symbol offsets.

    testsuite/
        * gcc.target/aarch64/symbol-range.c (foo): Set new limit.
        * gcc.target/aarch64/symbol-range-tiny.c (foo): Likewise.

--

On Tue, Jun 13, 2017 at 03:00:28PM +0100, Wilco Dijkstra wrote:
> 
> ping

I've been avoiding reviewing this patch as Richard was the last to comment
on it, and I wasn't sure that his comments had been resolved to his
satisfaction. The conversation was back in August 2016 on v1 of the patch:

> Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
> >
> > So isn't the real bug that we've permitted the user to create an object
> > that is too large for the data model?
> 
> No that's a different issue I'm not trying to address here. The key is that as long
> as the start of the symbol is in range, we should be able to link. Due to optimization
> the offset may be huge even when the object is tiny, so the offset must be limited.
> 
> > Consider, for example:
> 
> char fixed_regs[0x200000000ULL];
> char fixed_regs2[100];
> 
> int
> main()
> {
>   return fixed_regs[0] + fixed_regs2[0];
> }
> 
> > Neither offset is too large, but we still generate relocation errors
> > when trying to reference fixed_regs2.
> 
> But so would creating a million objects of size 1. The linker could warn about
> large objects as well as giving better error messages for relocations that are
> out of range. But that's mostly QoI, what we have here is a case where legal
> code fails to link due to optimization. The original example is from GCC itself,
> the fixed_regs array is small but due to optimization we can end up with
> &fixed_regs + 0xffffffff.

Richard, do you have anything further to say on this patch? Or can we start
progressing the review again.

Thanks,
James

Hi,

Let's get back to the patch and the bug it fixes. The only outstanding question is what
constant offsets we should allow when generating a relocation:

> So the question is whether we should allow
> largish offsets outside of the bounds of symbols (v1), no offsets (this version), or
> small offsets (small negative and positive offsets just outside a symbol are common).
> The only thing we can't allow is any offset like we currently do...

Wilco

On 14/06/17 15:07, James Greenhalgh wrote:
> On Tue, Jun 13, 2017 at 03:00:28PM +0100, Wilco Dijkstra wrote:
>>
>> ping
> 
> I've been avoiding reviewing this patch as Richard was the last to comment
> on it, and I wasn't sure that his comments had been resolved to his
> satisfaction. The conversation was back in August 2016 on v1 of the patch:
> 
>> Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
>>>
>>> So isn't the real bug that we've permitted the user to create an object
>>> that is too large for the data model?
>>
>> No that's a different issue I'm not trying to address here. The key is that as long
>> as the start of the symbol is in range, we should be able to link. Due to optimization
>> the offset may be huge even when the object is tiny, so the offset must be limited.
>>
>>> Consider, for example:
>>
>> char fixed_regs[0x200000000ULL];
>> char fixed_regs2[100];
>>
>> int
>> main()
>> {
>>   return fixed_regs[0] + fixed_regs2[0];
>> }
>>
>>> Neither offset is too large, but we still generate relocation errors
>>> when trying to reference fixed_regs2.
>>
>> But so would creating a million objects of size 1. The linker could warn about
>> large objects as well as giving better error messages for relocations that are
>> out of range. But that's mostly QoI, what we have here is a case where legal
>> code fails to link due to optimization. The original example is from GCC itself,
>> the fixed_regs array is small but due to optimization we can end up with
>> &fixed_regs + 0xffffffff.
> 
> Richard, do you have anything further to say on this patch? Or can we start
> progressing the review again.
> 
> Thanks,
> James
> 

Yes, I still believe that this is a bug in the way we've documented the
-mcmodel=tiny and -mcmodel=small options.

-mcmode=tiny should read:


@item -mcmodel=tiny
@opindex mcmodel=tiny
Generate code for the tiny code model.  The program and its static data
must fit within 1MB.  Programs can be statically or dynamically linked.
The limit is not enforced by the compiler, but if you exceed the limit
you may get errors during linking saying that relocations have been
truncated.


It's the same basic text for -mcmodel=small, except that the limit is 4GB.

R.

Richard Earnshaw wrote:
> Yes, I still believe that this is a bug in the way we've documented the
> -mcmodel=tiny and -mcmodel=small options.

In what way could this possibly be a documentation bug? It's not at all related
to the size of a binary. There is no limit to the offset you can apply to a symbol,
I can write int a; int *p = &a + 0x80000000; and GCC is happy to create a
relocation with that offset.

Wilco

On 15/06/17 17:55, Wilco Dijkstra wrote:
> Richard Earnshaw wrote:
>> Yes, I still believe that this is a bug in the way we've documented the
>> -mcmodel=tiny and -mcmodel=small options.
> 
> In what way could this possibly be a documentation bug? It's not at all
> related
> to the size of a binary. There is no limit to the offset you can apply
> to a symbol,
> I can write int a; int *p = &a + 0x80000000; and GCC is happy to create a
> relocation with that offset.
> 
> Wilco

You can write it, but it's meaningless by the C standard.  You can't
take the address beyond one after the size of the object, so anything
more than &a+1 has no meaning.

R.

Richard Earnshaw wrote:
>
> You can write it, but it's meaningless by the C standard.  You can't
> take the address beyond one after the size of the object, so anything
> more than &a+1 has no meaning.

No it's perfectly valid and such out-of-range cases occur thousands of
times when building any non-trivial code. For example a[i + C] transforms
into (&a + C)[i].

Wilco

On 15/06/17 18:51, Wilco Dijkstra wrote:
> Richard Earnshaw wrote:
>>
>> You can write it, but it's meaningless by the C standard.  You can't
>> take the address beyond one after the size of the object, so anything
>> more than &a+1 has no meaning.
> 
> No it's perfectly valid and such out-of-range cases occur thousands of
> times when building any non-trivial code. For example a[i + C] transforms
> into (&a + C)[i].
> 
> Wilco
> 
>    

C11: Summary of undefined behaviours.

— Addition or subtraction of a pointer into, or just beyond, an array
object and an
integer type produces a result that does not point into, or just beyond,
the same array
object (6.5.6).

R.

Richard Earnshaw wrote:

C11: Summary of undefined behaviours.

— Addition or subtraction of a pointer into, or just beyond, an array
object and an
integer type produces a result that does not point into, or just beyond,
the same array
object (6.5.6).

That's totally irrelevant given the addition is created by the optimizer.

Wilco

On 15/06/17 19:18, Wilco Dijkstra wrote:
> Richard Earnshaw wrote:
> 
> C11: Summary of undefined behaviours.
> 
> — Addition or subtraction of a pointer into, or just beyond, an array
> object and an
> integer type produces a result that does not point into, or just beyond,
> the same array
> object (6.5.6).
> 
> That's totally irrelevant given the addition is created by the optimizer.
> 
> Wilco
>    

No it's not.  The optimizer doesn't create totally random bases.  If the
code + data is less than 1M in size, then any offsets it does create
will fit within the size of the relocations selected by the compiler.

R.

Richard Earnshaw wrote:
> No it's not.  The optimizer doesn't create totally random bases.  If the
> code + data is less than 1M in size, then any offsets it does create
> will fit within the size of the relocations selected by the compiler.

No that's completely false. There is no way you can guarantee that without
my patch. My patch is precisely there to ensure we only allow offsets that
guarantee linking succeeds if all code and data fits in 1M or 4GB.

Even then there are many cases where we don't know the size of an object and
so have to constrain the offset to something conservative (which cannot be
the full range it allows today).

Wilco

On Thu, 15 Jun 2017, Wilco Dijkstra wrote:

> Richard Earnshaw wrote:
> > No it's not.  The optimizer doesn't create totally random bases.  If the
> > code + data is less than 1M in size, then any offsets it does create
> > will fit within the size of the relocations selected by the compiler.
> 
> No that's completely false. There is no way you can guarantee that without
> my patch. My patch is precisely there to ensure we only allow offsets that
> guarantee linking succeeds if all code and data fits in 1M or 4GB.

For example, given (array + (i - INT_MAX)), it's quite possible the 
compiler could create a relocation for array - INT_MAX, and the original 
expression is perfectly OK if i == INT_MAX even though array - INT_MAX is 
far out of range.  (And array - INT_MAX as a C expression is only 
undefined at runtime, not at compile time if it's in code that is never 
executed.)

On 06/15/2017 03:52 PM, Joseph Myers wrote:

> For example, given (array + (i - INT_MAX)), it's quite possible the
> compiler could create a relocation for array - INT_MAX, and the original
> expression is perfectly OK if i == INT_MAX even though array - INT_MAX is
> far out of range.  (And array - INT_MAX as a C expression is only
> undefined at runtime, not at compile time if it's in code that is never
> executed.)

Some targets (typically uclinux-like things) cannot support this, as 
they move the data segment relative to the text segment upon loading, 
and need to know whether an address is text-relative or data-relative 
(and symbol information may not be available).

There's a target hook for that, but I can't find it now.

nathan