Re: [PATCH] cogl: fix compile error with -Werror=maybe-uninitialized

[Changqing Li <changqing.li@windriver.com>]

On 4/3/19 5:58 PM, Adrian Bunk wrote:
> On Wed, Apr 03, 2019 at 10:29:08AM +0800, Changqing Li wrote:
>> On 4/2/19 7:31 PM, Adrian Bunk wrote:
>>> On Tue, Apr 02, 2019 at 05:08:33PM +0800, changqing.li@windriver.com wrote:
>>>> From: Changqing Li <changqing.li@windriver.com>
>>>>
>>>> fix below compile error with -Werror=maybe-uninitialized
>>>>
>>>> | ../../cogl-1.22.2/cogl/driver/gl/gles/cogl-driver-gles.c:217:17: error: 'gltype' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>> |      *out_gltype = gltype;
>>>> |      ~~~~~~~~~~~~^~~~~~~~
>>>> | ../../cogl-1.22.2/cogl/driver/gl/gles/cogl-driver-gles.c:213:22: error: 'glintformat' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>> |      *out_glintformat = glintformat;
>>>> |      ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
>>>> ...
>>> Looking at the code, is this a failure that only happens with DEBUG_FLAGS?
>> Yes, only happen with DEBUG_FLAGS
>>>> +--- a/cogl/driver/gl/gles/cogl-driver-gles.c
>>>> ++++ b/cogl/driver/gl/gles/cogl-driver-gles.c
>>>> +@@ -74,9 +74,9 @@ _cogl_driver_pixel_format_to_gl (CoglContext *context,
>>>> +                                  GLenum *out_gltype)
>>>> + {
>>>> +   CoglPixelFormat required_format;
>>>> +-  GLenum glintformat;
>>>> ++  GLenum glintformat = 0;
>>>> +   GLenum glformat = 0;
>>>> +-  GLenum gltype;
>>>> ++  GLenum gltype = 0;
>>>> ...
>>> Assigning random values to variables is a bug,
>>> they do not even seem to be valid values for these variables.
>> According to code logic,  glformat and glintformat will assigned
>> simultaneously and usually with same value,
> But glformat has an assert later to ensure the 0 doesn't leak anywhere.
>
>> and 0 mean invalid pixel format.
> For pixel format 0 means COGL_PIXEL_FORMAT_ANY.
>
> For glintformat I don't know what would happen if 0 is used.
>
>> [snip]
>>
>>      case COGL_PIXEL_FORMAT_DEPTH_24_STENCIL_8:
>>        glintformat = GL_DEPTH_STENCIL;
>>        glformat = GL_DEPTH_STENCIL;
>>        gltype = GL_UNSIGNED_INT_24_8;
>>        break;
>>
>> and for gtype, there also should be no problem according to the code logic.
> According to the current code.
>
> If some future change breaks the code, it would pass invalid data at
> runtime instead of giving a compile warning.
>
>> and the fix is just for eliminate the error when DEBUG_FLAGS is enabled.
> Looking at the code, does passing --enable-maintainer-flags=no
> to configure work around this gcc bug

Hi, Adrian

Thanks for your detailed comments. And the future change wrong is 
reasonable, but according to

current code,  seems it can assert from g_assert_not_reached or g_assert 
when glformat/glintformat is not assigned new value.

[snip]

     case xxx

            glformat=xxx

            glintformat=xxx

     case COGL_PIXEL_FORMAT_ANY:
     case COGL_PIXEL_FORMAT_YUV:
       g_assert_not_reached ();
       break;
     }

   /* All of the pixel formats are handled above so if this hits then
      we've been given an invalid pixel format */
   g_assert (glformat != 0);


And also upstream have use same way to fix under 
cogl/driver/gl/gl/cogl-driver-gl.c

https://gitlab.gnome.org/GNOME/cogl/commit/ca5226513eb64bceb38ca01994799c4e7cd9f5fa

so I think we can fix same way under this 
cogl/driver/gl/gles/cogl-driver-gles.c

>
> cu
> Adrian
>
-- 
BRs

Sandy(Li Changqing)