From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.gentoo.org (woodpecker.gentoo.org [140.211.166.183]) by sourceware.org (Postfix) with ESMTP id 37F263858D20 for ; Mon, 7 Feb 2022 13:11:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 37F263858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gentoo.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gentoo.org Received: by smtp.gentoo.org (Postfix, from userid 559) id BA565335DBD; Mon, 7 Feb 2022 13:11:08 +0000 (UTC) Date: Mon, 7 Feb 2022 08:11:09 -0500 From: Mike Frysinger To: newlib@sourceware.org Subject: Re: [PATCH 5/5] libgloss: merge bfin into top-level Makefile Message-ID: Mail-Followup-To: newlib@sourceware.org References: <20220205054656.11443-1-vapier@gentoo.org> <20220205054656.11443-6-vapier@gentoo.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ml6jwXgcr37s02p9" Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Feb 2022 13:11:13 -0000 --ml6jwXgcr37s02p9 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 +=3D \ > > > + %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 > >=20 > > 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? > >=20 > > 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. >=20 > 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 =3D %D%/crt0.o %C%_bsp_crts =3D %D%/basiccrt.o ...... multilibtool_DATA +=3D $(%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 +=3D %D%/crt0.o to before the "development board stuff" section. -mike --ml6jwXgcr37s02p9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEuQK1JxMl+JKsJRrUQWM7n+g39YEFAmIBGm0ACgkQQWM7n+g3 9YE8+A/+M3CI3vi/iAJY+LQ75yxPWeJuV9vWV61c2yQ5dUdZcWGEJHaz6/4yLFE9 abZ+z1+DhLyRiD+kIMecMrfvQzMlEXriVDqnxpTcozcCGkH6jTxuzZ41A1ll/yNC cJ/73gbbXgLtWeN+hGawIhCczEwp6Z7Vv83aDLAarpRAo8r9XgUi8y72MGN8V1vk +CcGGiBkN0m0Hm3iugznjfNW+tv84sgM/96O2t2BFDY8y5RCp/mmEwcI0sXzss6w XUDrc2vswDjS5NW39BU3nkFJ1LjTGtr9RBpkNyWebRr9pFR5r8AQaJJfMucmkcvn 9KTY9xEdAnW7w2gG/4z1jxRtiXjfhFifRJYqb5hBbFKgwoOGwErUl4S6s7zS+Ptg SWUXxZ4Up3rNNEC+6XYz8/mZlrMpTX4oQYMiHKoxOKyvTNd1CPPdUQ/P+9g0+fII Aa16iSD3WzEK6gYzHlAUQL1WrA5IGXQcn60+FrUJCjyJcq+1EZrMBY4rHrXzJUfT oOImkC0xu40jmKp7mKbe1acAGsbxGQlEKKm/BPrsor6QX3SrZhI3qiD3eKYDANfC /v3+Ncsa8u06QTU5yvC6zhAiWpSw8Q1rAq3Qh2R8Mr41GZxSZYr4XQxnuLXaQdfd vH2fiMiywjtmKluw4pxd3n6/WqNNwiHW/TyK+YCsZfVrAL6Ka6U= =/8b+ -----END PGP SIGNATURE----- --ml6jwXgcr37s02p9--