Bug 38045 – Avoid increasing required alignment of target type warning on ARM

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gabor Loki]

Although bug 16925 fixed the alignment problem on many platforms, the problem still exists on ARM (see the ARMv5 buildbot).

One example warning:
JavaScriptCore/wtf/Vector.h:484: warning: cast from 'WTF::AlignedBufferChar*' to 'JSC::Register*' increases required alignment of target type

The problem is that the reinterpret_cast tries to cast from char* to a pointer of a bigger type. A possible solution is adding alignment to the array elements instead of the whole array.

[Gabor Loki]

Created attachment 54156 [details]
Fix VectorBuffer's alignment on ARM

I saw the style errors, but I do not want to brake the current style. ;)
If you like, I could change this.

[WebKit Review Bot]

Attachment 54156 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/Vector.h:59:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:60:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:61:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:62:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:63:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:64:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:65:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:68:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:69:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:70:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:71:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:72:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:73:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/Vector.h:74:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 14 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 54156 [details]
Fix VectorBuffer's alignment on ARM

This patch does nothing to explain why this change works. What was wrong? How does this change improve things?

[Gabor Loki]

(Sorry, I had to run. So, here comes the explanation.)

The main problem is the declaration of the array. If it is casted to another array type, the alignment of the source elements should be the same (or bigger) as the alignment of the target elements. Lets see an example.

char F[6] __attribute__((__aligned__(4)));

This array takes 6 bytes for the value and 2 bytes for padding. So it takes 8 bytes in overall.

typedef struct {
  char c __attribute__((__aligned__(4)));
} S_t;
S_t S[2];

This second array takes 2 bytes for the value and 6 bytes for padding. Sum is 8 bytes as well. (You can ignore the different addressing possibilities of the character arrays. We will cast them before using.)

What is the difference between this two array? The alignment of the elements. You can easily see the S[0] and S[1] is aligned to 4 bytes. So a simple reinterpret_cast<int*>(S) conversion can be done without any alignment problem.
On the first case, the compiler cannot ensure that the F[1], F[2],... are aligned to 4 bytes (because the minimum required aligned is 1 for char and 4 for int). This is the reason of the warning.

Why does this solution improve things? It uses such a buffer where each element of the buffer is aligned to the target type. So the corresponding elements of the buffer have the same address in both types. There is no other magic in it.

[Thiago Macieira]

I think this is a GCC problem.

$ cat test.cpp
struct Foo
{
    char __attribute__((aligned(4))) c;
};

char junk;
Foo f;
char junk2;

int main()
{
    int *i = reinterpret_cast<int *>(&f.c);
}

$ arm-none-linux-gnueabi-g++ -Wcast-align -fsyntax-only /tmp/test.cpp
/tmp/test.cpp: In function 'int main()':
/tmp/test.cpp:8: warning: cast from 'char*' to 'int*' increases required alignment of target type

$ arm-none-linux-gnueabi-g++ -dumpversion 
4.4.1

Reading the assembly output, we can even see:
junk:
        .space  1
        .space  3
        .type   f, %object
        .size   f, 4
f:
        .space  4

Which proves that "f" was aligned to a 4-byte boundary. (note the 3-byte padding)

In Vector.h, we have:
    template <size_t size, size_t alignment> struct AlignedBuffer;
    template <size_t size> struct AlignedBuffer<size, 1> { AlignedBufferChar buffer[size]; };
    template <size_t size> struct AlignedBuffer<size, 2> { WTF_ALIGNED(AlignedBufferChar, buffer[size], 2);  };
    template <size_t size> struct AlignedBuffer<size, 4> { WTF_ALIGNED(AlignedBufferChar, buffer[size], 4);  };
    template <size_t size> struct AlignedBuffer<size, 8> { WTF_ALIGNED(AlignedBufferChar, buffer[size], 8);  };
[...]

lines 484 to 486:
        T* inlineBuffer() { return reinterpret_cast<T*>(m_inlineBuffer.buffer); }

        AlignedBuffer<m_inlineBufferSize, WTF_ALIGN_OF(T)> m_inlineBuffer;

So, by construction, m_inlineBuffer::buffer has the same alignment as T.

GCC threw the same warning as my example above, despite the type having the proper alignment.

[Thiago Macieira]

Reported to GCC: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43976

[Kent Tamura]

