[PATCH] Use common linker script for all i386-pc systems

[PATCH] Use common linker script for all i386-pc systems

[Pavel Roskin]

This allows us to remove checks for the linker symbols for the beginning
and the end of the .bss section.  Instead, we use the names from the
linker script.  Another benefit is a better unification of the build
system.

ChangeLog:
	* aclocal.m4: Remove grub_CHECK_BSS_START_SYMBOL and
	grub_CHECK_END_SYMBOL.
	* configure.ac: Don't call grub_CHECK_BSS_START_SYMBOL and
	grub_CHECK_END_SYMBOL.  Check for ${target_cpu}-${platform}.lds
	as the linker script.
	* conf/i386-pc-cygwin-img-ld.sc: Rename to ...
	* conf/i386-pc.lds: ... this.
	* kern/i386/pc/startup.S: Use __bss_start__ and __bss_end__
	instead of BSS_START_SYMBOL and END_SYMBOL.
---
 aclocal.m4                    |   77 -----------------------------------------
 conf/i386-pc-cygwin-img-ld.sc |   53 ----------------------------
 conf/i386-pc.lds              |   53 ++++++++++++++++++++++++++++
 configure.ac                  |   10 ++---
 kern/i386/pc/startup.S        |    8 ++--
 5 files changed, 60 insertions(+), 141 deletions(-)
 delete mode 100644 conf/i386-pc-cygwin-img-ld.sc
 create mode 100644 conf/i386-pc.lds

diff --git a/aclocal.m4 b/aclocal.m4
index 779df3d..9a073b5 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -224,83 +224,6 @@ fi
 AC_MSG_RESULT([$grub_cv_i386_asm_absolute_without_asterisk])])
 
 
