Discussion:
[PATCH 2/2] mesa: add typesize validation to resource functions
(too old to reply)
Tapani Pälli
2017-08-11 07:45:15 UTC
Permalink
Raw Message
This makes development/changes to program resource code more safe.
Patch also makes helper functions static.

Signed-off-by: Tapani Pälli <***@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));\
return (type *) res->Data; \
}
--
2.9.4
Timothy Arceri
2017-08-11 10:02:33 UTC
Permalink
Raw Message
Post by Tapani Pälli
This makes development/changes to program resource code more safe.
Patch also makes helper functions static.
---
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.

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);
Post by Tapani Pälli
return (type *) res->Data; \
}
Tapani Pälli
2017-08-11 10:18:42 UTC
Permalink
Raw Message
Post by Timothy Arceri
Post by Tapani Pälli
This makes development/changes to program resource code more safe.
Patch also makes helper functions static.
---
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).
Post by Timothy Arceri
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);
Post by Tapani Pälli
return (type *) res->Data; \
}
Sure, this looks fine approach to me, will make it more safe!

// Tapani
Tapani Pälli
2017-08-11 11:23:25 UTC
Permalink
Raw Message
Post by Tapani Pälli
Post by Timothy Arceri
Post by Tapani Pälli
This makes development/changes to program resource code more safe.
Patch also makes helper functions static.
---
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).
Post by Timothy Arceri
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);
Post by Tapani Pälli
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

Loading...