Re: AtomicCounter::is_always_lock_free on armel

classic Classic list List threaded Threaded
8 messages Options
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

[assuming

> Cc: "[hidden email] Stephan Bergmann" <[hidden email]>

was a typo, and the original mail was meant to be sent to
[hidden email]]

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 <[hidden email]>
> 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 <[hidden email]>
>
>
> 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.)

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
rene.engelhard rene.engelhard
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

Hi,

Am 6. November 2019 09:26:53 MEZ schrieb Stephan Bergmann <[hidden email]>:
>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

It built. No more data ;-)

, you could add an appropriate #if/else
>(with
>a useful comment) around the definition of AtomicCounter and the
>accompanying static_assert.  

Can do, yes, although I would like it more if it was fine upstream...

> (And address any resulting -Wvolatile on
>armel as appropriate for your needs.)

As it (is it?) only a warning one can also just ignore it ;-)

Regards

Rene

--
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

On 06/11/2019 10:47, [hidden email] wrote:
> Am 6. November 2019 09:26:53 MEZ schrieb Stephan Bergmann <[hidden email]>:
> , you could add an appropriate #if/else
>> (with
>> a useful comment) around the definition of AtomicCounter and the
>> accompanying static_assert.
>
> Can do, yes, although I would like it more if it was fine upstream...

That's what I meant, provide an upstream commit.

>> (And address any resulting -Wvolatile on
>> armel as appropriate for your needs.)
>
> As it (is it?) only a warning one can also just ignore it ;-)

Yes, that's likely all you need to do to address it appropriately for
your needs.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Rene Engelhard Rene Engelhard
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

In reply to this post by sberg
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
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

On 06/11/2019 18:39, Rene Engelhard wrote:

> 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/

I don't understand your "Given the introduced AtomicCounter is used
later..." reasoning above, but commented at
<https://gerrit.libreoffice.org/#/c/82165/> "disable static_assert on
AtomicCounter::is_always_lock_free on armel ..." now.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Rene Engelhard Rene Engelhard
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

Hi,

On Thu, Nov 07, 2019 at 10:18:06AM +0100, Stephan Bergmann wrote:

> On 06/11/2019 18:39, Rene Engelhard wrote:
> > 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/
>
> I don't understand your "Given the introduced AtomicCounter is used
> later..." reasoning above, but commented at

Obviously I meant

static AtomicCounter gnEnterCount;

which comes later (and wasn't there before)
and uses the using AtomicCounter = [...] definition, so
#ifdef'ing the whole block out would have been more invasive.

> <https://gerrit.libreoffice.org/#/c/82165/> "disable static_assert on
> AtomicCounter::is_always_lock_free on armel ..." now.

I don't care either way, if it trades some flakiness with some other
flakiness this is (as you would too based on the comment) somethinbg I'd
trade on for this "obsolete" architecture.

The patch is already in the current Debian upload ;-)

Regards,

Rene
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

On 07/11/2019 17:56, Rene Engelhard wrote:

> On Thu, Nov 07, 2019 at 10:18:06AM +0100, Stephan Bergmann wrote:
>> I don't understand your "Given the introduced AtomicCounter is used
>> later..." reasoning above, but commented at
>
> Obviously I meant
>
> static AtomicCounter gnEnterCount;
>
> which comes later (and wasn't there before)
> and uses the using AtomicCounter = [...] definition, so
> #ifdef'ing the whole block out would have been more invasive.

Ah.  What I'd meant was something like

> #if ...
> using AtomicCounter = std::atomic<std::make_unsigned_t<std::sig_atomic_t>>;
> static_assert(AtomicCounter::is_always_lock_free);
> #else
> using AtomicCounter = volatile std::make_unsigned_t<std::sig_atomic_t>;
> #endif

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Rene Engelhard Rene Engelhard
Reply | Threaded
Open this post in threaded view
|

Re: AtomicCounter::is_always_lock_free on armel

Hi,

On Fri, Nov 08, 2019 at 09:05:12AM +0100, Stephan Bergmann wrote:
> Ah.  What I'd meant was something like
>
> > #if ...
> > using AtomicCounter = std::atomic<std::make_unsigned_t<std::sig_atomic_t>>;
> > static_assert(AtomicCounter::is_always_lock_free);
> > #else
> > using AtomicCounter = volatile std::make_unsigned_t<std::sig_atomic_t>;
> > #endif

OK. changed and updated in gerrit. Verified in a armel build on
abel.debian.org and in my local amd64 build.

Regards,

Rene
>
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice