Re: [PATCH] PR ld/21233: Avoid sweeping forced-undefined symbols in section GC

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

> Date: Wed, 5 Apr 2017 17:03:22 +0100
> From: "Maciej W. Rozycki" <macro@imgtec.com>

> On Wed, 5 Apr 2017, Hans-Peter Nilsson wrote:
> 
> > > 2. File a PR referring to commit d968975277ba and its discussion and KFAIL 
> > >    the affected test cases for the problematic targets.
> > 
> > (or rather xfail; I don't know if there's a way to kfail with
> > run_ld_link_tests and I don't think we use that in binutils.)
> 
>  Of course there is, see commit 3807734dbe48 for a somewhat non-trivial 
> example.
>
>  AFAIK XFAIL is unsuitable as it marks a problem with the test environment 
> rather than a bug in the component being tested.  KFAIL OTOH forces you to 
> create a PR, which prompts you to have the issue recorded in the bug 
> tracker (also giving a chance for someone else to pick it up) rather than 
> papered over and then forgotten.

Sure.  Since it's your test and I felt compelled to fix my
indentation gotcha (for which I blame emacs) I changed to kfail.

JFTR, still argumenting for my preference for "2" above:

> > I'm not a general maintainer, but FWIW my preference would have
> > been this, to xfail the failing parts and also add affected
> > maintainers on the ticket.
> 
>  On second thoughts there can be multiple bugs here, quite likely one or 
> more per BFD backend.

But it's probably a horse rather than a zebra. :)

>  So it doesn't really scale well.  And any active 
> maintainer will notice the regression in their routine runs; XFAIL/KFAIL 
> may OTOH be missed (my test scripts certainly do not pay attention to 
> these on the basis of them being expected/known; I'd have to go through 
> full logs to catch them).

Keeping it simple, as well as the test being tied to a specific
PR, says to log it in that PR - at least until the issue
has been analyzed for that target.

> > To those interested: the run_ld_link_tests source shows how to
> > add xfails for a target or use this as an example.  (Not my
> > preferred test-driver function, I prefer to iterate on
> > run_dump_test *.d files; with a driver in place sometimes I only
> > have to add that one file with a descriptive comment inside.)
> 
>  I prefer `run_dump_test' too where feasible, but it is not suitable for 
> some test cases, not at least without bending backwards.

Sure; at a glance it didn't look like bending was required, but
then I haven't really looked close into these test-cases.

> > As a sanity check, I made sure mips-linux still passed all
> > tests.
> 
>  Thanks.
> 
> > diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
> > index be30ec0..291f9e1 100644
> > --- a/ld/testsuite/ld-elf/shared.exp
> > +++ b/ld/testsuite/ld-elf/shared.exp
> > @@ -152,7 +153,7 @@ if { [check_gc_sections_available] } {
> >  	    "tmpdir/libpr21233.so" "" \
> >  	    {pr21233.s} \
> >  	    {{readelf --dyn-syms pr21233.sd}} \
> > -	    "pr21233-3"]]
> > +	     "pr21233-3"]] "cris*-*-*"
> 
>  Indentation issue here.

Committed as above.  Unfortunately the kfails required splitting
the test-list.  Sanity-checked mips-linux again.  It's not lost
on me that the invested effort, keeping this up, will soon be on
par with fixing the actual issue...

 	PR ld/21233
	* testsuite/ld-elf/shared.exp: Change xfails to kfails and fix
	indentation issue introduced with last commit.

diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
index 291f9e1..4859170 100644
--- a/ld/testsuite/ld-elf/shared.exp
+++ b/ld/testsuite/ld-elf/shared.exp
@@ -131,6 +131,9 @@ if { [check_gc_sections_available] } {
 	    {pr21233-l.s} \
 	    {{readelf --dyn-syms pr21233-l.sd}} \
 	    "libpr21233.so"]]
+
+    setup_kfail "cris*-*-*" "ld/21233"
+
     run_ld_link_tests [list \
 	[list \
 	    "PR ld/21233 dynamic symbols with section GC (--undefined)" \
@@ -138,7 +141,11 @@ if { [check_gc_sections_available] } {
 	    "tmpdir/libpr21233.so" "" \
 	    {pr21233.s} \
 	    {{readelf --dyn-syms pr21233.sd}} \
-	    "pr21233-1"] \
+	    "pr21233-1"]]
+
+    setup_kfail "cris*-*-*" "ld/21233"
+
+    run_ld_link_tests [list \
 	[list \
 	    "PR ld/21233 dynamic symbols with section GC (--require-defined)" \
 	    "$LFLAGS --gc-sections -e foo --require-defined=bar\
@@ -146,14 +153,18 @@ if { [check_gc_sections_available] } {
 	    "tmpdir/libpr21233.so" "" \
 	    {pr21233.s} \
 	    {{readelf --dyn-syms pr21233.sd}} \
-	    "pr21233-2"] \
+	    "pr21233-2"]]
+
+    setup_kfail "cris*-*-*" "ld/21233"
+
+    run_ld_link_tests [list \
 	[list \
 	    "PR ld/21233 dynamic symbols with section GC (EXTERN)" \
 	    "$LFLAGS --gc-sections -e foo -T pr21233-e.ld" \
 	    "tmpdir/libpr21233.so" "" \
 	    {pr21233.s} \
 	    {{readelf --dyn-syms pr21233.sd}} \
-	     "pr21233-3"]] "cris*-*-*"
+	    "pr21233-3"]]
 }
 
 # Check to see if the C compiler works

brgds, H-P