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] Always define referenced __start_SECNAME/__stop_SECNAME


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.


-- 
H.J.
From b04772be9a254f6bcf0dab491dd0272ae4b4f6ee Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 13 Jun 2017 08:18:19 -0700
Subject: [PATCH] Add tests for -Ur

Test -Ur with __start_SECNAME, __stop_SECNAME, .startof.SECNAME and
.sizeof.SECNAME.  __start_SECNAME and __stop_SECNAME should be defined
to the start and the end of section SECNAME.  .startof.SECNAME and
.sizeof.SECNAME should be undefined.

	* testsuite/ld-elf/sizeof.d: New file.
	* testsuite/ld-elf/sizeof.s: Likewise.
	* testsuite/ld-elf/startof.d: Likewise.
	* testsuite/ld-elf/startof.s: Likewise.
---
 ld/ChangeLog                  |  7 +++++++
 ld/testsuite/ld-elf/sizeof.d  | 11 +++++++++++
 ld/testsuite/ld-elf/sizeof.s  | 11 +++++++++++
 ld/testsuite/ld-elf/startof.d | 11 +++++++++++
 ld/testsuite/ld-elf/startof.s | 11 +++++++++++
 5 files changed, 51 insertions(+)
 create mode 100644 ld/testsuite/ld-elf/sizeof.d
 create mode 100644 ld/testsuite/ld-elf/sizeof.s
 create mode 100644 ld/testsuite/ld-elf/startof.d
 create mode 100644 ld/testsuite/ld-elf/startof.s

diff --git a/ld/ChangeLog b/ld/ChangeLog
index 1c81211..309bb3a 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,10 @@
+2017-06-13  H.J. Lu  <hongjiu.lu@intel.com>
+
+	* testsuite/ld-elf/sizeof.d: New file.
+	* testsuite/ld-elf/sizeof.s: Likewise.
+	* testsuite/ld-elf/startof.d: Likewise.
+	* testsuite/ld-elf/startof.s: Likewise.
+
 2017-06-13  Renlin Li  <renlin.li@arm.com>
 
 	* testsuite/ld-elf/shared.exp (build_tests): Add --no-dynamic-linker
diff --git a/ld/testsuite/ld-elf/sizeof.d b/ld/testsuite/ld-elf/sizeof.d
new file mode 100644
index 0000000..7cad75a
--- /dev/null
+++ b/ld/testsuite/ld-elf/sizeof.d
@@ -0,0 +1,11 @@
+#ld: -Ur
+#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 +[0-9]+ +__stop_scnfoo
+#...
+ +[0-9]+: +[a-f0-9]+ +0 +NOTYPE +GLOBAL +DEFAULT +UND +.sizeof.scnfoo
+#pass
diff --git a/ld/testsuite/ld-elf/sizeof.s b/ld/testsuite/ld-elf/sizeof.s
new file mode 100644
index 0000000..016ac60
--- /dev/null
+++ b/ld/testsuite/ld-elf/sizeof.s
@@ -0,0 +1,11 @@
+        .section        scnfoo,"aw",%progbits
+        .zero 0x10
+
+        .globl  bar
+        .data
+        .align 8
+        .type   bar, %object
+        .size   bar, 8
+bar:
+        .dc.a   __stop_scnfoo
+        .dc.a  .sizeof. (scnfoo)
diff --git a/ld/testsuite/ld-elf/startof.d b/ld/testsuite/ld-elf/startof.d
new file mode 100644
index 0000000..cf1f064
--- /dev/null
+++ b/ld/testsuite/ld-elf/startof.d
@@ -0,0 +1,11 @@
+#ld: -Ur
+#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 +[0-9]+ +__start_scnfoo
+#pass
diff --git a/ld/testsuite/ld-elf/startof.s b/ld/testsuite/ld-elf/startof.s
new file mode 100644
index 0000000..982f347
--- /dev/null
+++ b/ld/testsuite/ld-elf/startof.s
@@ -0,0 +1,11 @@
+        .section        scnfoo,"aw",%progbits
+        .zero 0x10
+
+        .globl  bar
+        .data
+        .align 8
+        .type   bar, %object
+        .size   bar, 8
+bar:
+        .dc.a   __start_scnfoo
+        .dc.a  .startof. (scnfoo)
-- 
2.9.4


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