Re: [PATCH] PPC: drop redundant value conversion from md_assemble()

[Jan Beulich <jbeulich@suse.com>]

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