Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> Hi Richard,
>
>> If global_char really is a char then isn't that UB?
>
> No why?
Well, "simple expressions like &global_char + 0xffffff00" made it sound
like there really was a:
extern char global_char;
Only &global_char and &global_char + 1 are defined in that case.
I was probably taking the example too literally though.
> We can do all kinds of arithmetic based on pointers, either using
> pointer types or converted to uintptr_t. Note that the optimizer
> actually creates these expressions, for example arr[N-x] can be
> evaluated as (&arr[0] + N) - x. So this remains legal even if N is
> well outside the bounds of the array.
Yeah, true, and I remember valid "x[n - big_offset]" -> invalid
"x + big_offset - n" being a problem for MIPS too. It was one of
the things offset_within_block_p was helping to avoid.
>> I guess we still have to compile it without error though...
>
> *THIS* a million times... Any non-trivial application relies on UB behaviour
> with
> 100% certainty. But we still have to compile it correctly!
>
>>> To avoid this, limit the offset to +/-1MB so that the symbol needs to
>>> be within a
>>> 3.9GB 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.
>>
>> These new values seem a bit magical. Would it work for the original
>> testcase to use FORCE_TO_MEM if !offset_within_block_p (x, offset)?
>
> No - the testcases fail with that.
Hmm, OK. Could you give more details? What does the motivating case
actually look like?
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.
> 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.
I think the starting point for this stuff is that the address of every
referenced object has to be within reach for the code model. In other
words, offset 0 must be within reach. It follows that the end + 1
address of every referenced object except the last (in memory) will
also be within reach.
The question then is: do we want, as a special exception, to allow
the end + 1 address of the last object to be out of reach? I think
it would be very difficult to support that reliably without more
information. The compiler would be left guessing which object is
the final one and how much of it is out of reach.
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.
So when offset_within_block_p, it seems like we should apply the
documented ranges for the code model, or even ignore them altogether.
Limiting the range to 64k for the tiny model would regress valid
cases like:
char x[0x20000];
char f (void) { return x[0x1ffff]; }
I'm not saying this case is important. I'm just saying there's no
obvious reason why the offset shouldn't be valid given the above
assumptions.
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.
> So how would you want to make the offsets less magical? There isn't a clear
> limitation
> like for MOVW/MOVT relocations (which are limited to +-32KB), so it just is
> an arbitrary
> decision which allows this optimization for all frequently used offsets but
> avoids relocation
> failures caused by large offsets. The values I used were chosen so that
> codequality isn't
> affected, but it is practically impossible to trigger relocation errors.
>
> And they can always be changed again if required - this is not meant to be
> the final
> AArch64 patch ever!
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?
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.
Maybe we need a new command-line option. It seems like the cheap
way out, but it might be justified. The problem is that, without one,
the compiler would be unconditionally applying an offset range that has
no obvious relation (from a user's POV) to the documented -mcmodel range.
We can then set the option's default value to whatever we think is
reasonable. And FWIW I've no specific issue with the values you've
chosen except for the question above.
Thanks,
Richard
