tests: Build correct sandbox configuration on 32bit

Currently sandbox configuration defautls to 64bit and there is no
automation for building 32bit sandbox on 32bit hosts.

cpp does not know about target specification, code needs to be compiled
to determine integer width.

Add a test program that prints the integer width, and a make target that
aligns the sandbox configuration with the result.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---

 Makefile              |  6 ++++++
 doc/arch/sandbox.rst  | 16 +++++++++++-----
 test/py/conftest.py   |  1 +
 tools/Makefile        |  2 ++
 tools/bits-per-long.c | 14 ++++++++++++++
 5 files changed, 34 insertions(+), 5 deletions(-)
 create mode 100644 tools/bits-per-long.c

On 10/13/22 22:28, Michal Suchanek wrote:
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> cpp does not know about target specification, code needs to be compiled
> to determine integer width.
>
> Add a test program that prints the integer width, and a make target that
> aligns the sandbox configuration with the result.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>
>   Makefile              |  6 ++++++
>   doc/arch/sandbox.rst  | 16 +++++++++++-----
>   test/py/conftest.py   |  1 +
>   tools/Makefile        |  2 ++
>   tools/bits-per-long.c | 14 ++++++++++++++
>   5 files changed, 34 insertions(+), 5 deletions(-)
>   create mode 100644 tools/bits-per-long.c
>
> diff --git a/Makefile b/Makefile
> index 3866cc62f9..e5463573f3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
>   cross_tools: export CROSS_BUILD_TOOLS=y
>   cross_tools: tools ;
>
> +PHONY += set_host_bits
> +set_host_bits: tools
> +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
> +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
> +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
> +
>   .PHONY : CHANGELOG
>   CHANGELOG:
>   	git log --no-merges U-Boot-1_1_5.. | \
> diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
> index 068d4a3be4..d751205eba 100644
> --- a/doc/arch/sandbox.rst
> +++ b/doc/arch/sandbox.rst
> @@ -33,9 +33,11 @@ machines.
>
>   There are two versions of the sandbox: One using 32-bit-wide integers, and one
>   using 64-bit-wide integers. The 32-bit version can be build and run on either
> -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
> -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
> -integers can only be built on 64-bit hosts.
> +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
> +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
> +integers can only be built on 64-bit hosts. There is no automation for ensuring
> +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
> +config.
>
>   Note that standalone/API support is not available at present.
>
> @@ -51,7 +53,9 @@ Basic Operation
>
>   To run sandbox U-Boot use something like::
>
> -   make sandbox_defconfig all
> +   make sandbox_defconfig
> +   make set_host_bits
> +   make all

Thanks for addressing the problem of sandbox bitness.

We should not make building the sandbox more complicated. You could
integrate building set_host_bits into an existing target like u-boot.cfg:.

Overall an approach with an external program is too complicated.
CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
CONFIG_SANDBOX_BITS_PER_LONG.

We could add

#ifndef __LP64__
#undef SANDBOX_BITS_PER_LONG
#define SANDBOX_BITS_PER_LONG 32
#endif

to the top of arch/sandbox/include/asm/posix_types.h and use

#if defined(CONFIG_HOST_64BIT) && defined(__LP64__)

in drivers/misc/swap_case.c to solve the problem. This demonstrates that
CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.

Eliminating them and only using __LP64__ is the right approach.

@Simon:
We should add sandbox_defconfig built with -m32 to our Gitlab CI testing
after fixing the incompatibilities in the unit tests.

