Message ID | 20160615154119.GS4053@vapier.lan |
---|---|
State | Superseded |
Headers |
Received: (qmail 91808 invoked by alias); 15 Jun 2016 15:41:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 90948 invoked by uid 89); 15 Jun 2016 15:41:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=34310, Hx-languages-length:2888, personal, policy X-HELO: smtp.gentoo.org Date: Wed, 15 Jun 2016 11:41:19 -0400 From: Mike Frysinger <vapier@gentoo.org> To: Florian Weimer <fweimer@redhat.com> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>, libc-alpha@sourceware.org Subject: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation Message-ID: <20160615154119.GS4053@vapier.lan> Mail-Followup-To: Florian Weimer <fweimer@redhat.com>, Adhemerval Zanella <adhemerval.zanella@linaro.org>, libc-alpha@sourceware.org References: <1465941275-3459-1-git-send-email-adhemerval.zanella@linaro.org> <20160615053714.GM4053@vapier.lan> <177f35c1-5934-00c7-a24b-901947505fee@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Xr7TkeU61TuIa/oQ" Content-Disposition: inline In-Reply-To: <177f35c1-5934-00c7-a24b-901947505fee@redhat.com> |
Commit Message
Mike Frysinger
June 15, 2016, 3:41 p.m. UTC
On 15 Jun 2016 08:50, Florian Weimer wrote: > On 06/15/2016 07:37 AM, Mike Frysinger wrote: > >> +#define FAIL() \ > >> + do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0) > >> ... > >> + ret = pwritev (temp_fd, iov, 2, 0); > >> + if (ret == -1) > >> + FAIL (); > > > > as a personal stance, never been a big fan of messages that just have a > > line number. when you get reports/logs, you end having to chase down the > > exact same source tree as the reporter. > > At least there is a line number. Many existing tests do not even have that. > > > why can't we just assert() everywhere ? we rely on that in tests already > > and we wouldn't have to do any checking otherwise. > > ret = pwritev (temp_fd, iov, 2, 0); > > assert (ret == sizeof buf1 + sizeof buf2); > > assert writes to standard error, not standard output, where it will be > captured. I don't know why we don't capture both. we keep running into this issue. rather than try to be super vigilent all the time, why don't we simply: > There seems to be a policy against tests aborting on failure, too, but > we are not very consistent about that. In my experience, people who > build and test glibc are more likely to shrug off tests with non-zero > exit status than to ignore tests with aborts, although they really > should treat them the same. we've actively been adding tests with abort() usage. i'm not sure there is a policy against it. i'm not sure i agree on the non-zero-vs-abort issue, but i'm not everyone. i look at all tests that fail regardless of reason. > I think we should really have some sort of libtest.a, which provides > test helpers, error-checking functions such as xmalloc, and general > helpers for setting up special test environments. I'm a bit worried > about figuring out the proper dependencies, so that libtest.a is built > before all the tests are linked. isn't this what test-skeleton.c does for us now ? and we all agree that all tests should be using that. the only downside is the inclusion ordering ... a lot of tests do either: (1) at top (allows helper funcs to be used in the test) static int do_test (void); #define TEST_FUNCTION do_test () #include "../test-skeleton.c" (2) at bottom (does not allow access to helper funcs) #define TEST_FUNCTION do_test () #include "../test-skeleton.c" imo we should just mandate all tests use the entry point "do_test" and it take argc/argv args (even if they're unused), and then all tests can pull test-skeleton.c in at the top. -mike
Comments
On 06/15/2016 05:41 PM, Mike Frysinger wrote: >> I think we should really have some sort of libtest.a, which provides >> test helpers, error-checking functions such as xmalloc, and general >> helpers for setting up special test environments. I'm a bit worried >> about figuring out the proper dependencies, so that libtest.a is built >> before all the tests are linked. > > isn't this what test-skeleton.c does for us now ? and we all agree that > all tests should be using that. There are limits to what we can put into test-skeleton.c due to symbol conflicts and dependencies of the test harness which are incompatible with some things which we want to test. With libtest.a, we can still have clean compilation environments for tests, and the linker will sort out things for us. (We can even have ELF constructors which get pulled into the link as needed.) > imo we should just mandate all tests use the entry point "do_test" and it > take argc/argv args (even if they're unused), and then all tests can pull > test-skeleton.c in at the top. Yes, seems a reasonable improvement, and mostly unrelated. Thanks, Florian
On 22 Jun 2016 11:57, Florian Weimer wrote: > On 06/15/2016 05:41 PM, Mike Frysinger wrote: > >> I think we should really have some sort of libtest.a, which provides > >> test helpers, error-checking functions such as xmalloc, and general > >> helpers for setting up special test environments. I'm a bit worried > >> about figuring out the proper dependencies, so that libtest.a is built > >> before all the tests are linked. > > > > isn't this what test-skeleton.c does for us now ? and we all agree that > > all tests should be using that. > > There are limits to what we can put into test-skeleton.c due to symbol > conflicts and dependencies of the test harness which are incompatible > with some things which we want to test. symbol-wise, i don't think there's a realistic problem. we've rarely (ever?) run into this problem, and it's trivial to sort out by using a namespace prefix like "tst_". which we probably want to do anyways to keep the API more readable. dependency wise (e.g. dl or pthread usage), i'd wait until the need actually arises before i'd start worrying about it. > With libtest.a, we can still have clean compilation environments for > tests, and the linker will sort out things for us. (We can even have > ELF constructors which get pulled into the link as needed.) i don't think i'd agree with that. if our test API used tst_xxx and random tests started interposing their own variant, that's just ugly and a pita to digest to the point where it shouldn't really be allowed. > > imo we should just mandate all tests use the entry point "do_test" and it > > take argc/argv args (even if they're unused), and then all tests can pull > > test-skeleton.c in at the top. > > Yes, seems a reasonable improvement, and mostly unrelated. they are related when you look at the implications of the two paths. if we have a single test-skeleton.c, having a sep header for all the prototypes is kind of pointless and just busy work. but if there's a lib of test files, then having a sep header is pretty much required. if there isn't a sep header, then the include point of the skeleton must be at the top since it's acting as the header. -mike
On 06/22/2016 01:05 PM, Mike Frysinger wrote: > On 22 Jun 2016 11:57, Florian Weimer wrote: >> On 06/15/2016 05:41 PM, Mike Frysinger wrote: >>>> I think we should really have some sort of libtest.a, which provides >>>> test helpers, error-checking functions such as xmalloc, and general >>>> helpers for setting up special test environments. I'm a bit worried >>>> about figuring out the proper dependencies, so that libtest.a is built >>>> before all the tests are linked. >>> >>> isn't this what test-skeleton.c does for us now ? and we all agree that >>> all tests should be using that. >> >> There are limits to what we can put into test-skeleton.c due to symbol >> conflicts and dependencies of the test harness which are incompatible >> with some things which we want to test. > > symbol-wise, i don't think there's a realistic problem. we've rarely > (ever?) run into this problem, and it's trivial to sort out by using > a namespace prefix like "tst_". which we probably want to do anyways > to keep the API more readable. What I mean is that the test harness pulls in stuff that cause things to interfere with what we want to test. This could be magic symbols for stdio/libio compatibility, pthread symbols, or just calling mallopt (as discussed before). If the test helpers are not in just one monolithic .c file, it helps with achieving that. > dependency wise (e.g. dl or pthread usage), i'd wait until the need > actually arises before i'd start worrying about it. The need for pthread error-checking wrappers is already there. Our current policy is to check error returns from *all* pthread functions, and getting a nice error from the usually looks like this: int ret = ...; if (ret != 0) { errno = ret; printf ("error: ...: %m\n"); _exit (1); } with some special exceptions such us pthread_barrier_wait. Many tests are littered with that. >>> imo we should just mandate all tests use the entry point "do_test" and it >>> take argc/argv args (even if they're unused), and then all tests can pull >>> test-skeleton.c in at the top. >> >> Yes, seems a reasonable improvement, and mostly unrelated. > > they are related when you look at the implications of the two paths. > if we have a single test-skeleton.c, having a sep header for all the > prototypes is kind of pointless and just busy work. but if there's a > lib of test files, then having a sep header is pretty much required. > if there isn't a sep header, then the include point of the skeleton > must be at the top since it's acting as the header. In both cases, we want the include at the top, so the change your proposed goes in the right direction, whether we want a libtest.a or not. Thanks, Florian
On Thu, 23 Jun 2016, Florian Weimer wrote: > What I mean is that the test harness pulls in stuff that cause things to > interfere with what we want to test. This could be magic symbols for > stdio/libio compatibility, pthread symbols, or just calling mallopt (as > discussed before). > > If the test helpers are not in just one monolithic .c file, it helps with > achieving that. Also, test-skeleton.c doesn't work well with tests that wish to undefine _LIBC and _GNU_SOURCE in order to test non-GNU feature test macros / APIs / strict standards conformance options, since it uses GNU APIs itself. If we change things so that most of the functionality is in a separate library or object (built with the usual _GNU_SOURCE), and test-skeleton itself is very minimal, then more tests can be made to use it. (Of course, doing this means doing something about the various macros tests can define to change how test-skeleton behaves.)
--- a/test-skeleton.c +++ b/test-skeleton.c @@ -343,6 +343,10 @@ main (int argc, char *argv[]) setbuf (stdout, NULL); #endif + fclose (stderr); + dup2 (STDOUT_FILENO, STDERR_FILENO); + stderr = fdopen (STDERR_FILENO, "w"); + while ((opt = getopt_long (argc, argv, "+", options, NULL)) != -1) switch (opt) {