diff mbox series

lto: Set resolution to LDPR_PREVAILING_DEF for non-IR common symbol

Message ID CAMe9rOq6RthSWFSGyJKAw2wU_V2qo-cqbcn_nKAM1YyzaZWrGQ@mail.gmail.com
State New
Headers
Series lto: Set resolution to LDPR_PREVAILING_DEF for non-IR common symbol |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

H.J. Lu Aug. 27, 2026, 8:31 a.m. UTC 
  Since size or alignment of common symbol may be overridden by another
common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
undefined or common symbol from an IR object.

PR ld/34570
* plugin.c (get_symbols): Set resolution to LDPR_PREVAILING_DEF
for common symbol in regular object.
* testsuite/ld-plugin/pr34570a.c: New file.
* testsuite/ld-plugin/pr34570a.d: Likewise.
* testsuite/ld-plugin/pr34570b.c: Likewise.
* testsuite/ld-plugin/pr34570b.d: Likewise.
* testsuite/ld-plugin/pr34570c.c: Likewise.
* testsuite/ld-plugin/pr34570c.d: Likewise.

Comments

Jan Beulich Aug. 27, 2026, 11:52 a.m. UTC | #1 
On 27.08.2026 10:31, H.J. Lu wrote:
> Since size or alignment of common symbol may be overridden by another
> common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
> LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
> undefined or common symbol from an IR object.

Is this correct for the LDPK_COMMON case that's also covered by the if()
that you alter in get_symbols()?

Adding to the (my?) confusion is that after said

      if (syms[n].def == LDPK_UNDEF
	  || syms[n].def == LDPK_WEAKUNDEF
	  || syms[n].def == LDPK_COMMON)

there's an "else if" with this comment:

      /* Was originally def, weakdef, or common, but has been pre-empted.  */

With the cited if(), no originally common symbol can make it there, can it?

Jan
H.J. Lu Aug. 27, 2026, 12:04 p.m. UTC | #2 
On Thu, Aug 27, 2026 at 7:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.08.2026 10:31, H.J. Lu wrote:
> > Since size or alignment of common symbol may be overridden by another
> > common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
> > LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
> > undefined or common symbol from an IR object.
>
> Is this correct for the LDPK_COMMON case that's also covered by the if()
> that you alter in get_symbols()?

My patch covers the case of

syms[n].def == LDPK_COMMON && blhe->type == bfd_link_hash_common.

which wasn't handled before.

> Adding to the (my?) confusion is that after said
>
>       if (syms[n].def == LDPK_UNDEF
>           || syms[n].def == LDPK_WEAKUNDEF
>           || syms[n].def == LDPK_COMMON)
>
> there's an "else if" with this comment:
>
>       /* Was originally def, weakdef, or common, but has been pre-empted.  */
>
> With the cited if(), no originally common symbol can make it there, can it?

