Re: [Buildroot] [PATCH v3 1/6] package/binutils-bare-metal: new package

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

Hello Neal, Ibai,

thank you for your persistence in working on this!

The overall patch set appears pretty clean, except for a few remarks as
you can read in this and the other replies. 

I had a small hiccup while trying to apply your patches using 'git
am' from my inbox:

  error: cannot convert from y to UTF-8

This is probably due to this weird header value:

  Content-Type: text/plain; charset="y"

Probably some dirt in your git config. However the mbox file as
downloaded from patchwork did apply without issues. I don't think you
need to resend the series just for this.

On Mon, 4 Sep 2023 11:04:38 +0100
Neal Frager <neal.frager@amd.com> wrote:

> This patch adds a new package for building binutils for a bare-metal toolchain.
> The cpu architecture is defined by a toolchain-bare-metal virtual package.
> While any cpu architecture could be used, the default configuration will be a
> Xilinx microblaze little endian architecture, so that buildroot will be able
> to build the microblaze firmware applications for zynqmp and versal.
> 
> When configured for the Xilinx microblaze architecture, all of the binutils
> patches that are applied to the Xilinx distributed toolchain will be applied
> in order to generate a toolchain that is equivalent to what Xilinx distributes.
> 
> Signed-off-by: Ibai Erkiaga-Elorza <ibai.erkiaga-elorza@amd.com>
> Signed-off-by: Neal Frager <neal.frager@amd.com>

...

> diff --git a/package/binutils-bare-metal/Config.in.host b/package/binutils-bare-metal/Config.in.host
> new file mode 100644
> index 0000000000..036698d418
> --- /dev/null
> +++ b/package/binutils-bare-metal/Config.in.host
> @@ -0,0 +1,19 @@
> +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
> +	bool "host binutils-bare-metal"
> +	help
> +	  binutils-bare-metal is a host utility for a
> +	  bare-metal toolchain

"a host utility seems a bit of an understatement, I'd rather say "Build
to GNU binutils for a bare-metal toolchain" to clarify this is building
no less than the GNU binutils.

> +
> +if BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
> +
> +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_VERSION
> +	string
> +	default "2.39"
> +
> +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_EXTRA_CONFIG_OPTIONS
> +	string "Additional binutils options"
> +	default ""
> +	help
> +	  Any additional binutils options you may want to include

Do we really want this without a valid use case?

The same question could apply to the _VERSION setting, however I feel
it's reasonable to keep it...

This applies to patches 2 and 3 as well.

> diff --git a/package/binutils-bare-metal/binutils-bare-metal.mk b/package/binutils-bare-metal/binutils-bare-metal.mk
> new file mode 100644
> index 0000000000..fd983abc93
> --- /dev/null
> +++ b/package/binutils-bare-metal/binutils-bare-metal.mk
> @@ -0,0 +1,56 @@
> +################################################################################
> +#
> +# binutils-bare-metal
> +#
> +################################################################################
> +
> +HOST_BINUTILS_BARE_METAL_VERSION = $(call qstrip,$(BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_VERSION))
> +ifeq ($(HOST_BINUTILS_BARE_METAL_VERSION),)
> +HOST_BINUTILS_BARE_METAL_VERSION = 2.39
> +endif # BINUTILS_VERSION  
> +
> +HOST_BINUTILS_BARE_METAL_SITE ?= $(BR2_GNU_MIRROR)/binutils
> +HOST_BINUTILS_BARE_METAL_SOURCE ?= binutils-$(HOST_BINUTILS_BARE_METAL_VERSION).tar.xz
> +
> +HOST_BINUTILS_BARE_METAL_LICENSE = GPL-3.0+, libiberty LGPL-2.1+
> +HOST_BINUTILS_BARE_METAL_LICENSE_FILES = COPYING3 COPYING.LIB
> +HOST_BINUTILS_BARE_METAL_CPE_ID_VENDOR = gnu
> +
> +HOST_BINUTILS_BARE_METAL_DEPENDENCIES = host-zlib
> +
> +# if toolchain is for microblazeel-xilinx, apply Xilinx patch set
> +ifeq ($(BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_ARCH),"microblazeel-xilinx")

The toolchain-bare-metal is added in patch 4, thus this series as-is is
not bisectable. I don't see any obvious solutions as
toolchain-bare-metal selects binutils-bare-metal so I guess some maintainer
can provide some hints on how to handle this.

The same issue applies to patches 2 and 3.

> +HOST_BINUTILS_BARE_METAL_EXTRA_DOWNLOADS = https://github.com/Xilinx/meta-xilinx/archive/refs/tags/xlnx-rel-v2023.1.tar.gz

This is not a huge download (500 kB compressed, 5.6 MB  uncompressed),
but we use the binutils patches only, which account for 10% of the whole
archive. I wonder whether there is a way to download only a
subdirectory from github.

And looking at the patches themselves, I wonder how many are actually
needed. At a cursory look, some don't really look like production code.
Are those changes being mainlined?

> +define HOST_BINUTILS_BARE_METAL_EXTRACT_PATCHES
> +  mkdir -p $(@D)/patches
> +  tar -xf $(HOST_BINUTILS_BARE_METAL_DL_DIR)/xlnx-rel-v2023.1.tar.gz --strip-components=5 -C $(@D)/patches meta-xilinx-xlnx-rel-v2023.1/meta-microblaze/recipes-devtools/binutils/binutils

Please split this very long line. e.g.:

  tar -xf $(HOST_BINUTILS_BARE_METAL_DL_DIR)/xlnx-rel-v2023.1.tar.gz \
    --strip-components=5 \
    -C $(@D)/patches
    meta-xilinx-xlnx-rel-v2023.1/meta-microblaze/recipes-devtools/binutils/binutils

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot