Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Rainer Orth]

As reported in PR gas/12181, this patch

2010-10-23  Mark Mitchell  <mark@codesourcery.com>

	* config/obj-elf.c (elf_adjust_symtab): New.  Move group section
	processing here from elf_frob_file.  Ensure that group signature
	symbols have the name of the group.
	(elf_frob_file): Move group section processing to
	elf_adjust_symtab.
	* config/obj-elf.h (elf_adjust_symtab): Declare.
	(obj_adjust_symtab): Define.
	* config/tc-arm.c (arm_adjust_symtab): Call elf_adjust_symtab.

broke GCC mainline bootstrap on Solaris 11 with CVS gas and Sun ld.

The problem is seen with this testcase:

$ cat group.s
	.section	.text.D2Ev,"axG",@progbits,D5Ev,comdat
	.type	D2Ev, @function
D2Ev:
	.section	.text.D0Ev,"axG",@progbits,D5Ev,comdat
	.type	D0Ev, @function
D0Ev:
$  gas -o group.o group.s
$ ld -G -o group.so group.o
ld: fatal: file group.o: group section [1].group: invalid group symbol D5Ev
ld: fatal: file group.o: section [1].group: SHF_GROUP flag set, but no
corresponding SHT_GROUP section found
ld: fatal: file group.o: section [5].text.D2Ev: SHF_GROUP flag set, but no
corresponding SHT_GROUP section found
ld: fatal: file group.o: section [6].text.D0Ev: SHF_GROUP flag set, but no
corresponding SHT_GROUP section found
ld: fatal: file processing errors. No output written to group.so

Currently, ld chokes on the local group signature symbol.

While the Solaris linker maintainers are willing to change ld,
unfortunately nobody cared to comment on their analysis in the PR.
Could someone (Mark?) please to so to get some progress on the Sun ld
side of things?

On the other hand, this doesn't help for existing installations.  To
remedy this, I've tried my idea to make the group signature symbol
global hidden instead of local, and this worked without problems.

Therefore I'd like to propose the following patch, both for mainline and
the 2.21 branch:

2010-11-10  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gas:
	PR gas/12181
	* config/obj-elf.c (elf_adjust_symtab): Make sy global hidden.

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index 79f8033..34d11fe 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2142,7 +2142,8 @@ elf_adjust_symtab (void)
 	{
 	  /* Create the symbol now.  */
 	  sy = symbol_new (group_name, now_seg, (valueT) 0, frag_now);
-	  symbol_get_obj (sy)->local = 1;
+	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
+	  S_SET_OTHER (sy, STV_HIDDEN);
 	  symbol_table_insert (sy);
 	}
       elf_group_id (s) = symbol_get_bfdsym (sy);

Mainline GCC bootstrap with CVS gas and gld worked, and a bootstrap with
gas and Sun ld allowed libstdc++.so to link.  It later failed linking
various libjava tools against libgcj.so, but this is an artefact of the
Solaris symbol versioning support in GCC: the contrib/make_sunver.pl
script currently uses nm and doesn't take symbol visibility support into
account, so a global hidden symbol becomes global with default
visibility in libgcj.so.  I'm working to use elfdump -s instead of nm
there, both to avoid this issue and because it has become obvious that
the nm-based variant is very fragile.

Ok for mainline and 2.21 branch?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Mark Mitchell]

On 11/10/2010 4:50 AM, Rainer Orth wrote:

> 2010-10-23  Mark Mitchell  <mark@codesourcery.com>

> broke GCC mainline bootstrap on Solaris 11 with CVS gas and Sun ld.

I've been insanely busy for the last couple of weeks, so I haven't been
paying as close attention as I should have been.  I'm very sorry.

> While the Solaris linker maintainers are willing to change ld,
> unfortunately nobody cared to comment on their analysis in the PR.
> Could someone (Mark?) please to so to get some progress on the Sun ld
> side of things?

I've now commented in the PR.

> -	  symbol_get_obj (sy)->local = 1;
> +	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
> +	  S_SET_OTHER (sy, STV_HIDDEN);