My patch covers

      if (syms[n].def == LDPK_UNDEF
          || syms[n].def == LDPK_WEAKUNDEF
          || syms[n].def == LDPK_COMMON)
        {
             ...

It has nothing to do with the else clause.

> Jan
Jan Beulich Aug. 27, 2026, 2:28 p.m. UTC | #3 
On 27.08.2026 14:04, H.J. Lu wrote:
> On Thu, Aug 27, 2026 at 7:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.08.2026 10:31, H.J. Lu wrote:
>>> Since size or alignment of common symbol may be overridden by another
>>> common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
>>> LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
>>> undefined or common symbol from an IR object.
>>
>> Is this correct for the LDPK_COMMON case that's also covered by the if()
>> that you alter in get_symbols()?
> 
> My patch covers the case of
> 
> syms[n].def == LDPK_COMMON && blhe->type == bfd_link_hash_common.
> 
> which wasn't handled before.

Right, but that doesn't answer my question, hinting towards a situation where
two instances of a common symbol collide. And I can't really resolve that for
myself because ...

>> Adding to the (my?) confusion is that after said
>>
>>       if (syms[n].def == LDPK_UNDEF
>>           || syms[n].def == LDPK_WEAKUNDEF
>>           || syms[n].def == LDPK_COMMON)
>>
>> there's an "else if" with this comment:
>>
>>       /* Was originally def, weakdef, or common, but has been pre-empted.  */
>>
>> With the cited if(), no originally common symbol can make it there, can it?

... there's this comment which irritates me.

> My patch covers
> 
>       if (syms[n].def == LDPK_UNDEF
>           || syms[n].def == LDPK_WEAKUNDEF
>           || syms[n].def == LDPK_COMMON)
>         {
>              ...
> 
> It has nothing to do with the else clause.

Correct, and I didn't mean to say anything to the contrary.

Jan
H.J. Lu Aug. 27, 2026, 3:19 p.m. UTC | #4 
On Thu, Aug 27, 2026 at 10:28 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.08.2026 14:04, H.J. Lu wrote:
> > On Thu, Aug 27, 2026 at 7:52 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 27.08.2026 10:31, H.J. Lu wrote:
> >>> Since size or alignment of common symbol may be overridden by another
> >>> common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
> >>> LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
> >>> undefined or common symbol from an IR object.
> >>
> >> Is this correct for the LDPK_COMMON case that's also covered by the if()
> >> that you alter in get_symbols()?
> >
> > My patch covers the case of
> >
> > syms[n].def == LDPK_COMMON && blhe->type == bfd_link_hash_common.
> >
> > which wasn't handled before.
>
> Right, but that doesn't answer my question, hinting towards a situation where
> two instances of a common symbol collide. And I can't really resolve that for
> myself because ...
>
> >> Adding to the (my?) confusion is that after said
> >>
> >>       if (syms[n].def == LDPK_UNDEF
> >>           || syms[n].def == LDPK_WEAKUNDEF
> >>           || syms[n].def == LDPK_COMMON)
> >>

It has everything to do with

     if (syms[n].def == LDPK_UNDEF
          || syms[n].def == LDPK_WEAKUNDEF
          || syms[n].def == LDPK_COMMON)

In its else clause, syms[n].def is LDPK_DEF or LDPK_WEAKDEF.
Linker plugin tells linker that originally def, weakdef, or common,
has been pre-empted.

> >> there's an "else if" with this comment:
> >>
> >>       /* Was originally def, weakdef, or common, but has been pre-empted.  */
> >>
> >> With the cited if(), no originally common symbol can make it there, can it?
>
> ... there's this comment which irritates me.
>
> > My patch covers
> >
> >       if (syms[n].def == LDPK_UNDEF
> >           || syms[n].def == LDPK_WEAKUNDEF
> >           || syms[n].def == LDPK_COMMON)
> >         {
> >              ...
> >
> > It has nothing to do with the else clause.
>
> Correct, and I didn't mean to say anything to the contrary.
>
> Jan
Alan Modra Aug. 28, 2026, 5:59 a.m. UTC | #5 
On Thu, Aug 27, 2026 at 04:31:43PM +0800, H.J. Lu wrote:
> Since size or alignment of common symbol may be overridden by another
> common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
> LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
> undefined or common symbol from an IR object.
> 
> 	PR ld/34570
> 	* plugin.c (get_symbols): Set resolution to LDPR_PREVAILING_DEF
> 	for common symbol in regular object.
> 	* testsuite/ld-plugin/pr34570a.c: New file.
> 	* testsuite/ld-plugin/pr34570a.d: Likewise.
> 	* testsuite/ld-plugin/pr34570b.c: Likewise.
> 	* testsuite/ld-plugin/pr34570b.d: Likewise.
> 	* testsuite/ld-plugin/pr34570c.c: Likewise.
> 	* testsuite/ld-plugin/pr34570c.d: Likewise.

OK.  Would you please also apply the following along with your patch,
if you agree that this comment is incorrect regarding "or common" and
therefore confusing and unnecessary given the earlier comment.

diff --git a/ld/plugin.c b/ld/plugin.c
index 1cf5c915b00..e221e414db5 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -871,8 +871,6 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
 	res = LDPR_PREEMPTED_REG;
       else if (owner_sec->owner == abfd)
 	res = LDPR_PREVAILING_DEF_IRONLY;
-
-      /* Was originally def, weakdef, or common, but has been pre-empted.  */
       else if (is_ir_dummy_bfd (owner_sec->owner))
 	res = LDPR_PREEMPTED_IR;
       else
H.J. Lu Aug. 28, 2026, 6:05 a.m. UTC | #6 
On Fri, Aug 28, 2026 at 2:00 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Aug 27, 2026 at 04:31:43PM +0800, H.J. Lu wrote:
> > Since size or alignment of common symbol may be overridden by another
> > common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
> > LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
> > undefined or common symbol from an IR object.
> >
> >       PR ld/34570
> >       * plugin.c (get_symbols): Set resolution to LDPR_PREVAILING_DEF
> >       for common symbol in regular object.
> >       * testsuite/ld-plugin/pr34570a.c: New file.
> >       * testsuite/ld-plugin/pr34570a.d: Likewise.
> >       * testsuite/ld-plugin/pr34570b.c: Likewise.
> >       * testsuite/ld-plugin/pr34570b.d: Likewise.
> >       * testsuite/ld-plugin/pr34570c.c: Likewise.
> >       * testsuite/ld-plugin/pr34570c.d: Likewise.
>
> OK.  Would you please also apply the following along with your patch,
> if you agree that this comment is incorrect regarding "or common" and
> therefore confusing and unnecessary given the earlier comment.

Will do.

Thanks.

> diff --git a/ld/plugin.c b/ld/plugin.c
> index 1cf5c915b00..e221e414db5 100644
> --- a/ld/plugin.c
> +++ b/ld/plugin.c
> @@ -871,8 +871,6 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
>         res = LDPR_PREEMPTED_REG;
>        else if (owner_sec->owner == abfd)
>         res = LDPR_PREVAILING_DEF_IRONLY;
> -
> -      /* Was originally def, weakdef, or common, but has been pre-empted.  */
>        else if (is_ir_dummy_bfd (owner_sec->owner))
>         res = LDPR_PREEMPTED_IR;
>        else
>
> --
> Alan Modra
diff mbox series

Patch

From 75c48f7a28f23dcd4a345169a57906dbe39f1431 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 27 Aug 2026 15:49:08 +0800
Subject: [PATCH] lto: Set resolution to LDPR_PREVAILING_DEF for non-IR common
 symbol

Since size or alignment of common symbol may be overridden by another
common symbol, set symbol resolution to LDPR_PREVAILING_DEF, instead of
LDPR_RESOLVED_EXEC, for common symbol in regular object when seeing an
undefined or common symbol from an IR object.

	PR ld/34570
	* plugin.c (get_symbols): Set resolution to LDPR_PREVAILING_DEF
	for common symbol in regular object.
	* testsuite/ld-plugin/pr34570a.c: New file.
	* testsuite/ld-plugin/pr34570a.d: Likewise.
	* testsuite/ld-plugin/pr34570b.c: Likewise.
	* testsuite/ld-plugin/pr34570b.d: Likewise.
	* testsuite/ld-plugin/pr34570c.c: Likewise.
	* testsuite/ld-plugin/pr34570c.d: Likewise.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 ld/plugin.c                       |  2 +
 ld/testsuite/ld-plugin/lto.exp    | 67 +++++++++++++++++++++++++++++++
 ld/testsuite/ld-plugin/pr34570a.c |  9 +++++
 ld/testsuite/ld-plugin/pr34570a.d |  6 +++
 ld/testsuite/ld-plugin/pr34570b.c |  1 +
 ld/testsuite/ld-plugin/pr34570b.d |  6 +++
 ld/testsuite/ld-plugin/pr34570c.c |  9 +++++
 ld/testsuite/ld-plugin/pr34570c.d |  6 +++
 8 files changed, 106 insertions(+)
 create mode 100644 ld/testsuite/ld-plugin/pr34570a.c
 create mode 100644 ld/testsuite/ld-plugin/pr34570a.d
 create mode 100644 ld/testsuite/ld-plugin/pr34570b.c
 create mode 100644 ld/testsuite/ld-plugin/pr34570b.d
 create mode 100644 ld/testsuite/ld-plugin/pr34570c.c
 create mode 100644 ld/testsuite/ld-plugin/pr34570c.d

diff --git a/ld/plugin.c b/ld/plugin.c
index 1cf5c915b00..5752d647cfe 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -860,6 +860,8 @@  get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
 	  else if (owner_sec->owner != NULL
 		   && (owner_sec->owner->flags & DYNAMIC) != 0)
 	    res = LDPR_RESOLVED_DYN;
+	  else if (blhe->type == bfd_link_hash_common)
+	    res = LDPR_PREVAILING_DEF;
 	  else
 	    res = LDPR_RESOLVED_EXEC;
 	}
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index 4504fd4be01..476b605b721 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -275,6 +275,73 @@  set lto_link_tests [list \
    {} \
    "libcommon-2.a" \
   ] \
+  [list \
+   "Build pr34570a.exe" \
+   "-flto" \
+   "-flto -fcommon" \
+   {pr34570a.c pr34570b.c} \
+   {{readelf {-s --wide} pr34570a.d}} \
+   "pr34570a.exe" \
+  ] \
+  [list \
+   "Build pr34570b.exe" \
+   "-flto" \
+   "-flto -fcommon" \
+   {pr34570b.c pr34570a.c} \
+   {{readelf {-s --wide} pr34570a.d}} \
+   "pr34570b.exe" \
+  ] \
+  [list \
+   "Build pr34570a.o" \
+   "" \
+   "-O2 -fcommon -flto $lto_no_fat" \
+   {pr34570a.c} \
+   {} \
+   "" \
+   "c" \
+  ] \
+  [list \
+   "Build pr34570b.o" \
+   "" \
+   "-O2 -fcommon -fno-lto" \
+   {pr34570b.c} \
+   {} \
+   "" \
+   "c" \
+  ] \
+  [list \
+   "Build pr34570c.o" \
+   "" \
+   "-O2 -fcommon -flto $lto_no_fat" \
+   {pr34570c.c} \
+   {} \
+   "" \
+   "c" \
+  ] \
+  [list \
+   "Build pr34570c.exe" \
+   "-flto tmpdir/pr34570a.o tmpdir/pr34570b.o" \
+   "" \
+   {dummy.c} \
+   {{readelf {-s --wide} pr34570b.d}} \
+   "pr34570c.exe" \
+  ] \
+  [list \
+   "Build pr34570d.exe" \
+   "-flto tmpdir/pr34570b.o tmpdir/pr34570a.o" \
+   "" \
+   {dummy.c} \
+   {{readelf {-s --wide} pr34570b.d}} \
+   "pr34570d.exe" \
+  ] \
+  [list \
+   "Build pr34570e.exe" \
+   "-flto tmpdir/pr34570c.o tmpdir/pr34570b.o" \
+   "-flto -fcommon" \
+   {dummy.c} \
+   {{readelf {-s --wide} pr34570c.d}} \
+   "pr34570e.exe" \
+  ] \
 ]
 
 if { [at_least_gcc_version 10 0] && [check_lto_shared_available] } {
diff --git a/ld/testsuite/ld-plugin/pr34570a.c b/ld/testsuite/ld-plugin/pr34570a.c
new file mode 100644
index 00000000000..d3ad433e52f
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570a.c
@@ -0,0 +1,9 @@ 
+#include <stdint.h>
+
+char foo[8];
+
+int
+main ()
+{
+  return (uintptr_t) &foo == 0x12345678 ? 0 : 1;
+}
diff --git a/ld/testsuite/ld-plugin/pr34570a.d b/ld/testsuite/ld-plugin/pr34570a.d
new file mode 100644
index 00000000000..6bbc399bf77
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570a.d
@@ -0,0 +1,6 @@ 
+#...
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+#...
+ +[0-9]+: +[0-9a-f]+ +8 +OBJECT +LOCAL +DEFAULT .*[0-9]+ _?foo
+#pass
diff --git a/ld/testsuite/ld-plugin/pr34570b.c b/ld/testsuite/ld-plugin/pr34570b.c
new file mode 100644
index 00000000000..e308af97111
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570b.c
@@ -0,0 +1 @@ 
+char foo[1];
diff --git a/ld/testsuite/ld-plugin/pr34570b.d b/ld/testsuite/ld-plugin/pr34570b.d
new file mode 100644
index 00000000000..39d9d7315cb
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570b.d
@@ -0,0 +1,6 @@ 
+#...
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+#...
+ +[0-9]+: +[0-9a-f]+ +8 +OBJECT +GLOBAL +DEFAULT .*[0-9]+ _?foo
+#pass
diff --git a/ld/testsuite/ld-plugin/pr34570c.c b/ld/testsuite/ld-plugin/pr34570c.c
new file mode 100644
index 00000000000..ae3ce725e34
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570c.c
@@ -0,0 +1,9 @@ 
+#include <stdint.h>
+
+extern char foo[];
+
+int
+main ()
+{
+  return (uintptr_t) &foo == 0x12345678 ? 0 : 1;
+}
diff --git a/ld/testsuite/ld-plugin/pr34570c.d b/ld/testsuite/ld-plugin/pr34570c.d
new file mode 100644
index 00000000000..c512064fbeb
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr34570c.d
@@ -0,0 +1,6 @@ 
+#...
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size Type +Bind +Vis +Ndx Name
+#...
+ +[0-9]+: +[0-9a-f]+ +1 +OBJECT +GLOBAL +DEFAULT .*[0-9]+ _?foo
+#pass
-- 
2.55.0