[PATCH 1/2] LD/testsuite: Add STB_GNU_UNIQUE cross-linker test

[PATCH 1/2] LD/testsuite: Add STB_GNU_UNIQUE cross-linker test

[Maciej W. Rozycki]

Some targets are only really, or at least regularly, regression-tested 
in a crossed configuration.  Currently we only have native compiled test 
cases for the STB_GNU_UNIQUE feature in the linker test suite.  This is 
nice, covering run-time semantics even, but quite often not run at all.  
Consequently a regression may remain unnoticed for long.

Add a simple test case then to provide basic linker coverage with no 
need for a compiler or a native toolchain.

	ld/
	* testsuite/ld-unique/unique.d: New test.
	* ld/testsuite/ld-unique/unique.exp: Run the new test.  Adjust
	messages for compiled tests.
---
Hi,

 No regressions across my usual test target selection (currently 153, 
including 117 non-MIPS ones).  OK to apply?

  Maciej

binutils-ld-stb-gnu-unique-test.diff
Index: binutils/ld/testsuite/ld-unique/unique.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-unique/unique.d	2016-04-30 01:16:25.395609668 +0100
@@ -0,0 +1,9 @@
+#ld: -r
+#readelf: -sh
+#name: Linker setting GNU OSABI on STB_GNU_UNIQUE symbol (PR 10549)
+
+#...
+ *OS/ABI: +UNIX - GNU
+#...
+ *[0-9]+: +[0-9a-f]+ +[0-9]+ +OBJECT +UNIQUE +DEFAULT +[0-9]+ a
+#pass
Index: binutils/ld/testsuite/ld-unique/unique.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-unique/unique.exp	2016-04-30 01:16:25.334034718 +0100
+++ binutils/ld/testsuite/ld-unique/unique.exp	2016-04-30 01:16:25.402711134 +0100
@@ -42,19 +42,21 @@ if {!(([istarget "i?86-*-*"]
     return
 }
 
+run_dump_test "unique"
+
 # We need a native system.  FIXME: Strictly speaking this
 # is not true, we just need to know how to create a fully
 # linked executable, including the C and Z libraries, using
 # the linker that is under test.
 if ![isnative] {
-    verbose "UNIQUE tests not run - not a native toolchain"
+    verbose "UNIQUE compiled tests not run - not a native toolchain"
     return
 }
 
 # We need a working compiler.  (Strictly speaking this is
 # not true, we could use target specific assembler files).
 if { [which $CC] == 0 } {
-    verbose "UNIQUE tests not run - no compiler available"
+    verbose "UNIQUE compiled tests not run - no compiler available"
     return
 }
 

[PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Maciej W. Rozycki]

Expand LD STB_GNU_UNIQUE testing across all eligible targets, as per the 
selection made in GAS, that is ones setting the OSABI to ELFOSABI_NONE 
or ELFOSABI_GNU.

	ld/
	* testsuite/ld-unique/unique.exp: Run across all ELFOSABI_NONE 
	and ELFOSABI_GNU targets
---
Hi,

 At first I thought this would be a good idea, prompting architecture 
maintainers to fix their code.  It wouldn't be over 5 years from commit 
f64b2e8d60f2 for me to notice there was something wrong with the MIPS 
target if we had any test suite failure for this feature (and then only 
because the IFUNC feature currently under review touches related code).

 But then I realised there's probably something wrong in the first place 
that we require maintainers' intervention for LD whereas we handle this 
stuff automagically across all the eligible targets in GAS (see obj-elf.c 
and `obj_elf_type' there).

 There was an observation made in the original review process here: 
<https://sourceware.org/ml/binutils/2011-04/msg00144.html> that we should 
be limiting targets by the OSABI, but:

1. GAS does that already in generic code.

2. BFD per-target code doesn't do it anyway.

3. The choice whether to check the OSABI or not is independent from the 
   choice whether to handle this feature in generic or target-specific BFD 
   code.

So does anybody actually know why the current per-architecture approach 
was accepted in the review?

 If not then I'll post a proposal separately to gather all the bits 
scattered across target `*_add_symbol_hook' handlers and handle this 
feature in `elf_link_add_object_symbols'.

 Regardless, OK to apply this test suite change itself?

  Maciej

binutils-ld-stb-gnu-unique-test-targets.diff
Index: binutils/ld/testsuite/ld-unique/unique.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-unique/unique.exp	2016-04-30 06:35:48.586138927 +0100
+++ binutils/ld/testsuite/ld-unique/unique.exp	2016-04-30 17:10:41.934056441 +0100
@@ -24,20 +24,20 @@
 # Adapted for unique checking by Mark J. Wielaard <mjw@redhat.com>
 
 
-# STB_GNU_UNIQUE support has only been implemented for the ix86, x86_64,
-# arm, mips, powerpc, and sparc so far.
-if {!(([istarget "i?86-*-*"]
-       || [istarget "x86_64-*-*"]
-       || [istarget "arm*-*-*"]
-       || [istarget "mips*-*-*"]
-       || [istarget "powerpc*-*-*"]
-       || [istarget "sparc*-*-*"])
-      && ([istarget "*-*-elf*"]
-	  || [istarget "*-*-nacl*"]
-	  || (([istarget "*-*-linux*"]
-	       || [istarget "*-*-gnu*"])
-	      && ![istarget "*-*-*aout*"]
-	      && ![istarget "*-*-*oldld*"]))) } {
+# Exclude non-ELF targets.
+if { ![is_elf_format] } {
+    return
+}
+
+# Exclude some more targets; feel free to include your favorite one
+# if you like.  The MSP430 and Visium targets set the ELF header's
+# OSABI field to ELFOSABI_STANDALONE and cannot support STB_GNU_UNIQUE.
+if { !([istarget "*-*-elf*"]
+       && ![istarget "msp430-*-*"]
+       && ![istarget "visium-*-*"])
+     && ![istarget *-*-nacl*]
+     && ![istarget *-*-linux*]
+     && ![istarget *-*-gnu*] } {
     verbose "UNIQUE tests not run - target does not support UNIQUE"
     return
 }

RE: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Matthew Fortune]

Maciej Rozycki <Maciej.Rozycki@imgtec.com> writes:
> +# Exclude some more targets; feel free to include your favorite one #
> +if you like.  The MSP430 and Visium targets set the ELF header's #
> +OSABI field to ELFOSABI_STANDALONE and cannot support STB_GNU_UNIQUE.
> +if { !([istarget "*-*-elf*"]
> +       && ![istarget "msp430-*-*"]
> +       && ![istarget "visium-*-*"])
> +     && ![istarget *-*-nacl*]
> +     && ![istarget *-*-linux*]
> +     && ![istarget *-*-gnu*] } {
>      verbose "UNIQUE tests not run - target does not support UNIQUE"
>      return
>  }

Quite subjective but I found the new condition hard to read; the
following seems to match the comment more naturally to me:

if { (![istarget "*-*-elf*"]
      || [istarget "msp430-*-*"]
      || [istarget "visium-*-*"])
     && ![istarget *-*-nacl*]
     && ![istarget *-*-linux*]
     && ![istarget *-*-gnu*] } {

Matthew

Re: [PATCH 1/2] LD/testsuite: Add STB_GNU_UNIQUE cross-linker test

[Nick Clifton]

Hi Maciej,

> 	ld/
> 	* testsuite/ld-unique/unique.d: New test.
> 	* ld/testsuite/ld-unique/unique.exp: Run the new test.  Adjust
> 	messages for compiled tests.

Approved - please apply.

Cheers
  Nick

RE: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Maciej W. Rozycki]

On Wed, 4 May 2016, Matthew Fortune wrote:

> > +# Exclude some more targets; feel free to include your favorite one #
> > +if you like.  The MSP430 and Visium targets set the ELF header's #
> > +OSABI field to ELFOSABI_STANDALONE and cannot support STB_GNU_UNIQUE.
> > +if { !([istarget "*-*-elf*"]
> > +       && ![istarget "msp430-*-*"]
> > +       && ![istarget "visium-*-*"])
> > +     && ![istarget *-*-nacl*]
> > +     && ![istarget *-*-linux*]
> > +     && ![istarget *-*-gnu*] } {
> >      verbose "UNIQUE tests not run - target does not support UNIQUE"
> >      return
> >  }
> 
> Quite subjective but I found the new condition hard to read; the
> following seems to match the comment more naturally to me:
> 
> if { (![istarget "*-*-elf*"]
>       || [istarget "msp430-*-*"]
>       || [istarget "visium-*-*"])
>      && ![istarget *-*-nacl*]
>      && ![istarget *-*-linux*]
>      && ![istarget *-*-gnu*] } {

 I decided keeping all the conditions negated at the outer level was more 
consistent and natural actually.  I won't mind updating this statement to 
something like:

if { !([istarget "*-*-elf*"]
       && !([istarget "msp430-*-*"] || [istarget "visium-*-*"]))
     && ![istarget *-*-nacl*]
     && ![istarget *-*-linux*]
     && ![istarget *-*-gnu*] } {

though, or maybe even make another De Morgan transformation and have a 
single negation at the then outermost level.  Keeping the `*-*-elf*' 
exceptions on a single line might improve readability, although it'll be 
lost with the first addition of another exception.

  Maciej

Re: [PATCH 1/2] LD/testsuite: Add STB_GNU_UNIQUE cross-linker test

[Maciej W. Rozycki]

Hi Nick,

> > 	ld/
> > 	* testsuite/ld-unique/unique.d: New test.
> > 	* ld/testsuite/ld-unique/unique.exp: Run the new test.  Adjust
> > 	messages for compiled tests.
> 
> Approved - please apply.

 Thank you for your review.  Applied now (with the ChangeLog typo fixed).

  Maciej

RE: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Matthew Fortune]

Maciej Rozycki <Maciej.Rozycki@imgtec.com> writes:
> On Wed, 4 May 2016, Matthew Fortune wrote:
> 
> > > +# Exclude some more targets; feel free to include your favorite one
> > > +# if you like.  The MSP430 and Visium targets set the ELF header's
> > > +# OSABI field to ELFOSABI_STANDALONE and cannot support
> STB_GNU_UNIQUE.
> > > +if { !([istarget "*-*-elf*"]
> > > +       && ![istarget "msp430-*-*"]
> > > +       && ![istarget "visium-*-*"])
> > > +     && ![istarget *-*-nacl*]
> > > +     && ![istarget *-*-linux*]
> > > +     && ![istarget *-*-gnu*] } {
> > >      verbose "UNIQUE tests not run - target does not support UNIQUE"
> > >      return
> > >  }
> >
> > Quite subjective but I found the new condition hard to read; the
> > following seems to match the comment more naturally to me:
> >
> > if { (![istarget "*-*-elf*"]
> >       || [istarget "msp430-*-*"]
> >       || [istarget "visium-*-*"])
> >      && ![istarget *-*-nacl*]
> >      && ![istarget *-*-linux*]
> >      && ![istarget *-*-gnu*] } {
> 
>  I decided keeping all the conditions negated at the outer level was
> more consistent and natural actually.  I won't mind updating this
> statement to something like:
> 
> if { !([istarget "*-*-elf*"]
>        && !([istarget "msp430-*-*"] || [istarget "visium-*-*"]))
>      && ![istarget *-*-nacl*]
>      && ![istarget *-*-linux*]
>      && ![istarget *-*-gnu*] } {
> 
> though, or maybe even make another De Morgan transformation and have a
> single negation at the then outermost level.  Keeping the `*-*-elf*'
> exceptions on a single line might improve readability, although it'll be
> lost with the first addition of another exception.

Given you had already thought about it then I'd go with your original.

Matthew

Re: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Pedro Alves]

On 05/04/2016 03:20 PM, Maciej W. Rozycki wrote:

> though, or maybe even make another De Morgan transformation and have a 
> single negation at the then outermost level.  Keeping the `*-*-elf*' 
> exceptions on a single line might improve readability, although it'll be 
> lost with the first addition of another exception.

Or add a proc, which then allows early returns, making it
more readable, IMO:

proc supports_gnu_unique {} {
   # Only ELF.
   if {!([istarget "*-*-elf*"]
       || [istarget *-*-nacl*]
       || [istarget *-*-linux*]
       || [istarget *-*-gnu*]) } {
     return 0
   }

   # These targets set the ELF header's OSABI field to
   # ELFOSABI_STANDALONE and cannot support STB_GNU_UNIQUE.
   if {[istarget "msp430-*-*"]
       || [istarget "visium-*-*"]} {
     return 0
   }

   # Default to yes.
   return 1
}

...
  if { ![supports_gnu_unique] } {
      verbose "UNIQUE tests not run - target does not support UNIQUE"
      return
  }
...

Thanks,
Pedro Alves

Re: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Maciej W. Rozycki]

Hi Pedro,

> > though, or maybe even make another De Morgan transformation and have a 
> > single negation at the then outermost level.  Keeping the `*-*-elf*' 
> > exceptions on a single line might improve readability, although it'll be 
> > lost with the first addition of another exception.
> 
> Or add a proc, which then allows early returns, making it
> more readable, IMO:

 I like your proposal better, thanks for the suggestion.  Although I'll 
shuffle the conditions a bit, to reproduce the original semantics I 
proposed -- for GNU targets we do want to report STB_GNU_UNIQUE support 
unconditionally, no exceptions.

 I'll wait a couple of days for any response to my questions and post an
updated version then.

  Maciej

Re: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Nick Clifton]

Hi Maciej,

> So does anybody actually know why the current per-architecture approach 
> was accepted in the review?

No.  My guess is that it was not thought through properly.  Possibly a
developer wanted it for their architecture, but did not consider the impact
on other architectures.  So their patch went in for their architecture and
the feature proved to be useful.  Then users for other architectures (probably
kernel and library builders) decided that they wanted it too, and so a
piecemeal set of hacks were added to accommodate them.  Just a guess mind...



>  If not then I'll post a proposal separately to gather all the bits 
> scattered across target `*_add_symbol_hook' handlers and handle this 
> feature in `elf_link_add_object_symbols'.

Please do.

Cheers
  Nick

Re: [PATCH 2/2] LD/testsuite: Expand STB_GNU_UNIQUE test coverage

[Maciej W. Rozycki]

Hi Nick,

> > So does anybody actually know why the current per-architecture approach 
> > was accepted in the review?
> 
> No.  My guess is that it was not thought through properly.  Possibly a
> developer wanted it for their architecture, but did not consider the impact
> on other architectures.  So their patch went in for their architecture and
> the feature proved to be useful.  Then users for other architectures (probably
> kernel and library builders) decided that they wanted it too, and so a
> piecemeal set of hacks were added to accommodate them.  Just a guess mind...

 Thank you for your confirmation, I suspected this might have been the 
case.

> >  If not then I'll post a proposal separately to gather all the bits 
> > scattered across target `*_add_symbol_hook' handlers and handle this 
> > feature in `elf_link_add_object_symbols'.
> 
> Please do.

 I've got the test suite part done already, so I'll update code and post 
the two changes together.  I think it'll be the best if the code update 
goes in first or both are a single change, so as not to cause regressions 
which might break bisection.

  Maciej