[review] nptl: Add tests for internal pthread_rwlock_t offsets

[Florian Weimer (Code Review)]

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