On 03.11.2017 14:56, Miklós Máté wrote:
On 03/11/17 11:04, Nicolai Hähnle wrote:
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?
I don't know. This is not my code, I'm just trying to fix things with
minimal changes. ATI_fragment_shader is the programming language of
r200, but I have no such hardware for testing any major change. I can
only test on swrast, softpipe, llvmpipe and radeonsi.
Fair enough. I'd still prefer if you could test against DummyShader and
skip *before* the RefCount changes. Anything else can come later.
If it's OK to make major changes that improve spec conformance, but may
break r200, I can do that. My WIP piglit test series already uncovered a
few conformance issues, and I'm sure there will be more of those as I
make more tests.
Not sure what you're saying here :)
We shouldn't break r200. On the other hand, if r200 fails spec
conformance somewhere, then that should be fixed...
I have a few questions about testing:
Am I right to assume that everything is legal that is not explicitly
forbidden in the specification?
Mostly yes, especially where combinations of state are concerned. But
this kind of thing is better discussed with an actual example Ö=
If a test detects an error, should it terminate immediately or continue
and return fail at the end?
Depends on the type of error and the type of test. It's a trade-off
between saving some time on hopeless test cases and allowing a glance at
different subtests. Many piglit tests have explicit subtests, and in
those cases, all subtests should be run (but subtests can still bail out
early when an error is detected).
Should I post my piglit tests or my mesa changes first? My test series
is about 15% complete now.
Around the same time, there doesn't have to be a strict order. I have a
mild preference to *push* the piglit tests later (so that people don't
get new failures in their test reports), but posting the tests is
independent.
Cheers,
Nicolai
MM
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
