lkml.org 
[lkml]   [2009]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86: clean up vdso-layout.lds.S
    On Tue, Jun 09, 2009 at 09:26:58AM +0200, Petr Tesarik wrote:
    > Sam Ravnborg píše v Pá 05. 06. 2009 v 22:07 +0200:
    > > > > diff --git a/arch/x86/vdso/Makefile b/arch/x86/vdso/Makefile
    > > > > index 16a9020..8c7f06a 100644
    > > > > --- a/arch/x86/vdso/Makefile
    > > > > +++ b/arch/x86/vdso/Makefile
    > > > > @@ -23,7 +23,8 @@ $(obj)/vdso.o: $(obj)/vdso.so
    > > > >
    > > > > targets += vdso.so vdso.so.dbg vdso.lds $(vobjs-y)
    > > > >
    > > > > -export CPPFLAGS_vdso.lds += -P -C
    > > > > +vdso-cppflags = -P -C
    > > > > +export CPPFLAGS_vdso.lds += -m64 $(vdso-cppflags)
    > >
    > > I am wondering why we need -P -C here - but we do not need it for lds.S files?
    > > Seems like something we could let go.
    >
    > Frankly, I don't know, but it's been there for ages, and I don't see
    > what you mean, anyway. Doesn't the top-level Makefile contain this line,
    > for example:
    >
    > export CPPFLAGS_vmlinux.lds += -P -C -U$(ARCH)

    I had forgotten about the top-level Makefile setting these.
    They should have moved to Makefile.build long time ago :-(

    Keep them here.

    >
    > > > > VDSO_LDFLAGS_vdso.lds = -m elf_x86_64 -Wl,-soname=linux-vdso.so.1 \
    > > > > -Wl,-z,max-page-size=4096 -Wl,-z,common-page-size=4096
    > > > > @@ -68,7 +69,7 @@ vdso32.so-$(VDSO32-y) += sysenter
    > > > >
    > > > > vdso32-images = $(vdso32.so-y:%=vdso32-%.so)
    > > > >
    > > > > -CPPFLAGS_vdso32.lds = $(CPPFLAGS_vdso.lds)
    > > > > +CPPFLAGS_vdso32.lds = -m32 $(vdso-cppflags)
    > > > > VDSO_LDFLAGS_vdso32.lds = -m elf_i386 -Wl,-soname=linux-gate.so.1
    > > > >
    > > > > # This makes sure the $(obj) subdirectory exists even though vdso32/
    > > > > diff --git a/arch/x86/vdso/vdso-layout.lds.S b/arch/x86/vdso/vdso-layout.lds.S
    > > > > index 634a2cf..1f4b215 100644
    > > > > --- a/arch/x86/vdso/vdso-layout.lds.S
    > > > > +++ b/arch/x86/vdso/vdso-layout.lds.S
    > > > > @@ -22,16 +22,15 @@ SECTIONS
    > > > > .eh_frame : { KEEP (*(.eh_frame)) } :text
    > > > >
    > > > > .dynamic : { *(.dynamic) } :text :dynamic
    > > > > + .got : { *(.got.plt) *(.got) } :text
    > >
    > > The style we try to introduce for .lds files in
    > > arch/$ARCH/kernel/vmlinux.lds.S is much more C-like.
    > > The above would have been:
    > > .got : {
    > > *(.got.plt)
    > > *(.got)
    > > } :text
    > >
    > > Please use this all over so we have a consistent style in linker scripts.
    >
    > OK, so should I first post a patch which doesn't change anything but
    > adjusts the coding style of vdso-layout.lds.S?

    That would be great.
    Then your following changes would be easier to read/review.

    >
    >
    > > > > .data : {
    > > > > - *(.data*)
    > > > > - *(.sdata*)
    > > > > - *(.got.plt) *(.got)
    > > > > - *(.gnu.linkonce.d.*)
    > > > > - *(.bss*)
    > > > > - *(.dynbss*)
    > > > > - *(.gnu.linkonce.b.*)
    > > > > + *(.data .data.* .gnu.linkonce.d.*)
    > > > > + *(.bss .bss.* .gnu.linkonce.b.*)
    > > > > + *(COMMON)
    > > > > }
    > > Where did *(.sdata*) go?
    >
    > Nothing changed, really. This section is never produced for i386 or
    > x86_64. The line might have been a remnant of the (long abandoned) idea
    > that x86_64 would have .sdata and .data, but it then turned to be
    > rather .data and .ldata.
    >
    > > Why do we need *(.data .data.*) rather than *(.data*)?
    > > *(.dynbss*)?
    >
    > I don't have any strong opinion here, but the former is exactly what the
    > default linker script has.

    My general take on this is that we should know and deal with what the linker produces.
    So if we only expect .data then .data.* could go into .broken so we catch it.

    >
    > > In your changelog you say:
    > > "discard sections which are not useful to user-space" - but as you do not
    > > list which one it is hard to tell what you removed on purpose
    > > and what you removed by accident.
    >
    > All those which end up in the section ".broken". I'll make a better
    > wording next time.
    >
    > > > > }
    > > > >
    > > > > +ASSERT(!SIZEOF(.broken), "VDSO contains sections that don't work properly");
    > > Can you give any better hints where to look and what to look for?
    >
    > What would you expect? The linker script language is quite limited in
    > its capabilities... Best I could do is split the ".broken" section into
    > several sections and move the descriptions from the individual comments
    > above here. If this muckle of empty ".broken.*" sections gets correctly
    > discarded and triggers no bug in binutils, I can probably do it.

    I was more think of something like this:

    +/*
    + * This assert is triggered if the linker produces a non-empty section
    + * that is listed in the .broken section.
    + * Use objdump -h to see which is the offending section
    + */
    +ASSERT(!SIZEOF(.broken), "The vdso linker script found a section that is bad. See xx.lds for details");


    > > > > +/* Check that GOT has only the three entries defined by the ABI */
    > > > > +ASSERT(SIZEOF(.got) == 3*__SIZEOF_POINTER__,
    > > > > + "Found extra GOT entries. Check your use of external vars.");
    > > Can you give any better hints where to look and what to look for?
    >
    > What would you consider a better hint? If there are any entries in the
    > GOT, this means that the vDSO tries to access an external function or
    > variable. But since we don't have a real in-kernel linker for the vDSO,
    > these references will remain unresolved (and most likely cause a
    > segmentation fault at run-time).
    And here reword the above explanation a bit and give a hint
    how to see what is there that is unexpected.
    And stuff it in a comment.

    Sam
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-06-09 20:13    [W:0.030 / U:1.272 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site