[PATCH v2 02/10] GAS: Unify code for SET_SECTION_RELOCS call

[Jan Beulich]

On 13.11.2025 20:15, Maciej W. Rozycki wrote:
> On Mon, 10 Nov 2025, Jan Beulich wrote:
> 
>>>> It's not becoming clear to me how this will end up being useful. In gas
>>>> there's a single call site right now. Hence only a single place where
>>>> you would need to make adjustments. Whereas with the change in place,
>>>> in order to handle the error status, you will need to make changes in
>>>> at least two places.
>>>
>>>  That is not true, `bfd_set_reloc' is called both by `write_relocs' and by 
>>> `obj_mach_o_reorder_section_relocs',
>>
>> Hmm, I must have managed to overlook that in grep output. Yet then it's
>> going to be three places that need touching to deal with the error status
>> you add subsequently.
> 
>  So what?  An error code has to be propagated somehow.  Minimising changes 
> ought not to take priority over code quality.
> 
>> As an aside, Mach-O may be a particularly bad example. It looks pretty
>> much unmaintained / unfinished, and hence I have already raised the question
>> of whether we really want to keep (partial) support for in in binutils. I'll
>> need to re-raise that in a more targeted way ...
> 
>  Be it as it may it shows that backends can have different requirements.  
> 
>  This one has a legitimate reason to call `bfd_set_reloc' last, that is to 
> shuffle the relocations first, and that technical aspect stands regardless 
> of the maintenance state of the code.
> 
>  And that is a separate aspect which I think should not be considered for 
> the acceptance of this patch set.  If the backend were to be removed, then 
> it'd have to go through the usual deprecation cycle taking at least half a 
> year anyway.
> 
>>> and this change makes it called only 
>>> once, improving code structure and making error handling straightforward.  
>>> Backends can choose whether to call `bfd_set_reloc' first (such as COFF) 
>>> or last (such as Mach-O) in relation to their own additional actions.
>>>
>>>  I could have mentioned this peculiarity in the change description, but it 
>>> has been lost in processing since I wrote this piece long ago.  The change 
>>> description stands regardless, it's an obvious cleanup.
>>
>> "Obvious" as in your personal view. Without that Mach-O aspect (where,
>> while benign afaict, the function oddly is called a 2nd time right now,
>> once ahead of the SET_SECTION_RELOCS() invocation and once in the course
>> of it) it's pretty much non-obvious to me. You're hiding a particular call
>> inside a (rarely used and imo improperly named) macro.
> 
>  It's not benign in my view in that it disturbs the code structure.  It 
> shouldn't be that a target hook makes an API call second time after it has 
> been already called from generic code earlier on, based on an assumption 
> that the operation has no side effects that would make such a second call 
> unsafe.  This is just bad API design, and I daresay no design actually 
> happened here and we ended up with this code purely by chance (if not to 
> use a stronger term).
> 
>  I find the merge of two related API calls into one a code structure 
> improvement as well.  Initially I left them both in place as I developed 
> this code and I wasn't happy with the outcome at all, it became too 
> convoluted.
> 
>  I consider my proposed change good enough and fit for purpose.  The macro 
> retains the existing name and follows the name of the BFD handler, so it's 
> not a regression, and makes it obvious what it's meant to do.  It's easy 
> to check anyway.  I don't think it hides anything.  And the name is going 
> to be changed to match the intent rather than a particular action in the 
> next step (which you considered unnecessary for a change, as if using an 
> inadequate name wasn't hiding things; I'm confused now).
> 
>  And last but not least this is software after all.  If someone comes with
> a better solution later on, we can always change the code.  It's not a 
> public API or ABI that's going to be cast in stone.

All fair arguments, yet (for now at least) not ones which would make me
change my view on it. This isn't a NAK, though; if another maintainer
gave their okay, I wouldn't stand in the way. Or maybe, after thinking
it through some more, I might end up doing so.

Jan