Bug 178090 - [WPE] Build failure due to invalid cast of EGLNativeWindowType when targetting 64-bit ARM
Summary: [WPE] Build failure due to invalid cast of EGLNativeWindowType when targettin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit WPE (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on: 178147
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-09 12:21 PDT by Adrian Perez
Modified: 2017-10-16 10:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2017-10-09 12:28 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2017-10-09 12:36 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (1.77 KB, patch)
2017-10-10 05:05 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (2.56 KB, patch)
2017-10-10 15:55 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2017-10-09 12:21:44 PDT
This is trying to build the upstream code with Buildroot targeting
ARM Cortex A53 (AArch64):

  WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp: In member function ‘virtual uint64_t WebKit::AcceleratedSurfaceWPE::window() const’:
  WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp:78:99: error: invalid cast from type ‘EGLNativeWindowType {aka long unsigned int}’ to type ‘uint64_t {aka long long unsigned int}’
       return reinterpret_cast<uint64_t>(wpe_renderer_backend_egl_target_get_native_window(m_backend));
                                                                                                     ^

Changing the cast to use “static_cast” works fine, I'll send a patch.
Comment 1 Adrian Perez 2017-10-09 12:28:13 PDT
Created attachment 323197 [details]
Patch
Comment 2 Adrian Perez 2017-10-09 12:34:16 PDT
Comment on attachment 323197 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [WPE] Build failure in Buildroot when targetting 32-bit ARM

I have to edit the ChangeLog to follow the change in the bug title 8-)
Comment 3 Adrian Perez 2017-10-09 12:36:13 PDT
Created attachment 323199 [details]
Patch
Comment 4 Build Bot 2017-10-09 12:38:46 PDT
Attachment 323199 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: invalid cast  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Zan Dobersek 2017-10-10 03:55:36 PDT
Comment on attachment 323199 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp:75
> +

No need for this newline.
Comment 6 Adrian Perez 2017-10-10 04:50:45 PDT
(In reply to Zan Dobersek from comment #5)
> Comment on attachment 323199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323199&action=review
> 
> > Source/WebKit/WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp:75
> > +
> 
> No need for this newline.

Indeed, I'll remove it before landing. Thanks!
Comment 7 Adrian Perez 2017-10-10 05:05:26 PDT
Created attachment 323298 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2017-10-10 05:47:03 PDT
Comment on attachment 323298 [details]
Patch for landing

Clearing flags on attachment: 323298

Committed r223130: <http://trac.webkit.org/changeset/223130>
Comment 9 WebKit Commit Bot 2017-10-10 05:47:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Carlos Alberto Lopez Perez 2017-10-10 10:46:40 PDT
(In reply to WebKit Commit Bot from comment #8)
> Comment on attachment 323298 [details]
> Patch for landing
> 
> Clearing flags on attachment: 323298
> 
> Committed r223130: <http://trac.webkit.org/changeset/223130>

This has broken the build for me (GCC 5.3; ARMv7 Thumb2):


[.... gcc .... ] -c /home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp
/home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp: In member function 'virtual uint64_t WebKit::AcceleratedSurfaceWPE::window() const':
/home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/AcceleratedSurfaceWPE.cpp:79:94: error: invalid static_cast from type 'EGLNativeWindowType {aka wl_egl_window*}' to type 'uint64_t {aka long long unsigned int}'
     return static_cast<uint64_t>(wpe_renderer_backend_egl_target_get_native_window(m_backend));
Comment 11 Adrian Perez 2017-10-10 11:27:28 PDT
(In reply to Carlos Alberto Lopez Perez from comment #10)
> (In reply to WebKit Commit Bot from comment #8)
> > Comment on attachment 323298 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 323298
> > 
> > Committed r223130: <http://trac.webkit.org/changeset/223130>
> 
> This has broken the build for me (GCC 5.3; ARMv7 Thumb2):
> 
> 
> [.... gcc .... ] -c
> /home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-
> gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/
> AcceleratedSurfaceWPE.cpp
> /home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-
> gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/
> AcceleratedSurfaceWPE.cpp: In member function 'virtual uint64_t
> WebKit::AcceleratedSurfaceWPE::window() const':
> /home/igalia/clopez/wpe-test/tmp/work/cortexa9t2hf-neon-mx6qdl-poky-linux-
> gnueabi/wpewebkit/0.r223132-r0/trunk/Source/WebKit/WebProcess/WebPage/wpe/
> AcceleratedSurfaceWPE.cpp:79:94: error: invalid static_cast from type
> 'EGLNativeWindowType {aka wl_egl_window*}' to type 'uint64_t {aka long long
> unsigned int}'
>      return
> static_cast<uint64_t>(wpe_renderer_backend_egl_target_get_native_window(m_bac
> kend));

Right, we'll have to handle the configurations where “EGLNativeWindowType”
is a pointer.
Comment 12 Michael Catanzaro 2017-10-10 14:17:29 PDT
Would it be correct to use a C-style cast here, with a comment explaining why C++ casts won't work?
Comment 13 Adrian Perez 2017-10-10 15:43:13 PDT
(In reply to Michael Catanzaro from comment #12)
> Would it be correct to use a C-style cast here, with a comment explaining
> why C++ casts won't work?

Yes, I arrived to the same conclusion after checking the C++ standard
and confirming that e.g. reinterpret_cast will only convert types of
the same bit width (makes sense) and that static_cast will never convert
a pointer to an integer :-(

I've made a rollout of the previous patch and I will be posting a new
one so it's easier to track and backport the change to stable branches.
Comment 14 Adrian Perez 2017-10-10 15:55:24 PDT
Created attachment 323356 [details]
Patch
Comment 15 Build Bot 2017-10-10 15:58:50 PDT
Attachment 323356 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: invalid cast  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 WebKit Commit Bot 2017-10-16 10:31:03 PDT
Comment on attachment 323356 [details]
Patch

Clearing flags on attachment: 323356

Committed r223416: <https://trac.webkit.org/changeset/223416>
Comment 17 WebKit Commit Bot 2017-10-16 10:31:04 PDT
All reviewed patches have been landed.  Closing bug.