Discussion:
[PATCH v2 0/2] LD: SEGMENT_START expression handling fixes
Maciej W. Rozycki
2018-09-13 23:41:08 UTC
Permalink
Hi Alan,

I decided to complete this change after all. I split it into a small
patch series as an update you requested is logically separate. See
individual patch descriptions for further details.

As for historical reasons my personal development environment has quite
an old version of GCC testing these changes revealed a couple of build
issues across 3 targets. These turned out straightforward to fix, so I'll
be proposing them separately, as I believe we have no need to artificially
limit support for older compiler versions if that does not cause extra
maintenance burden, which is the case here.

With those fixes in place I saw no regressions with this patch series in
testing across my usual targets.

OK to apply?

Maciej
Maciej W. Rozycki
2018-09-13 23:41:15 UTC
Permalink
Avoid a division by zero and thus a linker crash in SEGMENT_START script
builtin function handling, by not checking the value supplied with a
`-T' command-line override against the maximum page size if that has not
been set.

ld/
* ldexp.c (fold_binary): Check that `config.maxpagesize' is
non-zero before using it as a divisor.
---
New in v2.
---
ld/ldexp.c | 1 +
1 file changed, 1 insertion(+)

binutils-ld-segment-start-maxpagesize.diff
Index: src/ld/ldexp.c
===================================================================
--- src.orig/ld/ldexp.c
+++ src/ld/ldexp.c
@@ -545,6 +545,7 @@ fold_binary (etree_type *tree)
{
if (!seg->used
&& config.magic_demand_paged
+ && config.maxpagesize != 0
&& (seg->value % config.maxpagesize) != 0)
einfo (_("%P: warning: address of `%s' "
"isn't multiple of maximum page size\n"),
Maciej W. Rozycki
2018-09-13 23:41:22 UTC
Permalink
From: Maciej W. Rozycki <***@mips.com>

Fix an issue with the SEGMENT_START builtin function where its result is
absolute when taken from the default supplied, and section-relative when
taken from a `-T' command-line override. This is against documentation,
inconsistent and unexpected, and with PIE executables gives an incorrect
result with the `__executable_start' symbol.

Make the result of SEGMENT_START always section-relative then.

ld/
* ldexp.c (fold_binary): Always make the result of SEGMENT_START
section-relative.
* testsuite/ld-scripts/segment-start.d: New test.
* testsuite/ld-scripts/segment-start.ld: New test linker script.
* testsuite/ld-scripts/segment-start.s: New test source.
* testsuite/ld-scripts/script.exp: Run the new test.
---
Hi Alan,
* ldexp.c (fold_binary): Always make the result of SEGMENT_START
section-relative.
The above is OK, but since you're changing this code it would be good
to avoid the divide by zero exposed by your override testcase on
non-ELF targets. Please add a "config.maxpagesize != 0" test before
we try to calculate "seg->value % config.maxpagesize".
Thanks for catching this, fix now sent as 1/2 in this series.
* testsuite/ld-scripts/segment-start.d: New test.
* testsuite/ld-scripts/segment-start.ld: New test linker script.
* testsuite/ld-scripts/segment-start.s: New test source.
* testsuite/ld-scripts/script.exp: Run the new test.
I'm inclined to think these tests should only be run on ELF targets.
Some formats don't support changing the text segment address.
With the division by zero fixed these tests work however across all but
just a bunch of targets. So why not have all the good targets covered?

I propose to have the few problematic targets XFAILed then, as in this
update. There are 6 such targets only really, the remaining ones are
aliases.

Maciej

Changes from v1:

- XFAIL targets that are not expected to handle SEGMENT_START correctly.
---
ld/ldexp.c | 4 +++-
ld/testsuite/ld-scripts/script.exp | 4 ++++
ld/testsuite/ld-scripts/segment-start.d | 18 ++++++++++++++++++
ld/testsuite/ld-scripts/segment-start.ld | 12 ++++++++++++
ld/testsuite/ld-scripts/segment-start.s | 2 ++
5 files changed, 39 insertions(+), 1 deletion(-)

binutils-ld-segment-start-default-rel.diff
Index: src/ld/ldexp.c
===================================================================
--- src.orig/ld/ldexp.c
+++ src/ld/ldexp.c
@@ -534,6 +534,7 @@ fold_binary (etree_type *tree)
operand, binary.rhs is first operand. */
if (expld.result.valid_p && tree->type.node_code == SEGMENT_START)
{
+ bfd_vma value = expld.result.value;
const char *segment_name;
segment_type *seg;

@@ -551,9 +552,10 @@ fold_binary (etree_type *tree)
"isn't multiple of maximum page size\n"),
segment_name);
seg->used = TRUE;
- new_rel_from_abs (seg->value);
+ value = seg->value;
break;
}
+ new_rel_from_abs (value);
return;
}

