Bug 43963 - Avoid increasing required alignment of target type warning
Summary: Avoid increasing required alignment of target type warning
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 43191
  Show dependency treegraph
 
Reported: 2010-08-13 03:51 PDT by Gabor Loki
Modified: 2010-11-18 05:51 PST (History)
8 users (show)

See Also:


Attachments
Fix platform independent alignment warnings (4.10 KB, patch)
2010-08-16 00:26 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Fix alignment warnings on Qt (7.79 KB, patch)
2010-08-16 00:27 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Fix platform independent alignment warnings (v2) (3.91 KB, patch)
2010-08-16 05:24 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
fix MIPS alignment warnings (for deepak) (3.60 KB, patch)
2010-08-17 01:07 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Warnings in Qt 4.6.3 (3.61 KB, text/plain)
2010-08-26 04:04 PDT, Csaba Osztrogonác
no flags Details
Fix MIPS warnings on Qt (1.67 KB, patch)
2010-10-22 11:55 PDT, Chao-ying Fu
no flags 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-08-13 03:51:33 PDT
The purpose of this bugzilla entry is to fix all 'increases required alignment of target type' warning in WebKit.

This kind of warnings typically occur on embedded systems (such as ARM or MIPS). The bug 38045 introduced a wrapper framework to fix those warning. 

I assume that more patches will be attached to this bug for multiple platforms. So, try not to close this bug after the first patch.
Comment 1 Gabor Loki 2010-08-16 00:26:50 PDT
Created attachment 64471 [details]
Fix platform independent alignment warnings
Comment 2 Gabor Loki 2010-08-16 00:27:31 PDT
Created attachment 64472 [details]
Fix alignment warnings on Qt
Comment 3 Balazs Kelemen 2010-08-16 01:17:19 PDT
Loki, what do you think about defining reinterpret_cast to reinterpret_cast_ptr in config.h or Platform.h? I think it will be very hard to teach every contributor about where should they use reinterpret_cast_ptr instead of reinterpret_cast. As I understood reinterpret_cast_ptr is more safe anyway.
Comment 4 Gabor Loki 2010-08-16 02:44:51 PDT
(In reply to comment #3)
> config.h or Platform.h? 

I see no space for reinterpret_cast_ptr in Platform.h. The platform-, architecture- and compiler-specific features and/or restrictions are in there. The config.h should be about the platform- or feature-specific includes (unfortunately there are other stuff in there). Well, Gavin and I thought the StdLibExtras would be the best place for this reinterpret_cast_ptr, but any suggestions are welcome.

> I think it will be very hard to teach every contributor about where should they use reinterpret_cast_ptr instead of reinterpret_cast.

Well, all contributor should learn not to allocate a char array if the object is used as an int array. ;) Basically most of the reinterpret_casts are used in a correct way. There are only several places where they cannot pass the alignment warning barrier. Although the compiler says warnings on those, those cases also allocate the objects with a good alignment.

Btw, after the ARM buildbots compile with -Werror the developers will be insulted with this alignment warning/error if they do not use the reinterpret_cast in a correct way. ;)

> As I understood reinterpret_cast_ptr is more safe anyway.

Safe? in debug mode on ARM? If so, you are right.
Well, the reinterpret_cast_ptr cannot be used in every situation. It is only useful if the source and the target type is a pointer and the size of target base type is differ from the source one (for example in a case where a reinterpret_cast<int*>(char*) structure is used).
Comment 5 Balazs Kelemen 2010-08-16 04:55:52 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > config.h or Platform.h? 
> 
> I see no space for reinterpret_cast_ptr in Platform.h. The platform-, architecture- and compiler-specific features and/or restrictions are in there. The config.h should be about the platform- or feature-specific includes (unfortunately there are other stuff in there). Well, Gavin and I thought the StdLibExtras would be the best place for this reinterpret_cast_ptr, but any suggestions are welcome.
> 

I was thinking about config.h because we use config.h like a prefix header so the define would be present without extra effort. Platform.h is similar from this viewpoint because it is included in config.h.

> > I think it will be very hard to teach every contributor about where should they use reinterpret_cast_ptr instead of reinterpret_cast.
> 
> Well, all contributor should learn not to allocate a char array if the object is used as an int array. ;) Basically most of the reinterpret_casts are used in a correct way. There are only several places where they cannot pass the alignment warning barrier. Although the compiler says warnings on those, those cases also allocate the objects with a good alignment.
>

I see. I thought that it is a more general case.

