[PATCH] x86: clean up vdso-layout.lds.S

[PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

The handling of various sections in the VDSO linker script
looks pretty haphazard. This patch cleans it up in this
regards:

 - re-order sections to more closely match the result of
   a normal shared link
 - discard sections which are not useful to user-space
 - issue a linker error if a section is encountered which
   is known not to work
 - check that the .got section is empty, except for the
   three entries defined by the ABI

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

---
 Makefile          |    5 ++--
 vdso-layout.lds.S |   57 +++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 51 insertions(+), 11 deletions(-)

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)
 
 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
 
-	.rodata		: { *(.rodata*) }		:text
+	.rodata		: {
+		*(.rodata .rodata.* .gnu.linkonce.r.*)
+	}
 	.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)
 	}
 
 	.altinstructions	: { *(.altinstructions) }
@@ -43,9 +42,49 @@ SECTIONS
 	 */
 	. = ALIGN(0x100);
 
-	.text		: { *(.text*) }			:text	=0x90909090
+	.text		: {
+		*(.text .text.* .gnu.linkonce.t.*)
+	}						:text	=0x90909090
+
+	/* We would need a more sophisticated dynamic linker for the
+	 * vDSO to make the following sections work.  Put them into
+	 * a special section and raise a link-time error if they get
+	 * used.
+	 */
+	.broken		: {
+		/* Code in the Procedure Linkage Table will segfault */
+		*(.plt)
+
+		/* Relocation will not be done, so any pointers will
+		 * still point to the prelinked address, which is wrong
+		 */
+		*(.data.rel.ro*)
+		*(.gnu.linkonce.d.rel.ro.*)
+
+		/* Initialization/termination won't work this way */
+		*(.init) *(.fini)
+		*(.preinit_array) *(.init_array*)
+		*(.fini_array*)
+
+		/* Thread-local data cannot be defined like this */
+		*(.tdata .tdata.* .gnu.linkonce.td.*)
+		*(.tbss .tbss.* .gnu.linkonce.tb.*)
+		*(.tcommon)
+	}
+
+	/* These sections are not useful */
+	/DISCARD/	: {
+		*(.gnu.warning.*)
+		*(.note.GNU-stack)
+	}
 }
 
+ASSERT(!SIZEOF(.broken), "VDSO contains sections that don't work properly");
+
+/* 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.");
+
 /*
  * Very old versions of ld do not recognize this name token; use the constant.
  */

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

Petr Tesarik píše v Po 01. 06. 2009 v 16:05 +0200:
> The handling of various sections in the VDSO linker script
> looks pretty haphazard. This patch cleans it up in this
> regards:
> 
>  - re-order sections to more closely match the result of
>    a normal shared link
>  - discard sections which are not useful to user-space
>  - issue a linker error if a section is encountered which
>    is known not to work
>  - check that the .got section is empty, except for the
>    three entries defined by the ABI
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

Any comments on this? It doesn't change anything. It only makes it
harder to break vDSOs by accident (such as the latest buglet with TSC
synchronization).

Petr Tesarik

