[Mesa-dev] [PATCH] anv/entrypoints: VkGetDeviceProcAddr returns NULL for core instance commands

[Iago Toral]

On Tue, 2018-03-06 at 14:17 +0100, Iago Toral wrote:
> On Tue, 2018-03-06 at 13:05 +0000, Emil Velikov wrote:
> > On 6 March 2018 at 12:26, Iago Toral <itoral at igalia.com> wrote:
> > > On Tue, 2018-03-06 at 12:16 +0000, Emil Velikov wrote:
> > > > On 6 March 2018 at 12:09, Iago Toral <itoral at igalia.com> wrote:
> > > > > On Tue, 2018-03-06 at 11:21 +0000, Emil Velikov wrote:
> > > > > > On 6 March 2018 at 09:57, Bas Nieuwenhuizen <basni at chromium
> > > > > > .o
> > > > > > rg>
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Tue, Mar 6, 2018 at 8:02 AM, Iago Toral <itoral at igalia
> > > > > > > .c
> > > > > > > om>
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > On Mon, 2018-03-05 at 12:11 +0000, Emil Velikov wrote:
> > > > > > > > > Hi Iago,
> > > > > > > > > 
> > > > > > > > > Top level questions:
> > > > > > > > > 
> > > > > > > > > I think this and the original commit should go to
> > > > > > > > > stable
> > > > > > > > > right?
> > > > > > > > 
> > > > > > > > I am not sure if this qualifies for stable: these
> > > > > > > > patches
> > > > > > > > don't
> > > > > > > > fix any
> > > > > > > > user-visible bugs. If an application was calling
> > > > > > > > vkGetDeviceProcAddr to
> > > > > > > > get pointers to non-device functions (which is
> > > > > > > > incorrect
> > > > > > > > by
> > > > > > > > the
> > > > > > > > spec)
> > > > > > > > the previous behavior would allow it to get away with
> > > > > > > > it
> > > > > > > > without
> > > > > > > > issues, bit with these patches it will start to crash
> > > > > > > > since
> > > > > > > > it
> > > > > > > > will
> > > > > > > > receive NULL pointers.
> > > > > > > > 
> > > > > > 
> > > > > > According to Lenny's comment in the github issue there's
> > > > > > nothing
> > > > > > to
> > > > > > be
> > > > > > concerned. Namely:
> > > > > >  - "The pointers being returned are invalid. ... trying to
> > > > > > use
> > > > > > them
> > > > > > will result in a crash."
> > > > > >  - "Wolfenstein was acquiring, but not using the pointers."
> > > > > 
> > > > > Because it is not using the pointers :), if some other app is
> > > > > using
> > > > > them it will start to crash.
> > > > > 
> > > > > But that was not my point, my point was that this doesn't fix
> > > > > anything
> > > > > for users, so I was questioning whether it was material for
> > > > > stable
> > > > > based on that.
> > > > > 
> > > > 
> > > > Surely we don't want to apps be written against the current
> > > > incorrect
> > > > behaviour?
> > > > Which may go unnoticed since there are no validation/loader
> > > > checks
> > > > for
> > > > this (based on the github issue).
> > > 
> > > If there is such an application, and this patch makes it into the
> > > stable release, that application will probably start to crash.
> > 
> > Such an application will crash _regardless_ of the patch, as
> > mentioned earlier.
> > That is as per Lenny's analysis - haven't checked it personally.
> 
> I am not sure why he says that, the point of using vkGet*Addr is to
> get
> a driver function pointer so you can skip the loader completely. If
> you
> get a valid pointer and you pass valid parameters to it, I think it
> should work be just fine, but have't tested it myself, I can try to
> do
> that and see what happens.

Ok, I tested this and it didn't crash for me, but Lenny is is right
that the loader is doing wrapping/unwrapping of some of the function
parameters before this reaches the driver, so even if it doesn't crash
calling these functions won't produce correct results.

This means that applications that were doing this and were not
crashing, will certainly start to crash now (if they actually use the
function pointers), but I don't think this is very relevant, since 
even if they were not crashing before the patch they certainly were not
working properly anyway.

Iago