Re: svn commit: r1868502 - /apr/apr/trunk/atomic/win32/apr_atomic64.c

[Ivan Zhakov <iv...@visualsvn.com>]

On Wed, 30 Oct 2019 at 13:18, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Oct 30, 2019 at 4:37 AM William A Rowe Jr <wr...@rowe-clan.net> wrote:
> >
> > On Wed, Oct 16, 2019 at 5:11 AM <iv...@apache.org> wrote:
> >>
> >> Author: ivan
> >> Date: Wed Oct 16 10:10:59 2019
> >> New Revision: 1868502
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1868502&view=rev
> >> Log:
> >> * atomic/win32/apr_atomic64.c
> >>   (apr_atomic_read64): Use direct memory read when compiled for x86_x64, since
> >>    64-bit reads are atomic in 64-bit Windows [1].
> >>
> >> +    /* https://docs.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access
> >> +     * "Simple reads and writes to properly aligned 64-bit variables are atomic
> >> +     * on 64-bit Windows."*/
> >> +    return *mem;
> >
> > Where are we[1] ensuring *mem is aligned on an 8 byte boundary?
>
> Since mem is apr_uint64_t, unless the caller is playing nasty casts we
> should be good, I think.
+1.

Also InterlockedCompareExchange64() as all other interlocked function
requires aligned data [1]. So APR already assumes that data is
properly aligned.

[1] https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64

-- 
Ivan Zhakov

Re: svn commit: r1868502 - /apr/apr/trunk/atomic/win32/apr_atomic64.c

[William A Rowe Jr <wr...@rowe-clan.net>]

On Fri, Nov 1, 2019 at 1:45 AM Ivan Zhakov <iv...@visualsvn.com> wrote:

> On Wed, 30 Oct 2019 at 13:18, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Wed, Oct 30, 2019 at 4:37 AM William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> > >
> > > On Wed, Oct 16, 2019 at 5:11 AM <iv...@apache.org> wrote:
> > >>
> > >> Author: ivan
> > >> Date: Wed Oct 16 10:10:59 2019
> > >> New Revision: 1868502
> > >>
> > >> URL: http://svn.apache.org/viewvc?rev=1868502&view=rev
> > >> Log:
> > >> * atomic/win32/apr_atomic64.c
> > >>   (apr_atomic_read64): Use direct memory read when compiled for
> x86_x64, since
> > >>    64-bit reads are atomic in 64-bit Windows [1].
> > >>
> > >> +    /*
> https://docs.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access
> > >> +     * "Simple reads and writes to properly aligned 64-bit variables
> are atomic
> > >> +     * on 64-bit Windows."*/
> > >> +    return *mem;
> > >
> > > Where are we[1] ensuring *mem is aligned on an 8 byte boundary?
> >
> > Since mem is apr_uint64_t, unless the caller is playing nasty casts we
> > should be good, I think.
>

I was envisioning more packed struct alignments...


> +1.
>
> Also InterlockedCompareExchange64() as all other interlocked function
> requires aligned data [1]. So APR already assumes that data is
> properly aligned.
>
> [1]
> https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64


Agreed. It would be good to leave a breadcrumb in the doxygen that we
require the pointers are using correct word alignment, and even the patch
I hinted at wouldn't be improved given the ICEx docs.