From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24356 invoked by alias); 11 Jul 2012 03:29:33 -0000 Received: (qmail 24347 invoked by uid 22791); 11 Jul 2012 03:29:31 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED,RCVD_IN_HOSTKARMA_NO,TW_BJ X-Spam-Check-By: sourceware.org Received: from dair.pair.com (HELO dair.pair.com) (209.68.1.49) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Wed, 11 Jul 2012 03:29:18 +0000 Received: (qmail 2296 invoked by uid 20157); 11 Jul 2012 03:29:16 -0000 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 11 Jul 2012 03:29:16 -0000 Date: Wed, 11 Jul 2012 03:29:00 -0000 From: Hans-Peter Nilsson To: "Maciej W. Rozycki" cc: Jan-Benedict Glaw , binutils@sourceware.org Subject: Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2012-07/txt/msg00106.txt.bz2 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: " 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: " 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