Index: src/ld/testsuite/ld-scripts/script.exp
===================================================================
--- src.orig/ld/testsuite/ld-scripts/script.exp
+++ src/ld/testsuite/ld-scripts/script.exp
@@ -231,3 +231,7 @@ foreach test_script $test_script_list {

run_dump_test "align-with-input"
run_dump_test "pr20302"
+
+run_dump_test "segment-start" {{name (default)}}
+run_dump_test "segment-start" {{name (overridden)} \
+ {ld -Ttext-segment=0x10000000}}
Index: src/ld/testsuite/ld-scripts/segment-start.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-scripts/segment-start.d
@@ -0,0 +1,19 @@
+#PROG: nm
+#name: SEGMENT_START expression not absolute
+#source: segment-start.s
+#ld: -e 0 -u __executable_start -T segment-start.ld
+#xfail: mmix-*-* pdp11-*-* powerpc-*-aix* powerpc-*-beos* rs6000-*-* sh-*-pe
+#xfail: c30-*-*aout* tic30-*-*aout* c54x*-*-*coff* tic54x-*-*coff*
+# XFAIL targets that are not expected to handle SEGMENT_START correctly.
+
+# Make sure `__executable_start' is regular:
+#
+# 10000000 T __executable_start
+#
+# not absolute:
+#
+# 10000000 A __executable_start
+
+#...
+0*10000000 T __executable_start
+#pass
Index: src/ld/testsuite/ld-scripts/segment-start.ld
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-scripts/segment-start.ld
@@ -0,0 +1,12 @@
+SECTIONS
+{
+ PROVIDE (__executable_start = SEGMENT_START ("text-segment", 0x10000000));
+ .text : { *(.text) }
+ .data : { *(.data) }
+ .bss : { *(.bss) }
+ .loader : { *(.loader) }
+ .symtab : { *(.symtab) }
+ .strtab : { *(.strtab) }
+ .shstrtab : { *(.shstrtab) }
+ /DISCARD/ : { *(*) }
+}
Index: src/ld/testsuite/ld-scripts/segment-start.s
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-scripts/segment-start.s
@@ -0,0 +1,2 @@
+ .text
+ .space 16
Alan Modra
2018-09-14 00:02:33 UTC
Permalink
Post by Maciej W. Rozycki
Hi Alan,
I decided to complete this change after all. I split it into a small
patch series as an update you requested is logically separate. See
individual patch descriptions for further details.
As for historical reasons my personal development environment has quite
an old version of GCC testing these changes revealed a couple of build
issues across 3 targets. These turned out straightforward to fix, so I'll
be proposing them separately, as I believe we have no need to artificially
limit support for older compiler versions if that does not cause extra
maintenance burden, which is the case here.
With those fixes in place I saw no regressions with this patch series in
testing across my usual targets.
OK to apply?
OK, thanks!
--
Alan Modra
Australia Development Lab, IBM
Maciej W. Rozycki
2018-09-14 19:26:30 UTC
Permalink
Post by Alan Modra
OK, thanks!
Committed now, thanks for your review.

Maciej

Loading...