(In reply to comment #4)
> (Sorry, I had to run. So, here comes the explanation.)

I think Darin's comment meant ChangeLog should have this explanation.

[Alexey Proskuryakov]

Obviously, there is no need to paste the above explanations in a ChangeLog. Something like "work around a gcc (4.3?) bug that results in an incorrect warning on ARM/on some platforms(?). No negative effect on memory use or performance" would suffice - of course, if that's true, I'm not sure if everyone is on the same page already.

[Gabor Loki]

Created attachment 58134 [details]
Fix VectorBuffer's alignment on ARM (v2)

I have just looked at C++ standard and found a better solution than the previous one.
We can use static_cast to void* which has no alignment problem on any type, and of course it will not break the value of m_inlineBuffer.buffer (which was an array type in the reinterpret_cast).

[Gabor Loki]

*** Bug 40410 has been marked as a duplicate of this bug. ***

[Gabor Loki]

I have merged this and bug 40410, because they should have the same fix to remove the alignment warning.

[Gabor Loki]

Created attachment 61650 [details]
Avoid increasing required alignment of target type warning on ARM

One or two weeks ago Gavin and I discussed this problem on IRC, and Gavin suggested to have reinterpret_cast_ptr<T>(void*) template function which bypasses this warning on ARM (and do not poke any other target). So, I have created a patch which solves this issue with this reinterpret_cast_ptr template function (and a wrapper macro for other targets).

This patch fixes the alignment warnings on ARM (and on Thumb-2 as well).

[WebKit Review Bot]

Attachment 61650 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/qt/StringQt.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabor Loki]

(In reply to comment #13)
> JavaScriptCore/wtf/qt/StringQt.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

There is no primary header for wtf/qt/StringQt.

[Gavin Barraclough]

Looks good to me.

[Csaba Osztrogonác]

(In reply to comment #15)
> Looks good to me.

You forgot to set r+ flag, but modifiied the status to resolved/fixed :)

[Gavin Barraclough]

Comment on attachment 61650 [details]
Avoid increasing required alignment of target type warning on ARM

D'oh, thanks ossy. :-)
r+

[Gavin Barraclough]

ooops, hit fixed by mistake, instead of r+!
reverting back to an open state, & going to find more coffee before doing anything else in bugzilla. ;-)

[Csaba Osztrogonác]

Comment on attachment 61650 [details]
Avoid increasing required alignment of target type warning on ARM

Clearing flags on attachment: 61650

Committed r64302: <http://trac.webkit.org/changeset/64302>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Gavin Barraclough]

Rolled out in: https://bugs.webkit.org/show_bug.cgi?id=43223

The implementation of reinterpret_cast_ptr for ARM/GCC should probably be more like:

template<typename Type>
bool isPointerTypeAlignmentOkay(Type* ptr)
{
    return !(reinterpret_cast<intptr_t>(ptr) % __alignof__(Type));
}

template<typename TypePtr>
TypePtr reinterpret_cast_ptr(void* ptr)
{
    ASSERT(isPointerTypeAlignmentOkay(reinterpret_cast<TypePtr>(ptr)));
    return reinterpret_cast<TypePtr>(ptr);
}

template<typename TypePtr>
const TypePtr reinterpret_cast_ptr(const void* ptr)
{
    ASSERT(isPointerTypeAlignmentOkay(reinterpret_cast<TypePtr>(ptr)));
    return reinterpret_cast<TypePtr>(ptr);
}

The ASSERT should be checking the alignment of the Type, not the type of TypePtr.  reinterpret_cast of a const pointer should not change constness.

[Gabor Loki]

(In reply to comment #21)
You are right the previous ASSERT was bogus. Thanks for the heads up.

[Gabor Loki]

Created attachment 63215 [details]
Avoid increasing required alignment of target type warning on ARM (v2)

- ASSERT is fixed as Gavin suggested.
- Modify the newly introduced reinterpret_casts in JSArray.(h|cpp).

[WebKit Review Bot]

Attachment 63215 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:72:  reinterpret_cast_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:79:  reinterpret_cast_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/qt/StringQt.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[deepak]

I am facing same Alignment warnings in JavaScriptCore/wtf/Vector.h ..
and few other warnings which i could fix by addin CPU(MIPS) checks in


/WebCore/platform/text/StringHash.h
./WebCore/platform/text/AtomicString.cpp

but im looking for the solution wrt JavaScriptCore/wtf/Vector.h

[Gabor Loki]

Committed revision 65311.

[Gabor Loki]

The bug 43963 is the follow up of this bug. Please, submit new patches there about the alignment warning!