>      ./u-boot
>
>   Note: If you get errors about 'sdl-config: Command not found' you may need to
> @@ -59,7 +63,9 @@ install libsdl2.0-dev or similar to get SDL support. Alternatively you can
>   build sandbox without SDL (i.e. no display/keyboard support) by removing
>   the CONFIG_SANDBOX_SDL line in include/configs/sandbox.h or using::
>
> -   make sandbox_defconfig all NO_SDL=1
> +   make sandbox_defconfig
> +   make set_host_bits
> +   make all NO_SDL=1
>      ./u-boot
>
>   U-Boot will start on your computer, showing a sandbox emulation of the serial
> diff --git a/test/py/conftest.py b/test/py/conftest.py
> index 304e93164a..3d1fd6883a 100644
> --- a/test/py/conftest.py
> +++ b/test/py/conftest.py
> @@ -104,6 +104,7 @@ def run_build(config, source_dir, build_dir, board_type, log):
>               o_opt = ''
>           cmds = (
>               ['make', o_opt, '-s', board_type + '_defconfig'],
> +            ['make', o_opt, '-s', 'set_host_bits'],
>               ['make', o_opt, '-s', '-j{}'.format(os.cpu_count())],
>           )
>           name = 'make'
> diff --git a/tools/Makefile b/tools/Makefile
> index 34a1aa7a8b..d6b585953d 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -68,6 +68,8 @@ HOSTCFLAGS_img2srec.o := -pedantic
>   hostprogs-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes
>   HOSTCFLAGS_xway-swap-bytes.o := -pedantic
>
> +hostprogs-y += bits-per-long
> +
>   hostprogs-y += mkenvimage
>   mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
>
> diff --git a/tools/bits-per-long.c b/tools/bits-per-long.c
> new file mode 100644
> index 0000000000..7630e1623f
> --- /dev/null
> +++ b/tools/bits-per-long.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +
> +int main(int argc, char **argv)
> +{
> +	unsigned long testvar = ~0UL;
> +	unsigned int i;
> +
> +	for (i = 0; testvar; i++, testvar >>= 1)
> +		;
> +
> +	return printf("%u\n", i);

return printf("%zd\n", 8 * sizeof(long));

> +}
> +

Please avoid empty lines at the end files.

Best regards

Heinrich

On Fri, Oct 14, 2022 at 05:05:26AM +0200, Heinrich Schuchardt wrote:
> On 10/13/22 22:28, Michal Suchanek wrote:
> > Currently sandbox configuration defautls to 64bit and there is no
> > automation for building 32bit sandbox on 32bit hosts.
> > 
> > cpp does not know about target specification, code needs to be compiled
> > to determine integer width.
> > 
> > Add a test program that prints the integer width, and a make target that
> > aligns the sandbox configuration with the result.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > 
> >   Makefile              |  6 ++++++
> >   doc/arch/sandbox.rst  | 16 +++++++++++-----
> >   test/py/conftest.py   |  1 +
> >   tools/Makefile        |  2 ++
> >   tools/bits-per-long.c | 14 ++++++++++++++
> >   5 files changed, 34 insertions(+), 5 deletions(-)
> >   create mode 100644 tools/bits-per-long.c
> > 
> > diff --git a/Makefile b/Makefile
> > index 3866cc62f9..e5463573f3 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
> >   cross_tools: export CROSS_BUILD_TOOLS=y
> >   cross_tools: tools ;
> > 
> > +PHONY += set_host_bits
> > +set_host_bits: tools
> > +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
> > +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
> > +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
> > +
> >   .PHONY : CHANGELOG
> >   CHANGELOG:
> >   	git log --no-merges U-Boot-1_1_5.. | \
> > diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
> > index 068d4a3be4..d751205eba 100644
> > --- a/doc/arch/sandbox.rst
> > +++ b/doc/arch/sandbox.rst
> > @@ -33,9 +33,11 @@ machines.
> > 
> >   There are two versions of the sandbox: One using 32-bit-wide integers, and one
> >   using 64-bit-wide integers. The 32-bit version can be build and run on either
> > -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
> > -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
> > -integers can only be built on 64-bit hosts.
> > +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
> > +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
> > +integers can only be built on 64-bit hosts. There is no automation for ensuring
> > +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
> > +config.
> > 
> >   Note that standalone/API support is not available at present.
> > 
> > @@ -51,7 +53,9 @@ Basic Operation
> > 
> >   To run sandbox U-Boot use something like::
> > 
> > -   make sandbox_defconfig all
> > +   make sandbox_defconfig
> > +   make set_host_bits
> > +   make all
> 
> Thanks for addressing the problem of sandbox bitness.
> 
> We should not make building the sandbox more complicated. You could
> integrate building set_host_bits into an existing target like u-boot.cfg:.
> 
> Overall an approach with an external program is too complicated.
> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
> CONFIG_SANDBOX_BITS_PER_LONG.
And for making SANDBOX64 depend on 64bit build.
> 
> We could add
> 
> #ifndef __LP64__
> #undef SANDBOX_BITS_PER_LONG
> #define SANDBOX_BITS_PER_LONG 32
> #endif

