[PATCH] x86: linker script syntax nits

[PATCH] x86: linker script syntax nits

[Roland McGrath]

The following changes since commit 80f506918fdaaca6b574ba931536a58ce015c7be:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.dk/linux-2.6-block

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/x86-lds-nits

Roland McGrath (1):
      x86: linker script syntax nits

 arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
 arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
 2 files changed, 10 insertions(+), 11 deletions(-)
---
[PATCH] x86: linker script syntax nits

The linker scripts grew some use of weirdly wrong linker script syntax.
It happens to work, but it's not what the syntax is documented to be.
Clean it up to use the official syntax.

Signed-off-by: Roland McGrath <roland@redhat.com>
CC: Ian Lance Taylor <iant@google.com>
---
 arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
 arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/acpi/realmode/wakeup.lds.S b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
index 7da00b7..0e50e1e 100644
--- a/arch/x86/kernel/acpi/realmode/wakeup.lds.S
+++ b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
@@ -56,6 +56,6 @@ SECTIONS
 	/DISCARD/ : {
 		*(.note*)
 	}
-
-	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
 }
+
+ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 92929fb..8d6001a 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -305,8 +305,8 @@ SECTIONS
 
 
 #ifdef CONFIG_X86_32
-. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
-	   "kernel image bigger than KERNEL_IMAGE_SIZE");
+ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
+       "kernel image bigger than KERNEL_IMAGE_SIZE");
 #else
 /*
  * Per-cpu symbols which need to be offset from __per_cpu_load
@@ -319,12 +319,12 @@ INIT_PER_CPU(irq_stack_union);
 /*
  * Build-time check on the image size:
  */
-. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
-	   "kernel image bigger than KERNEL_IMAGE_SIZE");
+ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
+       "kernel image bigger than KERNEL_IMAGE_SIZE");
 
 #ifdef CONFIG_SMP
-. = ASSERT((per_cpu__irq_stack_union == 0),
-           "irq_stack_union is not at start of per-cpu area");
+ASSERT((per_cpu__irq_stack_union == 0),
+       "irq_stack_union is not at start of per-cpu area");
 #endif
 
 #endif /* CONFIG_X86_32 */
@@ -332,7 +332,6 @@ INIT_PER_CPU(irq_stack_union);
 #ifdef CONFIG_KEXEC
 #include <asm/kexec.h>
 
-. = ASSERT(kexec_control_code_size <= KEXEC_CONTROL_CODE_MAX_SIZE,
-           "kexec control code size is too big");
+ASSERT(kexec_control_code_size <= KEXEC_CONTROL_CODE_MAX_SIZE,
+       "kexec control code size is too big");
 #endif
-

Re: [PATCH] x86: linker script syntax nits

[H. Peter Anvin]

On 10/14/2009 03:10 PM, Roland McGrath wrote:
> The following changes since commit 80f506918fdaaca6b574ba931536a58ce015c7be:
>   Linus Torvalds (1):
>         Merge branch 'for-linus' of git://git.kernel.dk/linux-2.6-block
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/x86-lds-nits
> 
> Roland McGrath (1):
>       x86: linker script syntax nits
> 
>  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
>  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
>  2 files changed, 10 insertions(+), 11 deletions(-)

Severely-Nacked-by: H. Peter Anvin <hpa@zytor.com>

> -	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
>  }
> +
> +ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");

Sinkless asserts are broken in older versions of ld.

	-hpa

Re: [PATCH] x86: linker script syntax nits

[Sam Ravnborg]

On Wed, Oct 14, 2009 at 03:10:24PM -0700, Roland McGrath wrote:
> The following changes since commit 80f506918fdaaca6b574ba931536a58ce015c7be:
>   Linus Torvalds (1):
>         Merge branch 'for-linus' of git://git.kernel.dk/linux-2.6-block
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/x86-lds-nits
> 
> Roland McGrath (1):
>       x86: linker script syntax nits
> 
>  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
>  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
>  2 files changed, 10 insertions(+), 11 deletions(-)
> ---
> [PATCH] x86: linker script syntax nits
> 
> The linker scripts grew some use of weirdly wrong linker script syntax.
> It happens to work, but it's not what the syntax is documented to be.
> Clean it up to use the official syntax.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>
> CC: Ian Lance Taylor <iant@google.com>
> ---
>  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
>  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/realmode/wakeup.lds.S b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> index 7da00b7..0e50e1e 100644
> --- a/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> +++ b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> @@ -56,6 +56,6 @@ SECTIONS
>  	/DISCARD/ : {
>  		*(.note*)
>  	}
> -
> -	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
>  }
> +
> +ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");

