This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] ld: Define _edata, __bss_start, and _end only for executables


On Fri, Jun 01, 2018 at 11:45:32AM -0700, H.J. Lu wrote:
> On Wed, May 30, 2018 at 5:44 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Wed, May 30, 2018 at 02:59:39PM -0700, H.J. Lu wrote:
> >> _edata, __bss_start, and _end are defined for executables.  FreeBSD's
> >> libc.so uses executable's _end to initialize curbrk.  But there is no
> >> good reason to access values of _edata, __bss_start, and _end defined
> >> in shared libraries.  We should define _edata, __bss_start, and _end
> >> only for executables.
> >
> > I agree with the idea, but the patch isn't complete.  You'll need to
> > look at all the target OTHER_END_SYMBOLS, OTHER_BSS_SYMBOLS, and
> > OTHER_BSS_END_SYMBOLS.  For instance, elf32mep.sh defines __heap = _end
> > in OTHER_END_SYMBOLS, which is going to go wrong when _end isn't
> > defined in a shared library..  Ah, no, mep uses its own mep.sc so
> > won't be affected by your elf.sc patch, but I'm sure you can see the
> > potential problem in other targets.  So please do look for _edata,
> > edata, __bss_start, _end and end in emulparams/*.sh.  A quick look
> > says this patch will likely cause fails on some targets, eg. see
> > elf32epiphany.sh EXECUTABLE_SYMBOLS.
> >
> > If you're lucky you might be able to wrap the occurrences of
> > OTHER_END_SYMBOLS, OTHER_BSS_SYMBOLS, OTHER_BSS_END_SYMBOLS and
> > EXECUTABLE_SYMBOLS in elf.sc with ${CREATE_SHLIB- }.
> >
> > Also, do test this sort of patch on a large set of ELF targets before
> > posting.
> >
> 
> The new set of patches are posted at
> 
> https://sourceware.org/ml/binutils/2018-06/msg00021.html

Did you do the analysis of emulparams files to see whether this is OK?

I suspect you may have taken my "If you're lucky" comment as meaning
to try that out and if there are no testsuite fails then the simple
approach is OK.  I meant that you should look at target usage of shell
variables like EXECUTABLE_SYMBOLS to see whether wrapping in
${CREATE_SHLIB-} make sense, and at least report back to the binutils
list about places where you are unsure.

For EXECUTABLE_SYMBOLS, it's fairly obvious that your patch will break
elf64bmip.sh since it defines symbols inside ${CREATE_SHLIB+}.  That
emulparams file should probably be using OTHER_SYMBOLS instead.
elf32bmipn32.sh is also a worry, and likely should be using
OTHER_SYMBOLS with the same sort of expression as elf64bmip.sh but
adjusted for 32-bit header size.  I'm unsure about elf32b4300.sh but
my guess is it will be OK to only define _DYNAMIC_LINK for
executables.  This doesn't mean you need to fix these MIPS problems
yourself, just raise them with Maciej.  Of course, if you like,
provide a patch like the ones I've attached.

elf32_tic6x_le_sh has another odd use of EXECUTABLE_SYMBOLS,
presumably because R_C6000_SBR_* relocs have an implicit reference to
__c6xabi_DSBT_BASE.  I suspect those relocs can appear in shared
libraries.  So again something for the port maintainer, Joseph, to
comment on.  (The implicit reference would be better handled by
generating the reference explicitly on encountering these relocs in
check_relocs, I think, rather than just using OTHER_SYMBOLS.)

elf64hppa.sh might also need to define OTHER_SYMBOLS rather than
EXECUTABLE_SYMBOLS, if __SYSTEM_ID or _FPU_STATUS can be referenced
from shared libraries.  Dave?

-- 
Alan Modra
Australia Development Lab, IBM
>From 499f6d687e07cd066d8e7e8e08b1c417bf535a2d Mon Sep 17 00:00:00 2001
From: Alan Modra <amodra@gmail.com>
Date: Sat, 2 Jun 2018 18:05:24 +0930
Subject: TIC6X __c6xabi_DSBT_BASE

Adding an undefined __c6xabi_DSBT_BASE via an EXTERN in the linker
script isn't ideal, as the symbol is not always needed.  This patch
adds the undefined symbol on encountering relocations where it is
implicitly referenced.

bfd/
	* elf32-tic6x.c (elf32_tic6x_check_relocs): Reference
	__c6xabi_DSBT_BASE explicitly for R_C6000_SBR_* relocs.
ld/
	* emulparams/elf32_tic6x_le.sh (EXECUTABLE_SYMBOLS): Don't define.

diff --git a/bfd/elf32-tic6x.c b/bfd/elf32-tic6x.c
index 96965b3992..768bfb8ca9 100644
--- a/bfd/elf32-tic6x.c
+++ b/bfd/elf32-tic6x.c
@@ -2972,6 +2972,19 @@ elf32_tic6x_check_relocs (bfd *abfd, struct bfd_link_info *info,
 	case R_C6000_SBR_H16_B:
 	case R_C6000_SBR_H16_H:
 	case R_C6000_SBR_H16_W:
+	  {
+	    /* These relocations implicitly reference __c6xabi_DSBT_BASE.
+	       Add an explicit reference so that the symbol will be
+	       provided by a linker script.  */
+	    struct bfd_link_hash_entry *bh = NULL;
+	    if (!_bfd_generic_link_add_one_symbol (info, abfd,
+						   "__c6xabi_DSBT_BASE",
+						   BSF_GLOBAL,
+						   bfd_und_section_ptr, 0,
+						   NULL, FALSE, FALSE, &bh))
+	      return FALSE;
+	    ((struct elf_link_hash_entry *) bh)->non_elf = 0;
+	  }
 	  if (h != NULL && bfd_link_executable (info))
 	    {
 	      /* For B14-relative addresses, we might need a copy
diff --git a/ld/emulparams/elf32_tic6x_le.sh b/ld/emulparams/elf32_tic6x_le.sh
index efd7b24602..d9ebe584a3 100644
--- a/ld/emulparams/elf32_tic6x_le.sh
+++ b/ld/emulparams/elf32_tic6x_le.sh
@@ -22,7 +22,6 @@ case ${target} in
 esac
 MAXPAGESIZE="CONSTANT (MAXPAGESIZE)"
 ARCH=tic6x
-EXECUTABLE_SYMBOLS="EXTERN (__c6xabi_DSBT_BASE);"
 OTHER_GOT_SYMBOLS="PROVIDE_HIDDEN (__c6xabi_DSBT_BASE = .);"
 # ".bss" is near (small) BSS, ".far" is far (normal) BSS, ".const" is
 # far read-only data, ".rodata" is near read-only data.  ".neardata"
>From a8efb1094f33e2710ea9bd210f98081ab38d4270 Mon Sep 17 00:00:00 2001
From: Alan Modra <amodra@gmail.com>
Date: Sat, 2 Jun 2018 18:09:03 +0930
Subject: EXECUTABLE_SYMBOLS -> OTHER_SYMBOLS

EXECUTABLE_SYMBOLS is supposed to be true to its name, only defining
symbols for the executable.

	* emulparams/elf64bmip.sh (EXECUTABLE_SYMBOLS): Don't define.
	(OTHER_SYMBOLS): Define this instead.
	* emulparams/elf32bmipn32.sh (EXECUTABLE_SYMBOLS): Don't define.
	(OTHER_SYMBOLS): Define similarly to elf64bmip.sh.
	* emulparams/elf64hppa.sh (EXECUTABLE_SYMBOLS): Don't define.
	(OTHER_SYMBOLS): Define instead.

diff --git a/ld/emulparams/elf32bmipn32.sh b/ld/emulparams/elf32bmipn32.sh
index c26b6b3dcb..4ad6681055 100644
--- a/ld/emulparams/elf32bmipn32.sh
+++ b/ld/emulparams/elf32bmipn32.sh
@@ -6,10 +6,12 @@ SHLIB_TEXT_START_ADDR=0x5ffe0000
 COMMONPAGESIZE="CONSTANT (COMMONPAGESIZE)"
 
 # IRIX6 defines these symbols.  0x34 is the size of the ELF header.
-EXECUTABLE_SYMBOLS="
+OTHER_SYMBOLS="
   __dso_displacement = 0;
-  __elf_header = ${TEXT_START_ADDR};
-  __program_header_table = ${TEXT_START_ADDR} + 0x34;
+  ${CREATE_SHLIB-${CREATE_PIE-__elf_header = ${TEXT_START_ADDR};}}
+  ${CREATE_SHLIB+__elf_header = ${SHLIB_TEXT_START_ADDR};}
+  ${CREATE_PIE+__elf_header = ${SHLIB_TEXT_START_ADDR};}
+  __program_header_table = __elf_header + 0x34;
 "
 
 # There are often dynamic relocations against the .rodata section.
diff --git a/ld/emulparams/elf64bmip.sh b/ld/emulparams/elf64bmip.sh
index 0df65285f5..281f516916 100644
--- a/ld/emulparams/elf64bmip.sh
+++ b/ld/emulparams/elf64bmip.sh
@@ -5,7 +5,7 @@ LITTLE_OUTPUT_FORMAT="elf64-littlemips"
 SHLIB_TEXT_START_ADDR=0x3ffffe0000
 
 # IRIX6 defines these symbols.  0x40 is the size of the ELF header.
-EXECUTABLE_SYMBOLS="
+OTHER_SYMBOLS="
   __dso_displacement = 0;
   ${CREATE_SHLIB-${CREATE_PIE-__elf_header = ${TEXT_START_ADDR};}}
   ${CREATE_SHLIB+__elf_header = ${SHLIB_TEXT_START_ADDR};}
diff --git a/ld/emulparams/elf64hppa.sh b/ld/emulparams/elf64hppa.sh
index eeeadeafca..1ed32f9721 100644
--- a/ld/emulparams/elf64hppa.sh
+++ b/ld/emulparams/elf64hppa.sh
@@ -67,7 +67,7 @@ PLT_BEFORE_GOT=
 TEXT_DYNAMIC=
 
 # The linker is required to define these two symbols.
-EXECUTABLE_SYMBOLS='PROVIDE (__SYSTEM_ID = 0x214); PROVIDE (_FPU_STATUS = 0x0);'
+OTHER_SYMBOLS='PROVIDE (__SYSTEM_ID = 0x214); PROVIDE (_FPU_STATUS = 0x0);'
 # The PA64 ELF port needs two additional initializer sections and also wants
 # a start/end symbol pair for the .init and .fini sections.
 INIT_START='KEEP (*(.HP.init)); PROVIDE (__preinit_start = .); KEEP (*(.preinit)); PROVIDE (__preinit_end = .); PROVIDE (__init_start = .);'

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]