Bug 124153 - [EFL] Fix several warnings for ARM cross compilation
Summary: [EFL] Fix several warnings for ARM cross compilation
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-11 09:22 PST by Gabor Rapcsanyi
Modified: 2017-03-11 10:43 PST (History)
13 users (show)

See Also:


Attachments
proposed patch (2.24 KB, patch)
2013-11-11 09:32 PST, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff
warnings (41.65 KB, text/plain)
2013-11-12 02:14 PST, Gabor Rapcsanyi
no flags Details
Fix for ARM warnings (21.90 KB, patch)
2013-11-15 04:37 PST, Gabor Rapcsanyi
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 Rapcsanyi 2013-11-11 09:22:38 PST
[EFL] Disable -Werror when cross compiling
Comment 1 Gabor Rapcsanyi 2013-11-11 09:32:45 PST
Created attachment 216581 [details]
proposed patch
Comment 2 Gyuyoung Kim 2013-11-11 22:47:09 PST
Comment on attachment 216581 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216581&action=review

> ChangeLog:3
> +        [EFL] Disable -Werror when cross compiling

I wonder why you need to disable -Werror when compiling on cross platform. Isn't it nice to remove all build warnings ?
Comment 3 Gabor Rapcsanyi 2013-11-12 02:14:00 PST
Created attachment 216650 [details]
warnings

We would like to make an ARM EFL bot but there are these warnings which block us. There are many 'increases required alignment of target type' warning because of -Wcast-align.
I made a small example and tried it on our ARM compiler (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)):

int main() {
  unsigned char* d1;
  short int d2;

  d2 = *reinterpret_cast<short int*>(d1);
}

g++ -Wcast-align main.cpp
main.cpp: In function ‘int main()’:
main.cpp:5:40: warning: cast from ‘unsigned char*’ to ‘short int*’ increases required alignment of target type [-Wcast-align]


With the x86 gcc there is no problem.
So while we fixing these warnings we would like to disable -Werror for cross compilation.
Comment 4 Ryuan Choi 2013-11-12 02:31:39 PST
(In reply to comment #3)
> Created an attachment (id=216650) [details]
> warnings
> 
> We would like to make an ARM EFL bot but there are these warnings which block us. There are many 'increases required alignment of target type' warning because of -Wcast-align.
> I made a small example and tried it on our ARM compiler (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)):
> 
> int main() {
>   unsigned char* d1;
>   short int d2;
> 
>   d2 = *reinterpret_cast<short int*>(d1);
> }
> 
> g++ -Wcast-align main.cpp
> main.cpp: In function ‘int main()’:
> main.cpp:5:40: warning: cast from ‘unsigned char*’ to ‘short int*’ increases required alignment of target type [-Wcast-align]
> 
> 
> With the x86 gcc there is no problem.
> So while we fixing these warnings we would like to disable -Werror for cross compilation.

Can't we use reinterpret_cast_ptr for it if casting is fine?
Comment 5 Csaba Osztrogonác 2013-11-12 02:36:35 PST
(In reply to comment #4)
> Can't we use reinterpret_cast_ptr for it if casting is fine?

Yes, it is the proper fix, but before using it, each cast should be checked 
one by one. When I fixed similar warnings in the past, I have to say it isn't
trivial to decide if a cast is fine or not in many cases.
Comment 6 Gabor Rapcsanyi 2013-11-12 02:37:43 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=216650) [details] [details]
> > warnings
> > 
> > We would like to make an ARM EFL bot but there are these warnings which block us. There are many 'increases required alignment of target type' warning because of -Wcast-align.
> > I made a small example and tried it on our ARM compiler (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)):
> > 
> > int main() {
> >   unsigned char* d1;
> >   short int d2;
> > 
> >   d2 = *reinterpret_cast<short int*>(d1);
> > }
> > 
> > g++ -Wcast-align main.cpp
> > main.cpp: In function ‘int main()’:
> > main.cpp:5:40: warning: cast from ‘unsigned char*’ to ‘short int*’ increases required alignment of target type [-Wcast-align]
> > 
> > 
> > With the x86 gcc there is no problem.
> > So while we fixing these warnings we would like to disable -Werror for cross compilation.
> 
> Can't we use reinterpret_cast_ptr for it if casting is fine?