-dnl Check what symbol is defined as a bss start symbol.
-dnl Written by Michael Hohmoth and Yoshinori K. Okuji.
-AC_DEFUN(grub_CHECK_BSS_START_SYMBOL,
-[AC_REQUIRE([AC_PROG_CC])
-AC_MSG_CHECKING([if __bss_start is defined by the compiler])
-AC_CACHE_VAL(grub_cv_check_uscore_uscore_bss_start_symbol,
-[AC_LINK_IFELSE([AC_LANG_PROGRAM([[]],
-		[[asm ("incl __bss_start")]])],
-		[grub_cv_check_uscore_uscore_bss_start_symbol=yes],
-		[grub_cv_check_uscore_uscore_bss_start_symbol=no])])
-
-AC_MSG_RESULT([$grub_cv_check_uscore_uscore_bss_start_symbol])
-
-AC_MSG_CHECKING([if edata is defined by the compiler])
-AC_CACHE_VAL(grub_cv_check_edata_symbol,
-[AC_LINK_IFELSE([AC_LANG_PROGRAM([[]],
-		[[asm ("incl edata")]])],
-		[grub_cv_check_edata_symbol=yes],
-		[grub_cv_check_edata_symbol=no])])
-
-AC_MSG_RESULT([$grub_cv_check_edata_symbol])
-
-AC_MSG_CHECKING([if _edata is defined by the compiler])
-AC_CACHE_VAL(grub_cv_check_uscore_edata_symbol,
-[AC_LINK_IFELSE([AC_LANG_PROGRAM([[]],
-		[[asm ("incl _edata")]])],
-		[grub_cv_check_uscore_edata_symbol=yes],
-		[grub_cv_check_uscore_edata_symbol=no])])
-
-AC_MSG_RESULT([$grub_cv_check_uscore_edata_symbol])
-
-AH_TEMPLATE([BSS_START_SYMBOL], [Define it to one of __bss_start, edata and _edata])
-
-if test "x$grub_cv_check_uscore_uscore_bss_start_symbol" = xyes; then
-  AC_DEFINE([BSS_START_SYMBOL], [__bss_start])
-elif test "x$grub_cv_check_edata_symbol" = xyes; then
-  AC_DEFINE([BSS_START_SYMBOL], [edata])
-elif test "x$grub_cv_check_uscore_edata_symbol" = xyes; then
-  AC_DEFINE([BSS_START_SYMBOL], [_edata])
-else
-  AC_MSG_ERROR([none of __bss_start, edata or _edata is defined])
-fi
-])
-
-dnl Check what symbol is defined as an end symbol.
-dnl Written by Yoshinori K. Okuji.
-AC_DEFUN(grub_CHECK_END_SYMBOL,
-[AC_REQUIRE([AC_PROG_CC])
-AC_MSG_CHECKING([if end is defined by the compiler])
-AC_CACHE_VAL(grub_cv_check_end_symbol,
-[AC_LINK_IFELSE([AC_LANG_PROGRAM([[]],
-		[[asm ("incl end")]])],
-		[grub_cv_check_end_symbol=yes],
-		[grub_cv_check_end_symbol=no])])
-
-AC_MSG_RESULT([$grub_cv_check_end_symbol])
-
-AC_MSG_CHECKING([if _end is defined by the compiler])
-AC_CACHE_VAL(grub_cv_check_uscore_end_symbol,
-[AC_LINK_IFELSE([AC_LANG_PROGRAM([[]],
-		[[asm ("incl _end")]])],
-		[grub_cv_check_uscore_end_symbol=yes],
-		[grub_cv_check_uscore_end_symbol=no])])
-
-AC_MSG_RESULT([$grub_cv_check_uscore_end_symbol])
-
-AH_TEMPLATE([END_SYMBOL], [Define it to either end or _end])
-
-if test "x$grub_cv_check_end_symbol" = xyes; then
-  AC_DEFINE([END_SYMBOL], [end])
-elif test "x$grub_cv_check_uscore_end_symbol" = xyes; then
-  AC_DEFINE([END_SYMBOL], [_end])
-else
-  AC_MSG_ERROR([neither end nor _end is defined])
-fi
-])
-
 dnl Check if the C compiler has a bug while using nested functions when
 dnl mregparm is used on the i386.  Some gcc versions do not pass the third
 dnl parameter correctly to the nested function.
diff --git a/conf/i386-pc-cygwin-img-ld.sc b/conf/i386-pc-cygwin-img-ld.sc
deleted file mode 100644
index a41cac7..0000000
--- a/conf/i386-pc-cygwin-img-ld.sc
+++ /dev/null
@@ -1,53 +0,0 @@
-/* Linker script to create grub .img files on Cygwin.  */
-
-SECTIONS
-{
-  .text :
-  {
-    start = . ;
-    *(.text)
-    etext = . ;
-  }
-  .data :
-  {
-    __data_start__ = . ;
-    *(.data)
-    __data_end__ = . ;
-  }
-  .rdata :
-  {
-    __rdata_start__ = . ;
-    *(.rdata)
-    __rdata_end__ = . ;
-  }
-  .pdata :
-  {
-    *(.pdata)
-    edata = . ;
-  }
-  .bss :
-  {
-    __bss_start__ = . ;
-    *(.bss)
-    __common_start__ = . ;
-    *(COMMON)
-    __bss_end__ = . ;
-  }
-  .edata :
-  {
-    *(.edata)
-    end = . ;
-  }
-  .stab :
-  {
-    *(.stab)
-  }
-  .stabstr :
-  {
-    *(.stabstr)
-  }
-}
-
-ASSERT("__rdata_end__"=="edata", ".pdata not empty")
-ASSERT("__bss_end__"  =="end"  , ".edata not empty")
-
diff --git a/conf/i386-pc.lds b/conf/i386-pc.lds
new file mode 100644
index 0000000..a41cac7
--- /dev/null
+++ b/conf/i386-pc.lds
@@ -0,0 +1,53 @@
+/* Linker script to create grub .img files on Cygwin.  */
+
+SECTIONS
+{
+  .text :
+  {
+    start = . ;
+    *(.text)
+    etext = . ;
+  }
+  .data :
+  {
+    __data_start__ = . ;
+    *(.data)
+    __data_end__ = . ;
+  }
+  .rdata :
+  {
+    __rdata_start__ = . ;
+    *(.rdata)
+    __rdata_end__ = . ;
+  }
+  .pdata :
+  {
+    *(.pdata)
+    edata = . ;
+  }
+  .bss :
+  {
+    __bss_start__ = . ;
+    *(.bss)
+    __common_start__ = . ;
+    *(COMMON)
+    __bss_end__ = . ;
+  }
+  .edata :
+  {
+    *(.edata)
+    end = . ;
+  }
+  .stab :
+  {
+    *(.stab)
+  }
+  .stabstr :
+  {
+    *(.stabstr)
+  }
+}
+
+ASSERT("__rdata_end__"=="edata", ".pdata not empty")
+ASSERT("__bss_end__"  =="end"  , ".edata not empty")
+
diff --git a/configure.ac b/configure.ac
index d84e2b6..a048828 100644
--- a/configure.ac
+++ b/configure.ac
@@ -209,10 +209,10 @@ AC_CHECK_FUNCS(posix_memalign memalign asprintf)
 
 # Use linker script if present, otherwise use builtin -N script.
 AC_MSG_CHECKING([for option to link raw image])
