[PATCH 2/3] msqid: Provide internal copy of struct __msqid64_ds
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Nov 12 14:50:32 GMT 2020
On 11/11/2020 20:57, Lukasz Majewski wrote:
> Hi Adhemerval,
>
>> On 09/11/2020 21:02, Lukasz Majewski wrote:
>>> Hi Joseph, Adhemerval,
>>>
>>>> On Mon, 9 Nov 2020, Lukasz Majewski wrote:
>>>>
>>>>>> If in a particular case defining the struct tag as a macro is
>>>>>> unsafe,
>>>>>
>>>>> Could you explain why having construct as:
>>>>>
>>>>> #define timespec __timespec64 in
>>>>> ./time/bits/types/struct_timespec.c is unsafe (after having the
>>>>> file ./time/bits/types/struct___timespec64.c exported)?
>>>>
>>>> I think "#define stat __stat64_time64" is unsafe, since people may
>>>> do "#undef stat" to get a function rather than a macro (and thereby
>>>> suppress the macro expansion for the struct tag as well).
>>>>
>>>> I'm less sure there would be any problems with "#define timespec
>>>> __timespec64". The POSIX namespace rules are clearer on e.g.
>>>> what's reserved with file scope, than on what's reserved as a
>>>> macro name that the user mustn't undefine.
>>>>
>>>
>>> Please correct me if I'm wrong, but the scheme shall be consistent.
>>> Which approach shall be taken then?
>>
>> As Joseph has put, some header name space allows the use of use
>> identifiers prefix by the implementation. However, there are
>> restriction as well of what the implementation is allowed to do,
>> check the 'Name Space' part at the POSIX definition [1].
>
> Thanks for sharing the link. Now I do understand a bit more.
>
>>
>> For instance, for the sys/msg.h interface we can export the
>> 'struct msqid64_ds' type since 'msg' is a reserve prefix. And I
>> think that doing
>>
>> #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
>> # define msqid_ds __msqid64_ds
>> #endif
>>
>> For 'sys/msg.h' should be safe as well.
>
> According to the link [1] - the "msg" prefix is reserved in the
> 'sys/msg.h', but we do #define msqid_ds - the msq prefix is not
> reserved...
>
> Is this correct?
Indeed, I overlooked this case. It seems we will need to go on the
same route I laid out for 'struct timespec' for the SysV interface
as well. It would require to change some arch-specific files I was
expecting to have to touch, but it seems there is no better option.
>
>>
>> However, doing:
>>
>> #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
>> # define timespec __timespec64
>> #endif
>>
>> It is not allowed since 'timespec' does not fall on the reserved
>> namespace of 'time.h' header.
>
> Definately - the timespec is not reserved in the time.h name space.
>
>> As indicated by Joseph, if in a
>> particular case defining the struct tag as a macro is unsafe (as for
>> timespec), we would need to redefine the struct member itself
>> depending of the _TIME_BITS.
>>
>> It means that for timespec, we will need to do something as:
>>
>> 1. On 'time/bits/types/struct_timespec.h' define 'struct timespec'
>> as:
>>
>> struct timespec
>> {
>> #if __TIMESIZE == 32 && defined __USE_TIME_BITS64
>> __tim64_t tv_sec; /* Seconds. */
>> #else
>> __time_t tv_sec;
>> #endif
>> #if __WORDSIZE == 64 \
>> || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>> || __TIMESIZE == 32
>> __syscall_slong_t tv_nsec; /* Nanoseconds. */
>> #else
>> # if __BYTE_ORDER == __BIG_ENDIAN
>> int: 32; /* Padding. */
>> long int tv_nsec; /* Nanoseconds. */
>> # else
>> long int tv_nsec; /* Nanoseconds. */
>> int: 32; /* Padding. */
>> # endif
>> #endif
>> };
>>
>
> I do follow this approach - such implementation is in [2].
Yeah, I though we would be able to simplify the boilerplate required,
but it seems we are constrained by the POSIX namespace names.
>
>>
>> 2. And for the interfaces that uses it, on time/time.h:
>>
>> [...]
>> #ifdef __USE_POSIX199309
>> [...]
>> #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
>> extern int nanosleep (const struct timespec *__requested_time,
>> struct timespec *__remaining);
>> #else
>> # ifdef __REDIRECT_NTH
>> extern int __REDIRECT_NTH (nanosleep (const struct timespec
>> *__requested_time, struct timespec *__remaining),
>> __nanosleep64);
>> # else
>> /* This might not be safe, but I don't see a better way to do it
>> without __REDIRECT_NTH support. */
>> # define nanosleep __nanosleep64
>> # endif
>>
>
> This is also done in the code from [2]. However, there is no need to
> have
> #if __TIMESIZE == 64 || !defined __USE_TIME_BITS64
> as we override it when __USE_TIME_BITS64 is defined
>
>>
>> 3. It would be good to add a internal tests to check if the export
>> ABI for timespec does in fact match the internal definition. A
>> test that does something:
>>
>> #define _TIME_SIZE 64
>>
>> _Static_assert (sizeof (struct timespec)
>> == sizeof (struct __timespec64),
>> ...);
>>
>> _Static_assert (sizeof ((struct timespec){0}.tv_sec)
>> == sizeof ((struct __timespec64){0}.tv_nsec),
>> ...);
>> _Static_assert (offsetof (struct timespec, tv_sec)
>> == offsetof (struct __timespec64, tv_sec),
>> ...);
>> _Static_assert (sizeof ((struct timespec){0}.tv_nsec)
>> == sizeof ((struct __timespec64){0}.tv_nsec),
>> ...);
>> _Static_assert (offsetof (struct timespec, tv_nsec)
>> == offsetof (struct __timespec64, tv_nsec),
>> ...);
>>
>
> This would be welcome, I will add such simple check when I add
> __USE_TIME_BITS64 support (i.e. Y2038 support).
>
>> I don't think it needs test for the internal fields (unless the
>> implementation does use it in some way, which I don't think it is the
>> case).
>>
>
> Ok.
>
>>
>> 4. For timespec we should change all symbols that uses it,
>> including compound types.
>
> Yes - this is also done in patch [2]. For example struct itimerspec is
> adjusted as well.
>
>>
>>
>> 5. We need to make sure glibc does not build or define _TIME_SIZE
>> internally (we already have issues if user tries to enable the LFS
>> support, so it should be ok as-is with default values).
>
> Do you think about _TIME_BITS=64 when the code is compiled
> (-D_TIME_BITS=64)?
> When -D_TIME_BITS=64 is passed during compilation of user program, the
> __USE_TIME_BITS64 is set to 1 in exported headers, so aliasing for
> Y2038 is enabled.
I meant building glibc itself, not user programs using glibc provided
headers. It is to ensure that internally 'struct timespec' (or any
affected struct by 64-bit time) will have 32-bit ABI layout for ABIs
that supports it since glibc itself includes installed headers.
>
> I'm not sure about the role of _TIME_SIZE. I only recall the __TIMESIZE
> which is set depending on __WORDSIZE (or explicitly to 64 on e.g.
> RISC-V).
My understanding is _TIME_SIZE will be the compiler flag used by the
programs to define which time ABI they want to use.
>
>>
>>
>> I think I have outlined what is required, but I might have overlooked
>> something.
>
> So to sum up:
>
> - When prefix is reserved in POSIX namespaces [1] it may be allowed to
> export __foo64 struct and make the aliasing in the corresponding
> exported header.
>
> - Otherwise, we need an internal glibc copy of the struct (i.e. struct
> __timespec64) and also corresponding exported struct (i.e. struct
> timespec) needs to be adjusted when __USE_TIME_BITS64 is defined.
>
> The second case seems to be pervasive and is already utilized in my WIP
> patch [2].
I think to simplify we should for [2] as you initially suggested. I
though we could mimic LFS support, but for y2038 support it seems that
we will need to re-define the types without using different names for the
affected types.
>
>>
>>
>> [1]
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
>>
>
>
> Links:
>
> [2] -
> https://github.com/lmajewski/y2038_glibc/commit/b5d9a0881ccc0a06e520c495ce7409f8e077fb20#diff-8297a1191cc06a8a7af6fc6ac33af4bef2d7d72efd901d1634d910b56631e0c7R13
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20201112/72be74cd/attachment-0001.sig>
More information about the Libc-alpha
mailing list