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.
Created attachment 64471 [details] Fix platform independent alignment warnings
Created attachment 64472 [details] Fix alignment warnings on Qt
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.
(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).
(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.
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.
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
> 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.
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. ;)
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.)
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
> 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
(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 ..
> 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?
(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 on attachment 64485 [details] Fix platform independent alignment warnings (v2) Committed revision 65995.
Comment on attachment 64472 [details] Fix alignment warnings on Qt Committed revision 65999.
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.
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
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.
(In reply to comment #19) > Created an attachment (id=71581) [details] It is fine by me. Ossy, please review it!
Comment on attachment 71581 [details] Fix MIPS warnings on Qt LGTM
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.
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 on attachment 71581 [details] Fix MIPS warnings on Qt Clearing flags on attachment: 71581 Committed r72289: <http://trac.webkit.org/changeset/72289>
All reviewed patches have been landed. Closing bug.