Bug 95468

Summary: [Qt] QNX triggers compile errors in ANGLE due to missing std::
Product: WebKit Reporter: Milian Wolff <milian.wolff>
Component: WebKit QtAssignee: Milian Wolff <milian.wolff>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abecsi, bfulgham, ddkilzer, dino, hausmann, iamsergio, kbr, pravind.2k4, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74040, 95466    
Attachments:
Description Flags
Patch
none
Patch none

Description Milian Wolff 2012-08-30 09:17:06 PDT
Various constructs in the ANGLE third party library reference code from cstring or similar headers without the explicit std:: namespace identifier. On other platforms this is apparently not an issue, one guess from Simon is:

[13:30] <tronical> milian: I suppose an implicit inclusion of string.h from somewhere, so that the extern "C" variant of strlen is chosen?

On QNX though, this is not the case and various compile errors arise. Expect a patch from me.
Comment 1 Milian Wolff 2012-08-31 04:02:31 PDT
Created attachment 161657 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-31 04:05:03 PDT
Attachment 161657 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ChangeLog', u'Sour..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:712:  More than one command on the same line in if  [whitespace/parens] [4]
Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:762:  More than one command on the same line in if  [whitespace/parens] [4]
Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:771:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Milian Wolff 2012-08-31 04:43:39 PDT
The style errors existed in ANGLE codebase already, I didn't change them on purpose. Should I though?
Comment 4 Pravin D 2012-08-31 08:46:49 PDT
(In reply to comment #3)
> The style errors existed in ANGLE codebase already, I didn't change them on purpose. Should I though?
> 

The previous code also had this problem(in correct style) i guess. 
The webkit style check script applies only to the code that has been changed. That is only to code added/modified by your patch. 

just bring the line content after the if() to the line below.
See 
http://www.webkit.org/coding/coding-style.html

Hope this helps...
Comment 5 Milian Wolff 2012-08-31 09:36:44 PDT
This means though that the patch will break the code style of the surrounding file, just to get it accepted in WebKit? Assuming I upstream the patch as-is to ANGLE, would I still need to change it to get it into WebKit and thus create a diversion?
Comment 6 Darin Adler 2012-08-31 10:25:02 PDT
Comment on attachment 161657 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:12
> +        Add the explicit std:: qualifier to uses of std functions and types.
> +        On other platforms this is apparently not required due to implicit
> +        inclusion of "string.h"-style headers containing extern "C" variants.
> +        On QNX at least this is not the case though, resulting in compile
> +        errors.

Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>?
Comment 7 Milian Wolff 2012-10-01 09:17:56 PDT
also blocks 74040
Comment 8 Milian Wolff 2012-10-12 04:39:13 PDT
Upstream patch: https://codereview.appspot.com/6653048
Comment 9 Simon Hausmann 2012-10-21 23:35:22 PDT
(In reply to comment #6)
> (From update of attachment 161657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161657&action=review
> 
> > Source/ThirdParty/ANGLE/ChangeLog:12
> > +        Add the explicit std:: qualifier to uses of std functions and types.
> > +        On other platforms this is apparently not required due to implicit
> > +        inclusion of "string.h"-style headers containing extern "C" variants.
> > +        On QNX at least this is not the case though, resulting in compile
> > +        errors.
> 
> Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>?

Millian, did you see Darin's comment? If that works then it would seem like a much simpler solution :)
Comment 10 Milian Wolff 2012-11-01 09:46:19 PDT
Yes, I saw Darin's comment, but wonder why it should be done instead of adding the explicit std:: qualified identifier.

Example: When trying to compile DirectiveParser.cpp into DirectiveParser.o, we get (without this patch):

qcc -Vgcc_ntoarmv7le -c -Wno-psabi -lang-c++ -O2 -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -D_REENTRANT -DQT_NO_CLIPBOARD -DQT_NO_LIBUDEV -DQT_NO_EVDEV -DQT_NO_XCB -DBUILDING_QT__=1 -DNDEBUG -DENABLE_3D_RENDERING=1 -DENABLE_BLOB=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_COMPOSITING=1 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_STICKY_POSITION=1 -DENABLE_DATALIST_ELEMENT=1 -DENABLE_DETAILS_ELEMENT=1 -DENABLE_FAST_MOBILE_SCROLLING=1 -DENABLE_FILTERS=1 -DENABLE_FTPDIR=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=1 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INPUT_TYPE_COLOR=1 -DENABLE_INSPECTOR=1 -DENABLE_INSPECTOR_SERVER=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_LEGACY_VIEWPORT_ADAPTION=1 -DENABLE_LEGACY_VENDOR_PREFIXES=1 -DENABLE_METER_ELEMENT=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_PROGRESS_ELEMENT=1 -DENABLE_RESOLUTION_MEDIA_QUERY=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DWTF_USE_TILED_BACKING_STORE=1 -DHAVE_QTQUICK=1 -DHAVE_QTPRINTSUPPORT=1 -DHAVE_QSTYLE=1 -DHAVE_QTTESTLIB=1 -DWTF_USE_ZLIB=1 -DPLUGIN_ARCHITECTURE_UNSUPPORTED=1 -DWTF_USE_3D_GRAPHICS=1 -DENABLE_WEBGL=1 -DENABLE_CSS_SHADERS=1 -DENABLE_TOUCH_SLIDER=1 -DENABLE_ACCELERATED_2D_CANVAS=0 -DENABLE_ANIMATION_API=0 -DENABLE_BATTERY_STATUS=0 -DENABLE_CSP_NEXT=0 -DENABLE_CSS_GRID_LAYOUT=0 -DENABLE_CSS_HIERARCHIES=0 -DENABLE_CSS_IMAGE_ORIENTATION=0 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_VARIABLES=0 -DENABLE_CSS3_CONDITIONAL_RULES=0 -DENABLE_CSS3_TEXT=0 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATAGRID=0 -DENABLE_DATA_TRANSFER_ITEMS=0 -DENABLE_DEVICE_ORIENTATION=0 -DENABLE_DIRECTORY_UPLOAD=0 -DENABLE_DOWNLOAD_ATTRIBUTE=0 -DENABLE_FILE_SYSTEM=0 -DENABLE_GAMEPAD=0 -DENABLE_GEOLOCATION=0 -DENABLE_HIGH_DPI_CANVAS=0 -DENABLE_INDEXED_DATABASE=0 -DENABLE_INPUT_SPEECH=0 -DENABLE_INPUT_TYPE_DATE=0 -DENABLE_INPUT_TYPE_DATETIME=0 -DENABLE_INPUT_TYPE_DATETIMELOCAL=0 -DENABLE_INPUT_TYPE_MONTH=0 -DENABLE_INPUT_TYPE_TIME=0 -DENABLE_INPUT_TYPE_WEEK=0 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LINK_PREFETCH=0 -DENABLE_LINK_PRERENDER=0 -DENABLE_MATHML=0 -DENABLE_MEDIA_SOURCE=0 -DENABLE_MEDIA_STATISTICS=0 -DENABLE_MEDIA_STREAM=0 -DENABLE_MHTML=0 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=0 -DENABLE_NAVIGATOR_CONTENT_UTILS=0 -DENABLE_NETSCAPE_PLUGIN_API=0 -DENABLE_NETWORK_INFO=0 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_QUOTA=0 -DENABLE_SCRIPTED_SPEECH=0 -DENABLE_SHADOW_DOM=0 -DENABLE_STYLE_SCOPED=0 -DENABLE_SVG_DOM_OBJC_BINDINGS=0 -DENABLE_TEXT_AUTOSIZING=0 -DENABLE_TEXT_NOTIFICATIONS_ONLY=0 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_UNDO_MANAGER=0 -DENABLE_VIBRATION=0 -DENABLE_VIDEO=0 -DENABLE_VIDEO_TRACK=0 -DENABLE_WEB_AUDIO=0 -DENABLE_XSLT=0 -DBUILDING_ANGLE -DBUILDING_WEBKIT -DQT_ASCII_CAST_WARNINGS -DQT_NO_EXCEPTIONS -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -DQT_NO_DEBUG -I/home/milian/projects/qt5/install-qnx650/mkspecs/qnx-armv7le-qcc -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE -I.moc/debug-shared -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/include/GLSLANG -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/include -I/home/milian/projects/qt5/qtwebkit/Source -I/home/milian/projects/qt5/build-qnx650/qtwebkit/Source/include -I/home/milian/projects/qt5/install-qnx650/include -I/home/milian/projects/qt5/install-qnx650/include/QtScript -I/opt/qnx650/target/qnx6/usr/include -I/opt/qnx650/target/qnx6/usr/include/freetype2 -I. -o obj/debug/src/compiler/preprocessor/new/DirectiveParser.o /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp
In file included from /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:16:
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:55: error: 'size_t' does not name a type                                                                                                                                         
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In constructor 'pp::MacroExpander::MacroContext::MacroContext()':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:58: error: class 'pp::MacroExpander::MacroContext' does not have any field named 'index'                                                                                         
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'bool pp::MacroExpander::MacroContext::empty() const':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:59: error: ISO C++ forbids comparison between pointer and integer                                                                                                                
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'const pp::Token& pp::MacroExpander::MacroContext::get()':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:60: error: ISO C++ forbids incrementing a pointer of type 'const char* (*)(const char*, int)'                                                                                    
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:60: error: lvalue required as increment operand                                                                                                                                  
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'void pp::MacroExpander::MacroContext::unget()':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:61: error: ISO C++ forbids decrementing a pointer of type 'const char* (*)(const char*, int)'                                                                                    
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:61: error: lvalue required as decrement operand                                                                                                                                  
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp: In member function 'void pp::DirectiveParser::parseVersion(pp::Token*)':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:712: error: 'atoi' was not declared in this scope                                                                                                                            
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp: In member function 'void pp::DirectiveParser::parseLine(pp::Token*)':
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:762: error: 'atoi' was not declared in this scope                                                                                                                            
/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:771: error: 'atoi' was not declared in this scope                                                                                                                            
cc: /opt/qnx650/host/linux/x86/usr/lib/gcc/arm-unknown-nto-qnx6.5.0eabi/4.4.2/cc1plus error 1
make[2]: *** [obj/debug/src/compiler/preprocessor/new/DirectiveParser.o] Error 1
make[2]: Leaving directory `/home/milian/projects/qt5/build-qnx650/qtwebkit/Source/ThirdParty/ANGLE'

Where would you add the include to string.h? Into MacroExpander.h?  DirectiveParser.cpp? But then again, if I'm patching ANGLE sources, I can also fix it by adding the std:: namespace identifier. Anyhow, I'll ask the ANGLE people on what they prefer, so far noone has taken an interest in my patch.
Comment 11 Sérgio Martins 2012-11-09 01:09:06 PST
> [13:30] <tronical> milian: I suppose an implicit inclusion of string.h from
> somewhere, so that the extern "C" variant of strlen is chosen?

string.h isn't being included. What happens is that some compilers, when including the cstring, cstdlib etc,  will also define the stuff in global namespace.

QNX's compiler does'n do this, but it isn't a compiler bug, as the standard doesn't specify: http://stackoverflow.com/questions/9149368/is-it-ok-to-qualify-c-functions-with-the-std-namespace


> > Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>?
> 
> Millian, did you see Darin's comment? If that works then it would seem like a much simpler solution :)

Build also fails on malloc(), we would need stdlib.h too, we're opening a can of worms.

IMO, duplicating C and C++ headers is not a good solution.
Besides, C header usage in C++ is discouraged by the standard.

A couple of "using" or std:: would be best.
Comment 12 Milian Wolff 2012-11-19 07:27:19 PST
Created attachment 174978 [details]
Patch

Updated patch. I'll still try to resolve this issue somehow in ANGLE upstream...
Comment 13 Milian Wolff 2012-11-19 08:53:46 PST
Upstream ANGLE diverged quite a bit, I've submitted a patch but I'm not sure whether that is all that is required to get it working for WebKit. See: https://codereview.appspot.com/6843083/
Comment 14 Milian Wolff 2012-11-30 05:32:07 PST
Ok, my patch was accepted upstream in ANGLE. What do I do now to get this issue resolved in WebKit? Do I have to submit a patch that updates the local checkout of ANGLE in the WebKit source tree?

See: https://codereview.appspot.com/6843083/

Considering that upstream ANGLE diverged, we cannot apply the patch from there 1to1 to the WebKit copy.
Comment 15 Andras Becsi 2012-11-30 07:06:16 PST
I've seen patches against the copy of ANGLE in WebKit, but I'm not sure that's the right way.

CC'ing a couple of people who might know how updates are done.
Comment 16 Kenneth Russell 2012-11-30 11:06:28 PST
@dino did the last major update of the copy of ANGLE in WebKit.
Comment 17 Dean Jackson 2012-12-10 11:48:55 PST
(In reply to comment #16)
> @dino did the last major update of the copy of ANGLE in WebKit.

WebKit ANGLE diverged from upstream ANGLE, but maybe we can sync back up now. I'll look into this.
Comment 18 Dean Jackson 2012-12-10 11:50:19 PST
(In reply to comment #17)
> (In reply to comment #16)
> > @dino did the last major update of the copy of ANGLE in WebKit.
> 
> WebKit ANGLE diverged from upstream ANGLE, but maybe we can sync back up now. I'll look into this.

If this is urgent, and it is already landed in upstream, then you can apply to the WebKit fork.

The goal is to get back onto the official ANGLE as soon as possible, so that we can avoid these situations.
Comment 19 Brent Fulgham 2014-01-09 20:58:34 PST
The QT port is no longer part of WebKit.