[PATCH] Ignore dynamic references on forced local symbols

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Ignore dynamic references on forced local symbols

H.J. Lu
We should ignore dynamic references on forced local symbols during
garbage collection since they can never be referenced dynamically.

OK for master?

H.J.
---
bfd/

        PR ld/22649
        * elflink.c (bfd_elf_gc_mark_dynamic_ref_symbol): Ignore dynamic
        references on forced local symbols.

ld/

        PR ld/22649
        * testsuite/ld-elf/pr22649-1.s: New file.
        * testsuite/ld-elf/pr22649-2a.s: Likewise.
        * testsuite/ld-elf/pr22649-2b.s: Likewise.
        * testsuite/ld-elf/pr22649.msg: Likewise.
        * testsuite/ld-elf/shared.exp: Run ld/22649 tests.
---
 bfd/elflink.c                    |  2 +-
 ld/testsuite/ld-elf/pr22649-1.s  |  4 ++++
 ld/testsuite/ld-elf/pr22649-2a.s |  8 +++++++
 ld/testsuite/ld-elf/pr22649-2b.s |  7 +++++++
 ld/testsuite/ld-elf/pr22649.msg  |  1 +
 ld/testsuite/ld-elf/shared.exp   | 45 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-elf/pr22649-1.s
 create mode 100644 ld/testsuite/ld-elf/pr22649-2a.s
 create mode 100644 ld/testsuite/ld-elf/pr22649-2b.s
 create mode 100644 ld/testsuite/ld-elf/pr22649.msg

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 4c92a048ce..e3751fa122 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -13277,7 +13277,7 @@ bfd_elf_gc_mark_dynamic_ref_symbol (struct elf_link_hash_entry *h, void *inf)
 
   if ((h->root.type == bfd_link_hash_defined
        || h->root.type == bfd_link_hash_defweak)
-      && (h->ref_dynamic
+      && ((h->ref_dynamic && !h->forced_local)
   || ((h->def_regular || ELF_COMMON_DEF_P (h))
       && ELF_ST_VISIBILITY (h->other) != STV_INTERNAL
       && ELF_ST_VISIBILITY (h->other) != STV_HIDDEN
diff --git a/ld/testsuite/ld-elf/pr22649-1.s b/ld/testsuite/ld-elf/pr22649-1.s
new file mode 100644
index 0000000000..9a7da7e919
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22649-1.s
@@ -0,0 +1,4 @@
+ .data
+ .globl foo
+foo:
+ .dc.a bar
diff --git a/ld/testsuite/ld-elf/pr22649-2a.s b/ld/testsuite/ld-elf/pr22649-2a.s
new file mode 100644
index 0000000000..b5908b061c
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22649-2a.s
@@ -0,0 +1,8 @@
+ .data
+ .hidden foo
+ .hidden bar
+ .globl foo
+ .globl bar
+foo:
+bar:
+ .dc.a foo
diff --git a/ld/testsuite/ld-elf/pr22649-2b.s b/ld/testsuite/ld-elf/pr22649-2b.s
new file mode 100644
index 0000000000..999686cb65
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22649-2b.s
@@ -0,0 +1,7 @@
+ .data
+ .hidden foo
+ .globl foo
+ .globl bar
+foo:
+bar:
+ .dc.a foo
diff --git a/ld/testsuite/ld-elf/pr22649.msg b/ld/testsuite/ld-elf/pr22649.msg
new file mode 100644
index 0000000000..b4e45bd662
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22649.msg
@@ -0,0 +1 @@
+.*: Removing unused section '\.data' in file 'tmpdir/pr22649-2.*\.o'
diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
index 0c54568879..95bac37900 100644
--- a/ld/testsuite/ld-elf/shared.exp
+++ b/ld/testsuite/ld-elf/shared.exp
@@ -81,6 +81,51 @@ run_ld_link_tests [list \
  {} \
  "pr22471" \
     ] \
+    [list \
+ "Build pr22649-1.so" \
+ "$LFLAGS -shared" \
+ "" \
+ "$AFLAGS_PIC" \
+ {pr22649-1.s} \
+ {} \
+ "pr22649-1.so" \
+    ] \
+    [list \
+ "Build pr22649-2a.so" \
+ "$LFLAGS -shared -gc-sections -print-gc-sections" \
+ "" \
+ "$AFLAGS_PIC" \
+ {pr22649-2a.s} \
+ {{ld pr22649.msg}} \
+ "pr22649-2a.so" \
+    ] \
+    [list \
+ "Build pr22649-2b.so" \
+ "$LFLAGS -shared -gc-sections -print-gc-sections" \
+ "tmpdir/pr22649-1.so" \
+ "$AFLAGS_PIC" \
+ {pr22649-2a.s} \
+ {{ld pr22649.msg}} \
+ "pr22649-2b.so" \
+    ] \
+    [list \
+ "Build pr22649-2c.so" \
+ "$LFLAGS -shared -gc-sections -print-gc-sections" \
+ "" \
+ "$AFLAGS_PIC" \
+ {pr22649-2b.s} \
+ {} \
+ "pr22649-2b.so" \
+    ] \
+    [list \
+ "Build pr22649-2d.so" \
+ "$LFLAGS -shared -gc-sections -print-gc-sections" \
+ "tmpdir/pr22649-1.so" \
+ "$AFLAGS_PIC" \
+ {pr22649-2b.s} \
+ {} \
+ "pr22649-2b.so" \
+    ] \
 ]
 
 run_ld_link_tests [list \
--
2.14.3

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Ignore dynamic references on forced local symbols

Alan Modra-3
On Thu, Jan 11, 2018 at 01:31:42PM -0800, H.J. Lu wrote:

> bfd/
>
> PR ld/22649
> * elflink.c (bfd_elf_gc_mark_dynamic_ref_symbol): Ignore dynamic
> references on forced local symbols.
>
> ld/
>
> PR ld/22649
> * testsuite/ld-elf/pr22649-1.s: New file.
> * testsuite/ld-elf/pr22649-2a.s: Likewise.
> * testsuite/ld-elf/pr22649-2b.s: Likewise.
> * testsuite/ld-elf/pr22649.msg: Likewise.
> * testsuite/ld-elf/shared.exp: Run ld/22649 tests.

OK.

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Ignore dynamic references on forced local symbols

Christophe Lyon-2
Hi,

On 12 January 2018 at 00:30, Alan Modra <[hidden email]> wrote:

> On Thu, Jan 11, 2018 at 01:31:42PM -0800, H.J. Lu wrote:
>> bfd/
>>
>>       PR ld/22649
>>       * elflink.c (bfd_elf_gc_mark_dynamic_ref_symbol): Ignore dynamic
>>       references on forced local symbols.
>>
>> ld/
>>
>>       PR ld/22649
>>       * testsuite/ld-elf/pr22649-1.s: New file.
>>       * testsuite/ld-elf/pr22649-2a.s: Likewise.
>>       * testsuite/ld-elf/pr22649-2b.s: Likewise.
>>       * testsuite/ld-elf/pr22649.msg: Likewise.
>>       * testsuite/ld-elf/shared.exp: Run ld/22649 tests.
>
> OK.
>

I've noticed that 2 of the new tests fail:
FAIL:    ld:Build pr22649-2a.so
FAIL:    ld:Build pr22649-2b.so

on arm*linux* and arm-none-nacl targets

Can you check?

Thanks,

Christophe

> --
> Alan Modra
> Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Ignore dynamic references on forced local symbols

Alan Modra-3
On Fri, Jan 12, 2018 at 09:32:44AM +0100, Christophe Lyon wrote:

> Hi,
>
> On 12 January 2018 at 00:30, Alan Modra <[hidden email]> wrote:
> > On Thu, Jan 11, 2018 at 01:31:42PM -0800, H.J. Lu wrote:
> >> bfd/
> >>
> >>       PR ld/22649
> >>       * elflink.c (bfd_elf_gc_mark_dynamic_ref_symbol): Ignore dynamic
> >>       references on forced local symbols.
> >>
> >> ld/
> >>
> >>       PR ld/22649
> >>       * testsuite/ld-elf/pr22649-1.s: New file.
> >>       * testsuite/ld-elf/pr22649-2a.s: Likewise.
> >>       * testsuite/ld-elf/pr22649-2b.s: Likewise.
> >>       * testsuite/ld-elf/pr22649.msg: Likewise.
> >>       * testsuite/ld-elf/shared.exp: Run ld/22649 tests.
> >
> > OK.
> >
>
> I've noticed that 2 of the new tests fail:
> FAIL:    ld:Build pr22649-2a.so
> FAIL:    ld:Build pr22649-2b.so
>
> on arm*linux* and arm-none-nacl targets
>
> Can you check?

I have a patch to fix this under test at the moment.

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Ignore dynamic references on forced local symbols

Maciej W. Rozycki-2
H.J.,

On Fri, 12 Jan 2018, Alan Modra wrote:

> > >>       PR ld/22649
> > >>       * testsuite/ld-elf/pr22649-1.s: New file.
> > >>       * testsuite/ld-elf/pr22649-2a.s: Likewise.
> > >>       * testsuite/ld-elf/pr22649-2b.s: Likewise.
> > >>       * testsuite/ld-elf/pr22649.msg: Likewise.
> > >>       * testsuite/ld-elf/shared.exp: Run ld/22649 tests.
> > >
> > > OK.
> > >
> >
> > I've noticed that 2 of the new tests fail:
> > FAIL:    ld:Build pr22649-2a.so
> > FAIL:    ld:Build pr22649-2b.so
> >
> > on arm*linux* and arm-none-nacl targets
> >
> > Can you check?
>
> I have a patch to fix this under test at the moment.

 This is still broken for several MIPS targets:

.../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
.../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
FAIL: Build pr22649-2c.so

and:

.../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
.../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
FAIL: Build pr22649-2d.so

Unfortunately the change description is terse enough I cannot figure out
how to address this properly.  E.g. is it OK to have:

#pass

as expected output from LD (and possibly verify afterwards with `objdump'
or `readelf' that the section in question has been retained)?  Or can you
make the test cases use a proper linker script selecting input and output
concerned and letting target maintainers define target-specific data to
/DISCARD/?  Or finally, should the test cases perhaps just use:

#failif
...

on the unwanted diagnostic output reporting section removal as noted in
the PR?

 Would you please run proper cross-target testing with your generic
changes you commit?  It doesn't require much effort, just some machine
processing time and it would let you avoid introducing regressions or new
failures due to mistakes in test cases.  Also I have asked you to do that
before already I believe.

  Maciej
Reply | Threaded
Open this post in threaded view
|

[committed] MIPS/LD/testsuite: Correct PR ld/22649 test case failures

Maciej W. Rozycki-2
Fix commit d664fd41e15f ("Ignore dynamic references on forced local
symbols") and use alternative test actions and match patterns to
correctly handle messages like:

.../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'

or:

.../ld/ld-new: Removing unused section '.MIPS.options' in file 'tmpdir/pr22649-2b.o'

produced by LD on MIPS targets, removing:

FAIL: Build pr22649-2c.so
FAIL: Build pr22649-2d.so

test suite failures and tightening checks made with `pr22649-2a.so' and
`pr22649-2b.so' test cases.

Keep the original empty action with `pr22649-2c.so' and `pr22649-2d.so'
links and MIPS/ELF targets though, because for them the linker does not
garbage-collect the `.reginfo' section.  This is because the section has
its flags set differently by code in GAS in `md_begin':

    if (strncmp (TARGET_OS, "elf", 3) != 0)
      flags |= SEC_ALLOC | SEC_LOAD;

and consequently BFD linker code in `_bfd_elf_gc_mark_extra_sections':

          else if (((isec->flags & SEC_DEBUGGING) != 0
                    || (isec->flags & (SEC_ALLOC | SEC_LOAD | SEC_RELOC)) == 0)
                   && elf_next_in_group (isec) == NULL)
            isec->gc_mark = 1;

marks these sections to be kept due to their SEC_ALLOC, SEC_LOAD and
SEC_RELOC flags all being zero (`.reginfo' sections never have
relocations attached).

        ld/
        PR ld/22649
        * testsuite/ld-elf/pr22649-2ab-mips.msg: New stderr output.
        * testsuite/ld-elf/pr22649-2cd-mips.msg: New stderr output.
        * testsuite/ld-elf/shared.exp: Use the new outputs with
        `mips*-*-*' targets.
---
On Mon, 22 Jan 2018, Maciej W. Rozycki wrote:

> > > >>       PR ld/22649
> > > >>       * testsuite/ld-elf/pr22649-1.s: New file.
> > > >>       * testsuite/ld-elf/pr22649-2a.s: Likewise.
> > > >>       * testsuite/ld-elf/pr22649-2b.s: Likewise.
> > > >>       * testsuite/ld-elf/pr22649.msg: Likewise.
> > > >>       * testsuite/ld-elf/shared.exp: Run ld/22649 tests.
> > > >
> > > > OK.
> > > >
> > >
> > > I've noticed that 2 of the new tests fail:
> > > FAIL:    ld:Build pr22649-2a.so
> > > FAIL:    ld:Build pr22649-2b.so
> > >
> > > on arm*linux* and arm-none-nacl targets
> > >
> > > Can you check?
> >
> > I have a patch to fix this under test at the moment.
>
>  This is still broken for several MIPS targets:
>
> .../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
> .../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
> FAIL: Build pr22649-2c.so
>
> and:
>
> .../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
> .../ld/ld-new: Removing unused section '.reginfo' in file 'tmpdir/pr22649-2b.o'
> FAIL: Build pr22649-2d.so

 Nobody bothered to respond, so I have committed this update, as obvious,
correcting all `mips*-*-*' target failures caused by the original commit,
with the exception of:

mips-sgi-irix5  +FAIL: Build pr22649-2a.so

which is legitimate, due to:

.../ld/ld-new: tmpdir/pr22649-2a.so: protected symbol `_procedure_table_size' isn't defined
.../ld/ld-new: final link failed: Bad value

(and was previously masked by the wildcard match in the generic expected
pattern), which I'll investigate later (the same problem triggers across
other test cases and MIPS targets as well, so it's not specific to this
single link or target; probably a linker script issue).

  Maciej

---
 ld/testsuite/ld-elf/pr22649-2ab-mips.msg |    2 ++
 ld/testsuite/ld-elf/pr22649-2cd-mips.msg |    1 +
 ld/testsuite/ld-elf/shared.exp           |   20 ++++++++++++++++----
 3 files changed, 19 insertions(+), 4 deletions(-)

binutils-mips-ld-test-pr22649.diff
Index: binutils/ld/testsuite/ld-elf/pr22649-2ab-mips.msg
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr22649-2ab-mips.msg 2018-01-26 03:09:44.936458908 +0000
@@ -0,0 +1,2 @@
+.*: Removing unused section '\.data' in file 'tmpdir/pr22649-2.\.o'
+.*: Removing unused section '\.(?:reginfo|MIPS\.options)' in file 'tmpdir/pr22649-2.\.o'
Index: binutils/ld/testsuite/ld-elf/pr22649-2cd-mips.msg
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr22649-2cd-mips.msg 2018-01-26 03:09:44.955674155 +0000
@@ -0,0 +1 @@
+.*: Removing unused section '\.(?:reginfo|MIPS\.options)' in file 'tmpdir/pr22649-2.\.o'
Index: binutils/ld/testsuite/ld-elf/shared.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-elf/shared.exp 2018-01-26 01:37:17.041655734 +0000
+++ binutils/ld/testsuite/ld-elf/shared.exp 2018-01-26 15:29:07.070336783 +0000
@@ -93,6 +93,11 @@ run_ld_link_tests [list \
 ]
 
 if { [check_gc_sections_available] } {
+    if [istarget mips*-*-*] {
+ set actions {{ld pr22649-2ab-mips.msg}}
+    } else {
+ set actions {{ld pr22649.msg}}
+    }
     run_ld_link_tests [list \
  [list \
     "Build pr22649-2a.so" \
@@ -100,7 +105,7 @@ if { [check_gc_sections_available] } {
     "" \
     "$AFLAGS_PIC" \
     {pr22649-2a.s} \
-    {{ld pr22649.msg}} \
+    $actions \
     "pr22649-2a.so" \
  ] \
  [list \
@@ -109,16 +114,23 @@ if { [check_gc_sections_available] } {
     "tmpdir/pr22649-1.so" \
     "$AFLAGS_PIC" \
     {pr22649-2a.s} \
-    {{ld pr22649.msg}} \
+    $actions \
     "pr22649-2b.so" \
  ] \
+    ]
+    if { [istarget mips*-*-*] && ![istarget *-*-elf*] } {
+ set actions {{ld pr22649-2cd-mips.msg}}
+    } else {
+ set actions {}
+    }
+    run_ld_link_tests [list \
  [list \
     "Build pr22649-2c.so" \
     "$LFLAGS -shared -gc-sections -print-gc-sections" \
     "" \
     "$AFLAGS_PIC" \
     {pr22649-2b.s} \
-    {} \
+    $actions \
     "pr22649-2b.so" \
  ] \
  [list \
@@ -127,7 +139,7 @@ if { [check_gc_sections_available] } {
     "tmpdir/pr22649-1.so" \
     "$AFLAGS_PIC" \
     {pr22649-2b.s} \
-    {} \
+    $actions \
     "pr22649-2b.so" \
  ] \
     ]