Re: (try 2)[PATCH 1/4] d3dcompiler: Add initial reflection parsing.

[Henri Verbeet]

2010/11/8 Rico Schüller <kgbricola(a)web.de>:
> -    object->vtbl = &d3dcompiler_shader_reflection_vtbl;
> -    object->refcount = 1;
> +    hr = d3dcompiler_shader_reflection_init(object, data, data_size, riid);
> +    if (FAILED(hr))
> +    {
> +        WARN("Failed to initialize shader reflection\n");
> +        IUnknown_Release((IUnknown *)object);
> +        return hr;
> +    }
> You can't do that. Since d3dcompiler_shader_reflection_init() is
> responsible for setting the vtbl, you can't assume it's set if
> d3dcompiler_shader_reflection_init() fails. You'll have to use
> HeapFree() here.
> +    if (!IsEqualGUID(riid, &IID_ID3D11ShaderReflection))
> +    {
> +        WARN("Wrong rrid %s, accept only %s!\n", debugstr_guid(riid), debugstr_guid(&IID_ID3D11ShaderReflection));
> +        return E_FAIL;
> +    }
> This can be done in D3DReflect() before even allocating the reflection
> object. (Also, "rrid" is a typo.)

(try 2)[PATCH 1/4] d3dcompiler: Add initial reflection parsing.

[Rico Schüller]

Am 09.11.2010 12:01, schrieb Henri Verbeet:
> 2010/11/8 Rico Schüller<kgbricola(a)web.de>:
> > -    object->vtbl =&d3dcompiler_shader_reflection_vtbl;
> > -    object->refcount = 1;
> > +    hr = d3dcompiler_shader_reflection_init(object, data, data_size, riid);
> > +    if (FAILED(hr))
> > +    {
> > +        WARN("Failed to initialize shader reflection\n");
> > +        IUnknown_Release((IUnknown *)object);
> > +        return hr;
> > +    }
>
> You can't do that. Since d3dcompiler_shader_reflection_init() is
> responsible for setting the vtbl, you can't assume it's set if
> d3dcompiler_shader_reflection_init() fails. You'll have to use
> HeapFree() here.

I made the assumption, because setting the vtbl is the first thing 
d3dcompiler_shader_reflection_init() does.
Well, a heap free isn't always enough, because there could already be 
allocated data to the object, which then would resolve in lost memory.

So my sugguestion is to move the HeapAlloc into 
d3dcompiler_shader_reflection_init(ID3D11ShaderReflection **reflector, 
const void *data, ...), from there it is safe to assume that the vtbl is 
set before the release.
> > +    if (!IsEqualGUID(riid,&IID_ID3D11ShaderReflection))
> > +    {
> > +        WARN("Wrong rrid %s, accept only %s!\n", debugstr_guid(riid), debugstr_guid(&IID_ID3D11ShaderReflection));
> > +        return E_FAIL;
> > +    }
>
> This can be done in D3DReflect() before even allocating the reflection
> object. (Also, "rrid" is a typo.)

Yes, that can be done.

Cheers
Rico

(try 2)[PATCH 1/4] d3dcompiler: Add initial reflection parsing.

[Henri Verbeet]

2010/11/9 Rico Schüller <kgbricola(a)web.de>:
> I made the assumption, because setting the vtbl is the first thing
> d3dcompiler_shader_reflection_init() does.
> It is, but you're not supposed to care. Abstractions aren't very
> useful if you have to care about the details of their internals.
> Well, a heap free isn't always enough, because there could already be
> allocated data to the object, which then would resolve in lost memory.

d3dcompiler_shader_reflection_init() should free anything it allocated
if it's going to fail.

(try 2)[PATCH 1/4] d3dcompiler: Add initial reflection parsing.

[Rico Schüller]

Am 09.11.2010 18:39, schrieb Henri Verbeet:
> 2010/11/9 Rico Schüller<kgbricola(a)web.de>:
> > I made the assumption, because setting the vtbl is the first thing
> > d3dcompiler_shader_reflection_init() does.
>
> It is, but you're not supposed to care. Abstractions aren't very
> useful if you have to care about the details of their internals.
> > Well, a heap free isn't always enough, because there could already be
> > allocated data to the object, which then would resolve in lost memory.
>
> d3dcompiler_shader_reflection_init() should free anything it allocated
> if it's going to fail.

Ok, it does that, if HeapAlloc and IUnknown_Release() is moved to 
d3dcompiler_shader_reflection_init(). So everything would be fine.