[Bug libstdc++/8670] Alignment problem in std::basic_string

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pinskia at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670


pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  BugsThisDependsOn|                            |10479


------- Additional Comments From pinskia at gcc dot gnu dot org  2003-10-01 21:47 -------
Depend on the bug which is about "__alignof__(double) not compile time constant inside template 
class".

[Bug libstdc++/8670] Alignment problem in std::basic_string

[gdr at integrable-solutions dot net]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670



------- Additional Comments From gdr at integrable-solutions dot net  2003-10-01 22:05 -------
Subject: Re:  Alignment problem in std::basic_string

"pinskia at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:

| Depend on the bug which is about "__alignof__(double) not compile time constant inside template 
| class".

The issue is not really that of __alignof__(double) not being an
integral constant -- surely it is.  Try to use it in contexts where
the Standard requires an integral constant expression.

The real issue is having to do with __aligned__.

Try

   struct A {
     enum { constant = 4 };
     __attribute__((__aligned__(constant))) char data;
   };

-- Gaby

[Bug libstdc++/8670] Alignment problem in std::basic_string

[giovannibajo at libero dot it]

-- 
Bug 8670 depends on bug 10479, which changed state.

Bug 10479 Summary: alignof and sizeof (and other expressions) in attributes does not compile inside template classes
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10479

           What    |Old Value                   |New Value
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-10-16 11:10 -------
I see this report is marked as a v3 report. I do not understand exactly what 
you still need to fix in C++. I have a patch in my (long) queue which fixes 
this:

template <int>
struct S {
   enum { K = 8 };
   char foo __attribute__((aligned(K)));
};

Can I assume that this bug is about this issue, and thus assigning this to me 
(if Gaby does not mind) or is it better if I open a new report?

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |giovannibajo at libero dot
                   |                            |it


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-10-16 12:38 -------
(In reply to comment #7)

> I do not understand exactly what you still need to fix in C++.

I meant: I do not understand exactly what needs to be fixed in the C++ FE and 
what needs to be fixed in v3.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[gdr at cs dot tamu dot edu]

------- Additional Comments From gdr at cs dot tamu dot edu  2004-10-16 14:48 -------
Subject: Re:  Alignment problem in std::basic_string

"giovannibajo at libero dot it" <gcc-bugzilla@gcc.gnu.org> writes:

| I see this report is marked as a v3 report. I do not understand exactly what 
| you still need to fix in C++. I have a patch in my (long) queue which fixes 
| this:
| 
| template <int>
| struct S {
|    enum { K = 8 };
|    char foo __attribute__((aligned(K)));
| };
| 
| Can I assume that this bug is about this issue, and thus assigning this to me 
| (if Gaby does not mind) or is it better if I open a new report?

I don't mind you take on bugs, as far as they are fixed :-)

The V3 bits in this is that we need to get basic_string fixed.
If the front-end ix fixed, then we can use the aligned attribute in a
meaningful way.

Thanks

-- Gaby


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[giovannibajo at libero dot it]

------- Additional Comments From giovannibajo at libero dot it  2004-10-16 15:14 -------
Ok, then this is mine for the time being. I will fix the testcases in comment 
#6 and comment #7 in a short while.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|gdr at gcc dot gnu dot org  |giovannibajo at libero dot
                   |                            |it


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2004-10-16 20:09 -------
Great! This means that, within the 3.4/4.0 ABI, will be able to provide an
additional range of improvements to the string class that I didn't expect!

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2005-01-18 09:17 -------
*** Bug 19495 has been marked as a duplicate of this bug. ***

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bje at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[ncm-nospam at cantrip dot org]

------- Additional Comments From ncm-nospam at cantrip dot org  2005-01-21 17:45 -------
Do I understand correctly that the FE fix has been applied?  I don't
see any corresponding __attribute__ in HEAD for basic_string.h.

Probably _Rep should be aligned not to a constant size, but rather to
the most stringent needs of _Rep_base's members and and the actual_
charT itself.  Can that be expressed?  If __align__ can't do it, an 
ugly union trick should work.  The tricky bit is to avoid wasting 
space if _charT is bigger than a member of _Rep_base, so we probasbly 
don't want to use Martin's trick directly.  (Anyway as written it 
doesn't work properly because you have to align the first member, not 
the last.)  The right fix involves aligning the whole struct, rather 
than the first member, almost as described in the proposed fix for 
19495, but looking like:

  union { _Atomic_word _M_w; size_type _M_s; _charT _M_c; } _Alloc_unit;

The only problem I see is that if sizeof _charT happens not to be a power 
of two, the "/ sizeof _Alloc_unit" doesn't optimize nicely to a shift.   
That seems tolerable.

The remaining problem is that the calculations "this+1" and "this-1" don't 
account for alignment.  It seems to need trickier casting, along the lines
of 
        _Rep*
        _M_rep() const
-       { return &((reinterpret_cast<_Rep*> (_M_data()))[-1]); }
+       { 
+         return reinterpret_cast<_Rep*>(
+           &((reinterpret_cast<_Alloc_unit*>(_M_data()))[-1]));
+       }

and

          _CharT*
          _M_refdata() throw()
-         { return reinterpret_cast<_CharT*>(this + 1); }
+         { 
+           return reinterpret_cast<_CharT*>(
+             reinterpret_cast<_Alloc_unit*>(this) + 1);
+         }

I wonder if these should use unions, instead of reinterpret_cast<>,
so as to be compatible with -fstrict-aliasing.  Then I guess they look 
more like

        _Rep*
        _M_rep() const
-       { return &((reinterpret_cast<_Rep*> (_M_data()))[-1]); }
+       { 
+         union { _charT* _M_cp,  _Alloc_unit* _M_ap; _Rep* _M_rp; } _Aligner;
+         _Aligner __p = { _M_data() };
+         --__p._M_ap;
+         return __p._M_rp;
+       }

and

          _CharT*
          _M_refdata() throw()
-         { return reinterpret_cast<_CharT*>(this + 1); }
+         { 
+           union { _Rep* _M_rp; _Alloc_unit* _M_ap;  _charT* _M_cp,} _Aligner;
+           _Aligner __p = { this };
+           ++__p._M_ap;
+           return __p._M_cp;
+         }

Some people might like them better that way anyhow.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ncm-nospam at cantrip dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2005-01-21 17:53 -------
> Do I understand correctly that the FE fix has been applied?  I don't
> see any corresponding __attribute__ in HEAD for basic_string.h.

No, largely __attribute__(aligned) is still broken, doesn't work with
dependent types, and we badly need that in our templates. We have at
least two open PRs about this (c++/19163 and c++/17743), 4.1 material.
We want to fix this issue cleanly, using a (working ;) attribute.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[ncm-nospam at cantrip dot org]

------- Additional Comments From ncm-nospam at cantrip dot org  2005-01-23 03:52 -------
Somebody mentioned that using unions for type punning was described
in the textbooks as extremely bad form.  That's how I always thought
of it, too, but it seems, at least in Gcc, unions are now the right 
way to do type punning, instead of casting. For reference, see the 
notes under "-fstrict-aliasing" (which is turned on by -O2) in

  http://gcc.gnu.org/onlinedocs/gcc-3.4.3/gcc/Optimize-Options.html

(Thanks to Robert Love for having pointed this out to me.)

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2005-01-23 10:30 -------
> Somebody mentioned that using unions for type punning was described

Thanks Nathan for the clarification. Actually, however, we really want
to deal with those issues via appropriate __attribute__, we debated the
issue in great detail (therefore we have to wait for the attributes to
work correctly, see PRs mentioned in my previous reply) 

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[ncm-nospam at cantrip dot org]

------- Additional Comments From ncm-nospam at cantrip dot org  2005-01-24 03:42 -------
I have read the discussion on 17744 and 19163.  Nothing there suggests
that there is any reason to prefer using an __attribute__ over using
the portable, stable, apparently already-working union approach, where
it serves.  The union approach, contrariwise, is manifestly better 
anywhere the __attribute__ feature is broken, which it is said still 
to be, proposed patches notwithstanding.

Furthermore, I have seen no suggestion that the __attribute__ approach
actually enables an alignment optimal for the actual template arguments,
as is easy when using a union.  (That is not to say I don't believe it 
can; it just doesn't appear to have been mentioned.)  The discussion 
seemed to suggest that there was no intention to align adaptively, but 
only pessimistically.  That seems wasteful.

Why should library fixes (specifically, 19495) wait unnecessarily on 
fixes for compiler extensions -- more particularly, extensions unlikely 
to be fixed in the older releases whose libraries we still maintain?  
What am I missing?

(Of course none of this is to suggest that the extension shouldn't
be fixed, too.)

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |19495
              nThis|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2005-01-24 09:45 -------
> I have read the discussion on 17744 and 19163.  Nothing there suggests
> that there is any reason to prefer using an __attribute__ over using
> the portable, stable, apparently already-working union approach, where
> it serves.  The union approach, contrariwise, is manifestly better 
> anywhere the __attribute__ feature is broken, which it is said still 
> to be, proposed patches notwithstanding.

The feature its broken and the proposed patches don't fix it. Plenty of
discussions on gcc-patches and elsewhere, no doubts about this. Also,
there is an agreement about the maintainers that from now one, really we
should concentrate our efforts in preparing a new implementation of
basic_string: these alignment problems are not new, always been there.

> Why should library fixes (specifically, 19495) wait unnecessarily on 
> fixes for compiler extensions -- more particularly, extensions unlikely 
> to be fixed in the older releases whose libraries we still maintain?  
> What am I missing?

I'm still trying to figure out a simple, non-invasive, clean, way to
implement your suggestions. We don't want loads of casts, or unions,
additional instantiations (requiring loads of additional includes) and
failing tests elsewhere (ext/array_allocator needs tweaks), uglyness,
in a word. I'm still trying to figure whether we can achieve that within
the current implementation and without subtracting too much energy to
other projects (among which the new implementation itself): please be
patient, thanks.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[ncm-nospam at cantrip dot org]

------- Additional Comments From ncm-nospam at cantrip dot org  2005-01-31 05:29 -------
Hmm, precisely two additional casts (or local unions), each in a 
statement where a cast already appears, hardly seems like a "load". 

But I'm always patient, right?  It's just that anyplace a standard
construct could be used instead of an extension, I'll vote for the 
standard one.

Question for the compiler jocks... why can unions enforce correct
alignment, but the __attribute__ form cannot?  Can the extension
not be recast to generate what amounts to an unnamed union, and 
thereby re-use the same machinery?

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|enhancement                 |normal


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[jamesc at dspsrv dot com]

------- Comment #20 from jamesc at dspsrv dot com  2007-04-03 16:30 -------
I can see this problem, solaris 10 and gcc 4.1.1.

The workaround suggested by the following person works:

Margarita Manterola
http://www.mail-archive.com/debian-bugs-rc@lists.debian.org/msg67774.html

via Darren Long
http://mailman.dtnrg.org/pipermail/dtn-users/2007-January/000448.html

// include iostream before string to avoid stdc++ bug
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670
#include <iostream>
#include <string>


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[pcarlini at suse dot de]

------- Comment #21 from pcarlini at suse dot de  2007-04-03 19:26 -------
A few issues of the current std::string should be really in suspended status,
thus clarifying that cannot be fixed within the current ABI. Anyway, in the
meanwhile, people are encouraged to try <ext/vstring.h>, a new, versatile
implementation which provides both a non-reference-counted-type implementation
(by default), or an improved reference-counted one (no alignment issues at
all).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8670

[Bug libstdc++/8670] Alignment problem in std::basic_string

[redi at gcc dot gnu.org]

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

--- Comment #22 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This bug is still present in the COW std::string, which is still supported even
though it's not the default.

There are two problems. The first is the one reported by James Kanze, that the
string contents need to be aligned to alignof(_CharT) but are currently aligned
to 1. The second, stated by Nathan in comment 13, is that the _Rep object needs
to be aligned to alignof(_Rep), not 1. The reference count in the _Rep ends up
misaligned, and the atomic operations on it are undefined.

Here's a C++17 test case that shows both problems:

#define _GLIBCXX_USE_CXX11_ABI 0

#include <string>

template<typename T>
struct alloc
{
  using value_type = T;

  alloc() = default;

  template<typename U>
    alloc(const alloc<U>&) { }

  T* allocate(std::size_t n)
  {
    if constexpr (std::is_same_v<T, char>)
      return next.allocate(n + 1) + 1;
    return next.allocate(n);
  }

  void deallocate(T* p, std::size_t n)
  {
    if constexpr (std::is_same_v<T, char>)
      return next.deallocate(p - 1, n + 1);
    return next.deallocate(p, n);
  }

  [[no_unique_address]] std::allocator<T> next;

  bool operator==(const alloc<T>&) const { return true; }
};

template<typename C>
  using String = std::basic_string<C, std::char_traits<C>, alloc<C>>;

int main()
{
  String<long double> sd(2, 0.0L);
  return sd[1];
}



This results in loads of UBsan errors like:

/usr/include/c++/13/bits/cow_string.h:3604:24: runtime error: member access
within misaligned address 0x0000006f22b1 for type 'struct _Rep', which requires
8 byte alignment
0x0000006f22b1: note: pointer points here
 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
00 00  00 00 00 00 00
              ^ 
...
/usr/include/c++/13/bits/char_traits.h:307:15: runtime error: store to
misaligned address 0x0000006f22c9 for type 'char_type', which requires 16 byte
alignment
0x0000006f22c9: note: pointer points here
 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
00 00  00 00 00 00 00
              ^ 
...
/usr/include/c++/13/bits/cow_string.h:252:46: runtime error: reference binding
to misaligned address 0x0000006f22e9 for type 'char_type', which requires 16
byte alignment
0x0000006f22e9: note: pointer points here
 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
00 00  00 00 00 00 00
              ^ 
...
/usr/include/c++/13/ext/atomicity.h:84:18: runtime error: load of misaligned
address 0x0000006f22c1 for type '_Atomic_word', which requires 4 byte alignment
0x0000006f22c1: note: pointer points here
 00 00 00  00 ff ff ff ff 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
00 00  00 00 00 00 00
              ^ 


It seems to me that we can just do:

      struct __attribute__((__aligned__(__alignof__(_CharT)))) _Rep_base
      {
        size_type               _M_length;
        size_type               _M_capacity;
        _Atomic_word            _M_refcount;
      };

And then stop allocating raw bytes (with alignment 1) to place the _Rep into,
and use this allocator type instead:

        typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template
          rebind<_Rep>::other _Rep_alloc;

Then of course we need to adjust the size that we (de)allocate, to be in units
of sizeof(_Rep) not bytes.

[Bug libstdc++/8670] Alignment problem in std::basic_string

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|giovannibajo at gmail dot com      |redi at gcc dot gnu.org

--- Comment #23 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Created attachment 56537
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56537&action=edit
Fix alignment of COW std::string storage

This fixes both problems, so re-assigning to myself.

[Bug libstdc++/8670] Alignment problem in std::basic_string

[redi at gcc dot gnu.org]

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

--- Comment #24 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oh, but this would be an ABI break. When using the explicit instantiation
definitions in libstdc++.so allocations and deallocations will match because
both will come from the library. But if anything is inlined, code compiled
against older gcc headers might allocate N bytes from _Raw_bytes_alloc, and
other code might deallocate N2 bytes from _Rep_alloc, where N != N2.