> ---
>  Makefile          |    5 ++--
>  vdso-layout.lds.S |   57 +++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 51 insertions(+), 11 deletions(-)
> 
> 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)
>  
>  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
>  
> -	.rodata		: { *(.rodata*) }		:text
> +	.rodata		: {
> +		*(.rodata .rodata.* .gnu.linkonce.r.*)
> +	}
>  	.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)
>  	}
>  
>  	.altinstructions	: { *(.altinstructions) }
> @@ -43,9 +42,49 @@ SECTIONS
>  	 */
>  	. = ALIGN(0x100);
>  
> -	.text		: { *(.text*) }			:text	=0x90909090
> +	.text		: {
> +		*(.text .text.* .gnu.linkonce.t.*)
> +	}						:text	=0x90909090
> +
> +	/* We would need a more sophisticated dynamic linker for the
> +	 * vDSO to make the following sections work.  Put them into
> +	 * a special section and raise a link-time error if they get
> +	 * used.
> +	 */
> +	.broken		: {
> +		/* Code in the Procedure Linkage Table will segfault */
> +		*(.plt)
> +
> +		/* Relocation will not be done, so any pointers will
> +		 * still point to the prelinked address, which is wrong
> +		 */
> +		*(.data.rel.ro*)
> +		*(.gnu.linkonce.d.rel.ro.*)
> +
> +		/* Initialization/termination won't work this way */
> +		*(.init) *(.fini)
> +		*(.preinit_array) *(.init_array*)
> +		*(.fini_array*)
> +
> +		/* Thread-local data cannot be defined like this */
> +		*(.tdata .tdata.* .gnu.linkonce.td.*)
> +		*(.tbss .tbss.* .gnu.linkonce.tb.*)
> +		*(.tcommon)
> +	}
> +
> +	/* These sections are not useful */
> +	/DISCARD/	: {
> +		*(.gnu.warning.*)
> +		*(.note.GNU-stack)
> +	}
>  }
>  
> +ASSERT(!SIZEOF(.broken), "VDSO contains sections that don't work properly");
> +
> +/* 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.");
> +
>  /*
>   * Very old versions of ld do not recognize this name token; use the constant.
>   */
> 
> 
> --
> 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/

Re: [PATCH] x86: clean up vdso-layout.lds.S

[H. Peter Anvin]

Andi Kleen wrote:
>> Any comments on this? It doesn't change anything. It only makes it
>> harder to break vDSOs by accident (such as the latest buglet with TSC
>> synchronization).
> 
> Looks ok to me. Although it would have been nicer if you hadn't mixed
> that many changes together.
> 
> Acked-by: Andi Kleen <ak@linux.intel.com>
> 

It would have.  That makes review harder, and right at the moment time
is a bit at a premium.  It looks like a good cleanup, but it also looks
like it's going to need some testing, which means that since it is not a
functional change I would feel best if we could put it on the .32 track
after the imminent merge window craziness is over.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Andi Kleen]

> Any comments on this? It doesn't change anything. It only makes it
> harder to break vDSOs by accident (such as the latest buglet with TSC
> synchronization).

Looks ok to me. Although it would have been nicer if you hadn't mixed
that many changes together.

Acked-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Sam Ravnborg]

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

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

> >  	.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?
Why do we need *(.data .data.*) rather than *(.data*)?
*(.dynbss*)?

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.

> >  
> >  	.altinstructions	: { *(.altinstructions) }
> > @@ -43,9 +42,49 @@ SECTIONS
> >  	 */
> >  	. = ALIGN(0x100);

What is 0x100?

> >  
> > -	.text		: { *(.text*) }			:text	=0x90909090
> > +	.text		: {
> > +		*(.text .text.* .gnu.linkonce.t.*)
> > +	}						:text	=0x90909090
> > +
> > +	/* We would need a more sophisticated dynamic linker for the
> > +	 * vDSO to make the following sections work.  Put them into
> > +	 * a special section and raise a link-time error if they get
> > +	 * used.
> > +	 */
> > +	.broken		: {
> > +		/* Code in the Procedure Linkage Table will segfault */
> > +		*(.plt)
> > +
> > +		/* Relocation will not be done, so any pointers will
> > +		 * still point to the prelinked address, which is wrong
> > +		 */
> > +		*(.data.rel.ro*)
> > +		*(.gnu.linkonce.d.rel.ro.*)
> > +
> > +		/* Initialization/termination won't work this way */
> > +		*(.init) *(.fini)
> > +		*(.preinit_array) *(.init_array*)
> > +		*(.fini_array*)
> > +
> > +		/* Thread-local data cannot be defined like this */
> > +		*(.tdata .tdata.* .gnu.linkonce.td.*)
> > +		*(.tbss .tbss.* .gnu.linkonce.tb.*)
> > +		*(.tcommon)
> > +	}
> > +
> > +	/* These sections are not useful */
> > +	/DISCARD/	: {
> > +		*(.gnu.warning.*)
> > +		*(.note.GNU-stack)
> > +	}
> >  }
> >  
> > +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?
> > +
> > +/* 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?

	Sam

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

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)

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


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

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

> > >  
> > >  	.altinstructions	: { *(.altinstructions) }
> > > @@ -43,9 +42,49 @@ SECTIONS
> > >  	 */
> > >  	. = ALIGN(0x100);
> 
> What is 0x100?

