Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols

[Hans-Peter Nilsson <hp at bitrange dot com>]

On Wed, 11 Jul 2012, Maciej W. Rozycki wrote:
> On Tue, 8 May 2012, Hans-Peter Nilsson wrote:
>
> > > 	ld/testsuite/
> > > 	* ld-vax-elf/plt-local-lib.dd: New test.
> > > 	* ld-vax-elf/plt-local-lib.ld: New test linker script.
> > > 	* ld-vax-elf/plt-local-lib.s: New test source.
> > > 	* ld-vax-elf/plt-local.dd: New test.
> > > 	* ld-vax-elf/plt-local.ld:New test linker script.
> > > 	* ld-vax-elf/plt-local.s: New test source.
> > > 	* ld-vax-elf/plt-local-hidden-pic.s: New test source.
> > > 	* ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> > > 	* ld-vax-elf/vax-elf.exp: New test script.
> >
> > > Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > > ===================================================================
> > > --- /dev/null
> > > +++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > > @@ -0,0 +1,50 @@
> > > +# Expect script for VAX ELF linker tests
> >
> > ...
> > > +
> > > +run_ld_link_tests [list \
> > > +    [list "PLT test (shared library)" \
> > > +	  "-shared -T plt-local-lib.ld" \
> > > +	  "-k" \
> > > +	  { plt-local-lib.s } \
> > > +	  { { objdump -d plt-local-lib.dd } } \
> > > +	  "plt-local-lib.so"] \
> > > +    [list "PLT test (object 1)" \
> > > +	  "-r" \
> >
> > ... (pruned for brevity)
> >
> > Ouch, another one of those tables-of-lists-of-tables and in a
> > brand-new .exp.
> >
> > Let me suggest instead using a mechanism such as run_dump_test
> > with depended parts (DSO's linked against) sorted before others.
> > This'd give easy drop-in of new tests without having to add to a
> > separate table entry somewhere.  See ld-cris/cris.exp.  Similar
> > new test: ld-arm/gc-hidden-1.d (unfortunately there's no
> > file-name iterator in arm-elf.exp, entries are still added
> > manually).
>
>  So I have found some time to look into your suggestion, and frankly, I am
> not happy with it at all.  In my opinion your approach merely avoids the
> shortcomings of the LD test framework in a different way.  If I could
> honestly say it is a solution, then I would certainly be eager to adopt
> it.
>
>  I find it to be a workaround because:
>
> 1. The dependency between the final executable and any shared libraries is
>    not expressed anywhere except from the linker flags in the ld:
>    specifier.  Of course keeping the dependency in a single place is good,
>    but the way it works in your scheme it doesn't actually cause the
>    framework to take it into account in any way, it just depends on
>    building all the shared libraries first.

That's the only drawback, IMO, and no worse than for tables.
Just add (framework for a) "dependency: <renamed-output-file>
<name-of-.d-file-for-previous-test>" and presto, it's explicit.
It's possible without changes to the tests themselves, in theory
at least, if run_dump_test sneaks a peek at the options in "ld:"
and "ld_after_inputfiles:" and arranges the order and naming of
the output files (mapping between .d and .so).  But, I'd prefer
the more explicit keyword.

> 2. Test cases are not grouped together anymore, if for example a test case
>    consists of a bunch of shared libraries and a bunch of executables
>    linked against them, then the shared libraries are tested at a
>    different stage than the executables.  That may make following the
>    dependencies a bit more difficult.

(This seems to me the same point as above.)  My take is that the
grouping is cluttering: I see them as different test-cases, so
they *should* be separate.  The order is no better or more
explicit with the ordered-in-the-table-approach.  The nacl-tests
made that clear; there were implicit orders in the table that
were hard enough to follow, that when the tests were rearranged
for one reason, the implicit order was broken.

> 3. Test cases are run implicitly -- you see it as an advantage, I do not.
>    I find such test suites harder to follow and prefer being able to add
>    any extra commentary or other arrangements in .exp files (cf. the MIPS
>    test suite and the "foreach { abi flag emul } $abis { ... }" loop
>    there).

It seems we are just of different opinions here with no factual
component.  Comments can be added nearby the options in both
cases.  The "foreach-cases" are IMHO just harder to follow, but
you can do that equally simple with the run_dump_test-based
test-cases if you really want.

> 4. It resorts to some fragile file shuffling code so that proper .so
>    objects are created.

(Still the same point as 1 and 2!)  Fragility is certainly in
the order in the table too, n.b. recent nacl-tests mentioned
above (broken-test-case complaints in the list archives).  At
least it's more explicit with the "fragile file shuffling code".

>  On the contrary my proposal makes it clearly a self-contained test case,

No self-containedness as far as I see it: you have to edit more
than one file for each new test; the new test and the framework
bits and flags to run it.  This is noticeable when you review a
patch adding more than one test-case in that you have to
double-check that the patch adds the framework bits for all
tests.

> it is easily readable and the only drawback is it uses an unnecessary
> incremental link because run_ld_link_tests does not support stopping after
> assembly for intermediate steps.

A drawback, there.

>  So yes, it has shortcomings, but your
> proposal has no clear advantage to me I am afraid.

Really?  For one, there is no risk of forgetting to update the
framework .exp file to actually run the test with the looped *.d
approach.  (Yes, I remember at least one such case in the past.)

Another is that you have the test-case (the expected output or
error text), and the flags to build it all in one single place.
And that same place for comments too.

A third is that with the tabled run_dump_test approach, there
are more options already present than for run_ld_link_tests.
Not a big thing, for example xfail is easy to add, seven lines.

Reading the ld-lib.exp source, I also see a fourth: a bug making
the machinery that enables you to run a subset of the tests
ineffective; for run_ld_link_tests; the $testname has to match.
(For run_dump_test cases, looped or explicit, this is always the
filename.)

A fifth follows; the testname isn't optional.  I prefer seeing
"FAIL: name-of-.d-file/file-with-expected-output" to "FAIL:
elaborate name".  It might be a matter of taste, but at least I
have the option for run_dump_tests (looped-implicit or
explicit).

>  NB the first shared-library dump is not strictly necessary and does not
> check anything related to this bug, I just added it to have extra
> coverage, because -- as you must have undoubtedly noticed -- current one
> for the VAX ELF target is particularly poor.  Your proposal does not
> address cases where a dump of any intermediate shared libraries is not
> required and in my version you can just remove the "objdump" clause.

But without the objdump or equivalent analyzer, you don't really
have the extra coverage which you apparently prefer. ;)
I always prefer it, so I don't see any inconvenience in the
extra line or three.  Patches for a keyword skipping the
analyzer are likely welcome; a two- or three-liner to
ld-lib.exp.  In the meantime, the more cryptic "PROG: :" and
"#pass" should work.

>  So I think the test case is as good as it could be as it stands.

Sure, I definitely have no intention to block test-cases, I was
just suggesting using a different and IMO preferable framework,
at the discretion of the author; you, and any approver.

>  That said however, I do not object discussing any improvement to the LD
> test framework.  Including in particular a way to pull shared library
> dependencies automatically from .d files.  I have a vague shape of how
> this might look like in my mind,

See above "dependency: <renamed-output-file>
<name-of-.d-file-for-previous-test>" for run_dump_test-based
tests.  Doesn't carry over to tabled run_ld_link_tests, but
that's IMHO another flaw in that machinery. :)

brgds, H-P