See the message http://gcc.gnu.org/ml/gcc-bugs/2002-11/msg01150.html Release: unknown Environment: Primarily on Sparc, but the nature of the bug may show up on any plateform for which alignof(size_t) != alignof(double) and which is picky about alignment
From: Martin Sebor <sebor@roguewave.com> To: gcc-gnats@gcc.gnu.org Cc: Subject: Re: libstdc++/8670: Alignment problem in std::basic_string Date: Thu, 21 Nov 2002 11:05:54 -0700 http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=8670 This caught my eye because we had the same problem some time ago (our reference counted implementation of string is similar to yours). Quickly looking at your code, the alignment bug is due to _CharT* basic_string<_CharT>::_Rep::_M_refdata() throw() { return reinterpret_cast<_CharT*>(this + 1); } and the definition of basic_string<_CharT>::_Rep struct _Rep { size_type _M_length; size_type _M_capacity; _Atomic_word _M_references; ... }; which may not have the same alignment requirement as _CharT. The trick we use is to change _Rep along the lines of struct _Rep { size_type _M_length; size_type _M_capacity; union { _CharT _M_align; _Atomic_word _M_references; } _M_ref; ... }; Note that this change will enforce the requirement that _CharT be a POD type. Regards Martin
Responsible-Changed-From-To: unassigned->gdr Responsible-Changed-Why: Knows well the issue.
State-Changed-From-To: open->analyzed State-Changed-Why: Hi Gaby. What's the status on this one? Shall we go with the 'quick and dirty' solution proposed by Martin? I seem to recall, however, that you had in mind something cleaner, using __attribute__((__aligned__(__alignof__ ... Thanks, Paolo.
From: Gabriel Dos Reis <gdr@integrable-solutions.net> To: paolo@gcc.gnu.org Cc: gcc-bugs@gcc.gnu.org, jkanze@caicheuvreux.com, gcc-gnats@gcc.gnu.org Subject: Re: libstdc++/8670: Alignment problem in std::basic_string Date: 18 Feb 2003 13:21:00 +0100 paolo@gcc.gnu.org writes: | Synopsis: Alignment problem in std::basic_string | | Responsible-Changed-From-To: unassigned->gdr | Responsible-Changed-By: paolo | Responsible-Changed-When: Tue Feb 18 11:17:01 2003 | Responsible-Changed-Why: | Knows well the issue. | State-Changed-From-To: open->analyzed | State-Changed-By: paolo | State-Changed-When: Tue Feb 18 11:17:01 2003 | State-Changed-Why: | Hi Gaby. What's the status on this one? Shall we go with | the 'quick and dirty' solution proposed by Martin? I seem | to recall, however, that you had in mind something cleaner, | using __attribute__((__aligned__(__alignof__ ... Thanks for the reminder. I'll resurect the corresponding patch I had in mind. Sorry, I was caught in front-end specific issues and it is no joy at all :-] -- Gaby
Depend on the bug which is about "__alignof__(double) not compile time constant inside template class".
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
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?
(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.
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
Ok, then this is mine for the time being. I will fix the testcases in comment #6 and comment #7 in a short while.
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!
*** Bug 19495 has been marked as a duplicate of this bug. ***
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.
> 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.
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.)
> 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)
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.)
> 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.
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?
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>
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).
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.
Created attachment 56537 [details] Fix alignment of COW std::string storage This fixes both problems, so re-assigning to myself.
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.