On Tue, 2013-10-22 at 13:38 -0700, Francisco Jerez wrote: > Jan Vesely <jan.ves...@rutgers.edu> writes: > > > On Mon, 2013-10-21 at 22:20 -0700, Francisco Jerez wrote: > >> Jan Vesely <jan.ves...@rutgers.edu> writes: > >> > >> > the specs say that clCreateContext reutrns error > >> > "if platform value specified in properties is not a valid platform" > >> > > >> > The orignal approach fials if invalid valu other than NULL pointer is > >> > provided. > >> > > >> > Fixes piglit cl-api-create-context. > >> > > >> Honestly, I don't think this test makes much sense. It's unreasonable > >> to expect that the CL will be able to catch any bad pointer you give it > >> as argument and fail gracefully. The only reliable solution that comes > >> to my mind would be to build a global hash table for each CL object type > >> that keeps track of the valid objects that have been allocated. That > >> seems like a lot of effort with the only purpose of finding out if the > >> user is doing something *very* stupid and very unlikely. > > > > Hi, > > > > My assumption was that if piglit is testing it, it should be handled, > > moreover this specific case (platform id), is really simple. > > > > I only partly agree with your statement. The specs speak about IDs, it's > > implementation decision to use pointers as IDs (unless I misread > > something). > > Not completely, the official Khronos OpenCL headers define all object > IDs as pointers already, and the ICD extension requires them to be > pointers with a dispatch table pointer located at offset 0. > > That means that all implementations using an ICD loader are also going > to crash on this test, no matter what Clover does. Neither ocl-icd nor > the official Khronos ICD loader bother to keep track of the valid object > pointers -- and I don't think there's a good reason for them to, it's a > specification requirement that implies a lot of work for almost no > benefit. > > > Not doing input checks increases distance from the specs, at least in > > error paths, do/should we care? > > > I'd rather treat this as a spec bug... It's the kind of thing that > should really result in unspecified behavior, no real application is > likely to rely on it, and most implementations are either ignoring it or > not doing it reliably. > > > Wouldn't std::set of all API relevant pointers/ids be enough to > > implement this? then the check could look like: (set.contains() && > > RTTI), and we don't even need the NULL check. > > > Yeah, that would be a slightly better solution, but it wouldn't be > enough. We're likely to move towards an architecture where we're always > loaded through the ICD, in that case we would crash at the ICD loader > before Clover gets control -- If anywhere the fix probably belongs in > the ICD loader and not in the CL implementation itself. > > Also note that using RTTI as you intend wouldn't work in the general > case, because Clover API objects don't share a common polymorphic base > class -- and in fact they can't, because API objects need to be standard > layout classes in order to have the guarantee that the dispatch table > pointer will be located at the expected offset so the ICD loader can > find it. So, yes, I think you'd absolutely need to use a separate > std::set for each object type, but again I don't think it's worth the > effort. > > >> > >> That said, we're already doing three forms of object validation: first, > >> the pointers provided by the user are compared against NULL; second, we > >> make sure that the dispatch table pointer is at the correct location in > >> memory; third, if the object is part of a non-trivial class hierarchy, > >> as is the case for events and memory objects, we use RTTI to make sure > >> that the object is of the expected type. I don't think we want or need > >> more validation, it would probably be more useful to drop that test from > >> piglit. > >> > >> Apparently nVidia's libOpenCL fails the test as well. > > Hm, I should have said that this test makes nVidia crash as well. > > > > > In that case, it might be better to remove the check entirely and never > > touch the provided pointer. > > It was the case before e5fc61fa, and unexpected success is imo better > > than a segfault vector. afaik intel opencl has this behavior. > > > Hm... I'd like us to have a consistent behavior across all object > types, and IMHO it's better to die with a segmentation fault when the > user passes garbage as one of the pointer arguments than to pretend that > everything is fine and a context has been successfully allocated in a > non-existing platform.
OK. Thanks for detailed explanation, I have two more patches that target error checking in clCreateBuffer, I'll post them soon. Jan > > Thanks. > > > thanks, > > Jan > > > > -- Jan Vesely <jan.ves...@rutgers.edu> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
