[Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices

[Francisco Jerez]

Tom Stellard <thomas.stellard at amd.com> writes:

> When probing for devices, clover will call pipe_loader_probe() twice.
> The first time to retrieve the number of devices, and then second time
> to retrieve the device structures.
>
> We currently assume that the return value of both calls will be the
> same, but this will not be the case if a device happens to disappear
> between the two calls.
>

Assuming that the two sequential calls will give the same result is bad,
but the likelihood of a device disappearing between the two calls is so
small it's unlikely to happen more than once in a million years.  If
this problem happens deterministically to you there's almost certainly a
more serious bug in pipe-loader that will cause it to return an
incorrect device count during the first call.

Indeed, pipe_loader_sw_probe() seems to return 1 unconditionally if ndev
is zero, so it will give a different device count depending on ndev if
pipe_loader_sw_probe_null() happens to fail.  Seems like a bug we don't
want to simply paper over.

Anyway I'm not saying that clover's assumption shouldn't be fixed too --
See comment below.

> This patch removes this assumption and checks the return value of the
> second pipe_loader_probe() call to ensure it does not try to initialize
> devices that no longer exits.
>
> CC: <mesa-stable at lists.freedesktop.org>
> ---
>  src/gallium/state_trackers/clover/core/platform.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/platform.cpp b/src/gallium/state_trackers/clover/core/platform.cpp
> index 328b71c..689d692 100644
> --- a/src/gallium/state_trackers/clover/core/platform.cpp
> +++ b/src/gallium/state_trackers/clover/core/platform.cpp
> @@ -28,9 +28,10 @@ platform::platform() : adaptor_range(evals(), devs) {
>     int n = pipe_loader_probe(NULL, 0);
>     std::vector<pipe_loader_device *> ldevs(n);
>  
> -   pipe_loader_probe(&ldevs.front(), n);
> +   n = pipe_loader_probe(&ldevs.front(), n);
>  
> -   for (pipe_loader_device *ldev : ldevs) {
> +   for (int i = 0; i < n; ++i) {
> +      pipe_loader_device *ldev = ldevs[i];

There are many reasons to prefer range loops to induction variables
(including iterators), and there's no need to change it here: Because
ldevs is zero-initialized you can just wrap the push_back(create()) call
around an 'if (ldev)' to make this a one-liner.

>        try {
>           devs.push_back(create<device>(*this, ldev));
>        } catch (error &) {
> -- 
> 2.0.4
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151128/bf0cc805/attachment.sig>