[Mesa-dev] [PATCH] mesa: prevent deleting the dummy ATI_fs

[Nicolai Hähnle]

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é <mtmkls at 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.