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

[Maciej W. Rozycki]

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.

  Maciej