Um. No idea. Roland, you added this line in commit
f6b46ebf904f69a73907a5e6b1ed2228e3f03d9e. Can you shed some light on
this magic constant?

> > > -	.text		: { *(.text*) }			:text	=0x90909090
> > > +	.text		: {
> > > +		*(.text .text.* .gnu.linkonce.t.*)
> > > +	}						:text	=0x90909090
> > > +
> > > +	/* We would need a more sophisticated dynamic linker for the
> > > +	 * vDSO to make the following sections work.  Put them into
> > > +	 * a special section and raise a link-time error if they get
> > > +	 * used.
> > > +	 */
> > > +	.broken		: {
> > > +		/* Code in the Procedure Linkage Table will segfault */
> > > +		*(.plt)
> > > +
> > > +		/* Relocation will not be done, so any pointers will
> > > +		 * still point to the prelinked address, which is wrong
> > > +		 */
> > > +		*(.data.rel.ro*)
> > > +		*(.gnu.linkonce.d.rel.ro.*)
> > > +
> > > +		/* Initialization/termination won't work this way */
> > > +		*(.init) *(.fini)
> > > +		*(.preinit_array) *(.init_array*)
> > > +		*(.fini_array*)
> > > +
> > > +		/* Thread-local data cannot be defined like this */
> > > +		*(.tdata .tdata.* .gnu.linkonce.td.*)
> > > +		*(.tbss .tbss.* .gnu.linkonce.tb.*)
> > > +		*(.tcommon)
> > > +	}
> > > +
> > > +	/* These sections are not useful */
> > > +	/DISCARD/	: {
> > > +		*(.gnu.warning.*)
> > > +		*(.note.GNU-stack)
> > > +	}
> > >  }
> > >  
> > > +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.

> > > +
> > > +/* 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).

Cheers,
Petr Tesarik

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

H. Peter Anvin píše v Pá 05. 06. 2009 v 09:24 -0700:
> Andi Kleen wrote:
> >> Any comments on this? It doesn't change anything. It only makes it
> >> harder to break vDSOs by accident (such as the latest buglet with TSC
> >> synchronization).
> > 
> > Looks ok to me. Although it would have been nicer if you hadn't mixed
> > that many changes together.
> > 
> > Acked-by: Andi Kleen <ak@linux.intel.com>
> > 
> 
> It would have.  That makes review harder, and right at the moment time
> is a bit at a premium.  It looks like a good cleanup, but it also looks
> like it's going to need some testing, which means that since it is not a
> functional change I would feel best if we could put it on the .32 track
> after the imminent merge window craziness is over.

You're right. It doesn't make any functional change, so I'm completely
fine with taking it on the .32 track.

For the time being, please hold on, as I'm going to split the patch into
a series to make reviewing easier. But to be honest, the current content
of vdso-layout.lds.S is such mess that I'm convinced it must have been
done in a big hurry (and then moved around a few times, so a quick
glance at git-blame doesn't tell you that the most messy pieces have
been there since the very beginning).

Petr Tesarik

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Roland McGrath]

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

AFAIK these only affect readability of the lds.s output files.
The difference should be entirely cosmetic.

> > > >  	.altinstructions	: { *(.altinstructions) }
> > > > @@ -43,9 +42,49 @@ SECTIONS
> > > >  	 */
> > > >  	. = ALIGN(0x100);
> > 
> > What is 0x100?
> 
> Um. No idea. Roland, you added this line in commit
> f6b46ebf904f69a73907a5e6b1ed2228e3f03d9e. Can you shed some light on
> this magic constant?

You mean this one:

	/*
	 * Align the actual code well away from the non-instruction data.
	 * This is the best thing for the I-cache.
	 */
	. = ALIGN(0x100);

Reading the comment might make it obvious that it's intended for optimal
code alignment.  I suspect someone at the time told me 256 is as big as an
I-cache line was ever likely to get.  You could use L1_CACHE_BYTES instead
I suppose.

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

