Re: AtomicCounter::is_always_lock_free on armel

[Rene Engelhard <rene@debian.org>]

Hi,

On Wed, Nov 06, 2019 at 09:26:53AM +0100, Stephan Bergmann wrote:
> > +    // 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.)

Given the introduced AtomicCounter is used later, too I tried the simplified

diff --git a/vcl/inc/opengl/zone.hxx b/vcl/inc/opengl/zone.hxx
index 3210186c3096..13ac3bf6793e 100644
--- a/vcl/inc/opengl/zone.hxx
+++ b/vcl/inc/opengl/zone.hxx
@@ -36,7 +36,9 @@ class VCL_DLLPUBLIC OpenGLZone {
     // 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>>;
+#if !defined ARM32 && !defined __ARM_PCS_VFP
     static_assert(AtomicCounter::is_always_lock_free);
+#endif
 
     /// how many times have we entered a GL zone
     static AtomicCounter gnEnterCount;

(atking that define from bridges...)

and that builds...

-> https://gerrit.libreoffice.org/#/c/82165/

Regards,

Rene