Re: Arm64: assembling adrp with operand involving .

[Jan Beulich <jbeulich@suse.com>]

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
>