-if test -f "${srcdir}/conf/${target_cpu}-${platform}-${host_os}-img-ld.sc"; then
-  TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/conf/${target_cpu}-${platform}-${host_os}-img-ld.sc"
+if test -f "${srcdir}/conf/${target_cpu}-${platform}.lds"; then
+  TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/conf/${target_cpu}-${platform}.lds"
   TARGET_IMG_LDFLAGS="-Wl,-T${TARGET_IMG_LDSCRIPT}"
-  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/conf/${target_cpu}-${platform}-${host_os}-img-ld.sc"
+  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/conf/${target_cpu}-${platform}.lds"
 else
   TARGET_IMG_LDSCRIPT=
   TARGET_IMG_LDFLAGS='-Wl,-N'
@@ -383,10 +383,6 @@ if test "x$target_cpu" = xi386; then
     # Check symbols provided by linker script.
     CFLAGS="$TARGET_CFLAGS -nostdlib $TARGET_IMG_LDFLAGS_AC -Wl,-Ttext,8000,--defsym,___main=0x8100"
   fi
-  if test "x$platform" = xpc; then
-    grub_CHECK_BSS_START_SYMBOL
-    grub_CHECK_END_SYMBOL
-  fi
   CFLAGS="$TARGET_CFLAGS"
   grub_I386_ASM_PREFIX_REQUIREMENT
   grub_I386_ASM_ADDR32
diff --git a/kern/i386/pc/startup.S b/kern/i386/pc/startup.S
index 8e8b661..471a8d3 100644
--- a/kern/i386/pc/startup.S
+++ b/kern/i386/pc/startup.S
@@ -241,7 +241,7 @@ codestart:
 	addl	%ecx, %esi
 	addl	$_start, %esi
 	decl	%esi
-	movl	$END_SYMBOL, %edi
+	movl	$__bss_end__, %edi
 	addl	%ecx, %edi
 	decl	%edi
 	std
@@ -250,10 +250,10 @@ codestart:
 #endif
 	
 	/* clean out the bss */
-	movl	$BSS_START_SYMBOL, %edi
+	movl	$__bss_start__, %edi
 
 	/* compute the bss length */
-	movl	$END_SYMBOL, %ecx
+	movl	$__bss_end__, %ecx
 	subl	%edi, %ecx
 			
 	/* clean out */
@@ -285,7 +285,7 @@ VARIABLE(grub_start_addr)
 	.long	_start
 
 VARIABLE(grub_end_addr)
