PPC: drop redundant value conversion from md_assemble()

  Just ahead of the enclosing OBJ_ELF conditional the exact same
conversion was already carried out, with "val" not further changed in
between.
---
Doesn't X_unsigned also need taking into account in the remaining
identical piece of code?
  

On Mon, Dec 16, 2024 at 12:40:22PM +0100, Jan Beulich wrote:
> Just ahead of the enclosing OBJ_ELF conditional the exact same
> conversion was already carried out, with "val" not further changed in
> between.

Thanks, yes, that duplication should be removed.

> Doesn't X_unsigned also need taking into account in the remaining
> identical piece of code?

Maybe.  In adding that piece of code, I was just interested in a very
limited set of expressions.  The trouble with caring too much about
correctness is that someone will come along and tell you that despite
your best efforts you still didn't get it correct.  ;-)

> --- a/gas/config/tc-ppc.c
> +++ b/gas/config/tc-ppc.c
> @@ -3516,13 +3516,6 @@ md_assemble (char *str)
>  	  char *orig_str = str;
>  	  bfd_reloc_code_real_type reloc = ppc_elf_suffix (&str, &ex);
>  
> -	  if (ex.X_op == O_constant)
> -	    {
> -	      val = ex.X_add_number;
> -	      if (sizeof (ex.X_add_number) < sizeof (val)
> -		  && (ex.X_add_number < 0) != ex.X_extrabit)
> -		val = val ^ ((addressT) -1 ^ (uint64_t) -1);
> -	    }
>  	  if (reloc != BFD_RELOC_NONE)
>  	    switch (reloc)
>  	      {
  

On 17.12.2024 00:42, Alan Modra wrote:
> On Mon, Dec 16, 2024 at 12:40:22PM +0100, Jan Beulich wrote:
>> Just ahead of the enclosing OBJ_ELF conditional the exact same
>> conversion was already carried out, with "val" not further changed in
>> between.
> 
> Thanks, yes, that duplication should be removed.
> 
>> Doesn't X_unsigned also need taking into account in the remaining
>> identical piece of code?
> 
> Maybe.  In adding that piece of code, I was just interested in a very
> limited set of expressions.  The trouble with caring too much about
> correctness is that someone will come along and tell you that despite
> your best efforts you still didn't get it correct.  ;-)

Right, I'm fully aware of this with past and present efforts to make
expression evaluation more predictable for "uncommon" cases. Let me
widen the question and pull in Nick as well.

According to a comment in expr.h, X_extrabit was introduced "so that
e.g. expressions used with .sleb128 directives can use the full range
available for an unsigned word, but can also properly represent all
values of a signed word." That alone, however, can be achieved with
properly setting X_unsigned, afaict. I'm therefore wondering whether
the comment is inaccurate (potentially [also] meaning "the full range
available for a negated unsigned word"), or whether X_extrabit should
actually be dropped again, ensuring X_unsigned is properly set in all
relevant places (in particular everywhere operations on two
O_constant-s are resolved).

Resolving the above of course is also somewhat related to the fact
that we resolve operations on constants in three places (expr(),
resolve_expression(), and resolve_symbol_value()), in three slightly
(or not so slightly) different ways. For this I haven't come to a
reasonable conclusion yet; one way or another we surely would benefit
from doing all of this in just one place.

And then finally (for the moment; surely there are more issues
lurking), there's an issue with .octa. By analogy, .octa on a BFD64
build ought to function similar to .quad on a !BFD64 build. The
latter, while (naturally) limited in value range, can still be used
with expressions which can only be resolved at the end of assembly.
.octa, otoh, can't be, as there's no way to internally represent the
necessary relocation. I'm uncertain, though, about the seemingly
"natural" approach of simply adding BFD_RELOC_128 and BFD_RELOC_PC128.
That's mainly connected to me not being certain whether I actually see
all the potential implications particularly on target-specific code
when adding such generic (internal) relocation types.

Jan
  

Hi Jan, Hi Alan,

>>> Doesn't X_unsigned also need taking into account in the remaining
>>> identical piece of code?
>>
>> Maybe.  In adding that piece of code, I was just interested in a very
>> limited set of expressions.  The trouble with caring too much about
>> correctness is that someone will come along and tell you that despite
>> your best efforts you still didn't get it correct.  ;-)

> According to a comment in expr.h, X_extrabit was introduced "so that
> e.g. expressions used with .sleb128 directives can use the full range
> available for an unsigned word, but can also properly represent all
> values of a signed word." That alone, however, can be achieved with
> properly setting X_unsigned, afaict. I'm therefore wondering whether
> the comment is inaccurate (potentially [also] meaning "the full range
> available for a negated unsigned word"), or whether X_extrabit should
> actually be dropped again, ensuring X_unsigned is properly set in all
> relevant places (in particular everywhere operations on two
> O_constant-s are resolved).

I get that doing this would help to simplify things and so make the code
cleaner.  But would it really be worth it, given the potential for
introducing new bugs ?  Ie, if it ain't broke, don't fix it.


> Resolving the above of course is also somewhat related to the fact
> that we resolve operations on constants in three places (expr(),
> resolve_expression(), and resolve_symbol_value()), in three slightly
> (or not so slightly) different ways. For this I haven't come to a
> reasonable conclusion yet; one way or another we surely would benefit
> from doing all of this in just one place.

Agreed - in principle at least.  All three functions are quite complex
however, so merging them would be a difficult process.  Perhaps if you
are able to identify common sub-parts of the functions and extract these
into shared small helper functions this would make things easier ?



> And then finally (for the moment; surely there are more issues
> lurking), there's an issue with .octa. By analogy, .octa on a BFD64
> build ought to function similar to .quad on a !BFD64 build. The
> latter, while (naturally) limited in value range, can still be used
> with expressions which can only be resolved at the end of assembly.
> .octa, otoh, can't be, as there's no way to internally represent the
> necessary relocation. I'm uncertain, though, about the seemingly
> "natural" approach of simply adding BFD_RELOC_128 and BFD_RELOC_PC128.
> That's mainly connected to me not being certain whether I actually see
> all the potential implications particularly on target-specific code
> when adding such generic (internal) relocation types.

Is there a need for this ?  That is - do we have users complaining that
the .octa pseudo-op is unable to handle unresolvable expressions ?  My
guess is that where this pseudo is used, it is always used with constants
or simple expressions that can be resolved at assembly time, and hence
no one has noticed the lack of relocation support.

Cheers
   Nick
  

On 19.12.2024 11:14, Nick Clifton wrote:
>>>> Doesn't X_unsigned also need taking into account in the remaining
>>>> identical piece of code?
>>>
>>> Maybe.  In adding that piece of code, I was just interested in a very
>>> limited set of expressions.  The trouble with caring too much about
>>> correctness is that someone will come along and tell you that despite
>>> your best efforts you still didn't get it correct.  ;-)
> 
>> According to a comment in expr.h, X_extrabit was introduced "so that
>> e.g. expressions used with .sleb128 directives can use the full range
>> available for an unsigned word, but can also properly represent all
>> values of a signed word." That alone, however, can be achieved with
>> properly setting X_unsigned, afaict. I'm therefore wondering whether
>> the comment is inaccurate (potentially [also] meaning "the full range
>> available for a negated unsigned word"), or whether X_extrabit should
>> actually be dropped again, ensuring X_unsigned is properly set in all
>> relevant places (in particular everywhere operations on two
>> O_constant-s are resolved).
> 
> I get that doing this would help to simplify things and so make the code
> cleaner.  But would it really be worth it, given the potential for
> introducing new bugs ?  Ie, if it ain't broke, don't fix it.

But it is quite broken, as I noticed while doing other work (and also
already earlier on), and as expressed in the last paragraph of the
description of "gas: special-case division / modulo by ±1".

>> Resolving the above of course is also somewhat related to the fact
>> that we resolve operations on constants in three places (expr(),
>> resolve_expression(), and resolve_symbol_value()), in three slightly
>> (or not so slightly) different ways. For this I haven't come to a
>> reasonable conclusion yet; one way or another we surely would benefit
>> from doing all of this in just one place.
> 
> Agreed - in principle at least.  All three functions are quite complex
> however, so merging them would be a difficult process.  Perhaps if you
> are able to identify common sub-parts of the functions and extract these
> into shared small helper functions this would make things easier ?

Yeah, this would likely be the approach to take.

>> And then finally (for the moment; surely there are more issues
>> lurking), there's an issue with .octa. By analogy, .octa on a BFD64
>> build ought to function similar to .quad on a !BFD64 build. The
>> latter, while (naturally) limited in value range, can still be used
>> with expressions which can only be resolved at the end of assembly.
>> .octa, otoh, can't be, as there's no way to internally represent the
>> necessary relocation. I'm uncertain, though, about the seemingly
>> "natural" approach of simply adding BFD_RELOC_128 and BFD_RELOC_PC128.
>> That's mainly connected to me not being certain whether I actually see
>> all the potential implications particularly on target-specific code
>> when adding such generic (internal) relocation types.
> 
> Is there a need for this ?  That is - do we have users complaining that
> the .octa pseudo-op is unable to handle unresolvable expressions ?

I am one such user. I was pretty surprised how limited the support there
(and to a lesser degree also for .quad) is.

>  My
> guess is that where this pseudo is used, it is always used with constants
> or simple expressions that can be resolved at assembly time, and hence
> no one has noticed the lack of relocation support.

Well. If .quad and .octa were documented to have several restrictions, one
might consider leaving them alone. "Might" because even then personally
I'd rather see us drop restrictions where feasible. Any such restrictions
only mean people have to invent their own workarounds.

Jan