Unfortunately this problem exists with the dependent libs as well:
/home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/eina-1/eina/eina_inline_hash.x:112:73: warning: cast from 'const unsigned char*' to 'const unsigned int*' increases required alignment of target type [-Wcast-align]
/home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/gstreamer-1.0/gst/gstbuffer.h:333:71: warning: cast from 'GstMiniObject* {aka _GstMiniObject*}' to 'GstBuffer* {aka _GstBuffer*}' increases required alignment of target type [-Wcast-align]
Comment 7 Ryuan Choi 2013-11-12 02:42:44 PST
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Created an attachment (id=216650) [details] [details] [details]
> > > warnings
> > > 
> > > We would like to make an ARM EFL bot but there are these warnings which block us. There are many 'increases required alignment of target type' warning because of -Wcast-align.
> > > I made a small example and tried it on our ARM compiler (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)):
> > > 
> > > int main() {
> > >   unsigned char* d1;
> > >   short int d2;
> > > 
> > >   d2 = *reinterpret_cast<short int*>(d1);
> > > }
> > > 
> > > g++ -Wcast-align main.cpp
> > > main.cpp: In function ‘int main()’:
> > > main.cpp:5:40: warning: cast from ‘unsigned char*’ to ‘short int*’ increases required alignment of target type [-Wcast-align]
> > > 
> > > 
> > > With the x86 gcc there is no problem.
> > > So while we fixing these warnings we would like to disable -Werror for cross compilation.
> > 
> > Can't we use reinterpret_cast_ptr for it if casting is fine?
> 
> Unfortunately this problem exists with the dependent libs as well:
> /home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/eina-1/eina/eina_inline_hash.x:112:73: warning: cast from 'const unsigned char*' to 'const unsigned int*' increases required alignment of target type [-Wcast-align]
> /home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/gstreamer-1.0/gst/gstbuffer.h:333:71: warning: cast from 'GstMiniObject* {aka _GstMiniObject*}' to 'GstBuffer* {aka _GstBuffer*}' increases required alignment of target type [-Wcast-align]

For EFL side, I landed https://git.enlightenment.org/core/efl.git/commit/?id=ec1ba326909fae86cf7738b8475bfd05bc2323cb

For jhbuild, I think that we can add the patch.
Can we make similiar patch for gstreamer?
Comment 8 Gabor Rapcsanyi 2013-11-13 06:24:19 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > Created an attachment (id=216650) [details] [details] [details] [details]
> > > > warnings
> > > > 
> > > > We would like to make an ARM EFL bot but there are these warnings which block us. There are many 'increases required alignment of target type' warning because of -Wcast-align.
> > > > I made a small example and tried it on our ARM compiler (gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)):
> > > > 
> > > > int main() {
> > > >   unsigned char* d1;
> > > >   short int d2;
> > > > 
> > > >   d2 = *reinterpret_cast<short int*>(d1);
> > > > }
> > > > 
> > > > g++ -Wcast-align main.cpp
> > > > main.cpp: In function ‘int main()’:
> > > > main.cpp:5:40: warning: cast from ‘unsigned char*’ to ‘short int*’ increases required alignment of target type [-Wcast-align]
> > > > 
> > > > 
> > > > With the x86 gcc there is no problem.
> > > > So while we fixing these warnings we would like to disable -Werror for cross compilation.
> > > 
> > > Can't we use reinterpret_cast_ptr for it if casting is fine?
> > 
> > Unfortunately this problem exists with the dependent libs as well:
> > /home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/eina-1/eina/eina_inline_hash.x:112:73: warning: cast from 'const unsigned char*' to 'const unsigned int*' increases required alignment of target type [-Wcast-align]
> > /home/rgabor/WebKit/WebKitBuild/Dependencies/Root/include/gstreamer-1.0/gst/gstbuffer.h:333:71: warning: cast from 'GstMiniObject* {aka _GstMiniObject*}' to 'GstBuffer* {aka _GstBuffer*}' increases required alignment of target type [-Wcast-align]
> 
> For EFL side, I landed https://git.enlightenment.org/core/efl.git/commit/?id=ec1ba326909fae86cf7738b8475bfd05bc2323cb
> 
> For jhbuild, I think that we can add the patch.
> Can we make similiar patch for gstreamer?

I will investigate this.
Comment 9 Gabor Rapcsanyi 2013-11-15 04:37:08 PST
Created attachment 217042 [details]
Fix for ARM warnings

This patch fixes all of the warnings. Mostly I changed some reinterpret_cast to reinterpret_cast_ptr and add some UNUSED_PARAM macro.
There was a strange warning here:
WebMemorySamplerLinux.cpp:70:19: warning: comparison is always false due to limited range of data type
As I found out on ARM architecture the char type is unsigned by default and this caused the problem. I changed that to signed char.
Comment 10 Philippe Normand 2013-11-15 06:43:39 PST
Comment on attachment 217042 [details]
Fix for ARM warnings

Have you submitted the patches upstream?
Comment 11 Gabor Rapcsanyi 2013-11-15 07:05:28 PST
(In reply to comment #10)
> (From update of attachment 217042 [details])
> Have you submitted the patches upstream?

I have made the bugreports:
Gstreamer: https://bugzilla.gnome.org/show_bug.cgi?id=712368
Glib: https://bugzilla.gnome.org/show_bug.cgi?id=712370
Comment 12 Gyuyoung Kim 2013-11-16 02:33:25 PST
Comment on attachment 217042 [details]
Fix for ARM warnings

View in context: https://bugs.webkit.org/attachment.cgi?id=217042&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL] Fix several warnings for ARM cross compilation

I think your patch doesn't only deal with EFL port area. So, it looks it would be better to split this patch into two patches, one is for WebCore and JS and WK2, other one is for only EFL. Anyway, current patch is not only for EFL port. So, [EFL] prefix is not proper for this patch.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:965
> +    UNUSED_PARAM(pc);

BTW, isn't the *pc* argument being used below line ?

http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp#L987
Comment 13 Gyuyoung Kim 2015-02-03 07:29:21 PST
Comment on attachment 217042 [details]
Fix for ARM warnings

Cleared review? from attachment 217042 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.
Comment 14 Michael Catanzaro 2017-03-11 10:43:29 PST
Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.