-	.long	END_SYMBOL
+	.long	__bss_end__
 	
 VARIABLE(grub_apm_bios_info)
 	.word	0	/* version */

Re: [PATCH] Use common linker script for all i386-pc systems

[Vladimir 'phcoder' Serbinenko]

Hello.

On Sun, May 17, 2009 at 7:33 AM, Pavel Roskin <proski@gnu.org> wrote:
> This allows us to remove checks for the linker symbols for the beginning
> and the end of the .bss section.  Instead, we use the names from the
> linker script.  Another benefit is a better unification of the build
> system.
>
I have nothing against this patch. However it modifies similar parts
of build system as my Apple's CC patch. Could we perhaps get my patch
in first? Also Apple's LD has no support for  linker scripts so
unification will not be complete.
> diff --git a/conf/i386-pc.lds b/conf/i386-pc.lds
> new file mode 100644
> index 0000000..a41cac7
> --- /dev/null
> +++ b/conf/i386-pc.lds
> @@ -0,0 +1,53 @@
> +/* Linker script to create grub .img files on Cygwin.  */
This comment is out of sync
>  VARIABLE(grub_end_addr)
> -       .long   END_SYMBOL
> +       .long   __bss_end__
This variable isn't used at all. In my Apple's CC patch I just remove it

-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Use common linker script for all i386-pc systems

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

Vladimir 'phcoder' Serbinenko wrote:
> Hello.
>
> On Sun, May 17, 2009 at 7:33 AM, Pavel Roskin <...> wrote:
>   
>> This allows us to remove checks for the linker symbols for the beginning
>> and the end of the .bss section.  Instead, we use the names from the
>> linker script.  Another benefit is a better unification of the build
>> system.
>>
>>     
>
>   

Good idea. Patch works on Cygwin. Does it work on Linux, FreeBSD, ... ?

The variable TARGET_IMG_LDFLAGS_AC can also be removed, see attached patch.

The linker script is actually identical to the script from my first 
Cygwin patch. It can be further simplified if desired.

>> @@ -0,0 +1,53 @@
>> +/* Linker script to create grub .img files on Cygwin.  */
>>     
> This comment is out of sync
>   
>>  VARIABLE(grub_end_addr)
>> -       .long   END_SYMBOL
>> +       .long   __bss_end__
>>     
> This variable isn't used at all. In my Apple's CC patch I just remove it
>
>   

'grub_start_addr' and '.globl start, _start' are also not used.


-- 
Christian Franke


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1277 bytes --]

diff --git a/configure.ac b/configure.ac
index a048828..e799a79 100644
--- a/configure.ac
+++ b/configure.ac
@@ -212,15 +212,13 @@ AC_MSG_CHECKING([for option to link raw image])
 if test -f "${srcdir}/conf/${target_cpu}-${platform}.lds"; then
   TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/conf/${target_cpu}-${platform}.lds"
   TARGET_IMG_LDFLAGS="-Wl,-T${TARGET_IMG_LDSCRIPT}"
-  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/conf/${target_cpu}-${platform}.lds"
 else
   TARGET_IMG_LDSCRIPT=
   TARGET_IMG_LDFLAGS='-Wl,-N'
-  TARGET_IMG_LDFLAGS_AC='-Wl,-N'
 fi
 AC_SUBST(TARGET_IMG_LDSCRIPT)
 AC_SUBST(TARGET_IMG_LDFLAGS)
-AC_MSG_RESULT([$TARGET_IMG_LDFLAGS_AC])
+AC_MSG_RESULT([$TARGET_IMG_LDFLAGS])
 
 # For platforms where ELF is not the default link format.
 AC_MSG_CHECKING([for command to convert module to ELF format])
@@ -379,11 +377,6 @@ grub_PROG_OBJCOPY_ABSOLUTE
 grub_PROG_LD_BUILD_ID_NONE
 grub_ASM_USCORE
 if test "x$target_cpu" = xi386; then
