On 03.11.2017 00:06, Miklós Máté wrote:
On 02/11/17 17:16, Nicolai Hähnle wrote:
On 01.11.2017 00:34, Miklós Máté wrote:
This fixes a crash upon context destruction when
glGenFragmentShadersATI() was used. Backtrace:
==15060== Invalid free() / delete / delete[] / realloc()
==15060== at 0x482F478: free (vg_replace_malloc.c:530)
==15060== by 0x57694F4: _mesa_delete_ati_fragment_shader
(atifragshader.c:68)
==15060== by 0x58B33AB: delete_fragshader_cb (shared.c:208)
==15060== by 0x5838836: _mesa_HashDeleteAll (hash.c:295)
==15060== by 0x58B365F: free_shared_state (shared.c:377)
==15060== by 0x58B3BC2: _mesa_reference_shared_state (shared.c:469)
==15060== by 0x578687F: _mesa_free_context_data (context.c:1366)
==15060== by 0x595E9EC: st_destroy_context (st_context.c:642)
==15060== by 0x5987057: st_context_destroy (st_manager.c:772)
==15060== by 0x5B018B6: dri_destroy_context (dri_context.c:217)
==15060== by 0x5B006D3: driDestroyContext (dri_util.c:511)
==15060== by 0x4A1CBE6: dri3_destroy_context (dri3_glx.c:170)
==15060== Address 0x7b5dae0 is 0 bytes inside data symbol "DummyShader"
This hasn't been an issue yet, because the only thing that calls
glGenFragmentShadersATI() is my WIP piglit test. SWKOTOR and Doom3
use hard-coded shader names.
The patch also fixes this:
name=glGenFragmentShadersATI(1);
glDeleteFragmentShaderATI(name);
Signed-off-by: Miklós Máté <mtm...@gmail.com>
---
src/mesa/main/atifragshader.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/mesa/main/atifragshader.c
b/src/mesa/main/atifragshader.c
index 27d8b86477..fdfaea0528 100644
--- a/src/mesa/main/atifragshader.c
+++ b/src/mesa/main/atifragshader.c
@@ -60,6 +60,9 @@ void
_mesa_delete_ati_fragment_shader(struct gl_context *ctx, struct
ati_fragment_shader *s)
{
GLuint i;
+
+ if (s == &DummyShader)
+ return;
+
for (i = 0; i < MAX_NUM_PASSES_ATI; i++) {
free(s->Instructions[i]);
free(s->SetupInst[i]);
@@ -295,7 +298,6 @@ _mesa_DeleteFragmentShaderATI(GLuint id)
if (prog) {
prog->RefCount--;
if (prog->RefCount <= 0) {
- assert(prog != &DummyShader);
_mesa_delete_ati_fragment_shader(ctx, prog);
This looks very much like it should be caught earlier by an error
check. Usually, the object ID 0 is silently ignored by glDelete*
commands (and negative values result in INVALID_VALUE).
Cheers,
Nicolai
It's not object ID 0. The DummyShader is used by GenFragmentShadersATI()
as a placeholder to mark those IDs as allocated.
DeleteFragmentShadersATI() and context cleanup should be able to remove
the placeholders without crashing.
Is there a reason why GenFragmentShadersATI doesn't just allocate a new
object? Is there precedent for this elsewhere? Using the DummyShader
approach seems fragile to me.
Besides, with the patch as-is, you'll still let RefCounts go to negative
values, which seems like a bad idea.
Why can't you just allocate the object immediately like is done for most
(all?) other objects, like textures or (GLSL) shaders instead of adding
hacks upon hacks?
Cheers,
Nicolai
I'll resend the patch with better commit message. I don't know why it
doesn't apply for Marek though, this file hasn't been touched in months.
MM
}
}
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
