Re: [Xen-devel] [PATCH v3 07/16] x86/boot: create *.lnk files with linker script

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.05.16 at 14:28, <daniel.kiper@xxxxxxxxxx> wrote:
> On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> 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@xxxxxxxxxx>
>> > + *
>> > + * 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


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