-  if test ! -z "$TARGET_IMG_LDSCRIPT"; then
-    # Check symbols provided by linker script.
-    CFLAGS="$TARGET_CFLAGS -nostdlib $TARGET_IMG_LDFLAGS_AC -Wl,-Ttext,8000,--defsym,___main=0x8100"
-  fi
-  CFLAGS="$TARGET_CFLAGS"
   grub_I386_ASM_PREFIX_REQUIREMENT
   grub_I386_ASM_ADDR32
   grub_I386_ASM_ABSOLUTE_WITHOUT_ASTERISK

Re: [PATCH] Use common linker script for all i386-pc systems

[Vladimir 'phcoder' Serbinenko]

On Mon, May 18, 2009 at 11:13 PM, Christian Franke
<Christian.Franke@t-online.de> wrote:
> Vladimir 'phcoder' Serbinenko wrote:
>>
>> Hello.
>>
>> On Sun, May 17, 2009 at 7:33 AM, Pavel Roskin <...> wrote:
>>
>>>
>>> This allows us to remove checks for the linker symbols for the beginning
>>> and the end of the .bss section.  Instead, we use the names from the
>>> linker script.  Another benefit is a better unification of the build
>>> system.
>>>
>>>
>>
>>
>
> Good idea. Patch works on Cygwin. Does it work on Linux, FreeBSD, ... ?
I will test it on Linux
>
> The variable TARGET_IMG_LDFLAGS_AC can also be removed, see attached patch.
>
> The linker script is actually identical to the script from my first Cygwin
> patch. It can be further simplified if desired.
>
Could you instead first have a look at my apple cc patch so we can
integrate it first instead of having conflicting patches
>
> 'grub_start_addr'
AFAIK this one can be removed
> and '.globl start, _start' are also not used.
Please keep them. They are required for at least some compilers to
work. If you look at my apple cc patch you'll see I had to add
start/_start in some files
>
>
> --
> Christian Franke
>
>
> diff --git a/configure.ac b/configure.ac
> index a048828..e799a79 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -212,15 +212,13 @@ AC_MSG_CHECKING([for option to link raw image])
>  if test -f "${srcdir}/conf/${target_cpu}-${platform}.lds"; then
>   TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/conf/${target_cpu}-${platform}.lds"
>   TARGET_IMG_LDFLAGS="-Wl,-T${TARGET_IMG_LDSCRIPT}"
> -
>  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/conf/${target_cpu}-${platform}.lds"
>  else
>   TARGET_IMG_LDSCRIPT=
>   TARGET_IMG_LDFLAGS='-Wl,-N'
> -  TARGET_IMG_LDFLAGS_AC='-Wl,-N'
>  fi
>  AC_SUBST(TARGET_IMG_LDSCRIPT)
>  AC_SUBST(TARGET_IMG_LDFLAGS)
> -AC_MSG_RESULT([$TARGET_IMG_LDFLAGS_AC])
> +AC_MSG_RESULT([$TARGET_IMG_LDFLAGS])
>
>  # For platforms where ELF is not the default link format.
>  AC_MSG_CHECKING([for command to convert module to ELF format])
> @@ -379,11 +377,6 @@ grub_PROG_OBJCOPY_ABSOLUTE
>  grub_PROG_LD_BUILD_ID_NONE
>  grub_ASM_USCORE
>  if test "x$target_cpu" = xi386; then
> -  if test ! -z "$TARGET_IMG_LDSCRIPT"; then
> -    # Check symbols provided by linker script.
> -    CFLAGS="$TARGET_CFLAGS -nostdlib $TARGET_IMG_LDFLAGS_AC
> -Wl,-Ttext,8000,--defsym,___main=0x8100"
> -  fi
> -  CFLAGS="$TARGET_CFLAGS"
>   grub_I386_ASM_PREFIX_REQUIREMENT
>   grub_I386_ASM_ADDR32
>   grub_I386_ASM_ABSOLUTE_WITHOUT_ASTERISK
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Use common linker script for all i386-pc systems