If we are willing to depend on this define which is clearly named as
compiler-internal we could do similar to cc-option to run something like
$(CC) -dM -E - < /dev/null | grep -q _LP64

> 
> to the top of arch/sandbox/include/asm/posix_types.h and use
> 
> #if defined(CONFIG_HOST_64BIT) && defined(__LP64__)
> 
> in drivers/misc/swap_case.c to solve the problem. This demonstrates that
> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.

Not really.

Thanks

Michal

Hi Michal,

On Thu, 13 Oct 2022 at 14:29, Michal Suchanek <msuchanek@suse.de> wrote:
>
> Currently sandbox configuration defautls to 64bit and there is no
> automation for building 32bit sandbox on 32bit hosts.
>
> cpp does not know about target specification, code needs to be compiled
> to determine integer width.
>
> Add a test program that prints the integer width, and a make target that
> aligns the sandbox configuration with the result.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>
>  Makefile              |  6 ++++++
>  doc/arch/sandbox.rst  | 16 +++++++++++-----
>  test/py/conftest.py   |  1 +
>  tools/Makefile        |  2 ++
>  tools/bits-per-long.c | 14 ++++++++++++++
>  5 files changed, 34 insertions(+), 5 deletions(-)
>  create mode 100644 tools/bits-per-long.c

This needs to be automatic, so that it builds the 32-bit version on
32-bit hosts, 64-bit version on 64-bit hosts.

See here for my attempt. I suspect it just needs your bits_per_long
thing brought in, but in any case I hope it gives you inspiration.

https://patchwork.ozlabs.org/project/uboot/patch/20220123195514.3152022-4-sjg@chromium.org/

Basically we should be able to build sandbox on any platform and it
should just work, without manual configuration.

Regards,
Simon

On 10/14/22 10:43, Michal Suchánek wrote:
> On Fri, Oct 14, 2022 at 05:05:26AM +0200, Heinrich Schuchardt wrote:
>> On 10/13/22 22:28, Michal Suchanek wrote:
>>> Currently sandbox configuration defautls to 64bit and there is no
>>> automation for building 32bit sandbox on 32bit hosts.
>>>
>>> cpp does not know about target specification, code needs to be compiled
>>> to determine integer width.
>>>
>>> Add a test program that prints the integer width, and a make target that
>>> aligns the sandbox configuration with the result.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>>
>>>    Makefile              |  6 ++++++
>>>    doc/arch/sandbox.rst  | 16 +++++++++++-----
>>>    test/py/conftest.py   |  1 +
>>>    tools/Makefile        |  2 ++
>>>    tools/bits-per-long.c | 14 ++++++++++++++
>>>    5 files changed, 34 insertions(+), 5 deletions(-)
>>>    create mode 100644 tools/bits-per-long.c
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 3866cc62f9..e5463573f3 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -2166,6 +2166,12 @@ tools-all: envtools tools ;
>>>    cross_tools: export CROSS_BUILD_TOOLS=y
>>>    cross_tools: tools ;
>>>
>>> +PHONY += set_host_bits
>>> +set_host_bits: tools
>>> +	$(Q)sed -i -e /CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT/d $(KCONFIG_CONFIG)
>>> +	$(Q)sed -i -E -e "s/CONFIG_HOST_(..)BIT=y/# CONFIG_HOST_\1BIT is not set/" $(KCONFIG_CONFIG)
>>> +	$(Q)echo CONFIG_HOST_$$($(objtree)/tools/bits-per-long)BIT=y >> $(KCONFIG_CONFIG)
>>> +
>>>    .PHONY : CHANGELOG
>>>    CHANGELOG:
>>>    	git log --no-merges U-Boot-1_1_5.. | \
>>> diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
>>> index 068d4a3be4..d751205eba 100644
>>> --- a/doc/arch/sandbox.rst
>>> +++ b/doc/arch/sandbox.rst
>>> @@ -33,9 +33,11 @@ machines.
>>>
>>>    There are two versions of the sandbox: One using 32-bit-wide integers, and one
>>>    using 64-bit-wide integers. The 32-bit version can be build and run on either
>>> -32 or 64-bit hosts by either selecting or deselecting CONFIG_SANDBOX_32BIT; by
>>> -default, the sandbox it built for a 32-bit host. The sandbox using 64-bit-wide
>>> -integers can only be built on 64-bit hosts.
>>> +32 or 64-bit hosts by either selecting or deselecting HOST_64BIT; by
>>> +default, the sandbox it built for a 64-bit host. The sandbox using 64-bit-wide
>>> +integers can only be built on 64-bit hosts. There is no automation for ensuring
>>> +32bit build on 32bit hosts - use ``make set_host_bits`` to adjust the sandbox
>>> +config.
>>>
>>>    Note that standalone/API support is not available at present.
>>>
>>> @@ -51,7 +53,9 @@ Basic Operation
>>>
>>>    To run sandbox U-Boot use something like::
>>>
>>> -   make sandbox_defconfig all
>>> +   make sandbox_defconfig
>>> +   make set_host_bits
>>> +   make all
>>
>> Thanks for addressing the problem of sandbox bitness.
>>
>> We should not make building the sandbox more complicated. You could
>> integrate building set_host_bits into an existing target like u-boot.cfg:.
>>
>> Overall an approach with an external program is too complicated.
>> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are used to define
>> CONFIG_SANDBOX_BITS_PER_LONG.
> And for making SANDBOX64 depend on 64bit build.

