On 01 Nov 2022 15:36, Weimin Pan via Binutils wrote:
> On 10/30/2022 7:03 AM, Mike Frysinger via Binutils wrote:
> > On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
> >> +static void
> >> +sframe_unwind (struct sframe_unwind_info *sf, void **ra_lst,
> >> +	       int *ra_size, int *errp)
> >> +{
> >> ...
> >> +#ifdef __x86_64__
> > this really needs better delegation for porting than inlined in the middle
> > of a large portable file.  can you at least factor it out into a header ?
> 
> Sorry, not sure I follow this comment. Would you please elaborate on 
> "factoring it out into a header"?

common code like this should not be stuffed with arch-specific checks and
implementations.  it makes it hard for more ports to happen, and for code
to be checked/validated.

further, as-written, this code can only be run on a system with a matching
runtime architecture.  normally a library that is meant for processing of
files is host-agnostic.  that's like the whole design of libbfd and other
tools in the binutils project.  i don't know how important that use case
is to this particular bit of code though.

to at least address the code health, create an API between the common code
and the arch-specific code, and then add arch-specific files that implement
that API.  it could be a header of small inline functions.

for the host-agnostic part, i haven't really read the library API to make
suggestions for how to address it.  libbfd takes objects/arches as arguments
and then operates according to those.
-mike