| Message ID | 1460723596-13261-8-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds > new file mode 100644 > index 0000000..47db9c4 > --- /dev/null > +++ b/xen/arch/x86/boot/build32.lds > @@ -0,0 +1,49 @@ > +/* > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + * Daniel Kiper <daniel.kiper@oracle.com> Drop the 'Daniel Kiper' part. Just Copyright (c) 2016 Oracle.
>>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > --- /dev/null > +++ b/xen/arch/x86/boot/build32.lds > @@ -0,0 +1,49 @@ > +/* > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + * Daniel Kiper <daniel.kiper@oracle.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +ENTRY(_start) > + > +SECTIONS > +{ > + /* Merge code and data into one section. */ > + .text : { > + *(.text) > + *(.text.*) > + *(.rodata) This (and the respective %.lnk rule change below) is not in line with the patch description. It's further suspicious that you only handle .rodata but not also .rodata.* here. > + } > + > + /DISCARD/ : { > + /* > + * .got.plt section is used only by dynamic linker > + * and our output is not supposed to be loaded by > + * dynamic linker. Additionally, it just contains > + * .PLT0 which is referenced from nowhere. So, we > + * can safely drop .got.plt here. > + * > + * Ha! This should be really discarded here. However, > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ > + * symbol too and it is used as a reference for relative > + * addressing (and only for that thing). Hence, ld > + * complains if we remove that section because it > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt > + * section during conversion to plain binary format. > + * Please check build32.mk for more details. > + */ > + /* *(.got.plt) */ > + } I'm afraid this needs more investigation: Afaik there should be no reason for the linker to create an otherwise empty .got.plt in the first place. And discarding it without being sure it is empty is not that good an idea anyway. Jan
On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote: > >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > > --- /dev/null > > +++ b/xen/arch/x86/boot/build32.lds > > @@ -0,0 +1,49 @@ > > +/* > > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > > + * Daniel Kiper <daniel.kiper@oracle.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License along > > + * with this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +ENTRY(_start) > > + > > +SECTIONS > > +{ > > + /* Merge code and data into one section. */ > > + .text : { > > + *(.text) > > + *(.text.*) > > + *(.rodata) > > This (and the respective %.lnk rule change below) is not in line with > the patch description. It's further suspicious that you only handle I am not sure what exactly do you mean by that. > .rodata but not also .rodata.* here. I did this deliberately. I just want to take only these sections which I know that contain required code and data. Nothing more. If in the future we find out that .rodata.* (or anything else) is needed then we can add it later. > > + } > > + > > + /DISCARD/ : { > > + /* > > + * .got.plt section is used only by dynamic linker > > + * and our output is not supposed to be loaded by > > + * dynamic linker. Additionally, it just contains > > + * .PLT0 which is referenced from nowhere. So, we > > + * can safely drop .got.plt here. > > + * > > + * Ha! This should be really discarded here. However, > > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ > > + * symbol too and it is used as a reference for relative > > + * addressing (and only for that thing). Hence, ld > > + * complains if we remove that section because it > > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt > > + * section during conversion to plain binary format. > > + * Please check build32.mk for more details. > > + */ > > + /* *(.got.plt) */ > > + } > > I'm afraid this needs more investigation: Afaik there should be no I am not sure what else we should look for. > reason for the linker to create an otherwise empty .got.plt in the As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used as a reference for relative addressing. > first place. And discarding it without being sure it is empty is not > that good an idea anyway. Good point! Potentially we can check is it empty, excluding _GLOBAL_OFFSET_TABLE_ symbol, in build32.mk. Anyway, there is a chance that these tricks will not be needed at all. I will try to link 32-bit code directly into Xen binary using objcopy, etc. which we discussed during hackathon. Daniel
>>> On 24.05.16 at 14:28, <daniel.kiper@oracle.com> wrote: > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote: >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> > --- /dev/null >> > +++ b/xen/arch/x86/boot/build32.lds >> > @@ -0,0 +1,49 @@ >> > +/* >> > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. >> > + * Daniel Kiper <daniel.kiper@oracle.com> >> > + * >> > + * This program is free software; you can redistribute it and/or modify >> > + * it under the terms of the GNU General Public License as published by >> > + * the Free Software Foundation; either version 2 of the License, or >> > + * (at your option) any later version. >> > + * >> > + * This program is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > + * GNU General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU General Public License along >> > + * with this program. If not, see <http://www.gnu.org/licenses/>. >> > + */ >> > + >> > +ENTRY(_start) >> > + >> > +SECTIONS >> > +{ >> > + /* Merge code and data into one section. */ >> > + .text : { >> > + *(.text) >> > + *(.text.*) >> > + *(.rodata) >> >> This (and the respective %.lnk rule change below) is not in line with >> the patch description. It's further suspicious that you only handle > > I am not sure what exactly do you mean by that. Quoting your commit message: "... merge all text and data sections into one .text section." Contrast this to the limited set of sections above. >> .rodata but not also .rodata.* here. > > I did this deliberately. I just want to take only these sections which I > know that > contain required code and data. Nothing more. If in the future we find out > that > .rodata.* (or anything else) is needed then we can add it later. > >> > + } >> > + >> > + /DISCARD/ : { >> > + /* >> > + * .got.plt section is used only by dynamic linker >> > + * and our output is not supposed to be loaded by >> > + * dynamic linker. Additionally, it just contains >> > + * .PLT0 which is referenced from nowhere. So, we >> > + * can safely drop .got.plt here. >> > + * >> > + * Ha! This should be really discarded here. However, >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ >> > + * symbol too and it is used as a reference for relative >> > + * addressing (and only for that thing). Hence, ld >> > + * complains if we remove that section because it >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt >> > + * section during conversion to plain binary format. >> > + * Please check build32.mk for more details. >> > + */ >> > + /* *(.got.plt) */ >> > + } >> >> I'm afraid this needs more investigation: Afaik there should be no > > I am not sure what else we should look for. The reason why such an empty .got.plt gets created in the first place. If e.g. that turns out to be a bug in (some versions of) binutils, then that bug should be named here as the reason. >> reason for the linker to create an otherwise empty .got.plt in the > > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used > as a reference for relative addressing. But we don't use any such, so without being needed I don't think the symbol needs to be created. >> first place. And discarding it without being sure it is empty is not >> that good an idea anyway. > > Good point! Potentially we can check is it empty, excluding > _GLOBAL_OFFSET_TABLE_ symbol, in build32.mk. Well, your comment above says it have .PLT0, which means it's not exactly empty. Jan
On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote: > >>> On 24.05.16 at 14:28, <daniel.kiper@oracle.com> wrote: > > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote: > >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> > --- /dev/null > >> > +++ b/xen/arch/x86/boot/build32.lds > >> > @@ -0,0 +1,49 @@ > >> > +/* > >> > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > >> > + * Daniel Kiper <daniel.kiper@oracle.com> > >> > + * > >> > + * This program is free software; you can redistribute it and/or modify > >> > + * it under the terms of the GNU General Public License as published by > >> > + * the Free Software Foundation; either version 2 of the License, or > >> > + * (at your option) any later version. > >> > + * > >> > + * This program is distributed in the hope that it will be useful, > >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> > + * GNU General Public License for more details. > >> > + * > >> > + * You should have received a copy of the GNU General Public License along > >> > + * with this program. If not, see <http://www.gnu.org/licenses/>. > >> > + */ > >> > + > >> > +ENTRY(_start) > >> > + > >> > +SECTIONS > >> > +{ > >> > + /* Merge code and data into one section. */ > >> > + .text : { > >> > + *(.text) > >> > + *(.text.*) > >> > + *(.rodata) > >> > >> This (and the respective %.lnk rule change below) is not in line with > >> the patch description. It's further suspicious that you only handle > > > > I am not sure what exactly do you mean by that. > > Quoting your commit message: "... merge all text and data sections > into one .text section." Contrast this to the limited set of sections > above. > > >> .rodata but not also .rodata.* here. > > > > I did this deliberately. I just want to take only these sections which I > > know that > > contain required code and data. Nothing more. If in the future we find out > > that > > .rodata.* (or anything else) is needed then we can add it later. > > > >> > + } > >> > + > >> > + /DISCARD/ : { > >> > + /* > >> > + * .got.plt section is used only by dynamic linker > >> > + * and our output is not supposed to be loaded by > >> > + * dynamic linker. Additionally, it just contains > >> > + * .PLT0 which is referenced from nowhere. So, we > >> > + * can safely drop .got.plt here. > >> > + * > >> > + * Ha! This should be really discarded here. However, > >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ > >> > + * symbol too and it is used as a reference for relative > >> > + * addressing (and only for that thing). Hence, ld > >> > + * complains if we remove that section because it > >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt > >> > + * section during conversion to plain binary format. > >> > + * Please check build32.mk for more details. > >> > + */ > >> > + /* *(.got.plt) */ > >> > + } > >> > >> I'm afraid this needs more investigation: Afaik there should be no > > > > I am not sure what else we should look for. > > The reason why such an empty .got.plt gets created in the first place. > If e.g. that turns out to be a bug in (some versions of) binutils, then > that bug should be named here as the reason. If PIC/PIE code is build then .got.plt exists in executable even if it is not linked with dynamic libraries. Then it is just placeholder for _GLOBAL_OFFSET_TABLE_ symbol and .PLT0. .PLT0 is filled by dynamic linker and our code is not supposed to be loaded by dynamic linker. So, from our point of view .PLT0 is unused. > >> reason for the linker to create an otherwise empty .got.plt in the > > > > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used > > as a reference for relative addressing. > > But we don't use any such, so without being needed I don't think > the symbol needs to be created. R_386_GOTPC and R_386_GOTOFF relocations use address of _GLOBAL_OFFSET_TABLE_ as a reference. So, it is needed during linking phase. However, later it is not needed. Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely be dropped. > >> first place. And discarding it without being sure it is empty is not > >> that good an idea anyway. > > > > Good point! Potentially we can check is it empty, excluding > > _GLOBAL_OFFSET_TABLE_ symbol, in build32.mk. > > Well, your comment above says it have .PLT0, which means it's not > exactly empty. Ditto. And right know I do not think that we need any safety checks here. Daniel
>>> On 17.06.16 at 11:06, <daniel.kiper@oracle.com> wrote: > On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote: >> >>> On 24.05.16 at 14:28, <daniel.kiper@oracle.com> wrote: >> > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote: >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: >> >> > + /DISCARD/ : { >> >> > + /* >> >> > + * .got.plt section is used only by dynamic linker >> >> > + * and our output is not supposed to be loaded by >> >> > + * dynamic linker. Additionally, it just contains >> >> > + * .PLT0 which is referenced from nowhere. So, we >> >> > + * can safely drop .got.plt here. >> >> > + * >> >> > + * Ha! This should be really discarded here. However, >> >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ >> >> > + * symbol too and it is used as a reference for relative >> >> > + * addressing (and only for that thing). Hence, ld >> >> > + * complains if we remove that section because it >> >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt >> >> > + * section during conversion to plain binary format. >> >> > + * Please check build32.mk for more details. >> >> > + */ >> >> > + /* *(.got.plt) */ >> >> > + } >> >> >> >> I'm afraid this needs more investigation: Afaik there should be no >> > >> > I am not sure what else we should look for. >> >> The reason why such an empty .got.plt gets created in the first place. >> If e.g. that turns out to be a bug in (some versions of) binutils, then >> that bug should be named here as the reason. > > If PIC/PIE code is build then .got.plt exists in executable even if it > is not linked with dynamic libraries. Well - then just don't force -fPIC or -fPIE for the compilation of this code? >> >> reason for the linker to create an otherwise empty .got.plt in the >> > >> > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used >> > as a reference for relative addressing. >> >> But we don't use any such, so without being needed I don't think >> the symbol needs to be created. > > R_386_GOTPC and R_386_GOTOFF relocations use address of > _GLOBAL_OFFSET_TABLE_ > as a reference. So, it is needed during linking phase. However, later it is > not needed. Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely > be dropped. These two relocation types should not appear for non-PIC/PIE code. Jan
On Fri, Jun 17, 2016 at 04:04:20AM -0600, Jan Beulich wrote: > >>> On 17.06.16 at 11:06, <daniel.kiper@oracle.com> wrote: > > On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote: > >> >>> On 24.05.16 at 14:28, <daniel.kiper@oracle.com> wrote: > >> > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote: > >> >> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote: > >> >> > + /DISCARD/ : { > >> >> > + /* > >> >> > + * .got.plt section is used only by dynamic linker > >> >> > + * and our output is not supposed to be loaded by > >> >> > + * dynamic linker. Additionally, it just contains > >> >> > + * .PLT0 which is referenced from nowhere. So, we > >> >> > + * can safely drop .got.plt here. > >> >> > + * > >> >> > + * Ha! This should be really discarded here. However, > >> >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ > >> >> > + * symbol too and it is used as a reference for relative > >> >> > + * addressing (and only for that thing). Hence, ld > >> >> > + * complains if we remove that section because it > >> >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt > >> >> > + * section during conversion to plain binary format. > >> >> > + * Please check build32.mk for more details. > >> >> > + */ > >> >> > + /* *(.got.plt) */ > >> >> > + } > >> >> > >> >> I'm afraid this needs more investigation: Afaik there should be no > >> > > >> > I am not sure what else we should look for. > >> > >> The reason why such an empty .got.plt gets created in the first place. > >> If e.g. that turns out to be a bug in (some versions of) binutils, then > >> that bug should be named here as the reason. > > > > If PIC/PIE code is build then .got.plt exists in executable even if it > > is not linked with dynamic libraries. > > Well - then just don't force -fPIC or -fPIE for the compilation of this > code? No way. Then final code is not relocatable. And this code must be relocatable. > >> >> reason for the linker to create an otherwise empty .got.plt in the > >> > > >> > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used > >> > as a reference for relative addressing. > >> > >> But we don't use any such, so without being needed I don't think > >> the symbol needs to be created. > > > > R_386_GOTPC and R_386_GOTOFF relocations use address of > > _GLOBAL_OFFSET_TABLE_ > > as a reference. So, it is needed during linking phase. However, later it is > > not needed. Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely > > be dropped. > > These two relocation types should not appear for non-PIC/PIE code. Sure but as above. Daniel
diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds new file mode 100644 index 0000000..47db9c4 --- /dev/null +++ b/xen/arch/x86/boot/build32.lds @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * Daniel Kiper <daniel.kiper@oracle.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +ENTRY(_start) + +SECTIONS +{ + /* Merge code and data into one section. */ + .text : { + *(.text) + *(.text.*) + *(.rodata) + } + + /DISCARD/ : { + /* + * .got.plt section is used only by dynamic linker + * and our output is not supposed to be loaded by + * dynamic linker. Additionally, it just contains + * .PLT0 which is referenced from nowhere. So, we + * can safely drop .got.plt here. + * + * Ha! This should be really discarded here. However, + * .got.plt section contains _GLOBAL_OFFSET_TABLE_ + * symbol too and it is used as a reference for relative + * addressing (and only for that thing). Hence, ld + * complains if we remove that section because it + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt + * section during conversion to plain binary format. + * Please check build32.mk for more details. + */ + /* *(.got.plt) */ + } +} diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk index 4a7d388..eb02b4b 100644 --- a/xen/arch/x86/boot/build32.mk +++ b/xen/arch/x86/boot/build32.mk @@ -12,20 +12,24 @@ CFLAGS := $(filter-out -flto,$(CFLAGS)) (od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \ sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@ +# +# Drop .got.plt during conversion to plain binary format. +# Please check build32.lds for more details. +# %.bin: %.lnk - $(OBJCOPY) -O binary $< $@ + $(OBJCOPY) -O binary -R .got.plt $< $@ %.lnk: %.o $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' |\ while read idx name sz rest; do \ case "$$name" in \ - .data|.data.*|.rodata|.rodata.*|.bss|.bss.*) \ + .data|.data.*|.rodata.*|.bss|.bss.*) \ test $$sz != 0 || continue; \ echo "Error: non-empty $$name: 0x$$sz" >&2; \ exit $$(expr $$idx + 1);; \ esac; \ done - $(LD) $(LDFLAGS_DIRECT) -N -Ttext 0 -o $@ $< + $(LD) $(LDFLAGS_DIRECT) -N -T build32.lds -o $@ $< %.o: %.c $(CC) $(CFLAGS) -c -fpic $< -o $@
Newer GCC (e.g. gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)) does some code optimizations by creating data sections (e.g. jump addresses for C switch/case are calculated using data in .rodata section). This thing is not accepted by *.lnk build recipe which requires that only .text section lives in output. Potentially we can inhibit this GCC behavior by using special options, e.g. -fno-tree-switch-conversion. However, this does not guarantee that in the future new similar optimizations or anything which creates not accepted sections will not break our build recipes again. Additionally, I do not mention that probably this is not good idea to just disable random optimizations. So, take over full control on *.lnk linking process by using linker script and merge all text and data sections into one .text section. Additionally, remove .got.plt section which is not used in our final code. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- xen/arch/x86/boot/build32.lds | 49 +++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/boot/build32.mk | 10 ++++++--- 2 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 xen/arch/x86/boot/build32.lds