This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[review] nptl: Add tests for internal pthread_rwlock_t offsets

Adhemerval Zanella has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/516
......................................................................


Patch Set 1:

(3 comments)

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?

Done


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 th […]

The __flags cover the static initializers, which is what really defines the ABI.  The other members might be used along with a shared memory (PTHREAD_PROCESS_SHARED), but I don't think we support using shared pthread objects across different glibc versions.

I have added all the fields to certify the subsequent code refactoring (that add rwlock-internal.h) do not change the current internal layout.

I will add a comment stating that ABI-wise only __flags should be checked, the other fields are an implementation detail that can be changed over the releases.


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 don't have a strong opinion here.



-- 
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: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 17:06:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: comment
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]