Arm64: assembling adrp with operand involving .

Arm64: assembling adrp with operand involving .

[Jan Beulich]

Nick,

I would expect these two

	adrp	x0, .
1:	adrp	x0, 1b

to assemble to the same encoding with similar attached relocations.
The first, however, appears to have evaluation of . deferred until the
end of assembly, i.e. the resulting operand depends on the number of
subsequent insns in the same source file. At a guess this is fallout
from eac4eb8ecb26 "Fix a problem assembling AArch64 sources when a
relocation is generated against a..." (for some reason the title is
truncated). According to my observations other insns aren't affected,
yet the change to parse_adrp() doesn't really stand out in said commit.
Hence I'm neither really certain that's the one, nor how a possible fix
could look like. Do you have any thoughts?

Thanks, Jan

Re: Arm64: assembling adrp with operand involving .

[Nick Clifton]

Hi Jan,

> I would expect these two
> 
> 	adrp	x0, .
> 1:	adrp	x0, 1b
> 
> to assemble to the same encoding with similar attached relocations.

Me too.

> The first, however, appears to have evaluation of . deferred until the
> end of assembly, i.e. the resulting operand depends on the number of
> subsequent insns in the same source file. At a guess this is fallout
> from eac4eb8ecb26 "Fix a problem assembling AArch64 sources when a
> relocation is generated against a..." (for some reason the title is
> truncated).

That is because I put the rest of the description on a second line, sorry.
The second line reads:

  generated against a symbol that has a defined value.

The patch was created as a fix for PR 27217.

>  According to my observations other insns aren't affected,
> yet the change to parse_adrp() doesn't really stand out in said commit.
> Hence I'm neither really certain that's the one, nor how a possible fix
> could look like. Do you have any thoughts?

Well the change added a new argument to the ...get_expression() function,
so all callers were updated.  There was no specific intention to change
parse_adrp for some other reason.

Anyway - this does look like a bug, although I think that it might be
restricted to just an unadorned reference to dot.  ie:

   adrp	x0, .
   1:	adrp	x0, 1b
   adrp	x0, . - 8

When assembled and then dumped, gives:

0000000000000000 <.text>:
    0:	90000000 	adrp	x0, 0 <.text>
			0: R_AARCH64_ADR_PREL_PG_HI21	.text+0xc
    4:	90000000 	adrp	x0, 0 <.text>
			4: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4
    8:	90000000 	adrp	x0, 0 <.text>
			8: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4

So the ". - 8" expression has evaluated correctly, but the "." expression
has not.  Would you care to open a BZ for this ?

Cheers
   Nick

Re: Arm64: assembling adrp with operand involving .

[Jan Beulich]

On 14.02.2022 14:35, Nick Clifton wrote:
>> I would expect these two
>>
>> 	adrp	x0, .
>> 1:	adrp	x0, 1b
>>
>> to assemble to the same encoding with similar attached relocations.
> 
> Me too.
> 
>> The first, however, appears to have evaluation of . deferred until the
>> end of assembly, i.e. the resulting operand depends on the number of
>> subsequent insns in the same source file. At a guess this is fallout
>> from eac4eb8ecb26 "Fix a problem assembling AArch64 sources when a
>> relocation is generated against a..." (for some reason the title is
>> truncated).
> 
> That is because I put the rest of the description on a second line, sorry.
> The second line reads:
> 
>   generated against a symbol that has a defined value.
> 
> The patch was created as a fix for PR 27217.
> 
>>  According to my observations other insns aren't affected,
>> yet the change to parse_adrp() doesn't really stand out in said commit.
>> Hence I'm neither really certain that's the one, nor how a possible fix
>> could look like. Do you have any thoughts?
> 
> Well the change added a new argument to the ...get_expression() function,
> so all callers were updated.  There was no specific intention to change
> parse_adrp for some other reason.
> 
> Anyway - this does look like a bug, although I think that it might be
> restricted to just an unadorned reference to dot.  ie:
> 
>    adrp	x0, .
>    1:	adrp	x0, 1b
>    adrp	x0, . - 8
> 
> When assembled and then dumped, gives:
> 
> 0000000000000000 <.text>:
>     0:	90000000 	adrp	x0, 0 <.text>
> 			0: R_AARCH64_ADR_PREL_PG_HI21	.text+0xc
>     4:	90000000 	adrp	x0, 0 <.text>
> 			4: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4
>     8:	90000000 	adrp	x0, 0 <.text>
> 			8: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4
> 
> So the ". - 8" expression has evaluated correctly, but the "." expression
> has not.