This breaks with older binutils.
See d2ba8b211bb8abc29aa627dbd4dce08cfbc8082b for reference.

Same goes for the other cahnges in this post.

Yup - it looks ugly :-(

	Sam

[tip:x86/urgent] Revert "x86: linker script syntax nits"

[tip-bot for Ingo Molnar]

Commit-ID:  db8590f5043f3436a65b24155a3a7af2604df876
Gitweb:     http://git.kernel.org/tip/db8590f5043f3436a65b24155a3a7af2604df876
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Thu, 15 Oct 2009 08:08:12 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 15 Oct 2009 08:09:55 +0200

Revert "x86: linker script syntax nits"

This reverts commit e9a63a4e559fbdc522072281d05e6b13c1022f4b.

This breaks older binutils, where sink-less asserts are broken.

See this commit for further details:

  d2ba8b2: x86: Fix assert syntax in vmlinux.lds.S

Acked-by: "H. Peter Anvin" <hpa@zytor.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <4AD6523D.5030909@zytor.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
 arch/x86/kernel/vmlinux.lds.S              |   17 +++++++++--------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/acpi/realmode/wakeup.lds.S b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
index 0e50e1e..7da00b7 100644
--- a/arch/x86/kernel/acpi/realmode/wakeup.lds.S
+++ b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
@@ -56,6 +56,6 @@ SECTIONS
 	/DISCARD/ : {
 		*(.note*)
 	}
-}
 
-ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
+	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
+}
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 8d6001a..92929fb 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -305,8 +305,8 @@ SECTIONS
 
 
 #ifdef CONFIG_X86_32
-ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
-       "kernel image bigger than KERNEL_IMAGE_SIZE");
+. = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
+	   "kernel image bigger than KERNEL_IMAGE_SIZE");
 #else
 /*
  * Per-cpu symbols which need to be offset from __per_cpu_load
@@ -319,12 +319,12 @@ INIT_PER_CPU(irq_stack_union);
 /*
  * Build-time check on the image size:
  */
-ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
-       "kernel image bigger than KERNEL_IMAGE_SIZE");
+. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),
+	   "kernel image bigger than KERNEL_IMAGE_SIZE");
 
 #ifdef CONFIG_SMP
-ASSERT((per_cpu__irq_stack_union == 0),
-       "irq_stack_union is not at start of per-cpu area");
+. = ASSERT((per_cpu__irq_stack_union == 0),
+           "irq_stack_union is not at start of per-cpu area");
 #endif
 
 #endif /* CONFIG_X86_32 */