> Well, the reinterpret_cast_ptr cannot be used in every situation. It is only useful if the source and the target type is a pointer and the size of target base type is differ from the source one (for example in a case where a reinterpret_cast<int*>(char*) structure is used).

Well, this situation is the only one when a reinterpret_cast should be used. Anyway, if reinterpret_cast_ptr is needed only in special cases I agree that is better to do it explicitly.
Comment 6 Gabor Loki 2010-08-16 05:24:58 PDT
Created attachment 64485 [details]
Fix platform independent alignment warnings (v2)

There was an unnecessary rewrite of reinterpret_cast in TextCodecLatin1.cpp which fails in debug mode.
Comment 7 deepak 2010-08-16 22:22:35 PDT
Hi Loki, and everybody

I am not that good at C++, So please excuse if you find my question is silly.

The following patch will fix Vector buffer alignment warnings , which is done by casting m_inlineBuffer.buffer to static_cast<void*> 

So my question can we use reinterpret_cast_ptr in place of reinterpret_cast, instead of casting it to static_cast<void*> ? If yes then which one is safe solution

https://bug-38045-attachments.webkit.org/attachment.cgi?id=58134


I have ported 1.2.0 Gtk webkit release on both Arm and Mips ,But i get warnings only with Mips. And in Mips I also get alignment warnings at following lines. And as Balazs Kelemen said I am not sure if in all places if i use reinterpret_cast_ptr instead of reinterpret_cast , will resolve this warnings..


WebCore/platform/graphics/cairo/ImageBufferCairo.cpp lines 128 , 181 , 249: warning: cast from 'unsigned char*' to 'unsigned int*' increases required alignment of target type

WebCore/platform/graphics/cairo/ImageCairo.cpp:242: warning: cast from 'unsigned char*' to 'unsigned int*' increases required alignment of target type

WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:347: warning: cast from 'jpeg_error_mgr*' to 'WebCore::decoder_error_mgr*' increases required a    lignment of target type

WebKit/gtk/webkit/webkitwebview.cpp:2972: warning: cast from 'GTypeInstance*' to 'GtkAdjustment*' increases required alignment of target type
WebKit/gtk/webkit/webkitwebview.cpp:2973: warning: cast from 'GTypeInstance*' to 'GtkAdjustment*' increases required alignment of target type

JavaScriptCore/pcre/pcre_compile.cpp:2592: warning: cast from 'char*' to 'JSRegExp*' increases required alignment of target type

Best Regards
Comment 8 Gabor Loki 2010-08-16 23:41:35 PDT
> The following patch will fix Vector buffer alignment warnings , which is done by casting m_inlineBuffer.buffer to static_cast<void*> 
> https://bug-38045-attachments.webkit.org/attachment.cgi?id=58134

Did you mean it removes the warnings on *MIPS* ? If so, you should use reinterpret_cast_ptr in the same way on MIPS as it is used on ARM. See the condition at http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/StdLibExtras.h?rev=65311#L64
Try using the following condition instead of the current one: #if COMPILER(GCC) && (CPU(ARM) || CPU(MIPS))
With this change the reinterpret_cast_ptr<T>(ptr) will be an implicit reinterpret_cast<T>(static_cast<void*>(ptr)) structure (done by the preprocessor).

> So my question can we use reinterpret_cast_ptr in place of reinterpret_cast, instead of casting it to static_cast<void*> ? If yes then which one is safe solution

You can use reinterpret_cast_ptr where these alignment warnings occur. Although you should make sure that the original code uses a proper allocation (aligned to the target type). If a reinterpret_cast_ptr is used and a target address is not aligned to the target type, an ASSERT will be called at that place in debug mode.

So, the reinterpret_cast_ptr does not solve any alignment warnings, just hides them in release mode. Although in debug mode an ASSERT will inform where the use of the original reinterpret_casts were bogus.

> I have ported 1.2.0 Gtk webkit release on both Arm and Mips ,But i get warnings only with Mips. And in Mips I also get alignment warnings at following lines.

I can take a look.
Comment 9 Gabor Loki 2010-08-17 00:57:49 PDT
First of all almost every allocator endeavor to allocate an aligned memory range for an object, but the compilers do not know anything about the memory allocators. So, the alignment warnings can be safely ignored if reinterpret_cast is used on the beginning of the allocated object.

For example:
char* c = new char[1024]; // in most cases it is aligned to sizeof(int) bytes
unsigned* d = reinterpret_cast<unsigned*>(c); // Safe way
unsigned* d = reinterpret_cast<unsigned*>(&c[2]); // Bogus

