Bug 44672 – Fix warning in wtf/ByteArray.h

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 66218 [details]
possibly fix

possibly fix for EWS bots without changelog, please don't review

[Gabor Loki]

> 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

[Csaba Osztrogonác]

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

[Gabor Loki]

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.

[WebKit Review Bot]

Attachment 66218 [details] did not build on win:
Build output: http://queues.webkit.org/results/3939031

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Created attachment 66300 [details]
possibly hack to make MSVC and GCC happy

Test for EWS bots, without changelog, please don't review.

[Gabor Loki]

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];

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Eric Seidel (no email)]

Attachment 66348 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3927051

[Csaba Osztrogonác]

Comment on attachment 66348 [details]
possibly fix for Win-EWS

I'm going to fix it on Mac.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 66364 [details]
proposed fix

Clearing flags on attachment: 66364

Committed r66729: <http://trac.webkit.org/changeset/66729>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.