@@ -332,6 +332,7 @@ ASSERT((per_cpu__irq_stack_union == 0),
 #ifdef CONFIG_KEXEC
 #include <asm/kexec.h>
 
-ASSERT(kexec_control_code_size <= KEXEC_CONTROL_CODE_MAX_SIZE,
-       "kexec control code size is too big");
+. = ASSERT(kexec_control_code_size <= KEXEC_CONTROL_CODE_MAX_SIZE,
+           "kexec control code size is too big");
 #endif
+

Re: [PATCH] x86: linker script syntax nits

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> The following changes since commit 80f506918fdaaca6b574ba931536a58ce015c7be:
>   Linus Torvalds (1):
>         Merge branch 'for-linus' of git://git.kernel.dk/linux-2.6-block
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/x86-lds-nits
> 
> Roland McGrath (1):
>       x86: linker script syntax nits
> 
>  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
>  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
>  2 files changed, 10 insertions(+), 11 deletions(-)
> ---
> [PATCH] x86: linker script syntax nits
> 
> The linker scripts grew some use of weirdly wrong linker script syntax.
> It happens to work, but it's not what the syntax is documented to be.
> Clean it up to use the official syntax.

I've queued up a revert based on Peter's and Sam's objections.

I'd also like to point out a few commit log quality nits. A standard x86 
commit should have the following title:

    x86: Clean up linker script syntax nits

not:

    x86: linker script syntax nits

Note that we always try to add a verb to that sentence, so that it's 
more natural-language alike. (also note the capitalization (we 
capitalize the sentence after the 'x86:' prefix))

(Plus i always try to line-wrap the commit log to 65 columns, to make it 
easy to quote to several levels and still be within the various viewer 
column widths that people use - and to allow for the +4 columns used up 
by git log's indentation.)

Thanks,

	Ingo

Re: [PATCH] x86: linker script syntax nits

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> On Wed, Oct 14, 2009 at 03:10:24PM -0700, Roland McGrath wrote:
> > The following changes since commit 80f506918fdaaca6b574ba931536a58ce015c7be:
> >   Linus Torvalds (1):
> >         Merge branch 'for-linus' of git://git.kernel.dk/linux-2.6-block
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/x86-lds-nits
> > 
> > Roland McGrath (1):
> >       x86: linker script syntax nits
> > 
> >  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
> >  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
> >  2 files changed, 10 insertions(+), 11 deletions(-)
> > ---
> > [PATCH] x86: linker script syntax nits
> > 
> > The linker scripts grew some use of weirdly wrong linker script syntax.
> > It happens to work, but it's not what the syntax is documented to be.
> > Clean it up to use the official syntax.
> > 
> > Signed-off-by: Roland McGrath <roland@redhat.com>
> > CC: Ian Lance Taylor <iant@google.com>
> > ---
> >  arch/x86/kernel/acpi/realmode/wakeup.lds.S |    4 ++--
> >  arch/x86/kernel/vmlinux.lds.S              |   17 ++++++++---------
> >  2 files changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/acpi/realmode/wakeup.lds.S b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> > index 7da00b7..0e50e1e 100644
> > --- a/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> > +++ b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
> > @@ -56,6 +56,6 @@ SECTIONS
> >  	/DISCARD/ : {
> >  		*(.note*)
> >  	}
> > -
> > -	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
> >  }
> > +
> > +ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
> 
> This breaks with older binutils. See 
> d2ba8b211bb8abc29aa627dbd4dce08cfbc8082b for reference.
> 
> Same goes for the other cahnges in this post.
> 
> Yup - it looks ugly :-(

We could introduce a COMPAT_ASSERT() wrapper perhaps, to move it more in 
line with the 'official' syntax.

Or we could wrap ASSERT() itself (this runs through the preprocessor 
before going to the linker) - although that would be a pretty obfuscated 
move.

At minimum we should add a comment to the first use of ASSERT() here 
that we assign the current address due to compatibility reasons. (same 
goes for arch/x86/boot/setup.ld)

Anyway - any such cleanup would be .33 material.

	Ingo

Re: [PATCH] x86: linker script syntax nits

[H. Peter Anvin]

On 10/14/2009 11:38 PM, Ingo Molnar wrote:
> 
> We could introduce a COMPAT_ASSERT() wrapper perhaps, to move it more in 
> line with the 'official' syntax.
> 
> Or we could wrap ASSERT() itself (this runs through the preprocessor 
> before going to the linker) - although that would be a pretty obfuscated 
> move.
> 
> At minimum we should add a comment to the first use of ASSERT() here 
> that we assign the current address due to compatibility reasons. (same 
> goes for arch/x86/boot/setup.ld)
> 
> Anyway - any such cleanup would be .33 material.
> 

The tricky part about wrapping ASSERT() is that you have to have a sink
for the old versions of ld.  We use . as the sink, but that changes .
which you don't want to do silently.

The other -- and somewhat less cantankerous way -- is to use a dummy symbol:

#define LINUX_ASSERT(expr, name, string) name = ASSERT(expr, string)

"name" would have to be unique for each instance.

	-hpa

Re: [PATCH] x86: linker script syntax nits

[Roland McGrath]

> The other -- and somewhat less cantankerous way -- is to use a dummy symbol:
> 
> #define LINUX_ASSERT(expr, name, string) name = ASSERT(expr, string)
> 
> "name" would have to be unique for each instance.

I wouldn't like to pollute the symtabs with those useless symbols.

#define LINUX_ASSERT(expr, string) PROVIDE(_assert_bogon = ASSERT(expr, string))

works with a recent ld.  I don't know what the problematic old ld versions
were.  Do you have such on hand to test?

Ian may have another suggestion.


Thanks,
Roland

Re: [PATCH] x86: linker script syntax nits

[H. Peter Anvin]

On 10/15/2009 11:26 AM, Roland McGrath wrote:
>> The other -- and somewhat less cantankerous way -- is to use a dummy symbol:
>>
>> #define LINUX_ASSERT(expr, name, string) name = ASSERT(expr, string)
>>
>> "name" would have to be unique for each instance.
> 
> I wouldn't like to pollute the symtabs with those useless symbols.
> 
> #define LINUX_ASSERT(expr, string) PROVIDE(_assert_bogon = ASSERT(expr, string))
> 
> works with a recent ld.  I don't know what the problematic old ld versions
> were.  Do you have such on hand to test?
> 
> Ian may have another suggestion.
> 

I don't remember how old they have to be... I think RHEL 4 timeframe,
though.

	-hpa

Re: [PATCH] x86: linker script syntax nits

[Sam Ravnborg]

On Thu, Oct 15, 2009 at 11:26:16AM -0700, Roland McGrath wrote:
> > The other -- and somewhat less cantankerous way -- is to use a dummy symbol:
> > 
> > #define LINUX_ASSERT(expr, name, string) name = ASSERT(expr, string)
> > 
> > "name" would have to be unique for each instance.
> 
> I wouldn't like to pollute the symtabs with those useless symbols.
> 
> #define LINUX_ASSERT(expr, string) PROVIDE(_assert_bogon = ASSERT(expr, string))
> 
> works with a recent ld.  I don't know what the problematic old ld versions
> were.  Do you have such on hand to test?
Jean reported binutils 2.14.90.0.6.

See: http://marc.info/?l=linux-kbuild&m=124930110427870&w=2

	Sam

[tip:x86/urgent] x86: Document linker script ASSERT() quirk

[tip-bot for Ingo Molnar]

Commit-ID:  a5912f6b3e20c137172460e6d4dd180866c00963
Gitweb:     http://git.kernel.org/tip/a5912f6b3e20c137172460e6d4dd180866c00963
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Fri, 16 Oct 2009 07:18:46 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 16 Oct 2009 07:18:46 +0200

x86: Document linker script ASSERT() quirk

Older binutils breaks if ASSERT() is used without a sink
for the output.

For example 2.14.90.0.6 is known to be broken, the link
fails with:

  LD      .tmp_vmlinux1
  ld:arch/x86/kernel/vmlinux.lds:678: parse error

Document this quirk in all three files that use it.

  See:    http://marc.info/?l=linux-kbuild&m=124930110427870&w=2
  See[2]: d2ba8b2 ("x86: Fix assert syntax in vmlinux.lds.S")

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Roland McGrath <roland@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
LKML-Reference: <4AD6523D.5030909@zytor.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/boot/setup.ld                     |    3 +++
 arch/x86/kernel/acpi/realmode/wakeup.lds.S |    3 +++
 arch/x86/kernel/vmlinux.lds.S              |    3 +++
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
index 0f6ec45..03c0683 100644
--- a/arch/x86/boot/setup.ld
+++ b/arch/x86/boot/setup.ld
@@ -53,6 +53,9 @@ SECTIONS
 
 	/DISCARD/ : { *(.note*) }
 
+	/*
+	 * The ASSERT() sink to . is intentional, for binutils 2.14 compatibility:
+	 */
 	. = ASSERT(_end <= 0x8000, "Setup too big!");
 	. = ASSERT(hdr == 0x1f1, "The setup header has the wrong offset!");
 	/* Necessary for the very-old-loader check to work... */
diff --git a/arch/x86/kernel/acpi/realmode/wakeup.lds.S b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
index 7da00b7..060fff8 100644
--- a/arch/x86/kernel/acpi/realmode/wakeup.lds.S
+++ b/arch/x86/kernel/acpi/realmode/wakeup.lds.S
@@ -57,5 +57,8 @@ SECTIONS
 		*(.note*)
 	}
 
+	/*
+	 * The ASSERT() sink to . is intentional, for binutils 2.14 compatibility:
+	 */
 	. = ASSERT(_end <= WAKEUP_SIZE, "Wakeup too big!");
 }
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 92929fb..3c68fe2 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -305,6 +305,9 @@ SECTIONS
 
 
 #ifdef CONFIG_X86_32
+/*
+ * The ASSERT() sink to . is intentional, for binutils 2.14 compatibility:
+ */
 . = ASSERT((_end - LOAD_OFFSET <= KERNEL_IMAGE_SIZE),
 	   "kernel image bigger than KERNEL_IMAGE_SIZE");
 #else