> WebCore/platform/graphics/cairo/ImageBufferCairo.cpp lines 128 , 181 , 249: warning: cast from 'unsigned char*' to 'unsigned int*' increases required alignment of target type

128 and 249: I am not sure how Cairo allocates the data member of a cairo_image_surface structure, but I assume it is allocated as an RGBA container which can be an unsigned int array. It looks like a safe way to use reinterpret_cast here.

181: Fine

> WebCore/platform/graphics/cairo/ImageCairo.cpp:242: warning: cast from 'unsigned char*' to 'unsigned int*' increases required alignment of target type

242: same as above

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:347: warning: cast from 'jpeg_error_mgr*' to 'WebCore::decoder_error_mgr*' increases required a    lignment of target type

347: Fine

> WebKit/gtk/webkit/webkitwebview.cpp:2972: warning: cast from 'GTypeInstance*' to 'GtkAdjustment*' increases required alignment of target type
> WebKit/gtk/webkit/webkitwebview.cpp:2973: warning: cast from 'GTypeInstance*' to 'GtkAdjustment*' increases required alignment of target type
> 

I cannot found them in the trunk.

> JavaScriptCore/pcre/pcre_compile.cpp:2592: warning: cast from 'char*' to 'JSRegExp*' increases required alignment of target type

It will be fine if the StdLibExtras.h is modified.

I will create a patch for you which can tested in release and debug mode as well. ;)
Comment 10 Gabor Loki 2010-08-17 01:07:17 PDT
Created attachment 64562 [details]
fix MIPS alignment warnings (for deepak)

Deepak, please test this patch on MIPS in release and debug mode as well. (Unfortunately, I have no such a device.)
Comment 11 deepak 2010-08-17 01:10:43 PDT
As i mentioned Earlier , I never had Alignment problem with Arm, So i di not apply any patch for compilation, But on mips i started getting warnings, even rectangle draw operations are not happening properly .. And i had to disable fast-malloc support ,cause i was getting lots of Segmentation faults, Strange thing is that I never faced this issues with Arm (armv5 le)

I did reinterpret_cast_ptr change in StdLibExtras.h and used reinterpret_cast_ptr wherever i was getting alignment warning .. It removed almost 90% of the warnings .. But at following line i couldn't use it..

JavaScriptCore/pcre/pcre_compile.cpp +2592

and in following lines also

WebCore/platform/gtk/ScrollbarGtk.cpp +53
WebKit/gtk/webkit/webkitwebview.cpp   2972 and 2973
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp +347


I think your new patch should solve JPEGImageDecoder.cpp warning.. cause i din try in that way :)

Thanks
Comment 12 Gabor Loki 2010-08-17 01:25:07 PDT
> JavaScriptCore/pcre/pcre_compile.cpp +2592

Did you mean this one?
http://trac.webkit.org/browser/trunk/JavaScriptCore/pcre/pcre_compile.cpp#L2594
JSRegExp* re = reinterpret_cast_ptr<JSRegExp*>(new char[size]);


> and in following lines also
> 
> WebCore/platform/gtk/ScrollbarGtk.cpp +53

I am not familiar with Gtk+. What is GTK_ADJUSTMENT ?
Probably you can use reinterpret_cast_ptr here as well:
http://trac.webkit.org/browser/trunk/WebCore/platform/gtk/ScrollbarGtk.cpp#L55

> WebKit/gtk/webkit/webkitwebview.cpp   2972 and 2973