I've mentioned this aspect in the bug (see below), but now that I look
again I'm not sure anymore: Wouldn't this be .text+0? And isn't it
getting close only because no further insns are following?

>  Would you care to open a BZ for this ?

Bug 28888.

Jan

Re: Arm64: assembling adrp with operand involving .

[Jan Beulich]

On 14.02.2022 14:35, Nick Clifton wrote:
>>  According to my observations other insns aren't affected,
>> yet the change to parse_adrp() doesn't really stand out in said commit.
>> Hence I'm neither really certain that's the one, nor how a possible fix
>> could look like. Do you have any thoughts?
> 
> Well the change added a new argument to the ...get_expression() function,
> so all callers were updated.  There was no specific intention to change
> parse_adrp for some other reason.
> 
> Anyway - this does look like a bug,

First of all tc-spu.c has a comment precisely to this effect:
deferred_expression() does not evaluate . at the point of use of the
expression. But I think beyond the patch sent earlier today to fix
some quirks of the original change, there's a more fundamental
question: Was it really intented to reference the (equated) symbol
in the relocations? There's now an inconsistency in that some
relocations against constants reference the constants (when
aarch64_get_expression()'s defer_resolution is false) while for the
ones here and a few others it's the symbol which is referenced. I
think one or the other ought to be used consistently, and judging
from other targets (x86 for example) it _may_ be that it should be
the constants (but it may also be that x86 is flawed [there are
quite certainly also quirks there]; from a purely abstract pov I'd
say the symbols ought to be used if they're global and constants
ought to be used when the symbols aren't global, or are e.g. hidden).

Some of my confusion with the original change is that, rather than
doing more evaluation of the involved expression, you switched to
doing less. This felt the wrong way round. I've come up with this
(not cleaned up) alternative, which makes . uses work as expected
while still taking care of the issue in the original bug report:

--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -604,7 +604,7 @@ aarch64_get_expression (expressionS *  e
   input_line_pointer = *str;
   in_aarch64_get_expression = true;
   if (defer_resolution)
-    seg = deferred_expression (ep);
+    seg = /*deferred_*/expression (ep);
   else
     seg = expression (ep);
   in_aarch64_get_expression = false;
@@ -8824,7 +8824,8 @@ md_apply_fix (fixS * fixP, valueT * valP
 
   /* Note whether this will delete the relocation.  */
 
-  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel)
+  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel
+      && aarch64_force_reloc (fixP->fx_r_type) <= 0)
     fixP->fx_done = 1;
 
   /* Process the relocations.  */

But when using a transitive equate, the intermediate symbol is then
used, which still doesn't feel quite right. Hence my thought of
doing yet more evaluation of the expression.

If, otoh, the symbol references in the relocs are indeed meant to be
as they are now, then the only other solution to the problem with .
that I can see is to introduce yet another expression parsing mode,
which would resolve . but defer everything else. One problem with
skipping evaluation of the expression is that in

	.text

	.set x, 0x12345678
	.eqv bar, x
foo:
//	adrp	x0, x
//	add	x0, x0, :lo12:x
	adrp	x0, bar
	add	x0, x0, :lo12:bar

	.set x, 0x98765432
	adrp	x0, x
	add	x0, x0, :lo12:x
	adrp	x0, bar
	add	x0, x0, :lo12:bar

the two pairs of relocations against "bar" necessarily can only
reflect one value, while "bar" being a forward ref would suggest
x'es original value to be used in the first pair, but x'es final
value to be used by the latter one. (Additionally the commented out
lines cause "redefined symbol cannot be used on reloc", which I
expect would go away as well when fully resolving the expressions.)

The one argument speaking against resolving expressions right in
aarch64_get_expression() is that symbol properties (global, hidden)
aren't necessarily known until quite a bit later. So perhaps really
a two-phase approach would be needed - aarch64_get_expression() to
simply call expression(), but fixup processing then resolving the
expressions as long as no global is involved. Which in turn doesn't
feel like it should be arch-specific ...

Jan

> although I think that it might be
> restricted to just an unadorned reference to dot.  ie:
> 
>    adrp	x0, .
>    1:	adrp	x0, 1b
>    adrp	x0, . - 8
> 
> When assembled and then dumped, gives:
> 
> 0000000000000000 <.text>:
>     0:	90000000 	adrp	x0, 0 <.text>
> 			0: R_AARCH64_ADR_PREL_PG_HI21	.text+0xc
>     4:	90000000 	adrp	x0, 0 <.text>
> 			4: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4
>     8:	90000000 	adrp	x0, 0 <.text>
> 			8: R_AARCH64_ADR_PREL_PG_HI21	.text+0x4
> 
> So the ". - 8" expression has evaluated correctly, but the "." expression
> has not.  Would you care to open a BZ for this ?
> 
> Cheers
>    Nick
> 

Re: Arm64: assembling adrp with operand involving .

[Nick Clifton]

Hi Jan,

   [I must admit that I have the lost the original context for this thread...]

> Was it really intented to reference the (equated) symbol
> in the relocations? There's now an inconsistency in that some
> relocations against constants reference the constants (when
> aarch64_get_expression()'s defer_resolution is false) while for the
> ones here and a few others it's the symbol which is referenced. I
> think one or the other ought to be used consistently,

I totally agree - the assembler should behave consistently if at all possible,

> from a purely abstract pov I'd
> say the symbols ought to be used if they're global

(or otherwise visible and alterable outside of the current context, yes ?)

> and constants
> ought to be used when the symbols aren't global, or are e.g. hidden).

Agreed.

> Some of my confusion with the original change is that, rather than
> doing more evaluation of the involved expression, you switched to
> doing less. 

(This was probably due to my being lazy and not really thinking the problem through ...)

> This felt the wrong way round. I've come up with this
> (not cleaned up) alternative, which makes . uses work as expected
> while still taking care of the issue in the original bug report:


> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -604,7 +604,7 @@ aarch64_get_expression (expressionS *  e
>     input_line_pointer = *str;
>     in_aarch64_get_expression = true;
>     if (defer_resolution)
> -    seg = deferred_expression (ep);
> +    seg = /*deferred_*/expression (ep);
>     else
>       seg = expression (ep);
>     in_aarch64_get_expression = false;
> @@ -8824,7 +8824,8 @@ md_apply_fix (fixS * fixP, valueT * valP
>   
>     /* Note whether this will delete the relocation.  */
>   
> -  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel)
> +  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel
> +      && aarch64_force_reloc (fixP->fx_r_type) <= 0)
>       fixP->fx_done = 1;
>   
>     /* Process the relocations.  */
> 
> But when using a transitive equate, the intermediate symbol is then
> used, which still doesn't feel quite right. Hence my thought of
> doing yet more evaluation of the expression.

I do not get what you are saying here.  Could you provide a small
example of code that might experience this problem ?

Still, given that this approach appears to be simple, self-contained
and correct, I am inclined to think that it is the best solution...


> If, otoh, the symbol references in the relocs are indeed meant to be
> as they are now,

As a general rule, this is one of those things that the target's assembler
language specification ought to define.  Of course not all targets do have
a well defined assembler language specification, so then we have to do the
best that we can.


> then the only other solution to the problem with .
> that I can see is to introduce yet another expression parsing mode,
> which would resolve . but defer everything else. One problem with
> skipping evaluation of the expression is that in
> 
> 	.text
> 
> 	.set x, 0x12345678
> 	.eqv bar, x
> foo:
> //	adrp	x0, x
> //	add	x0, x0, :lo12:x
> 	adrp	x0, bar
> 	add	x0, x0, :lo12:bar
> 
> 	.set x, 0x98765432
> 	adrp	x0, x
> 	add	x0, x0, :lo12:x
> 	adrp	x0, bar
> 	add	x0, x0, :lo12:bar
> 
> the two pairs of relocations against "bar" necessarily can only
> reflect one value, while "bar" being a forward ref would suggest
> x'es original value to be used in the first pair, but x'es final
> value to be used by the latter one.

Which would definitely confuse the programmer.  So I think that we
should avoid this route unless specifically required by the target's
assembler language specification.  (IE, for the AArch64, I could not
find anything saying that this behaviour is required, so I think that
we should not go down this route...)

Cheers
   Nick

Re: Arm64: assembling adrp with operand involving .

[Jan Beulich]

On 09.05.2022 17:21, Nick Clifton wrote:
>> --- a/gas/config/tc-aarch64.c
>> +++ b/gas/config/tc-aarch64.c
>> @@ -604,7 +604,7 @@ aarch64_get_expression (expressionS *  e
>>     input_line_pointer = *str;
>>     in_aarch64_get_expression = true;
>>     if (defer_resolution)
>> -    seg = deferred_expression (ep);
>> +    seg = /*deferred_*/expression (ep);
>>     else
>>       seg = expression (ep);
>>     in_aarch64_get_expression = false;
>> @@ -8824,7 +8824,8 @@ md_apply_fix (fixS * fixP, valueT * valP
>>   
>>     /* Note whether this will delete the relocation.  */
>>   
>> -  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel)
>> +  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel
>> +      && aarch64_force_reloc (fixP->fx_r_type) <= 0)
>>       fixP->fx_done = 1;
>>   
>>     /* Process the relocations.  */
>>
>> But when using a transitive equate, the intermediate symbol is then
>> used, which still doesn't feel quite right. Hence my thought of
>> doing yet more evaluation of the expression.
> 
> I do not get what you are saying here.  Could you provide a small
> example of code that might experience this problem ?

	.text

	.set x, 0x12345678
	.eqv bar, x
foo:
	adrp	x0, x
	add	x0, x0, :lo12:x

	adrp	x0, bar
	add	x0, x0, :lo12:bar

With the proposed change, the first two relocations would be against
*ABS*:0x12345678, whereas the latter two relocations would be against
x. (Prior to the proposed change, the first two relocations are
against x and the latter two against bar.)

> Still, given that this approach appears to be simple, self-contained
> and correct, I am inclined to think that it is the best solution...
> 
> 
>> If, otoh, the symbol references in the relocs are indeed meant to be
>> as they are now,
> 
> As a general rule, this is one of those things that the target's assembler
> language specification ought to define.  Of course not all targets do have
> a well defined assembler language specification, so then we have to do the
> best that we can.

Hmm, I would have thought such rules ought to be largely arch-agnostic.

In this context, do you have any thoughts about "Arm64: follow-on to PR
gas/27217 fix"? I don't really feel comfortable committing this without
either you (as the author of the original fix) or one of the arch
maintainers having signaled agreement in some way. Of course, depending
on the further route taken for 28888 some (most) of what's done there
may disappear again (alongside parts of the original fix for 27217).

Jan

>> then the only other solution to the problem with .
>> that I can see is to introduce yet another expression parsing mode,
>> which would resolve . but defer everything else. One problem with
>> skipping evaluation of the expression is that in
>>
>> 	.text
>>
>> 	.set x, 0x12345678
>> 	.eqv bar, x
>> foo:
>> //	adrp	x0, x
>> //	add	x0, x0, :lo12:x
>> 	adrp	x0, bar
>> 	add	x0, x0, :lo12:bar
>>
>> 	.set x, 0x98765432
>> 	adrp	x0, x
>> 	add	x0, x0, :lo12:x
>> 	adrp	x0, bar
>> 	add	x0, x0, :lo12:bar
>>
>> the two pairs of relocations against "bar" necessarily can only
>> reflect one value, while "bar" being a forward ref would suggest
>> x'es original value to be used in the first pair, but x'es final
>> value to be used by the latter one.
> 
> Which would definitely confuse the programmer.  So I think that we
> should avoid this route unless specifically required by the target's
> assembler language specification.  (IE, for the AArch64, I could not
> find anything saying that this behaviour is required, so I think that
> we should not go down this route...)
> 
> Cheers
>    Nick
> 

Re: Arm64: assembling adrp with operand involving .

[Jan Beulich]

On 18.05.2022 09:26, Jan Beulich via Binutils wrote:
> On 09.05.2022 17:21, Nick Clifton wrote:
>>> --- a/gas/config/tc-aarch64.c
>>> +++ b/gas/config/tc-aarch64.c
>>> @@ -604,7 +604,7 @@ aarch64_get_expression (expressionS *  e
>>>     input_line_pointer = *str;
>>>     in_aarch64_get_expression = true;
>>>     if (defer_resolution)
>>> -    seg = deferred_expression (ep);
>>> +    seg = /*deferred_*/expression (ep);
>>>     else
>>>       seg = expression (ep);
>>>     in_aarch64_get_expression = false;
>>> @@ -8824,7 +8824,8 @@ md_apply_fix (fixS * fixP, valueT * valP
>>>   
>>>     /* Note whether this will delete the relocation.  */
>>>   
>>> -  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel)
>>> +  if (fixP->fx_addsy == 0 && !fixP->fx_pcrel
>>> +      && aarch64_force_reloc (fixP->fx_r_type) <= 0)
>>>       fixP->fx_done = 1;
>>>   
>>>     /* Process the relocations.  */
>>>
>>> But when using a transitive equate, the intermediate symbol is then
>>> used, which still doesn't feel quite right. Hence my thought of
>>> doing yet more evaluation of the expression.
>>
>> I do not get what you are saying here.  Could you provide a small
>> example of code that might experience this problem ?
> 
> 	.text
> 
> 	.set x, 0x12345678
> 	.eqv bar, x
> foo:
> 	adrp	x0, x
> 	add	x0, x0, :lo12:x
> 
> 	adrp	x0, bar
> 	add	x0, x0, :lo12:bar
> 
> With the proposed change, the first two relocations would be against
> *ABS*:0x12345678, whereas the latter two relocations would be against
> x. (Prior to the proposed change, the first two relocations are
> against x and the latter two against bar.)
> 
>> Still, given that this approach appears to be simple, self-contained
>> and correct, I am inclined to think that it is the best solution...

I wonder whether we can come to some sort of resolution here for 2.39.
I'd be happy to tidy up my patch and submit it, as long as its
downsides are understood and deemed acceptable (i.e. at least slightly
better than the present state).

Jan

>>> If, otoh, the symbol references in the relocs are indeed meant to be
>>> as they are now,
>>
>> As a general rule, this is one of those things that the target's assembler
>> language specification ought to define.  Of course not all targets do have
>> a well defined assembler language specification, so then we have to do the
>> best that we can.
> 
> Hmm, I would have thought such rules ought to be largely arch-agnostic.
> 
> In this context, do you have any thoughts about "Arm64: follow-on to PR
> gas/27217 fix"? I don't really feel comfortable committing this without
> either you (as the author of the original fix) or one of the arch
> maintainers having signaled agreement in some way. Of course, depending
> on the further route taken for 28888 some (most) of what's done there
> may disappear again (alongside parts of the original fix for 27217).
> 
> Jan
> 
>>> then the only other solution to the problem with .
>>> that I can see is to introduce yet another expression parsing mode,
>>> which would resolve . but defer everything else. One problem with
>>> skipping evaluation of the expression is that in
>>>
>>> 	.text
>>>
>>> 	.set x, 0x12345678
>>> 	.eqv bar, x
>>> foo:
>>> //	adrp	x0, x
>>> //	add	x0, x0, :lo12:x
>>> 	adrp	x0, bar
>>> 	add	x0, x0, :lo12:bar
>>>
>>> 	.set x, 0x98765432
>>> 	adrp	x0, x
>>> 	add	x0, x0, :lo12:x
>>> 	adrp	x0, bar
>>> 	add	x0, x0, :lo12:bar
>>>
>>> the two pairs of relocations against "bar" necessarily can only
>>> reflect one value, while "bar" being a forward ref would suggest
>>> x'es original value to be used in the first pair, but x'es final
>>> value to be used by the latter one.
>>
>> Which would definitely confuse the programmer.  So I think that we
>> should avoid this route unless specifically required by the target's
>> assembler language specification.  (IE, for the AArch64, I could not
>> find anything saying that this behaviour is required, so I think that
>> we should not go down this route...)
>>
>> Cheers
>>    Nick
>>
> 

Re: Arm64: assembling adrp with operand involving .

[Nick Clifton]

Hi Jan,

> I wonder whether we can come to some sort of resolution here for 2.39.
> I'd be happy to tidy up my patch and submit it, as long as its
> downsides are understood and deemed acceptable (i.e. at least slightly
> better than the present state).

I know that you have asked me for an opinion on this problem, but the
truth is that I do not understand it well enough to have one.  So I am
happy to go with whatever you feel is best.  Making things better is
good, and the downsides can always be documented and if necessary,
addressed later.

Cheers
   Nick