FWIW, this seems like a hack to me.  I think the symbol really should be
local; why expose it to other systems?  If we want to do this for
Solaris ld compatibility (which I agree is a valid reason), I think we
should do conditionally on Solaris.

Thank you,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Rainer Orth]

Mark Mitchell <mark@codesourcery.com> writes:

> On 11/10/2010 4:50 AM, Rainer Orth wrote:
>
>> 2010-10-23  Mark Mitchell  <mark@codesourcery.com>
>
>> broke GCC mainline bootstrap on Solaris 11 with CVS gas and Sun ld.
>
> I've been insanely busy for the last couple of weeks, so I haven't been
> paying as close attention as I should have been.  I'm very sorry.

No problem.  I just wanted to make sure that we have some sort of
solution for binutils 2.21.

>> While the Solaris linker maintainers are willing to change ld,
>> unfortunately nobody cared to comment on their analysis in the PR.
>> Could someone (Mark?) please to so to get some progress on the Sun ld
>> side of things?
>
> I've now commented in the PR.

Thanks.  I'll pass that on; I'm confident this will be fixed in the
Solaris ld soon.

>> -	  symbol_get_obj (sy)->local = 1;
>> +	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
>> +	  S_SET_OTHER (sy, STV_HIDDEN);
>
> FWIW, this seems like a hack to me.  I think the symbol really should be
> local; why expose it to other systems?  If we want to do this for
> Solaris ld compatibility (which I agree is a valid reason), I think we
> should do conditionally on Solaris.

Ok.  My reasoning for doing this in every case was that I wanted to
avoid target-specific code if we can do without.  I'll wrap the global
hidden change in #ifdef TE_SOLARIS with an appropriate comment (do we
list PRs in code comments for purposes like this?), retest and resubmit.

Thanks.
	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Rainer Orth]

Mark Mitchell <mark@codesourcery.com> writes:

>> While the Solaris linker maintainers are willing to change ld,
>> unfortunately nobody cared to comment on their analysis in the PR.
>> Could someone (Mark?) please to so to get some progress on the Sun ld
>> side of things?
>
> I've now commented in the PR.

Thanks.  I've passed this on and the fill will appear in Solaris 11
build 154.

>> -	  symbol_get_obj (sy)->local = 1;
>> +	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
>> +	  S_SET_OTHER (sy, STV_HIDDEN);
>
> FWIW, this seems like a hack to me.  I think the symbol really should be
> local; why expose it to other systems?  If we want to do this for
> Solaris ld compatibility (which I agree is a valid reason), I think we
> should do conditionally on Solaris.

Here's the patch that does this.  It allowed me too bootstrap GCC
mainline with both Sun ld (unfixed) and CVS GNU ld without regressions.

Ok for mainline and 2.2.1 branch now?

Thanks.
	Rainer


2010-11-10  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR gas/12181
	* config/obj-elf.c (elf_adjust_symtab) [TE_SOLARIS]: Make sy
	global hidden.

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index 79f8033..4b84bdb 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2142,7 +2142,14 @@ elf_adjust_symtab (void)
 	{
 	  /* Create the symbol now.  */
 	  sy = symbol_new (group_name, now_seg, (valueT) 0, frag_now);
+#ifdef TE_SOLARIS
+	  /* Before Solaris 11 build 154, Sun ld rejects local group
+	     signature symbols, so make them global hidden instead.  */
+	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
+	  S_SET_OTHER (sy, STV_HIDDEN);
+#else
 	  symbol_get_obj (sy)->local = 1;
+#endif
 	  symbol_table_insert (sy);
 	}
       elf_group_id (s) = symbol_get_bfdsym (sy);

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Mark Mitchell]

On 11/15/2010 11:48 AM, Rainer Orth wrote:

> Here's the patch that does this.  It allowed me too bootstrap GCC
> mainline with both Sun ld (unfixed) and CVS GNU ld without regressions.
> 
> Ok for mainline and 2.2.1 branch now?

