Re: [Buildroot] [PATCH v3 4/8] arch/arm: add support for FDPIC

From: "Yann E. MORIN" <yann.morin.1998@free.fr>
To: Ben Wolsieffer <ben.wolsieffer@hefring.com>
Cc: Vladimir Murzin <vladimir.murzin@arm.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	buildroot@buildroot.org, Romain Naour <romain.naour@gmail.com>,
	Giulio Benetti <giulio.benetti@benettiengineering.com>,
	Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Subject: Re: [Buildroot] [PATCH v3 4/8] arch/arm: add support for FDPIC
Date: Sun, 16 Apr 2023 18:22:00 +0200	[thread overview]
Message-ID: <20230416162200.GC2819@scaer> (raw)
In-Reply-To: <20220819151734.926106-5-Ben.Wolsieffer@hefring.com>

Ben, All,

Sorry for the overly long delay, but as you can imagine, it is not
something that is trivial to review. Even so, here's an long overdue
review...

On 2022-08-19 11:17 -0400, Ben Wolsieffer spake thusly:
> Linux on ARM supports FDPIC binaries intended for use on no-MMU systems.
> This patch enables support for building a toolchain that produces FDPIC
> binaries.
> 
> The target name for an FDPIC toolchain must be
> arm-<vendor>-uclinuxfdpiceabi, which doesn't follow the standard format
> and requires a special case.
> 
> According to the kernel help for CONFIG_BINFMT_ELF_FDPIC, "It is also
> possible to run FDPIC ELF binaries on MMU linux," so FDPIC support is
> available on all ARM platforms, not just no-MMU.
> 
> Signed-off-by: Ben Wolsieffer <Ben.Wolsieffer@hefring.com>
> ---
>  arch/Config.in      | 1 +
>  package/Makefile.in | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch/Config.in b/arch/Config.in
> index 4cd58041a4..c639738f5f 100644
> --- a/arch/Config.in
> +++ b/arch/Config.in
> @@ -39,6 +39,7 @@ config BR2_arceb
>  
>  config BR2_arm
>  	bool "ARM (little endian)"
> +	select BR2_ARCH_HAS_FDPIC_SUPPORT
>  	# MMU support is set by the subarchitecture file, arch/Config.in.arm
>  	help
>  	  ARM is a 32-bit reduced instruction set computer (RISC)
> diff --git a/package/Makefile.in b/package/Makefile.in
> index ff60f85092..81a7028275 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -37,7 +37,13 @@ $(error BR2_TOOLCHAIN_BUILDROOT_VENDOR cannot be 'unknown'. \
>  endif
>  
>  # Compute GNU_TARGET_NAME
> +# FDPIC on ARM requires a special target name: it has no OS field and must
> +# use the suffix -uclinuxfdpiceabi.
> +ifeq ($(BR2_arm)$(BR2_armeb):$(BR2_BINFMT_FDPIC),y:y)
> +GNU_TARGET_NAME = $(ARCH)-$(TARGET_VENDOR)-uclinuxfdpiceabi

This looks weird: it looks like they coalesced OS and ABI, and dropped
the LIBC part...

> +else
>  GNU_TARGET_NAME = $(ARCH)-$(TARGET_VENDOR)-$(TARGET_OS)-$(LIBC)$(ABI)
> +endif

I am not too fond of this special casing. What about something along the
lines of (existing comments stripped):

    TARGET_NAME = $(ARCH)-$(TARGET_VENDOR)-$(TARGET_OS)$(LIBC)$(ABI)))
    # Note no dash between OS and LIBC  --------------^^

    ifeq ($(BR2_arm)$(BR2_armeb)$(BR2_BINFMT_FDPIC),yy)
    # For ARM FDPIC, there is no separation between OS and ABI
    TARGET_OS = uclinux
    else ifeq ($(BR2_BINFMT_FLAT):$(BR2_RISCV_64),y:)
    TARGET_OS = uclinux-
    else
    TARGET_OS = linux-
    endif

    ifeq ($(BR2_TOOLCHAIN_USES_UCLIBC),y)
    # For ARM FDPIC, there's no LIBC part
    ifneq ($(BR2_arm)$(BR2_armeb)$(BR2_BINFMT_FDPIC),yy)
    LIBC = uclibc
    endif
    else ifeq ($(BR2_TOOLCHAIN_USES_MUSL),y)
    LIBC = musl
    else ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y)
    LIBC = gnu
    else ifeq ($(BR_BUILDING),y)
    $(error No C library enabled, this is not possible.)
    endif

    ifeq ($(BR2_arm)$(BR2_armeb),y)
    ifeq ($(LIBC),uclibc)
    ABI = $(if $(BR2_BINFMT_FDPIC),fdpic,gnu)eabi
    else
    ABI = eabi
    endif

    ifeq ($(BR2_ARM_EABIHF),y)
    # FDPIC is always HF, so don't append the hf suffix (FIXME!)
    ABI := $(ABI)$(if $(BR2_BINFMT_FDPIC),,hf)
    endif
    endif

Note that this is not perfect, as there are a few assumptions hard-coded
in there, but I'm afraid we can't do much much better...

Note that I have no idea about the HF part, so I wrote something for
illustration purposes only...

Regards,
Yann E. MORIN.

>  # FLAT binary format needs uclinux, except RISC-V 64-bits which needs
>  # the regular linux name.
> -- 
> 2.37.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot