[0/3] Use POSIX "$((..))" instead of bashism "$[...]"
diff mbox

Message ID 1371129052-31506-1-git-send-email-geert@linux-m68k.org
State New, archived
Headers show

Commit Message

Geert Uytterhoeven June 13, 2013, 1:10 p.m. UTC
This patch series replaces the bash-specific "$[...]" by POSIX "$((..))"
for arithmetic expansion, and gets rid of some dependencies on bash.

  [1/3] h8300/boot: Use POSIX "$((..))" instead of bashism "$[...]"
  [2/3] ARM: shmobile: Use POSIX "$((..))" instead of bashism "$[...]"
  [3/3] sh/boot: Use POSIX "$((..))" instead of bashism "$[...]"

The first one is a build fix on systems where /bin/sh is not bash, as it
uses the bashism without forcing bash.

This series was tested using the patch at the bottom, and running

    make ARCH=h8300 clean
    make ARCH=arm clean
    make ARCH=sh clean

with an optional "SHELL=/bin/..." appended, to test difference shells,
on Ubuntu 10.04.4 LTS.

It works fine with bash (4.1-2ubuntu3) and dash (0.5.5.1-3ubuntu2).

With mksh (39.3-1ubuntu3), 32-bit integers seem to be sign-extended to
64-bit integers, e.g.

arch/sh/boot/Makefile: KERNEL_MEMORY = 0x0c000000
arch/sh/boot/Makefile: KERNEL_LOAD = 0xffffffff8c001000
arch/sh/boot/Makefile: KERNEL_ENTRY = 0xffffffff8c002000
arch/sh/boot/compressed/Makefile: IMAGE_OFFSET = 0xffffffff8c800000

Should these be truncated to 32-bit explicitly, or is this a bug in mksh?

Thanks for your comments!


Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds
In-Reply-To: 

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Thorsten Glaser June 13, 2013, 7:16 p.m. UTC | #1
Geert Uytterhoeven dixit:

>Should these be truncated to 32-bit explicitly, or is this a bug in mksh?

mksh in “mksh mode” operates specifically on 32-bit integer data
types with defined wraparound and other guarantees beyond what
POSIX does. There is an “lksh” binary in the mksh binary package
in Debian and derivates now, which is the “legacy mode” that uses
the host system’s “long” data type, as POSIX demands, and is now
(mksh_46-2) used for /bin/sh.

However, on mixed 32/64-bit systems, /bin/sh can have either bit
size. Additionally, it is not possible, in POSIX, without invoking
ISO C “Undefined Behaviour”, to find out the bitsize of the shell
arithmetics.

Furthermore, Linux can be cross-compiled, so when building kernels
for 64-bit platforms on 32-bit systems, the arithmetics used MUST
NOT overflow beyond a signed 32-bit “long”.

>  [1/3] h8300/boot: Use POSIX "$((..))" instead of bashism "$[...]"
>  [2/3] ARM: shmobile: Use POSIX "$((..))" instead of bashism "$[...]"
>  [3/3] sh/boot: Use POSIX "$((..))" instead of bashism "$[...]"

Independent of the above, I’ve verified all three and can state
that they
? are no regression relative to existing behaviour
? do not invoke any features not in POSIX $((…)) arithmetics
? do not invoke any features not in mksh $((…)) arithmetics

This means you can add my
Signed-off: Thorsten Glaser <tg@mirbsd.org>

However, I urge you to check whether any of these arithmetics
can go beyond 32 bit. If they have even the slightest chance
to do that, you MUST replace them by something different. One
method could be to use bc(1):

$(shell printf 'obase=16\nibase=16\n%s+%s\n' $(FOO) $(BAR) | bc)

Another method could be to operate on the upper half and the
lower half of the 64-bit quantities separately, assuming that
calculations do not overflow (in POSIX sh, overflow is, like
in ISO C, Undefined Behaviour; a C compiler is permitted to
compile the source code in the shell that invokes it to run
“rm -rf ~ /” instead) or carry over (e.g. if it’s known that
the first half is always 0x7FFFFFFF or the last half always
00000000 you can just add that as “string”).

bye,
//mirabilos

Patch
diff mbox

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 1ba358b..5029108 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -319,3 +319,5 @@  define archhelp
   echo  '                  (distribution) /sbin/$(INSTALLKERNEL) or'
   echo  '                  install to $$(INSTALL_PATH) and run lilo'
 endef
+
+include arch/arm/mach-shmobile/Makefile.boot
diff --git a/arch/arm/mach-shmobile/Makefile.boot b/arch/arm/mach-shmobile/Makefile.boot
index 498efd9..95ebc7c 100644
--- a/arch/arm/mach-shmobile/Makefile.boot
+++ b/arch/arm/mach-shmobile/Makefile.boot
@@ -7,3 +7,5 @@  __ZRELADDR	:= $(shell /bin/bash -c 'printf "0x%08x" \
 #
 #params_phys-y (Instead: Pass atags pointer in r2)
 #initrd_phys-y (Instead: Use compiled-in initramfs)
+
+XXX := $(shell echo arch/arm/mach-shmobile/Makefile.boot: __ZRELADDR = $(__ZRELADDR) >> /tmp/loggy)
diff --git a/arch/h8300/boot/compressed/Makefile b/arch/h8300/boot/compressed/Makefile
index 6745cb1..94faad1 100644
--- a/arch/h8300/boot/compressed/Makefile
+++ b/arch/h8300/boot/compressed/Makefile
@@ -35,3 +35,5 @@  OBJCOPYFLAGS := -O binary
 
 $(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.gz FORCE
 	$(call if_changed,ld)
+
+XXX := $(shell echo arch/h8300/boot/compressed/Makefile: IMAGE_OFFSET = $(IMAGE_OFFSET) >> /tmp/loggy)
diff --git a/arch/sh/boot/Makefile b/arch/sh/boot/Makefile
index 58592df..4ab2ba5 100644
--- a/arch/sh/boot/Makefile
+++ b/arch/sh/boot/Makefile
@@ -113,3 +113,7 @@  $(obj)/uImage: $(obj)/uImage.$(suffix-y)
 export CONFIG_PAGE_OFFSET CONFIG_MEMORY_START CONFIG_BOOT_LINK_OFFSET \
        CONFIG_PHYSICAL_START CONFIG_ZERO_PAGE_OFFSET CONFIG_ENTRY_OFFSET \
        KERNEL_MEMORY suffix-y
+
+XXX := $(shell echo arch/sh/boot/Makefile: KERNEL_MEMORY = $(KERNEL_MEMORY) >> /tmp/loggy)
+XXX := $(shell echo arch/sh/boot/Makefile: KERNEL_LOAD = $(KERNEL_LOAD) >> /tmp/loggy)
+XXX := $(shell echo arch/sh/boot/Makefile: KERNEL_ENTRY = $(KERNEL_ENTRY) >> /tmp/loggy)
diff --git a/arch/sh/boot/compressed/Makefile b/arch/sh/boot/compressed/Makefile
index 23bc849..43d1b9a 100644
--- a/arch/sh/boot/compressed/Makefile
+++ b/arch/sh/boot/compressed/Makefile
@@ -79,3 +79,5 @@  LDFLAGS_piggy.o := -r --format binary --oformat $(ld-bfd) -T
 
 $(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix-y) FORCE
 	$(call if_changed,ld)
+
+XXX := $(shell echo arch/sh/boot/compressed/Makefile: IMAGE_OFFSET = $(IMAGE_OFFSET) >> /tmp/loggy)