It looks entirely reasonable to me, but for avoidance of doubt, I don't
have approval privileges.

Thank you,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Alan Modra]

On Mon, Nov 15, 2010 at 08:48:33PM +0100, Rainer Orth wrote:
> +	  /* Before Solaris 11 build 154, Sun ld rejects local group
> +	     signature symbols, so make them global hidden instead.  */
> +	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
> +	  S_SET_OTHER (sy, STV_HIDDEN);

It occurs to me that making the symbol BSF_GLOBAL may cause link
errors.  BSF_WEAK would be better, assuming that the Solaris linker
likes that.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Rainer Orth]

Alan Modra <amodra@gmail.com> writes:

> On Mon, Nov 15, 2010 at 08:48:33PM +0100, Rainer Orth wrote:
>> +	  /* Before Solaris 11 build 154, Sun ld rejects local group
>> +	     signature symbols, so make them global hidden instead.  */
>> +	  symbol_get_bfdsym (sy)->flags |= BSF_OBJECT | BSF_GLOBAL;
>> +	  S_SET_OTHER (sy, STV_HIDDEN);
>
> It occurs to me that making the symbol BSF_GLOBAL may cause link
> errors.  BSF_WEAK would be better, assuming that the Solaris linker
> likes that.

Works just as well, both for the testcase and GCC.  I've just
successfully bootstrapped GCC mainline on i386-pc-solaris2.11 with the
patch below.  I've omitted the BSF_OBJECT part since the non-Solaris
cases produced symbols with NOTYPE as well and it works fine this way.

Ok for mainline and 2.21 branch now?

Thanks.
	Rainer


2010-11-10  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gas:
	PR gas/12181
	* config/obj-elf.c (elf_adjust_symtab) [TE_SOLARIS]: Make sy
	weak hidden.

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index 8eb66ed..c6dc8d6 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2142,7 +2142,14 @@ elf_adjust_symtab (void)
 	{
 	  /* Create the symbol now.  */
 	  sy = symbol_new (group_name, now_seg, (valueT) 0, frag_now);
+#ifdef TE_SOLARIS
+	  /* Before Solaris 11 build 154, Sun ld rejects local group
+	     signature symbols, so make them weak hidden instead.  */
+	  symbol_get_bfdsym (sy)->flags |= BSF_WEAK;
+	  S_SET_OTHER (sy, STV_HIDDEN);
+#else
 	  symbol_get_obj (sy)->local = 1;
+#endif
 	  symbol_table_insert (sy);
 	}
       elf_group_id (s) = symbol_get_bfdsym (sy);


-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Alan Modra]

On Wed, Nov 17, 2010 at 08:46:52PM +0100, Rainer Orth wrote:
> 	PR gas/12181
> 	* config/obj-elf.c (elf_adjust_symtab) [TE_SOLARIS]: Make sy
> 	weak hidden.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[Tristan Gingold]

On Nov 17, 2010, at 10:33 PM, Alan Modra wrote:

> On Wed, Nov 17, 2010 at 08:46:52PM +0100, Rainer Orth wrote:
>> 	PR gas/12181
>> 	* config/obj-elf.c (elf_adjust_symtab) [TE_SOLARIS]: Make sy
>> 	weak hidden.
> 
> OK.

Just in case, OK too for the branch.

Tristan.

> 
> -- 
> Alan Modra
> Australia Development Lab, IBM

Re: Avoid link error with Solaris ld and local COMDAT group signature symbols (PR gas/12181)

[H.J. Lu]

On Wed, Nov 17, 2010 at 1:33 PM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Nov 17, 2010 at 08:46:52PM +0100, Rainer Orth wrote:
>>       PR gas/12181
>>       * config/obj-elf.c (elf_adjust_symtab) [TE_SOLARIS]: Make sy
>>       weak hidden.
>
> OK.
>

This is wrong and caused:

FAIL: ld-elf/pr22836-1a
FAIL: ld-elf/pr22836-1b

on Solaris.

-- 
H.J.