Re: [Xen-devel] [PATCH v4 03/19] x86/boot: create *.lnk files with linker script

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> 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
> else which creates not accepted sections will not break our build recipes
> again. 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 required 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@xxxxxxxxxx>
> ---
> v4 - suggestions/fixes:
>    - remove my name from copyright (Oracle requirement)
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve comments,
>      (suggested by Jan Beulich),
>    - improve commit message
>      (suggested by Jan Beulich).
> ---
>  xen/arch/x86/boot/build32.lds |   51 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/boot/build32.mk  |   10 +++++---
>  2 files changed, 58 insertions(+), 3 deletions(-)
>  create mode 100644 xen/arch/x86/boot/build32.lds
> 
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
> new file mode 100644
> index 0000000..b14c7d5
> --- /dev/null
> +++ b/xen/arch/x86/boot/build32.lds
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + * 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)
> +  }

*(.rodata.*) ?

How about any other sections? Following the much improved
(thanks!) description, we can't exclude their appearance.

> +  /DISCARD/ : {
> +        /*
> +         * PIC/PIE executable contains .got.plt section even
> +         * if it is not linked with dynamic libraries. In such
> +         * case 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.
> +         * This means that there is pretty good chance that
> +         * we can safely drop .got.plt as a whole here. Sadly
> +         * this is not true. _GLOBAL_OFFSET_TABLE_ is used as
> +         * a reference for relative addressing (and only for
> +         * that thing) and ld complains if we remove .got.plt
> +         * section here because it cannot find required symbol.
> +         * However, _GLOBAL_OFFSET_TABLE_ is no longer needed
> +         * in final output. So, drop .got.plt section during
> +         * conversion to plain binary format.

Commonly we have a problem the other way around; here I'd like
to ask that you don't break lines this early (often hardly going
meaningfully beyond 60 columns).

> --- 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

Please avoid the two blank comment lines.

> -     $(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

This logic largely contradicts the use of a linker script: I'd much
rather see you include all the sections checked to be empty
here, and instead add a check that .got.plt (which you now
mean to drop) is of the expected size. Of course such a check
would need to go into the other rule's commands.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel