[Mesa-dev] [PATCH v2] egl/dri2: Fix GCC maybe-uninitialized warning.

[Emil Velikov]

On 26 March 2015 at 00:40, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Wed, Mar 25, 2015 at 8:31 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Wed, Mar 25, 2015 at 4:15 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>>> On Wed, 2015-03-25 at 18:55 -0400, Ilia Mirkin wrote:
>>>> On Wed, Mar 25, 2015 at 6:51 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>>>> > On Fri, 2015-03-06 at 23:54 -0800, Vinson Lee wrote:
>>>> >> egl_dri2.c: In function ‘dri2_bind_tex_image’:
>>>> >> egl_dri2.c:1240:4: warning: ‘format’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>> >>     (*dri2_dpy->tex_buffer->setTexBuffer2)(dri2_ctx->dri_context,
>>>> >>     ^
>>>> >>
>>>> >> Suggested-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>> >> Signed-off-by: Vinson Lee <vlee at freedesktop.org>
>>>> >> ---
>>>> >>  src/egl/drivers/dri2/egl_dri2.c | 6 ++++--
>>>> >>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>> >>
>>>> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>>>> >> index d503196..c5c475d 100644
>>>> >> --- a/src/egl/drivers/dri2/egl_dri2.c
>>>> >> +++ b/src/egl/drivers/dri2/egl_dri2.c
>>>> >> @@ -1226,7 +1226,8 @@ dri2_bind_tex_image(_EGLDriver *drv,
>>>> >>        format = __DRI_TEXTURE_FORMAT_RGBA;
>>>> >>        break;
>>>> >>     default:
>>>> >> -      assert(0);
>>>> >> +      _eglError(EGL_BAD_SURFACE, "unrecognized format");
>>>> >> +      return EGL_FALSE;
>>>> >
>>>> > does using:
>>>> > unreachable("unrecognized format");
>>>> > instead of
>>>> > assert(0);
>>>> > fix the warning?
>>>>
>>>> unreachable is for *truly* unreachable code... it sounded like this
>>>> was reachable with bad input.
>>>
>>> maybe I misunderstood the situation.
>>> since there is assert(0) I assumed it can never happen.
>>> combination of assert(0) and return is very confusing:
>>> either the code is reachable and should have a correct error path (in
>>> which case there should not be assert(0)),
>>> or the code is not reachable in which case unreachable does just fine
>>> and you should not have the error path.
>>>
>>> it looks to me that using assert and return just makes sure that the
>>> error path is never run on debug build.
>>>
>>> anyway, it was just a suggestion. I won't argue one way or another,
>>> since I don't work with/understand those parts of the code.
>>
>> I agree. If the code had an assert(0) it's pretty clearly a case for
>> unreachable.
>
> I dunno, I've seen assert's thrown in all over the place where the
> assumption was that they'd only trigger on debug builds. Not sure if
> this is one of those cases, but I have a hard time convincing myself
> that there's no way an unexpected value can get in there. The downside
> of unreachable() is that it ends up as an infinite loop or other sorts
> of funny control flow, which can be quite difficult to debug (in a
> production build).
Fwiw I agree with Ilia here. Currently we have cases where asset(0)
does not (seem to) imply unreachable. That may have been the original
idea, although not any more. In the odd case like this, a few bytes of
code might be better than getting stuck in an infinite loop ?

-Emil