Re: [PATCH][AArch64] Fix symbol offset limit

[Wilco Dijkstra <Wilco.Dijkstra@arm.com>]

Hi Richard,

>> No - the testcases fail with that.
>
> Hmm, OK.  Could you give more details?  What does the motivating case
> actually look like?

Well it's now a very long time ago since I first posted this patch but the failure
was in SPEC. It did something like &array[0xffffff000 - x], presumably after some
optimization with some specific options I was using at the time. The exact details
don't matter since I've got minimal testcases.

> One of the reasons this is a hard patch to review is that it doesn't
> include a testcase for the kind of situation it's trying to fix.

There is a very simple testcase which fails before and passes with my patch.

>> It also reduces codequality by not allowing commonly used offsets as
>> part of the symbol relocation.
>
> Which kinds of cases specifically?  Although I'm guessing the problem is
> when indexing into external arrays of unknown size.

Well there are 41000 uses in SPEC2017 that fail the offset_within_block_p
test but pass my range test. There are 3 cases where the reverse is true
(a huge offset: 17694720).

Overall my range test allows 99.99% of the offsets, so we can safely conclude
my patch doesn't regress any existing code.

> So IMO we should be able to assume that the start and end + 1 addresses
> of any referenced object are within reach.  For section anchors, we can
> extend that to the containing anchor block, which is really just a
> super-sized object.

This isn't about section anchors - in many cases the array is an extern.

> The question then is what to do about symbols whose size isn't known.
> And I agree that unconditionally applying the full code-model offset
> range is too aggressive in that case.

That's very common indeed, so we need to apply some kind of reasonable
range check.

> Well, for one thing, if code quality isn't affected by using +/-64k
> for the tiny model, do we have any evidence that we need a larger offset
> for the other code models?

Certainly for SPEC +-64KB is too small, but SPEC won't build in the tiny code
model. For the tiny code model the emphasis should be on ensuring that code
that fits should build correctly rather than trying to optimize it to the max and
getting relocations that are out of range... So I believe it is reasonable to use a 
more conservative range in the tiny model.

> But more importantly, we can't say definitively that code quality isn't
> affected, only that it wasn't affected for the cases we've looked at.
> People could patch the compiler if the new ranges turn out not to strike
> the right balance for their use cases, but not everyone wants to do that.

Well if we're worried about codequality then the offset in block approach
affects it the most. Offsets larger than 1MB are extremely rare, so the chance
that there will ever be a request for a larger range is simply zero.

> Maybe we need a new command-line option.

That's way overkill... All this analysis is overcomplicating what is really a very
basic problem with a very simple solution.

Cheers,
Wilco