Bug 43963 – Avoid increasing required alignment of target type warning

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gabor Loki]

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.

[Gabor Loki]

Created attachment 64471 [details]
Fix platform independent alignment warnings

[Gabor Loki]

Created attachment 64472 [details]
Fix alignment warnings on Qt

[Balazs Kelemen]

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.

[Gabor Loki]

(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).

[Balazs Kelemen]

(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.

[Gabor Loki]

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.

[deepak]

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

[Gabor Loki]

> 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.

[Gabor Loki]

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. ;)

[Gabor Loki]

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.)

[deepak]

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

[Gabor Loki]

> 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

[deepak]

(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 ..

[Gabor Loki]

> 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?

[deepak]

(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..

[Gabor Loki]

Comment on attachment 64485 [details]
Fix platform independent alignment warnings (v2)

Committed revision 65995.

[Gabor Loki]

Comment on attachment 64472 [details]
Fix alignment warnings on Qt

Committed revision 65999.

[Csaba Osztrogonác]

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.

[Chao-ying Fu]

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

[WebKit Review Bot]

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.

[Gabor Loki]

(In reply to comment #19)
> Created an attachment (id=71581) [details]

It is fine by me. Ossy, please review it!

[Csaba Osztrogonác]

Comment on attachment 71581 [details]
Fix MIPS warnings on Qt

LGTM

[WebKit Commit Bot]

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 [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 71581 [details]
Fix MIPS warnings on Qt

Clearing flags on attachment: 71581

Committed r72289: <http://trac.webkit.org/changeset/72289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.