[Mesa-dev] [PATCH 03/20] mesa: separate legacy stuff from gl_texture_unit into gl_fixedfunc_texture_unit

[Marek Olšák]

On Wed, Nov 22, 2017 at 10:15 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 11/21/2017 10:01 AM, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> ---
>>  src/mesa/drivers/common/meta.c                  | 18 +++---
>>  src/mesa/drivers/dri/i915/i830_texblend.c       |  3 +-
>>  src/mesa/drivers/dri/nouveau/nouveau_util.h     |  2 +-
>>  src/mesa/drivers/dri/nouveau/nv04_context.c     | 14 +++--
>>  src/mesa/drivers/dri/nouveau/nv04_state_frag.c  |  6 +-
>>  src/mesa/drivers/dri/nouveau/nv10_state_frag.c  |  4 +-
>>  src/mesa/drivers/dri/nouveau/nv10_state_tex.c   |  5 +-
>>  src/mesa/drivers/dri/nouveau/nv20_state_tex.c   |  3 +-
>>  src/mesa/drivers/dri/r200/r200_tex.c            |  3 +-
>>  src/mesa/drivers/dri/r200/r200_texstate.c       | 31 +++++----
>>  src/mesa/drivers/dri/radeon/radeon_maos_verts.c |  2 +-
>>  src/mesa/drivers/dri/radeon/radeon_state.c      |  2 +-
>>  src/mesa/drivers/dri/radeon/radeon_tex.c        |  3 +-
>>  src/mesa/drivers/dri/radeon/radeon_texstate.c   | 13 ++--
>>  src/mesa/main/attrib.c                          | 17 ++---
>>  src/mesa/main/context.c                         |  6 +-
>>  src/mesa/main/enable.c                          | 23 ++++---
>>  src/mesa/main/ff_fragment_shader.cpp            |  3 +-
>>  src/mesa/main/ffvertex_prog.c                   |  5 +-
>>  src/mesa/main/get.c                             |  7 ++-
>>  src/mesa/main/get_hash_params.py                | 10 +--
>>  src/mesa/main/mtypes.h                          | 44 +++++++------
>>  src/mesa/main/rastpos.c                         |  5 +-
>>  src/mesa/main/texenv.c                          | 40 +++++++-----
>>  src/mesa/main/texgen.c                          | 18 +++---
>>  src/mesa/main/texstate.c                        | 83 ++++++++++++++-----------
>>  src/mesa/main/texstate.h                        | 15 +++++
>>  src/mesa/program/prog_statevars.c               | 20 +++---
>>  src/mesa/swrast/s_context.c                     |  2 +-
>>  src/mesa/swrast/s_texcombine.c                  |  3 +-
>>  src/mesa/swrast/s_triangle.c                    | 10 +--
>>  src/mesa/tnl/t_vb_texgen.c                      | 10 +--
>>  32 files changed, 253 insertions(+), 177 deletions(-)
>>
>
> [snip]
>
>> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
>> index 83838d8..cc18ac1 100644
>> --- a/src/mesa/main/get.c
>> +++ b/src/mesa/main/get.c
>> @@ -1357,21 +1357,20 @@ static const struct value_desc error_value =
>>   *
>>   * \return the struct value_desc corresponding to the enum or a struct
>>   *     value_desc of TYPE_INVALID if not found.  This lets the calling
>>   *     glGet*v() function jump right into a switch statement and
>>   *     handle errors there instead of having to check for NULL.
>>   */
>>  static const struct value_desc *
>>  find_value(const char *func, GLenum pname, void **p, union value *v)
>>  {
>>     GET_CURRENT_CONTEXT(ctx);
>> -   struct gl_texture_unit *unit;
>>     int mask, hash;
>>     const struct value_desc *d;
>>     int api;
>>
>>     api = ctx->API;
>>     /* We index into the table_set[] list of per-API hash tables using the API's
>>      * value in the gl_api enum. Since GLES 3 doesn't have an API_OPENGL* enum
>>      * value since it's compatible with GLES2 its entry in table_set[] is at the
>>      * end.
>>      */
>> @@ -1412,22 +1411,24 @@ find_value(const char *func, GLenum pname, void **p, union value *v)
>>     case LOC_BUFFER:
>>        *p = ((char *) ctx->DrawBuffer + d->offset);
>>        return d;
>>     case LOC_CONTEXT:
>>        *p = ((char *) ctx + d->offset);
>>        return d;
>>     case LOC_ARRAY:
>>        *p = ((char *) ctx->Array.VAO + d->offset);
>>        return d;
>>     case LOC_TEXUNIT:
>> -      unit = &ctx->Texture.Unit[ctx->Texture.CurrentUnit];
>> -      *p = ((char *) unit + d->offset);
>> +      if (ctx->Texture.CurrentUnit < ARRAY_SIZE(ctx->Texture.FixedFuncUnit)) {
>> +         unsigned index = ctx->Texture.CurrentUnit;
>> +         *p = ((char *)&ctx->Texture.FixedFuncUnit[index] + d->offset);
>> +      }
>
> Presumably this is an error?  Looking at the surrounding code, it's not
> obvious that the error is signaled.  If we hit this patch, nothing ever
> initializes *p, and _mesa_GetIntegerv, for example, would dereference
> random junk.
>
> If the error was previously signaled, then execution shouldn't even get
> here in the ctx->Texture.CurrentUnit >=
> ARRAY_SIZE(ctx->Texture.FixedFuncUnit) case, right?

This is only used by GL_TEXTURE_GEN_* queries. It seems to be a
discrepancy in the GL spec. It's allowed to set and get the gen enables
for 192 combined texture units, but only 8 are usable in practice.
Reporting an error would be out-of-spec, returning garbage (like here)
is also out-of-spec, but it's an unusable state and therefore wasted memory.

If ARRAY_SIZE(ctx->Texture.FixedFuncUnit) ==
MAX_COMBINED_TEXTURE_IMAGE_UNITS, the condition is always true.

If ARRAY_SIZE(ctx->Texture.FixedFuncUnit) == MAX_TEXTURE_COORD_UNITS,
GetIntegerv returns garbage if active texture >= 8;

Marek