Re: [PATCH v3 00/17] Consolidate Linux sysvipc implementation

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 09/12/2016 16:30, Steve Ellcey wrote:
> On Wed, 2016-12-07 at 17:13 -0200, Adhemerval Zanella wrote:
>> Ping x2.
>>
>>> On 08/11/2016 18:29, Adhemerval Zanella wrote:
>>>>
>>>> Changes from previous version:
>>>>
>>>>   - Refactor ipc_priv.h header to have working definition for all
>>>>     architectures.  The header basically contains Linux specific
>>>>     ABI definitions for the architecture to correctly use default
>>>>     implementation, including old ipc_perm definition, calling
>>>>     convention, and __IPC_64 value.
>>>>
>>>>   - Set all architectures to use the default implementation.  It
>>>>     leaded to some code changes to accomodate all the
>>>> architectures
>>>>     calling convention (with ipc_priv.h change) and some fixes
>>>>     on new default implementation.
>>>>
>>>>   - Change mips64 implementation way to use the default one.
>>>>
>>>>   - Some tests changes (name typos).
>>>>
>>>> --
> 
> Adhemerval,
> 
> It looks like you may have to tweek your change to test-skeleton.c in
> this patch.  The FAIL_EXIT macro was moved from test-skeleton.c
> to support/check.h so I assume your new FAIL_UNSUPPORTED macro should
> be moved there as well.

Yes, I did not rebase against new source changes and looks like I will
need to do so.  And it is worth a v4, so I will send it a new set for
review.

> 
> I have been doing aarch64 and x86 builds here with your patch and have
> not had any problems except with my new aarch64 ILP32 code.  There I
> had to change the definition of __IPC_64 in the new header
> file sysdeps/unix/sysv/linux/aarch64/ipc_priv.h from:
> 
> #define __IPC_64     0x0
> 
> to:
> 
> #ifdef __LP64__
> # define __IPC_64     0x0
> #else /* __ILP32  */
> # define __IPC_64     0x100
> #endif
> 
> I don't know if that is something that you want to incorporate into
> your patch or if I should leave it as part of my overall aarch64 ILP32
> patch.   Right now it is hard to include it in my patch since that
> header file isn't in the official sources yet (being a new file in your
> patch).

If this is the correct value for ILP32 I will add, but from Arnd reply
it might an issue with your kernel config.

> 
> Hopefully someone will approve your patch soon and then I can just
> build my patch on ToT instead of having to also include your patch.
> I don't see any other problems with your patch myself but I am not that
> familiar with this part of glibc so I would like to see someone else
> review/approve this patch before you check it in but I would also like
> to see it checked in soon so that I can move ahead with my aarch64
> ILP32 patches before we get too close to the GLIBC 2.25 cutoff date.

My idea is to push this next week to avoid issues with cutoff date.

> 
> Steve Ellcey
> sellcey@caviumnetworks.com
>