This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Always define referenced __start_SECNAME/__stop_SECNAME
On Tue, Jun 13, 2017 at 12:07 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jun 13, 2017 at 8:22 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Jun 13, 2017 at 7:28 AM, Alan Modra <amodra@gmail.com> wrote:
>>> On Tue, Jun 13, 2017 at 05:38:45AM -0700, H.J. Lu wrote:
>>>> On Tue, Jun 13, 2017 at 5:26 AM, Alan Modra <amodra@gmail.com> wrote:
>>>> > On Mon, Jun 12, 2017 at 07:10:47PM -0700, H.J. Lu wrote:
>>>> >> On Mon, Jun 12, 2017 at 6:54 PM, Alan Modra <amodra@gmail.com> wrote:
>>>> >> > On Mon, Jun 12, 2017 at 05:18:58PM -0700, H.J. Lu wrote:
>>>> >> >> On Mon, Jun 12, 2017 at 4:32 PM, Alan Modra <amodra@gmail.com> wrote:
>>>> >> >> > On Sat, Jun 10, 2017 at 03:46:49PM -0700, H.J. Lu wrote:
>>>> >> >> >> This patch changes linker to always define referenced __start_SECNAME and
>>>> >> >> >> __stop_SECNAME if the input section name is the same output section name,
>>>> >> >> >> which is always true for orphaned sections, and SECNAME is a C identifier.
>>>> >> >> >
>>>> >> >> > I think this change is reasonable.
>>>> >> >> >
>>>> >> >> >> Also __start_SECNAME and __stop_SECNAME symbols are marked as hidden by
>>>> >> >> >> ELF linker.
>>>> >> >> >
>>>> >> >> > Why is this necessary? Also, you make another change in behaviour
>>>> >> >>
>>>> >> >> It is to make sure that __start_SECNAME and __stop_SECNAME
>>>> >> >> symbols for section SECNAME in different modules are unique.
>>>> >> >
>>>> >> > I understand, and it would have been good if these symbols were
>>>> >> > defined that way in the beginning, but they weren't. I'm concerned
>>>> >> > that if you make this change as well we will find some code that
>>>> >> > relies on the symbols being dynamic.
>>>> >>
>>>> >> My change fixes a real bug:
>>>> >>
>>>> >> https://sourceware.org/bugzilla/show_bug.cgi?id=20022
>>>> >
>>>> > Really, your change is a workaround not a fix. You avoid exporting
>>>> > __start or __stop symbols because doing so shows a problem with gc. A
>>>> > real fix for the PR would make ld do the right thing even in the
>>>> > presense of exported dynamic symbols.
>>>>
>>>> Exporting __start_SECNAME and __stop_SECNAME as dynamic
>>>> symbols may lead to unexpected behavior. Reference to __start_SECNAME
>>>> and __stop_SECNAME to section SECNAME within a DSO will be
>>>> resolved to __start_SECNAME and __stop_SECNAME in another DSO
>>>> or executable. In most cases, it isn't the intended behavior and there are
>>>> workaround if it is needed.
>>>
>>> You're correct regarding shared libraries. I was thinking more about
>>> an executable when I said the patch was a workaround. OK, so I
>>> suppose we should leave that in, and wait to see if we get complaints.
>>>
>>>> >> We can suggest workarounds if some codes depend on that.
>>>> >>
>>>> >> >>
>>>> >> >> > that you don't mention: __start and __stop symbols were previously
>>>> >> >> > defined by ld -Ur, not just at final link. You'll need to modify
>>>> >> >>
>>>> >> >> Is there a testcase for this behavior?
>>>> >> >
>>>> >> > Not that I'm aware of.
>>>> >> >
>>>> >>
>>>> >> >From ld manual:
>>>> >>
>>>> >> '-Ur'
>>>> >> For anything other than C++ programs, this option is equivalent to
>>>> >> '-r': it generates relocatable output--i.e., an output file that
>>>> >> can in turn serve as input to 'ld'. When linking C++ programs,
>>>> >> '-Ur' _does_ resolve references to constructors, unlike '-r'. It
>>>> >> does not work to use '-Ur' on files that were themselves linked
>>>> >> with '-Ur'; once the constructor table has been built, it cannot be
>>>> >> added to. Use '-Ur' only for the last partial link, and '-r' for
>>>> >> the others.
>>>> >>
>>>> >> It isn't intended for __start_SECNAME and __stop_SECNAME. Check
>>>> >> config.build_constructors for __start_SECNAME and __stop_SECNAME
>>>> >> was introduced by:
>>>> >>
>>>> >> commit 71bfc0aef6964c54b8e29466e97fb246cdeb2049
>>>> >> Author: Alan Modra <amodra@gmail.com>
>>>> >> Date: Thu Sep 7 07:08:58 2000 +0000
>>>> >>
>>>> >> Fix list handling for orphan section output statements.
>>>> >
>>>> > That's a long time ago. Prior to that patch, ld defined __start and
>>>> > __stop symbols for ld -r, which can lead to unexpected results for the
>>>> > unwary. I can't remember, but I suspect I changed ld to define the
>>>> > symbols for -Ur so that people who wanted the previous ld -r behaviour
>>>> > for __start and __stop could get that by using ld -Ur.
>>>> >
>>>>
>>>> If it is the case, it isn't intended usage for ld -Ur and people shouldn't
>>>> depend on it.
>>>
>>> Please leave that behaviour unchanged.
>>>
>>
>> I am checking this patch to verify it and I will update my patch.
>>
>>
>
> I checked in this patch to add some tests for .startof.SECNAME
> and sizeof.SECNAME.
>
Don't define __start_SECNAME/__stop_SECNAME for -r.
--
H.J.
From b27685f2016c510d03ac9a64f7b04ce8efcf95c4 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 13 Jun 2017 13:04:56 -0700
Subject: [PATCH] ld: Don't define __start_SECNAME/__stop_SECNAME for -r
__start_SECNAME and __stop_SECNAME shouldn't be defined for "ld -r".
* ldlang.c (lang_set_startof): Skip if config.build_constructors
is FALSE.
* testsuite/ld-elf/sizeofc.d: New file.
* testsuite/ld-elf/startofc.d: Likewise.
---
ld/ChangeLog | 7 +++++++
ld/ldlang.c | 14 ++++++++++----
ld/testsuite/ld-elf/sizeofc.d | 12 ++++++++++++
ld/testsuite/ld-elf/startofc.d | 12 ++++++++++++
4 files changed, 41 insertions(+), 4 deletions(-)
create mode 100644 ld/testsuite/ld-elf/sizeofc.d
create mode 100644 ld/testsuite/ld-elf/startofc.d
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 59c99ae..7995de8 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,5 +1,12 @@
2017-06-13 H.J. Lu <hongjiu.lu@intel.com>
+ * ldlang.c (lang_set_startof): Skip if config.build_constructors
+ is FALSE.
+ * testsuite/ld-elf/sizeofc.d: New file.
+ * testsuite/ld-elf/startofc.d: Likewise.
+
+2017-06-13 H.J. Lu <hongjiu.lu@intel.com>
+
* testsuite/ld-elf/sizeof.d: Renamed to ...
* testsuite/ld-elf/sizeofa.d: This. Updated.
* testsuite/ld-elf/startof.d: Renamed to ...
diff --git a/ld/ldlang.c b/ld/ldlang.c
index f8e4f52..eb4ba9e 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -5892,9 +5892,15 @@ lang_set_startof (void)
{
asection *s;
char leading_char;
- bfd_boolean is_elf = (bfd_get_flavour (link_info.output_bfd)
- == bfd_target_elf_flavour);
- bfd_boolean is_elocatable = bfd_link_relocatable (&link_info);
+ bfd_boolean is_elf;
+ bfd_boolean is_relocatable;
+
+ if (!config.build_constructors)
+ return;
+
+ is_elf = (bfd_get_flavour (link_info.output_bfd)
+ == bfd_target_elf_flavour);
+ is_relocatable = bfd_link_relocatable (&link_info);
leading_char = bfd_get_symbol_leading_char (link_info.output_bfd);
@@ -5907,7 +5913,7 @@ lang_set_startof (void)
secname = bfd_get_section_name (link_info.output_bfd, s);
buf = (char *) xmalloc (10 + strlen (secname));
- if (!is_elocatable)
+ if (!is_relocatable)
{
sprintf (buf, ".startof.%s", secname);
h = bfd_link_hash_lookup (link_info.hash, buf, FALSE, FALSE,
diff --git a/ld/testsuite/ld-elf/sizeofc.d b/ld/testsuite/ld-elf/sizeofc.d
new file mode 100644
index 0000000..1cff854
--- /dev/null
+++ b/ld/testsuite/ld-elf/sizeofc.d
@@ -0,0 +1,12 @@
+#source: sizeof.s
+#ld: -r
+#readelf: -sW
+
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+ +0: 0+ +0 +NOTYPE +LOCAL +DEFAULT +UND +
+#...
+ +[0-9]+: +[a-f0-9]+ +0 +NOTYPE +GLOBAL +DEFAULT +UND +__stop_scnfoo
+#...
+ +[0-9]+: +[a-f0-9]+ +0 +NOTYPE +GLOBAL +DEFAULT +UND +.sizeof.scnfoo
+#pass
diff --git a/ld/testsuite/ld-elf/startofc.d b/ld/testsuite/ld-elf/startofc.d
new file mode 100644
index 0000000..4005625
--- /dev/null
+++ b/ld/testsuite/ld-elf/startofc.d
@@ -0,0 +1,12 @@
+#source: startof.s
+#ld: -r
+#readelf: -sW
+
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+ +0: 0+ +0 +NOTYPE +LOCAL +DEFAULT +UND +
+#...
+ +[0-9]+: +[a-f0-9]+ +0 +NOTYPE +GLOBAL +DEFAULT +UND +.startof.scnfoo
+#...
+ +[0-9]+: +[a-f0-9]+ +0 +NOTYPE +GLOBAL +DEFAULT +UND +__start_scnfoo
+#pass
--
2.9.4