Adding more sections and section names unnecessarily bloats the size of the
vDSO image.  Keep the set of output sections to the necessary minimum.


Thanks,
Roland

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

Roland McGrath píše v Út 09. 06. 2009 v 00:52 -0700:
> > > 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.
> 
> AFAIK these only affect readability of the lds.s output files.
> The difference should be entirely cosmetic.
> 
> > > > >  	.altinstructions	: { *(.altinstructions) }
> > > > > @@ -43,9 +42,49 @@ SECTIONS
> > > > >  	 */
> > > > >  	. = ALIGN(0x100);
> > > 
> > > What is 0x100?
> > 
> > Um. No idea. Roland, you added this line in commit
> > f6b46ebf904f69a73907a5e6b1ed2228e3f03d9e. Can you shed some light on
> > this magic constant?
> 
> You mean this one:
> 
> 	/*
> 	 * Align the actual code well away from the non-instruction data.
> 	 * This is the best thing for the I-cache.
> 	 */
> 	. = ALIGN(0x100);
> 
> Reading the comment might make it obvious that it's intended for optimal
> code alignment.  I suspect someone at the time told me 256 is as big as an
> I-cache line was ever likely to get.  You could use L1_CACHE_BYTES instead
> I suppose.
> 
> > 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.
> 
> Adding more sections and section names unnecessarily bloats the size of the
> vDSO image.  Keep the set of output sections to the necessary minimum.

Which version of binutils are you using? I'm using binutils-2.19, and
all empty sections get completely discarded, i.e. they do not affect the
size of the output in any way.

Petr Tesarik

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Roland McGrath]

> Which version of binutils are you using? I'm using binutils-2.19, and
> all empty sections get completely discarded, i.e. they do not affect the
> size of the output in any way.

Oh, empty, sure.  So then what is ".broken" for at all?
I thought this was for the nonempty-but-useless ones like .got et al
that the linker will insist on creating but we don't really need in the vDSO.


Thanks,
Roland

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Petr Tesarik]

Roland McGrath píše v Út 09. 06. 2009 v 01:09 -0700:
> > Which version of binutils are you using? I'm using binutils-2.19, and
> > all empty sections get completely discarded, i.e. they do not affect the
> > size of the output in any way.
> 
> Oh, empty, sure.  So then what is ".broken" for at all?
> I thought this was for the nonempty-but-useless ones like .got et al
> that the linker will insist on creating but we don't really need in the vDSO.

No, that's for collecting sections which should never ever appear in a
vDSO. Or rather, when these sections do appear in a vDSO, I know for
sure that the resulting vDSO will not work properly.

Petr Tesarik

Re: [PATCH] x86: clean up vdso-layout.lds.S

[H. Peter Anvin]

Roland McGrath wrote:
> 
> You mean this one:
> 
> 	/*
> 	 * Align the actual code well away from the non-instruction data.
> 	 * This is the best thing for the I-cache.
> 	 */
> 	. = ALIGN(0x100);
> 
> Reading the comment might make it obvious that it's intended for optimal
> code alignment.  I suspect someone at the time told me 256 is as big as an
> I-cache line was ever likely to get.  You could use L1_CACHE_BYTES instead
> I suppose.
> 

Most likely 256 was chosen as a compromise between the the
then-documented value for coherency avoidance (128), future-proofing,
and waste.

I don't think we want to use different values on different platforms,
and end up with dramatically different vdsos when they still need to fit
in the same size envelope.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Sam Ravnborg]

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

Re: [PATCH] x86: clean up vdso-layout.lds.S

[H. Peter Anvin]

Sam Ravnborg wrote:
>>
>>> 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. 
> 

That will break if we change to per-function sections.

	-hpa

Re: [PATCH] x86: clean up vdso-layout.lds.S

[Sam Ravnborg]

On Tue, Jun 09, 2009 at 11:16:57AM -0700, H. Peter Anvin wrote:
> Sam Ravnborg wrote:
> >>
> >>> 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. 
> > 
> 
> That will break if we change to per-function sections.

But adding it to .broken would catch this at first build which would
have told us so.
We shall try to make it -ffunction-sections compatible in first try though.

	Sam