Bug 44672

Summary: Fix warning in wtf/ByteArray.h
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, loki, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 43191    
Attachments:
Description Flags
possibly fix
none
possibly hack to make MSVC and GCC happy
none
possibly fix for Win-EWS
none
proposed fix none

Description Csaba Osztrogonác 2010-08-26 02:39:37 PDT
gcc warning:
../../../JavaScriptCore/wtf/ByteArray.h: In member function ‘bool WebCore::FELighting::drawLighting(WebCore::CanvasPixelArray*, int, int)’:
../../../JavaScriptCore/wtf/ByteArray.h:66: warning: array subscript is above array bounds

inlined getter functions used in drawLighting() cause this warning: 
ALWAYS_INLINE int upLeftPixelValue();
ALWAYS_INLINE int upPixelValue();
ALWAYS_INLINE int upRightPixelValue();
ALWAYS_INLINE int leftPixelValue();
ALWAYS_INLINE int centerPixelValue();
ALWAYS_INLINE int rightPixelValue();
ALWAYS_INLINE int downLeftPixelValue();
ALWAYS_INLINE int downPixelValue();
ALWAYS_INLINE int downRightPixelValue();

eg:
ALWAYS_INLINE int FELighting::LightingData::upLeftPixelValue()
{
    return static_cast<int>(pixels->get(offset - widthMultipliedByPixelSize - cPixelSize + cAlphaChannelOffset));
}

How do you guarantee avoiding incorrect array subscripts?

Zoltan, could you check and fix this bug, please? It would be 
the last step before enabling -Werror for QtWebKit x86-Linux build.
Comment 1 Csaba Osztrogonác 2010-09-01 09:04:59 PDT
Created attachment 66218 [details]
possibly fix

possibly fix for EWS bots without changelog, please don't review
Comment 2 Gabor Loki 2010-09-01 10:03:42 PDT
> possibly fix for EWS bots without changelog, please don't review

It is not a good solution. This will fail with msvc!
See: http://msdn.microsoft.com/en-us/library/79wf64bc.aspx
Comment 3 Csaba Osztrogonác 2010-09-01 11:05:46 PDT
(In reply to comment #2)
> > possibly fix for EWS bots without changelog, please don't review
> 
> It is not a good solution. This will fail with msvc!
> See: http://msdn.microsoft.com/en-us/library/79wf64bc.aspx

It is an other idea, I try unsized array instead of 
zero sized. It should work now. I hope. :)
Comment 4 Gabor Loki 2010-09-01 12:19:47 PDT
Practically it is the same trick.
MSVC 6.0 and MSVC 2003 say the same C4200 warning on it. I've tried it before.

Only the MSVC 2010 allows something: http://msdn.microsoft.com/en-us/library/b6fae073.aspx

So, I think it is not a good idea to use an unsized array with several compiler specific guards in order to avoid a bogus warning of GCC.
Comment 5 WebKit Review Bot 2010-09-01 15:47:18 PDT
Attachment 66218 [details] did not build on win:
Build output: http://queues.webkit.org/results/3939031
Comment 6 Csaba Osztrogonác 2010-09-01 17:01:25 PDT
(In reply to comment #4)
> Practically it is the same trick.
> MSVC 6.0 and MSVC 2003 say the same C4200 warning on it. I've tried it before.
> 
> Only the MSVC 2010 allows something: http://msdn.microsoft.com/en-us/library/b6fae073.aspx
> 
> So, I think it is not a good idea to use an unsized array with several compiler specific guards in order to avoid a bogus warning of GCC.

Unsized arrays are supported by all MSVC
    * Visual Studio 2010
    * Visual Studio 2008
    * Visual Studio 2005
    * Visual Studio .NET 2003

But http://webkit.org/building/tools.html say that _only_ VS2005 is supported for building WebKit.

Additionally the C99 standard [ISO/IEC 9899:1999] introduces flexible array members into the language, it should be supported by all compiler.

Unfortunately I don't know what the problem is on Win EWS until https://bugs.webkit.org/show_bug.cgi?id=39199 is fixed.
Comment 7 Csaba Osztrogonác 2010-09-01 17:06:21 PDT
Created attachment 66300 [details]
possibly hack to make MSVC and GCC happy

Test for EWS bots, without changelog, please don't review.
Comment 8 Gabor Loki 2010-09-02 00:14:25 PDT
Ossy, I suggest you to use the following form (or a similar one) to avoid the warnings:

+#if COMPILER(MSVC)
+// These compilers do not support zero-sized array
+#define DECLARATION_SIZE_OF_BYTEARRAY 0xffff
+#else
+#define DECLARATION_SIZE_OF_BYTEARRAY 0
+#endif

...

-unsigned char m_data[sizeof(size_t)];
+unsigned char m_data[DECLARATION_SIZE_OF_BYTEARRAY];
Comment 9 Csaba Osztrogonác 2010-09-02 01:07:26 PDT
Comment on attachment 66300 [details]
possibly hack to make MSVC and GCC happy

Status bubbles are green, I'll add a changlog and upload again.
Comment 10 Csaba Osztrogonác 2010-09-02 03:35:57 PDT
Created attachment 66348 [details]
possibly fix for Win-EWS

warning C4200: nonstandard extension used : zero-sized array in struct/union
Cannot generate copy-ctor or copy-assignment operator when UDT contains a zero-sized array

I think a possibly fix is defining private copy constructor 
and private operator= . MSVC can't generate them, I think 
nobody can generate correct copy constructor for this class.
So we can implement them if we need it. I think we shouldn't
copying a ByteArray object, so I prefer never used functions.

Please don't review it, I would like to wait Windows EWS.
If it will be green, I'll upload the patch with changelog
for review.
Comment 11 Eric Seidel (no email) 2010-09-02 04:15:22 PDT
Attachment 66348 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3927051
Comment 12 Csaba Osztrogonác 2010-09-02 04:19:20 PDT
Comment on attachment 66348 [details]
possibly fix for Win-EWS

I'm going to fix it on Mac.
Comment 13 Csaba Osztrogonác 2010-09-02 06:28:47 PDT
Created attachment 66364 [details]
proposed fix

First, I tried to use defined but not implemented 
copy constructor and operator=. And then I tried
to implement them with ASSERT_NOT_REACHED(), but
MSVC still warns.

I propose to use maximal array size for MSVC
and unsized array for other compilers. It won't
cause memory problems, because you can create
a ByteArray only with its create method, which
absolutely ignores the sized of m_data array.
Comment 14 Csaba Osztrogonác 2010-09-03 06:57:24 PDT
Comment on attachment 66364 [details]
proposed fix

Clearing flags on attachment: 66364

Committed r66729: <http://trac.webkit.org/changeset/66729>
Comment 15 Csaba Osztrogonác 2010-09-03 06:57:34 PDT
All reviewed patches have been landed.  Closing bug.