Bug 38045 - Avoid increasing required alignment of target type warning on ARM
Summary: Avoid increasing required alignment of target type warning on ARM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 40410 (view as bug list)
Depends on: 43223
Blocks: 37916 43191
  Show dependency treegraph
 
Reported: 2010-04-23 07:17 PDT by Gabor Loki
Modified: 2010-08-13 03:55 PDT (History)
9 users (show)

See Also:


Attachments
Fix VectorBuffer's alignment on ARM (3.49 KB, patch)
2010-04-23 07:25 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Fix VectorBuffer's alignment on ARM (v2) (2.32 KB, patch)
2010-06-08 06:34 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Avoid increasing required alignment of target type warning on ARM (13.31 KB, patch)
2010-07-15 06:47 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Avoid increasing required alignment of target type warning on ARM (v2) (16.74 KB, patch)
2010-08-02 07:08 PDT, Gabor Loki
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Loki 2010-04-23 07:17:30 PDT
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.
Comment 1 Gabor Loki 2010-04-23 07:25:27 PDT
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.
Comment 2 WebKit Review Bot 2010-04-23 07:27:31 PDT
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.
Comment 3 Darin Adler 2010-04-23 08:20:59 PDT
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?
Comment 4 Gabor Loki 2010-04-23 14:46:49 PDT
(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.
Comment 5 Thiago Macieira 2010-05-03 07:23:05 PDT
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.
Comment 6 Thiago Macieira 2010-05-03 07:40:53 PDT
Reported to GCC: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43976
Comment 7 Kent Tamura 2010-06-04 01:08:23 PDT
(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.
Comment 8 Alexey Proskuryakov 2010-06-04 10:21:27 PDT
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.
Comment 9 Gabor Loki 2010-06-08 06:34:42 PDT
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).
Comment 10 Gabor Loki 2010-07-15 05:58:20 PDT
*** Bug 40410 has been marked as a duplicate of this bug. ***
Comment 11 Gabor Loki 2010-07-15 06:01:34 PDT
I have merged this and bug 40410, because they should have the same fix to remove the alignment warning.
Comment 12 Gabor Loki 2010-07-15 06:47:12 PDT
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).
Comment 13 WebKit Review Bot 2010-07-15 06:54:31 PDT
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.
Comment 14 Gabor Loki 2010-07-15 06:58:18 PDT
(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.
Comment 15 Gavin Barraclough 2010-07-29 12:34:52 PDT
Looks good to me.
Comment 16 Csaba Osztrogonác 2010-07-29 12:40:14 PDT
(In reply to comment #15)
> Looks good to me.

You forgot to set r+ flag, but modifiied the status to resolved/fixed :)
Comment 17 Gavin Barraclough 2010-07-29 12:41:44 PDT
Comment on attachment 61650 [details]
Avoid increasing required alignment of target type warning on ARM

D'oh, thanks ossy. :-)
r+
Comment 18 Gavin Barraclough 2010-07-29 12:43:06 PDT
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. ;-)
Comment 19 Csaba Osztrogonác 2010-07-29 12:52:28 PDT
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>
Comment 20 Csaba Osztrogonác 2010-07-29 12:52:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Gavin Barraclough 2010-07-29 19:49:34 PDT
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.
Comment 22 Gabor Loki 2010-08-02 06:58:38 PDT
(In reply to comment #21)
You are right the previous ASSERT was bogus. Thanks for the heads up.
Comment 23 Gabor Loki 2010-08-02 07:08:46 PDT
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).
Comment 24 WebKit Review Bot 2010-08-02 07:10:07 PDT
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.
Comment 25 deepak 2010-08-12 01:10:55 PDT
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
Comment 26 Gabor Loki 2010-08-13 03:49:10 PDT
Committed revision 65311.
Comment 27 Gabor Loki 2010-08-13 03:55:44 PDT
The bug 43963 is the follow up of this bug. Please, submit new patches there about the alignment warning!