[Pavel Roskin]

Quoting Christian Franke <Christian.Franke@t-online.de>:

> Good idea. Patch works on Cygwin. Does it work on Linux, FreeBSD, ... ?

It works on Linux.  I assume it should work on every OS that has GNU ld.

> The variable TARGET_IMG_LDFLAGS_AC can also be removed, see attached patch.

I think we can do a big cleanup.  The start address could be specified  
in the linker script.  Some tests will become unnecessary.  We should  
be able to move a lot of code from the configure script into the  
makefiles.

>>> +/* Linker script to create grub .img files on Cygwin.  */
>>>
>> This comment is out of sync

I know.

>>> VARIABLE(grub_end_addr)
>>> -       .long   END_SYMBOL
>>> +       .long   __bss_end__
>>>
>> This variable isn't used at all. In my Apple's CC patch I just remove it

OK, let's remove it.

> 'grub_start_addr' and '.globl start, _start' are also not used.

Patches are welcome.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Use common linker script for all i386-pc systems

[Pavel Roskin]

Quoting Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>:

> Hello.
>
> On Sun, May 17, 2009 at 7:33 AM, Pavel Roskin <proski@gnu.org> wrote:
>> This allows us to remove checks for the linker symbols for the beginning
>> and the end of the .bss section.  Instead, we use the names from the
>> linker script.  Another benefit is a better unification of the build
>> system.
>>
> I have nothing against this patch. However it modifies similar parts
> of build system as my Apple's CC patch. Could we perhaps get my patch
> in first? Also Apple's LD has no support for  linker scripts so
> unification will not be complete.

I appreciate a lot of work you put into the Apple CC support, but your  
patch complicates the code a lot.  All features have a maintenance  
cost.  Somebody will need to fix your code when it breaks.   
Considering that it can only be tested on Darwin, I'm afraid it will  
be vulnerable to bitrot.

My patch simplifies things and allows further simplifications, some of  
which could be beneficial to your effort.

Perhaps it would be better if you split uncontroversial or simple  
parts of your patch and submit them.  Also, maybe it would be better  
to limit Apple support to the EFI platforms?  That would make your  
patch less intrusive.

>>  VARIABLE(grub_end_addr)
>> -       .long   END_SYMBOL
>> +       .long   __bss_end__
> This variable isn't used at all. In my Apple's CC patch I just remove it

That change doesn't need to be in the Apple patch.  Let's do the  
simplifications first.

I, for one, didn't push support for compiling GRUB for i386-pc on pure  
x86_64 Linux because it wasn't ready.  But I committed the simple  
parts that remove unnecessary tests and simplify the code.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Use common linker script for all i386-pc systems

[Pavel Roskin]

On Tue, 2009-05-19 at 17:15 -0400, Pavel Roskin wrote:

> I appreciate a lot of work you put into the Apple CC support, but your  
> patch complicates the code a lot.  All features have a maintenance  
> cost.  Somebody will need to fix your code when it breaks.   
> Considering that it can only be tested on Darwin, I'm afraid it will  
> be vulnerable to bitrot.
> 
> My patch simplifies things and allows further simplifications, some of  
> which could be beneficial to your effort.
> 
> Perhaps it would be better if you split uncontroversial or simple  
> parts of your patch and submit them.  Also, maybe it would be better  
> to limit Apple support to the EFI platforms?  That would make your  
> patch less intrusive.

The original of this message can be found here:
http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00238.html

There was no reply to that message.  It means that the Apple support was
committed despite my objections.  I wasn't the maintainer at the time,
but neither was Vladimir.  We should not do it again.

-- 
Regards,
Pavel Roskin