Messages in this thread | | | Date | Tue, 7 Apr 2020 19:10:08 +0100 (BST) | From | "Maciej W. Rozycki" <> | Subject | Re: [PATCH v2] MIPS: Truncate load-y into 32bit for 32bit kernel |
| |
On Tue, 7 Apr 2020, Nick Desaulniers wrote:
> V2 is way too clever, V1 was much more readable.
I think V2 is a step in the right direction except it still has some issues, and also I'd simplify it as there's surely too much processing there.
OTOH V1 is going to be a maintenance nightmare, as you need to handle all platforms individually whether they want different 32-bit and 64-bit load addresses or not.
> > diff --git a/arch/mips/Makefile b/arch/mips/Makefile > > index e1c44aed8156..f8fd3c39fb55 100644 > > --- a/arch/mips/Makefile > > +++ b/arch/mips/Makefile > > @@ -286,6 +286,9 @@ ifdef CONFIG_64BIT > > $(error CONFIG_CPU_DADDI_WORKAROUNDS unsupported without -msym32) > > endif > > endif > > +else > > + # Truncate address into 32-bit > > + load-y := 0x$(shell echo "$(load-y)" | rev | head -c 8 | rev)
You cannot just truncate `load-y' in place like this as it will break logic with `expr' used elsewhere in this Makefile (your original change would too) that does a string comparison on this variable. So you need to define another variable for the sole linker's use, like `load-ld'.
Then I think there's no need to invoke multiple programs, especially ones we don't currently rely on (`rev'). How about:
load-ld = $(shell echo "$(load-y)" | sed 's/.\{8\}\(.\{8\}\)$/\1/')
Also this really needs to be placed elsewhere, as it has nothing to do with KBUILD_SYM32 it has been attached to with this change, and explain why it is done rather than what (it's obvious from the command it's meant to truncate the address).
So use something along the lines of:
# When linking a 32-bit executable the LLVM linker cannot cope with a # 32-bit load address that has been sign-extended to 64 bits. Simply # remove the upper 32 bits then, as it is safe to do so with other # linkers. ifdef CONFIG_64BIT load-ld = $(load-y) else load-ld = $(shell echo "$(load-y)" | sed 's/.\{8\}\(.\{8\}\)$/\1/') endif
just above the use place, and then adjust the place to refer `load-ld' rather than `load-y'.
Put the justification for this change (feel free to reuse observations I made here), like why a new variable, in the change description.
Maciej
| |