Re: AtomicCounter::is_always_lock_free on armel

[assuming

> Cc: "libreoffice@lists.freedesktop.org Stephan Bergmann" <sbergman@redhat.com>

was a typo, and the original mail was meant to be sent to 
libreoffice@lists.freedesktop.org]

On 06/11/2019 07:05, Rene Engelhard wrote:
> LibreOffice 6.4.0 alpha1 was just accepted into Debian experimental and failed on armel
> (old arm gnueabi):
>
> In file included from /<<PKGBUILDDIR>>/vcl/source/app/svmain.cxx:90:
> /<<PKGBUILDDIR>>/vcl/inc/opengl/zone.hxx:39:34: error: static assertion failed
>     39 |     static_assert(AtomicCounter::is_always_lock_free);
>        |                   ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> make[2]: *** [/<<PKGBUILDDIR>>/solenv/gbuild/LinkTarget.mk:296: 
> /<<PKGBUILDDIR>>/workdir/CxxObject/vcl/source/app/svmain.o] Error 1
>
> If I run git blame/log I see
>
> commit ec17c8ec5256386b0197a8ffe5d7cad3e7d70f8f
> Author: Stephan Bergmann <sbergman@redhat.com>
> Date:   Tue Sep 17 20:39:43 2019 +0200
>
>      -Werror=volatile in OpenGLZone
>      
>      Recent GCC 10 trunk in C++20 mode reports issues like
>      
>      > vcl/inc/opengl/zone.hxx:37:21: error: ‘++’ expression of ‘volatile’-qualified type is deprecated [-Werror=volatile]
>      >    37 |      OpenGLZone() { gnEnterCount++; }
>      >       |                     ^~~~~~~~~~~~
>      
>      so look for a type that is more appropriate here (see the comment added to
>      vcl/inc/opengl/zone.hxx for details).  (Though calls like
>      OpenGLZone::isInZone(), comparing gnEnterCount and gnLeaveCount, in
>      OpenGLWatchdogThread::execute are still not done atomically, of course.)
>      
>      Change-Id: Ie5563addc65f629336f89cbccb73f7b9ac5e9870
>      Reviewed-on: https://gerrit.libreoffice.org/79072
>      Tested-by: Jenkins
>      Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
>
>
> which added
>
> +    // gnEnterCount and gnLeaveCount are accessed both from multiple threads (cf.
> +    // OpenGLWatchdogThread::execute; so need to be of atomic type) and from signal handlers (cf.
> +    // VCLExceptionSignal_impl; so need to be of lock-free atomic type).  sig_atomic_t is chosen as
> +    // the underlying type under the assumption that it is most likely to lead to an atomic type
> +    // that is actually lock-free.  However, gnEnterCount and gnLeaveCount are both monotonically
> +    // increasing, so will eventually overflow, so the underlying type better be unsigned, which
> +    // sig_atomic_t is not guaranteed to be:
> +    using AtomicCounter = std::atomic<std::make_unsigned_t<std::sig_atomic_t>>;
> +    static_assert(AtomicCounter::is_always_lock_free);
>
> Looking at https://en.cppreference.com/w/cpp/atomic/atomic/is_always_lock_free it is "/* implemtation 
> defined */"
> so is it always false on armel?

Yeah, my hope was that we won't ever encounter platforms where this is 
false.

> Does that mean we need to drop LibreOffice support on armel or is there some way out of it?
> (even though OpenGL is probably no thing on armel, I'd be wary to just
> remove the assert...)

Given that the code used "static volatile sal_uInt64" for 
genEnter/LeaveCount before ec17c8ec5256386b0197a8ffe5d7cad3e7d70f8f, we 
don't make things worse than they originally were if we fall back to 
that type again on armel.  So if the original code happened to work well 
enough on armel in practice, you could add an appropriate #if/else (with 
a useful comment) around the definition of AtomicCounter and the 
accompanying static_assert.  (And address any resulting -Wvolatile on 
armel as appropriate for your needs.)