Sandbox64 is about the width of phys_addr_t and not about the bitness of
the build. sandbox64_defconfig builds fine on ilp32 and many aspects
work there.

We should test that 64bit phys_addr_t works on ilp32 systems. Sandbox64
would we the right way to do this.

Best regards

Heinrich

>>
>> We could add
>>
>> #ifndef __LP64__
>> #undef SANDBOX_BITS_PER_LONG
>> #define SANDBOX_BITS_PER_LONG 32
>> #endif
>
> If we are willing to depend on this define which is clearly named as
> compiler-internal we could do similar to cc-option to run something like
> $(CC) -dM -E - < /dev/null | grep -q _LP64
>
>>
>> to the top of arch/sandbox/include/asm/posix_types.h and use
>>
>> #if defined(CONFIG_HOST_64BIT) && defined(__LP64__)
>>
>> in drivers/misc/swap_case.c to solve the problem. This demonstrates that
>> CONFIG_HOST_32BIT and CONFIG_HOST_64BIT are superfluous symbols.
>
> Not really.
>
> Thanks
>
> Michal

On 10/14/22 17:56, Simon Glass wrote:
> Hi Michal,
>
> On Thu, 13 Oct 2022 at 14:29, Michal Suchanek <msuchanek@suse.de> wrote:
>>
>> Currently sandbox configuration defautls to 64bit and there is no
>> automation for building 32bit sandbox on 32bit hosts.
>>
>> cpp does not know about target specification, code needs to be compiled
>> to determine integer width.
>>
>> Add a test program that prints the integer width, and a make target that
>> aligns the sandbox configuration with the result.
>>
>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> ---
>>
>>   Makefile              |  6 ++++++
>>   doc/arch/sandbox.rst  | 16 +++++++++++-----
>>   test/py/conftest.py   |  1 +
>>   tools/Makefile        |  2 ++
>>   tools/bits-per-long.c | 14 ++++++++++++++
>>   5 files changed, 34 insertions(+), 5 deletions(-)
>>   create mode 100644 tools/bits-per-long.c
>
> This needs to be automatic, so that it builds the 32-bit version on
> 32-bit hosts, 64-bit version on 64-bit hosts.

We should be able to test the following:

* 64bit phys_addr_t on ilp32.
* 32bit phys_addr_t on ilp32.
* 64bit phys_addr_t on lp64.

Gitlab CI currently tests:

* 64bit phys_addr_t on lp64.
* 32bit phys_addr_t on lp64.

Best regards

Heinrich

>
> See here for my attempt. I suspect it just needs your bits_per_long
> thing brought in, but in any case I hope it gives you inspiration.
>
> https://patchwork.ozlabs.org/project/uboot/patch/20220123195514.3152022-4-sjg@chromium.org/
>
> Basically we should be able to build sandbox on any platform and it
> should just work, without manual configuration.
>
> Regards,
> Simon