This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[review] nptl: Add tests for internal pthread_rwlock_t offsets
- From: "Florian Weimer (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, libc-alpha at sourceware dot org
- Cc: Florian Weimer <fweimer at redhat dot com>
- Date: Thu, 7 Nov 2019 11:08:14 -0500
- Subject: [review] nptl: Add tests for internal pthread_rwlock_t offsets
- Auto-submitted: auto-generated
- References: <gerrit.1573136666000.Iccc103d557de13d17e4a3f59a0cad2f4a640c148@gnutoolchain-gerrit.osci.io>
- Reply-to: gnutoolchain-gerrit at osci dot io
Florian Weimer has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Still need to run build-many-glibcs.py against this patch.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1//COMMIT_MSG
Commit Message:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1//COMMIT_MSG@16
PS1, Line 16:
11 | field layout should be preserved due static initializators, the patch
12 | also adds tests for the futexes that may be used in a shared memory
13 | (although using different libc version in such scenario is not really
14 | supported).
15 |
16 > Checked with a build against all affected ABIs.
17 |
18 | Change-Id: Iccc103d557de13d17e4a3f59a0cad2f4a640c148
Did you build the tests too?
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1/nptl/pthread_rwlock_init.c
File nptl/pthread_rwlock_init.c:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1/nptl/pthread_rwlock_init.c@51
PS1, Line 51:
33 | __pthread_rwlock_init (pthread_rwlock_t *rwlock,
| ...
46 | ASSERT_PTHREAD_INTERNAL_OFFSET (pthread_rwlock_t, __data.__cur_writer,
47 | __PTHREAD_RWLOCK_CUR_WRITER_OFFSET);
48 | ASSERT_PTHREAD_INTERNAL_OFFSET (pthread_rwlock_t, __data.__shared,
49 | __PTHREAD_RWLOCK_SHARED_OFFSET);
50 | ASSERT_PTHREAD_INTERNAL_OFFSET (pthread_rwlock_t, __data.__flags,
51 > __PTHREAD_RWLOCK_FLAGS_OFFSET);
52 |
53 | const struct pthread_rwlockattr *iattr;
54 |
55 | iattr = ((const struct pthread_rwlockattr *) attr) ?: &default_rwlockattr;
56 |
Do we really need to assert on all those implementation details? Or is covering __flags enough? I think from an ABI perspective, only the position of the non-zero bits in __flags matters.
If you want to leave in all the asserts, I'd suggest to single out __flags with a comment.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1/sysdeps/mips/nptl/pthread-offsets.h
File sysdeps/mips/nptl/pthread-offsets.h:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516/1/sysdeps/mips/nptl/pthread-offsets.h@36
PS1, Line 36:
31 | # define __PTHREAD_RWLOCK_FLAGS_OFFSET 27
32 | # else
33 | # define __PTHREAD_RWLOCK_SHARED_OFFSET 25
34 | # define __PTHREAD_RWLOCK_FLAGS_OFFSET 24
35 | # endif
36 > #endif
I wonder if that should be handled with sysdeps overrides instead of conditionals?
I kind of like the conditionals because they better highlight commonalities, though.
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Iccc103d557de13d17e4a3f59a0cad2f4a640c148
Gerrit-Change-Number: 516
Gerrit-PatchSet: 1
Gerrit-Owner: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 16:08:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment