Re: [PATCH 5/5] libgloss: merge bfin into top-level Makefile

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

On 07 Feb 2022 13:07, Corinna Vinschen wrote:
> On Feb  7 12:44, Corinna Vinschen wrote:
> > On Feb  5 00:46, Mike Frysinger wrote:
> > > Avoid a recursive make to speed things up a bit.
> > > ---
> > >  libgloss/Makefile.am       |   9 +
> > >  libgloss/Makefile.in       | 342 +++++++++++++++++++++++++++++++------
> > >  libgloss/bfin/Makefile.in  | 207 ----------------------
> > >  libgloss/bfin/Makefile.inc |  71 ++++++++
> > >  libgloss/configure         |  33 +++-
> > >  libgloss/configure.ac      |   5 +-
> > >  6 files changed, 406 insertions(+), 261 deletions(-)
> > >  delete mode 100644 libgloss/bfin/Makefile.in
> > >  create mode 100644 libgloss/bfin/Makefile.inc
> > > + [...]
> > > +# Here is all of the development board stuff.
> > > +multilibtool_DATA += \
> > > +	%D%/crt0.o \
> > > +	%D%/basiccrt.o \
> > > +	%D%/basiccrts.o \
> > > +	%D%/basiccrt561.o \
> > > +	%D%/basiccrt561s.o \
> > > +	%D%/basiccrt561b.o
> > > +# 	%D%/basiccrt60x.o \
> > > +#	%D%/basiccrt60xs.o \
> > > +#	%D%/basiccrt60xc1.o
> > 
> > Not sure how you guys actually building and using this stuff think about
> > this, but I, for one, am a little put off by this change.  The old code
> > in bfin/Makefile.in had a few nice definitions like BOARD_BSP,
> > BOARD_CRT0S, SIM_BSP, SIM_CRT0, etc.  Why didn't you take them over?
> > 
> > That's not saying we can't push this patch, but somehow it's losing
> > readability.  Now it's just a list of arbitrary files to create.
> 
> Also, what about the install-sim / install-board split?  Do we know
> for sure that they are not used by somebody?

tl;dr: i think it's fine.

the bsp/sim logic looks like blind copy & paste between some subdirs.
today, we have about 50 targets in libgloss.  "install-sim" exists in
only 11 of them.  "install-board" only exists in bfin.  neither of the
rules are exposed in the topdir, so you'd have to be in libgloss/bfin/
and run `make install-xxx` for them to work.  but that won't really work
because (1) the multilib multiplex logic only exists in the top-level
libgloss/ dir and (2) none of the other vars (e.g. compiler settings)
are exported there ... instead, we rely on the recursive make out of the
top-level libgloss/ to pass things down via $(FLAGS_TO_PASS).

for bfin specifically, tbh, the bsp side of things has always been a bit
underdeveloped.  we never had a lot of customer requests, and anyone who
did use newlib would write their own glue logic.  other than the basic
crt, there's only 2 functions in there ... clear_cache_range is internal
hook for gcc to workaround hardware anomalies, and exit which just hangs
the CPU while triggering JTAG ICE exceptions.  even the basic crt was
mostly copied from the proprietary side of the company, and has had some
feedback from people that it kind of sucks.  so i think at this point,
the bfin bsp is likely to not gain a whole lot more.

on the otherhand, the bfin sim has seen a lot of work which is why it has
a bit more logic to it.

but let's look at your "readability" feedback:
* we still have sep libsim.a & libbfinbsp.a targets to keep the diff env
  separate since our linker scripts refer to them directly.  that takes
  care of the *_BSP & *_OBJS & *_LDFLAGS variables.
* the Makefile.in today has sep install targets, although we always install
  both.  the new Makefile.inc doesn't need them since the variables imply
  whether things are installed.  that takes care of install-* and *_INSTALL.
* that leaves *_SCRIPTS & *_CRT0 variables.  i'm adding them to the same
  multilibtool_DATA variable which is what you highlighted above.  i could
  add some variables for these if you really wanted, it's just that automake
  constrains the namespace a little, as does recursive make.  so it'd have
  to be something like:
    %C%_sim_crts = %D%/crt0.o
    %C%_bsp_crts = %D%/basiccrt.o ......
    multilibtool_DATA += $(%C%_sim_crts) $(%C%_bsp_crts)
  and at this point, considering how simple the existing logic is, and how
  unlikely it is to change, it didn't seem worth adding sep vars.  i could
  at least keep separate comment blocks if you wanted.  so i guess move the
    multilibtool_DATA += %D%/crt0.o
  to before the "development board stuff" section.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]