[Bug libstdc++/109741] New: alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[Bug libstdc++/109741] New: alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[janezz55 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

            Bug ID: 109741
           Summary: alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: janezz55 at gmail dot com
  Target Milestone: ---

This line:

struct alignas(64) M : __gnu_cxx::__mutex { };

has been an eyesore for me for a number of years. I propose to change it into:

struct alignas(std::max_align_t) M : __gnu_cxx::__mutex { };

in which case <cstddef> needs to be #included as well. The magic number 64
alone should be raising some eyebrows and some targets do not support this
alignment.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[janezz55 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #1 from Janez Zemva <janezz55 at gmail dot com> ---
alternatively, the line could be changed into:

struct alignas(void*) M : __gnu_cxx::__mutex { };

since this was probably meant with the magic number 64.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think it should really be either __GCC_DESTRUCTIVE_SIZE or
__GCC_CONSTRUCTIVE_SIZE (I always forgot which one it really).

>some targets do not support this alignment.
Which targets don't support alingment of 64bytes?

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Janez Zemva from comment #1)
> alternatively, the line could be changed into:
> 
> struct alignas(void*) M : __gnu_cxx::__mutex { };
> 
> since this was probably meant with the magic number 64.

The magic number 64 is the cache line size to make sure there are less sharing.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> (In reply to Janez Zemva from comment #1)
> > alternatively, the line could be changed into:
> > 
> > struct alignas(void*) M : __gnu_cxx::__mutex { };
> > 
> > since this was probably meant with the magic number 64.
> 
> The magic number 64 is the cache line size to make sure there are less
> sharing.

That is even the comment:
    // increase alignment to put each lock on a separate cache line

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[janezz55 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #5 from Janez Zemva <janezz55 at gmail dot com> ---
This line has been patched out by djgpp builds for a long time now.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=80208

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh that has a max file object alignment of 16 bytes. Might be the only target
that is that low.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-05-04
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think the fix would be export MAX_OFILE_ALIGNMENT as a predefined macro, say
__GCC_MAX_OFILE_ALIGNMENT and then do the following libstdc++:

__GCC_DESTRUCTIVE_SIZE > __GCC_MAX_OFILE_ALIGNMENT ? __GCC_MAX_OFILE_ALIGNMENT 
: __GCC_DESTRUCTIVE_SIZE 

Confirmed.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[janezz55 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #8 from Janez Zemva <janezz55 at gmail dot com> ---
Here is the compile error:
../../../../../gcc-13.1.0/libstdc++-v3/src/c++11/shared_ptr.cc: In function
'__gnu_cxx::__mutex& __gnu_internal::get_mutex(unsigned char)':
../../../../../gcc-13.1.0/libstdc++-v3/src/c++11/shared_ptr.cc:42:44: error:
requested alignment '64' exceeds object file maximum 16
   42 |       char buffer[(sizeof (M)) * (mask + 1)];
      |                                            ^

I agree with your findings.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think it intentionally uses 64 rather than the controversial macros.
Perhaps it should at configure time check if such alignment is possible and
otherwise simply don't align it beyond mutex natural alignment?

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Janez Zemva from comment #0)
> This line:
> 
> struct alignas(64) M : __gnu_cxx::__mutex { };
> 
> has been an eyesore for me for a number of years. I propose to change it

That belongs on the mailing list, not in bugzilla.

> into:
> 
> struct alignas(std::max_align_t) M : __gnu_cxx::__mutex { };

That would be completely wrong though.


(In reply to Janez Zemva from comment #1)
> alternatively, the line could be changed into:
> 
> struct alignas(void*) M : __gnu_cxx::__mutex { };
> 
> since this was probably meant with the magic number 64.

That would be even more wrong. Please just read the comment instead of making
silly assumptions about what we meant.


(In reply to Janez Zemva from comment #5)
> This line has been patched out by djgpp builds for a long time now.

Then say that, instead of suggesting incorrect changes.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #9)
> I think it intentionally uses 64 rather than the controversial macros.

It's been there since before we had those macros. I think using the destructive
interference size makes sense, now that we have a constant for it.

> Perhaps it should at configure time check if such alignment is possible and
> otherwise simply don't align it beyond mutex natural alignment?

Agreed.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #11)
> > Perhaps it should at configure time check if such alignment is possible and
> > otherwise simply don't align it beyond mutex natural alignment?
> 
> Agreed.

We could as well for those arches use just larger unaligned buffer and align
manually.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[janezz55 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #13 from Janez Zemva <janezz55 at gmail dot com> ---
@Jonathan Wakely
I asked ChatGPT about this:

What is the most common size of a cache line?
The most common size of a cache line is 64 bytes. This size is used by most
modern CPUs because it strikes a balance between the amount of data that can be
stored in a cache line and the amount of overhead required to manage the cache.
However, it's important to note that different CPUs and cache designs may use
different cache line sizes based on their specific requirements and trade-offs
between performance and resource usage.

This means the "right" number is architecture-dependent and my proposals were
not  wrong for every possible architecture.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Libstdc++ doesn't work on "every possible architecture", just the ones GCC
supports.

I am 100% confident that alignas(void*) is wrong for every architecture GCC
supports, and I'm pretty confident alignof(max_align_t) is wrong for all of
them too (since that's either 8 or 16). I'm also confident that 64 is a good
choice for nearly all targets GCC supports.

64 is much less wrong than any of your suggestions, and is right for most GCC
targets.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:94a311abf783de754f0f1b2d4c1f00a9788e795b

commit r14-918-g94a311abf783de754f0f1b2d4c1f00a9788e795b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue May 16 15:09:20 2023 +0100

    libstdc++: Disable cacheline alignment for DJGPP [PR109741]

    DJGPP (and maybe other targets) uses MAX_OFILE_ALIGNMENT=16 which means
    that globals (and static objects) can't have alignment greater than 16.
    This causes an error for the locks defined in src/c++11/shared_ptr.cc
    because we try to align them to the cacheline size, to avoid false
    sharing.

    Add a configure check for the increased alignment, and live with false
    sharing where we can't increase the alignment.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109741
            * acinclude.m4 (GLIBCXX_CHECK_ALIGNAS_CACHELINE): Define.
            * config.h.in: Regenerate.
            * configure: Regenerate.
            * configure.ac: Use GLIBCXX_CHECK_ALIGNAS_CACHELINE.
            * src/c++11/shared_ptr.cc (__gnu_internal::get_mutex): Do not
            align lock table if not supported. use __GCC_DESTRUCTIVE_SIZE
            instead of hardcoded 64.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This should be fixed on trunk now.

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.5

[Bug libstdc++/109741] alignas(64) in libstdc++-v3/src/c++11/shared_ptr.cc

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109741

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:dbd4acd72274f3b3d542ebf68f9962eda8f8b769

commit r13-7509-gdbd4acd72274f3b3d542ebf68f9962eda8f8b769
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue May 16 15:09:20 2023 +0100

    libstdc++: Disable cacheline alignment for DJGPP [PR109741]

    DJGPP (and maybe other targets) uses MAX_OFILE_ALIGNMENT=16 which means
    that globals (and static objects) can't have alignment greater than 16.
    This causes an error for the locks defined in src/c++11/shared_ptr.cc
    because we try to align them to the cacheline size, to avoid false
    sharing.

    Add a configure check for the increased alignment, and live with false
    sharing where we can't increase the alignment.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109741
            * acinclude.m4 (GLIBCXX_CHECK_ALIGNAS_CACHELINE): Define.
            * config.h.in: Regenerate.
            * configure: Regenerate.
            * configure.ac: Use GLIBCXX_CHECK_ALIGNAS_CACHELINE.
            * src/c++11/shared_ptr.cc (__gnu_internal::get_mutex): Do not
            align lock table if not supported. use __GCC_DESTRUCTIVE_SIZE
            instead of hardcoded 64.

    (cherry picked from commit 94a311abf783de754f0f1b2d4c1f00a9788e795b)