[Mesa-dev] [PATCH 2/2] mesa: add typesize validation to resource functions

[Tapani Pälli]

On 08/11/2017 01:18 PM, Tapani Pälli wrote:
> 
> 
> On 08/11/2017 01:02 PM, Timothy Arceri wrote:
>>
>>
>> On 11/08/17 17:45, Tapani Pälli wrote:
>>> This makes development/changes to program resource code more safe.
>>> Patch also makes helper functions static.
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>   src/mesa/main/shader_query.cpp | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/main/shader_query.cpp 
>>> b/src/mesa/main/shader_query.cpp
>>> index f2bdcaa..d1fc5e9 100644
>>> --- a/src/mesa/main/shader_query.cpp
>>> +++ b/src/mesa/main/shader_query.cpp
>>> @@ -49,8 +49,9 @@ program_resource_location(struct 
>>> gl_program_resource *res,
>>>    * Warning! this is not type safe so be *very* careful when using 
>>> these.
>>>    */
>>>   #define DECL_RESOURCE_FUNC(name, type) \
>>> -const type * RESOURCE_ ## name (gl_program_resource *res) { \
>>> +static const type * RESOURCE_ ## name (gl_program_resource *res) { \
>>>      assert(res->Data); \
>>> +   assert(res->TypeSize == sizeof(type));\
>>
>> I'm not really a fan of adding members and passing them around just 
>> for the sake of debug builds.
> 
> Fair enough, this was meant to bring some additional safety as one can 
> use the functions with wrong type without any errors (there was actual 
> bug about this when adding the initial code where wrong type was used 
> with some resource and it was very hard to track down, cannot seem to 
> find it now though).
> 
>> What exactly is the problem you are running into? Can it be solved by 
>> using the existing members?
>>
>> Could you just do this instead?
>>
>> #define DECL_RESOURCE_FUNC(name, type, data_type) \
>> const type * RESOURCE_ ## name (gl_program_resource *res) { \
>>     assert(res->Type == type); \
>>     assert(res->Data); \
>>     return (type *) res->Data; \
>> }
>>
>> DECL_RESOURCE_FUNC(ATC, GL_ATOMIC_COUNTER_BUFFER,
>>                     gl_active_atomic_buffer);
>>
>>
>>
>>>      return (type *) res->Data; \
>>>   }
>>>
> 
> Sure, this looks fine approach to me, will make it more safe!

Or well .. I'll think about it and experiment a bit. This would mean 
introducing more of the helper funcs (not really big problem?), since 
now at least RESOURCE_VAR and RESOURCE_UNI cover multiple types.

// Tapani