Could you point those lines in this link:
http://trac.webkit.org/browser/trunk/WebKit/gtk/webkit/webkitwebview.cpp
Comment 13 deepak 2010-08-17 02:20:25 PDT
(In reply to comment #12)
> > JavaScriptCore/pcre/pcre_compile.cpp +2592
> 
> Did you mean this one?
> http://trac.webkit.org/browser/trunk/JavaScriptCore/pcre/pcre_compile.cpp#L2594

Yes but sorry , that was my mistake .. i didn't "StdLibExtras.h"now it is not giving warnings.

> JSRegExp* re = reinterpret_cast_ptr<JSRegExp*>(new char[size]);
> 
> 
> > and in following lines also
> > 
> > WebCore/platform/gtk/ScrollbarGtk.cpp +53
> 
> I am not familiar with Gtk+. What is GTK_ADJUSTMENT ?
> Probably you can use reinterpret_cast_ptr here as well:
> http://trac.webkit.org/browser/trunk/WebCore/platform/gtk/ScrollbarGtk.cpp#L55

check my comment below
 
> 
> > WebKit/gtk/webkit/webkitwebview.cpp   2972 and 2973
> 
> Could you point those lines in this link:
> http://trac.webkit.org/browser/trunk/WebKit/gtk/webkit/webkitwebview.cpp

http://trac.webkit.org/browser/trunk/WebKit/gtk/webkit/webkitwebview.cpp#L3087
http://trac.webkit.org/browser/trunk/WebKit/gtk/webkit/webkitwebview.cpp#L3088

http://trac.webkit.org/browser/trunk/WebCore/platform/gtk/ScrollbarGtk.cpp#L55

And i replaced them like this (as well as in ScrollbarGtk.cpp#L55)

reinterpret_cast_ptr<GtkAdjustment*>((gtk_adjustment_new(0.0, 0.0, 0.0, 0.0, 0.0, 0.0)));

And all warnings disappeared :)

Now i don see any alignment warnings with all these changes :) (atleast in release build) .. I will enable debug and re compile again.

Thanks Loki, I hope this will help others also ..
Comment 14 Gabor Loki 2010-08-17 02:48:23 PDT
> Now i don see any alignment warnings with all these changes :) (atleast in release build) .. I will enable debug and re compile again.
> 
> I hope this will help others also ..

You are welcome!
May I ask you to create a patch which disables MIPS alignment warning in the trunk?
Comment 15 deepak 2010-08-17 03:54:55 PDT
(In reply to comment #14)
> > Now i don see any alignment warnings with all these changes :) (atleast in release build) .. I will enable debug and re compile again.
> > 
> > I hope this will help others also ..
> 
> You are welcome!
> May I ask you to create a patch which disables MIPS alignment warning in the trunk?

Yea ,  right now i am not using trunk , im using release version .. I have noted down all changes, ..Ill definitely checkout trunk and upload the patch after im done with all changes..
Comment 16 Gabor Loki 2010-08-25 01:28:04 PDT
Comment on attachment 64485 [details]
Fix platform independent alignment warnings (v2)

Committed revision 65995.
Comment 17 Gabor Loki 2010-08-25 02:37:32 PDT
Comment on attachment 64472 [details]
Fix alignment warnings on Qt

Committed revision 65999.
Comment 18 Csaba Osztrogonác 2010-08-26 04:04:08 PDT
Created attachment 65542 [details]
Warnings in Qt 4.6.3

To enable -Werror compiler flag for QtWebKit ARM builds too,
we should fix some warnings in Qt inside. Gabor, what do you
think, is it possible without modifying Qt?

Or should we ignore warnings in Qt with
using -isystem instead of -I for Qt headers?
But I'm not sure if qmake has ability to do it.
Comment 19 Chao-ying Fu 2010-10-22 11:55:29 PDT
Created attachment 71581 [details]
Fix MIPS warnings on Qt

Hi,

  These two fixes remove all alignment warnings on MIPS JIT build with Qt (--qt).  Test results are ok.  Thanks!

Regards,
Chao-ying
Comment 20 WebKit Review Bot 2010-10-22 14:29:14 PDT
Attachment 71581 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:64:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Gabor Loki 2010-11-18 00:24:17 PST
(In reply to comment #19)
> Created an attachment (id=71581) [details]

It is fine by me. Ossy, please review it!
Comment 22 Csaba Osztrogonác 2010-11-18 01:41:57 PST
Comment on attachment 71581 [details]
Fix MIPS warnings on Qt

LGTM
Comment 23 WebKit Commit Bot 2010-11-18 04:53:37 PST
The commit-queue encountered the following flaky tests while processing attachment 71581 [details]:

transitions/transition-end-event-transform.html
fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dino@apple.com, dumi@chromium.org, ojan@chromium.org, and pol@apple.com.  The commit-queue is continuing to process your patch.
Comment 24 WebKit Commit Bot 2010-11-18 05:49:38 PST
The commit-queue encountered the following flaky tests while processing attachment 71581 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 25 WebKit Commit Bot 2010-11-18 05:51:39 PST
Comment on attachment 71581 [details]
Fix MIPS warnings on Qt

Clearing flags on attachment: 71581

Committed r72289: <http://trac.webkit.org/changeset/72289>
Comment 26 WebKit Commit Bot 2010-11-18 05:51:46 PST
All reviewed patches have been landed.  Closing bug.