Fix for Vector::m_inlineBuffer alignment?
I see that JavaScriptCore/wtf/Vector.h has this: // FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize]; And I've heard reports about people having alignment crashes on some hardware. Something like the code below could rectify this in a portable way. This could be made into a patch as-is, though the first part of the code really belongs in a separate PlatformDefs.h-style header. I'm wondering if WebKit has a central place for such a thing that I'm not aware of. // Portable facilities to detect and set alignment #if defined(__GNUC__) || defined(__MWERKS__) #define WTF_ALIGN_OF(type) __alignof__(type) #define WTF_PREFIX_ALIGN(n) #define WTF_POSTFIX_ALIGN(n) __attribute__((aligned(n))) #elif defined(_MSC_VER) #define WTF_ALIGN_OF(type) __alignof(type) #define WTF_PREFIX_ALIGN(n) __declspec(align(n)) // n must be a literal integer, it cannot be a general constant expression. #define WTF_POSTFIX_ALIGN(n) #else #error need alignment control #endif // Portable aliasing support. #if defined(__GNUC__) && (((__GNUC__ * 100) + __GNUC_MINOR__) >= 303) typedef char __attribute__((__may_alias__)) aligned_buffer_char; #else typedef char aligned_buffer_char; #endif // Portable aligned char buffer. // VC++ can't compile__declspec(align(__alignof(T)), so we solve this with template specialization. template <size_t size, size_t alignment> struct aligned_buffer { aligned_buffer_char buffer[size]; }; template<size_t size> struct aligned_buffer<size, 2> { WTF_PREFIX_ALIGN(2) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(2); }; template<size_t size> struct aligned_buffer<size, 4> { WTF_PREFIX_ALIGN(4) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(4); }; template<size_t size> struct aligned_buffer<size, 8> { WTF_PREFIX_ALIGN(8) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(8); }; template<size_t size> struct aligned_buffer<size, 16> { WTF_PREFIX_ALIGN(16) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(16); }; template<size_t size> struct aligned_buffer<size, 32> { WTF_PREFIX_ALIGN(32) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(32); }; template<size_t size> struct aligned_buffer<size, 64> { WTF_PREFIX_ALIGN(64) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(64); }; template<typename T, size_t inlineCapacity> class VectorBuffer : private VectorBufferBase<T> { . . . - T* inlineBuffer() { return reinterpret_cast<T*>(&m_inlineBuffer); } - // FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. - char m_inlineBuffer[m_inlineBufferSize]; + T* inlineBuffer() { return reinterpret_cast<T*>(m_inlineBuffer.buffer); } + aligned_buffer<m_inlineBufferSize, WTF_ALIGN_OF(T)> m_inlineBuffer; };
It sounds like the first part should go in JavaScriptCore/wtf/Platform.h. Could you open a new bug on <https://bugs.webkit.org/> and post this as a patch for review? Thanks! Dave On Wed, 9/3/08, Paul Pedriana <ppedriana@gmail.com> wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
And I've heard reports about people having alignment crashes on some hardware. Something like the code below could rectify this in a portable way. This could be made into a patch as-is, though the first part of the code really belongs in a separate PlatformDefs.h-style header. I'm wondering if WebKit has a central place for such a thing that I'm not aware of.
// Portable facilities to detect and set alignment #if defined(__GNUC__) || defined(__MWERKS__) #define WTF_ALIGN_OF(type) __alignof__(type) #define WTF_PREFIX_ALIGN(n) #define WTF_POSTFIX_ALIGN(n) __attribute__((aligned(n))) #elif defined(_MSC_VER) #define WTF_ALIGN_OF(type) __alignof(type) #define WTF_PREFIX_ALIGN(n) __declspec(align(n)) // n must be a literal integer, it cannot be a general constant expression. #define WTF_POSTFIX_ALIGN(n) #else #error need alignment control #endif
// Portable aliasing support. #if defined(__GNUC__) && (((__GNUC__ * 100) + __GNUC_MINOR__) >= 303) typedef char __attribute__((__may_alias__)) aligned_buffer_char; #else typedef char aligned_buffer_char; #endif
// Portable aligned char buffer. // VC++ can't compile__declspec(align(__alignof(T)), so we solve this with template specialization. template <size_t size, size_t alignment> struct aligned_buffer { aligned_buffer_char buffer[size]; };
template<size_t size> struct aligned_buffer<size, 2> { WTF_PREFIX_ALIGN(2) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(2); };
template<size_t size> struct aligned_buffer<size, 4> { WTF_PREFIX_ALIGN(4) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(4); };
template<size_t size> struct aligned_buffer<size, 8> { WTF_PREFIX_ALIGN(8) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(8); };
template<size_t size> struct aligned_buffer<size, 16> { WTF_PREFIX_ALIGN(16) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(16); };
template<size_t size> struct aligned_buffer<size, 32> { WTF_PREFIX_ALIGN(32) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(32); };
template<size_t size> struct aligned_buffer<size, 64> { WTF_PREFIX_ALIGN(64) aligned_buffer_char buffer[size] WTF_POSTFIX_ALIGN(64); };
template<typename T, size_t inlineCapacity> class VectorBuffer : private VectorBufferBase<T> { . . .
- T* inlineBuffer() { return reinterpret_cast<T*>(&m_inlineBuffer); } - // FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. - char m_inlineBuffer[m_inlineBufferSize];
+ T* inlineBuffer() { return reinterpret_cast<T*>(m_inlineBuffer.buffer); } + aligned_buffer<m_inlineBufferSize, WTF_ALIGN_OF(T)> m_inlineBuffer; };
On Sep 4, 2008, at 2:05 AM, Paul Pedriana wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
We have a bug for this: <https://bugs.webkit.org/show_bug.cgi? id=16925>. Also, <https://bugs.webkit.org/show_bug.cgi?id=19775>. There are several patches and suggestions in these bugs, which wait for someone to submit them as clean patches. - WBR, Alexey Proskuryakov
Well the proposed solution in <https://bugs.webkit.org/show_bug.cgi?id=16925> doesn't work, as VC++ doesn't accept that syntax. And the solutions in <https://bugs.webkit.org/show_bug.cgi?id=19775> whereby a uint32_t or uint64_t buffer are made don't work for larger types and use more memory than necessary. They might be useful as a fallback for compilers that don't support alignment directives, though the primary implementations of every compiler that I work with (GCC, VC++, CodeWarrior, EDG, SN, IBM) support it. I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925>, if that's OK. Paul
On Sep 4, 2008, at 2:05 AM, Paul Pedriana wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
We have a bug for this: <https://bugs.webkit.org/show_bug.cgi?id=16925>. Also, <https://bugs.webkit.org/show_bug.cgi?id=19775>.
There are several patches and suggestions in these bugs, which wait for someone to submit them as clean patches.
- WBR, Alexey Proskuryakov
On Sep 4, 2008, at 12:20 PM, Paul Pedriana wrote:
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925
, if that's OK.
That would be great! One thing I'm not sure about is whether we want to enforce alignment on platforms that don't require it - performance testing should answer this. - WBR, Alexey Proskuryakov
This is a hardware issue and can vary between different versions. Some hardware of course generates an unhandled exception on unaligned access; some hardware generates an internally handled exception and restarts the access with a different and more expensive pathway or with microcode; some hardware efficiently handles it as it goes and has minimal impact. Sometimes this behavior is different depending on whether the hardware is in big endian vs. little endian mode. A new generation of the same processor family or a version by a different company may act differently than another. Finally, if that memory is accessed by a different means than the CPU (e.g. via DMA or the video bus), then it may require alignment where it doesn't otherwise. What I'm getting at here is that I think we would need to be careful about disabling it for any hardware, including even forgiving hardware like x86.
On Sep 4, 2008, at 12:20 PM, Paul Pedriana wrote:
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925>, if that's OK.
That would be great!
One thing I'm not sure about is whether we want to enforce alignment on platforms that don't require it - performance testing should answer this.
- WBR, Alexey Proskuryakov
That's a great list of subtle issues (I can add that atomicity guarantees are different for non-aligned access sometimes) - it would be fantastic if we could amend it with actual data, say SunSpider results and/or DOM performance tests on Intel Mac and/or Windows. If we got an improvement on SunSpider/Mac, we wouldn't have to discuss processors running in non-native endianness modes :-) - WBR, Alexey Proskuryakov On Sep 4, 2008, at 1:24 PM, Paul Pedriana wrote:
This is a hardware issue and can vary between different versions. Some hardware of course generates an unhandled exception on unaligned access; some hardware generates an internally handled exception and restarts the access with a different and more expensive pathway or with microcode; some hardware efficiently handles it as it goes and has minimal impact. Sometimes this behavior is different depending on whether the hardware is in big endian vs. little endian mode. A new generation of the same processor family or a version by a different company may act differently than another. Finally, if that memory is accessed by a different means than the CPU (e.g. via DMA or the video bus), then it may require alignment where it doesn't otherwise.
What I'm getting at here is that I think we would need to be careful about disabling it for any hardware, including even forgiving hardware like x86.
On Sep 4, 2008, at 12:20 PM, Paul Pedriana wrote:
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925
, if that's OK.
That would be great!
One thing I'm not sure about is whether we want to enforce alignment on platforms that don't require it - performance testing should answer this.
- WBR, Alexey Proskuryakov
On Sep 4, 2008, at 1:20 AM, Paul Pedriana wrote:
Well the proposed solution in <https://bugs.webkit.org/show_bug.cgi?id=16925> doesn't work, as VC++ doesn't accept that syntax. And the solutions in <https://bugs.webkit.org/show_bug.cgi?id=19775> whereby a uint32_t or uint64_t buffer are made don't work for larger types and use more memory than necessary. They might be useful as a fallback for compilers that don't support alignment directives, though the primary implementations of every compiler that I work with (GCC, VC++, CodeWarrior, EDG, SN, IBM) support it.
The buffer should definitely remain char to avoid strict aliasing problems. However, I think the most portable way to ensure alignment is to union the buffer with a one-element array of the template parameter type T. - Maciej
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925>, if that's OK.
Paul
On Sep 4, 2008, at 2:05 AM, Paul Pedriana wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
We have a bug for this: <https://bugs.webkit.org/show_bug.cgi?id=16925>. Also, <https://bugs.webkit.org/show_bug.cgi?id=19775>.
There are several patches and suggestions in these bugs, which wait for someone to submit them as clean patches.
- WBR, Alexey Proskuryakov
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
I agree that a union is most portable, but there are two problems with it: - The C++ Standard (section 9.5 p1) disallows union members that have ctors, dtors, or assignment operators. This includes the case whereby the class is the only non-POD of the union. - It would create an instance of type T. Thus an empty Vector would construct an instance of T, resizing it would overwrite the instance of T, and on destruction it would destruct the T. Paul
On Sep 4, 2008, at 1:20 AM, Paul Pedriana wrote:
Well the proposed solution in <https://bugs.webkit.org/show_bug.cgi?id=16925> doesn't work, as VC++ doesn't accept that syntax. And the solutions in <https://bugs.webkit.org/show_bug.cgi?id=19775> whereby a uint32_t or uint64_t buffer are made don't work for larger types and use more memory than necessary. They might be useful as a fallback for compilers that don't support alignment directives, though the primary implementations of every compiler that I work with (GCC, VC++, CodeWarrior, EDG, SN, IBM) support it.
The buffer should definitely remain char to avoid strict aliasing problems. However, I think the most portable way to ensure alignment is to union the buffer with a one-element array of the template parameter type T.
- Maciej
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925>, if that's OK.
Paul
On Sep 4, 2008, at 2:05 AM, Paul Pedriana wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
We have a bug for this: <https://bugs.webkit.org/show_bug.cgi?id=16925>. Also, <https://bugs.webkit.org/show_bug.cgi?id=19775>.
There are several patches and suggestions in these bugs, which wait for someone to submit them as clean patches.
- WBR, Alexey Proskuryakov
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Sep 7, 2008, at 10:20 AM, Paul Pedriana wrote:
I agree that a union is most portable, but there are two problems with it:
- The C++ Standard (section 9.5 p1) disallows union members that have ctors, dtors, or assignment operators. This includes the case whereby the class is the only non-POD of the union.
- It would create an instance of type T. Thus an empty Vector would construct an instance of T, resizing it would overwrite the instance of T, and on destruction it would destruct the T.
Oliver also pointed out the constructor / destructor issue, so yeah, this wouldn't work. We could use double to force 8-byte alignment but that might be wasteful for vector types where wider alignment is not required. - Maciej
Paul
On Sep 4, 2008, at 1:20 AM, Paul Pedriana wrote:
Well the proposed solution in <https://bugs.webkit.org/show_bug.cgi?id=16925> doesn't work, as VC ++ doesn't accept that syntax. And the solutions in <https://bugs.webkit.org/show_bug.cgi?id=19775> whereby a uint32_t or uint64_t buffer are made don't work for larger types and use more memory than necessary. They might be useful as a fallback for compilers that don't support alignment directives, though the primary implementations of every compiler that I work with (GCC, VC++, CodeWarrior, EDG, SN, IBM) support it.
The buffer should definitely remain char to avoid strict aliasing problems. However, I think the most portable way to ensure alignment is to union the buffer with a one-element array of the template parameter type T.
- Maciej
I'll make a patch and attach it to <https://bugs.webkit.org/show_bug.cgi?id=16925>, if that's OK.
Paul
On Sep 4, 2008, at 2:05 AM, Paul Pedriana wrote:
I see that JavaScriptCore/wtf/Vector.h has this:
// FIXME: Nothing guarantees this buffer is appropriately aligned to hold objects of type T. char m_inlineBuffer[m_inlineBufferSize];
We have a bug for this: <https://bugs.webkit.org/show_bug.cgi?id=16925>. Also, <https://bugs.webkit.org/show_bug.cgi?id=19775>.
There are several patches and suggestions in these bugs, which wait for someone to submit them as clean patches.
- WBR, Alexey Proskuryakov
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
participants (4)
-
Alexey Proskuryakov
-
David Kilzer
-
Maciej Stachowiak
-
Paul Pedriana