Khronos Public Bugzilla

Bug 254

Summary: cl.hpp: Program::getInfo<CL_PROGRAM_BINARIES> crashes
Product: OpenCL Reporter: Stefan <stefan_j_harwood>
Component: C++ Bindings (cl.hpp)Assignee: Benedict Gaster <bgaster>
Status: NEW --- QA Contact:
Severity: normal    
Priority: P3 CC: coleb, dale, sschuberth
Version: 1.0   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: Proposed patch for getInfo<CL_PROGRAM_BINARIES>
Proposed patch for getInfo<CL_PROGRAM_BINARIES>
Introduce a wrapper class for program sources and binaries

Description Stefan 2010-02-02 08:02:41 PST
Summary: cl::Program::getInfo for CL_PROGRAM_BINARIES causes crash.

The implementation (specialization) of GetInfoHelper for VECTOR_CLASS (line 793) is broken where the contained type is a pointer [to a pre-allocated buffer]; probably only for CL_PROGRAM_BINARIES.

Solution: afraid nothing jumps to mind, sorry :-(
Comment 1 Sebastian Schuberth 2010-12-28 17:51:10 PST
Created attachment 64 [details]
Proposed patch for getInfo<CL_PROGRAM_BINARIES>

Here's a proposed patch. I've also changed the return type from "char" to "unsigned char" as that's what the spec says.
Comment 2 Benedict Gaster 2010-12-29 12:20:41 PST
Thanks for the patch. We now have 3 different proposed solutions: 1 from here; 1 i had been working on; and one from the other maintainer Brian Cole. I will review all 3 when I get back to the US next week and we can discuss here what is the best to proceed with.

Ben
Comment 3 Brian Cole 2010-12-29 12:30:01 PST
(In reply to comment #2)
> Thanks for the patch. We now have 3 different proposed solutions: 1 from here;
> 1 i had been working on; and one from the other maintainer Brian Cole. I will
> review all 3 when I get back to the US next week and we can discuss here what
> is the best to proceed with.
> 
> Ben

I like Sebastions patch as it automatically manages memory. Calling operator new without a delete made me feel all dirty. Only reason I didn't do it that way was I wasn't sure how backwards compatible we need to keep things.

Also, any reason to prefer a vector<unsigned char> over string? I think this is covered in an effective C++ chapter that I'll go dig up.
Comment 4 Sebastian Schuberth 2010-12-29 12:40:23 PST
(In reply to comment #3)

> Also, any reason to prefer a vector<unsigned char> over string? I think this is
> covered in an effective C++ chapter that I'll go dig up.

Well, we're requesting a binary after all (although this "binary" turns out to be PTX source code for NVIDIA, for example). That's why I felt a string to be just not right here (for the same reason I changed "char" to "unsigned char"; it's meant to be an array of bytes, not a string).
Comment 5 Brian Cole 2011-01-17 13:42:47 PST
(In reply to comment #4)
> (In reply to comment #3)
> 
> > Also, any reason to prefer a vector<unsigned char> over string? I think this is
> > covered in an effective C++ chapter that I'll go dig up.
> 
> Well, we're requesting a binary after all (although this "binary" turns out to
> be PTX source code for NVIDIA, for example). That's why I felt a string to be
> just not right here (for the same reason I changed "char" to "unsigned char";
> it's meant to be an array of bytes, not a string).

Sebastion, just talked to Ben and he gave the 'ok' for breaking backwards compatibility, he would rather get it right now while usage is still quite low. 

With that in mind I'm thinking that we could and maybe should change the Binaries typedef to the following: 

typedef VECTOR_CLASS<VECTOR_CLASS<unsigned char> > Binaries; 

It would be useful to be able to do the following: 

cl::Program::Binaries bins = program.getInfo<CL_PROGRAM_BINARIES>();

Though it does bear the question of whether we should also change the Sources typedef to the more C++ friendly:
typedef VECTOR_CLASS<STRING_CLASS> Sources; 

Thoughts?


Also, on orthogonal note I looked over Sebastion's patch and the only issue I have with it is that it specializes GetInfoHelper with the assumption that it will only be used with CL_PROGRAM_BINARIES by hardcoding CL_PROGRAM_BINARY_SIZES into it. Whatever we decide we should probably come up with something a little more general in order to better support future additions to the API more easily.
Comment 6 Sebastian Schuberth 2011-01-17 14:30:26 PST
> With that in mind I'm thinking that we could and maybe should change the
> Binaries typedef to the following: 
> 
> typedef VECTOR_CLASS<VECTOR_CLASS<unsigned char> > Binaries; 

I agree.

> Though it does bear the question of whether we should also change the Sources
> typedef to the more C++ friendly:
> typedef VECTOR_CLASS<STRING_CLASS> Sources; 

There's no API function returning an array of strings AFAIK, so this typedef isn't necessary (as the source code is device-independent there is no need to retrieve source code for each device, in contrast to the binaries).

We could add this anyway for symmetry and convenience. I have done so in my mirror repository, see

http://repo.or.cz/w/opencl-headers/bugzilla-patches.git/commitdiff/3fd8a77b86af564eb442d5526b78119c8548631f

I'll also update the patch attached to this issue.

> Also, on orthogonal note I looked over Sebastion's patch and the only issue I
> have with it is that it specializes GetInfoHelper with the assumption that it
> will only be used with CL_PROGRAM_BINARIES by hardcoding
> CL_PROGRAM_BINARY_SIZES into it. Whatever we decide we should probably come up
> with something a little more general in order to better support future
> additions to the API more easily.

If it was guaranteed that all names like CL_PROGRAM_BINARIES and CL_PROGRAM_BINARY_SIZES always relate like CL_PROGRAM_BINARIES - CL_PROGRAM_BINARY_SIZES = 1, we could simply replace

cl_int err = GetInfoHelper<Func, VECTOR_CLASS< ::size_t> >::get(f, CL_PROGRAM_BINARY_SIZES, &sizes);

with

cl_int err = GetInfoHelper<Func, VECTOR_CLASS< ::size_t> >::get(f, name-1, &sizes);

But I guess that's too much a hack, or?
Comment 7 Sebastian Schuberth 2011-01-17 14:35:21 PST
Created attachment 68 [details]
Proposed patch for getInfo<CL_PROGRAM_BINARIES>
Comment 8 Brian Cole 2011-01-17 14:42:33 PST
(In reply to comment #6)
> > With that in mind I'm thinking that we could and maybe should change the
> > Binaries typedef to the following: 
> > 
> > typedef VECTOR_CLASS<VECTOR_CLASS<unsigned char> > Binaries; 
> 
> I agree.
> 
> > Though it does bear the question of whether we should also change the Sources
> > typedef to the more C++ friendly:
> > typedef VECTOR_CLASS<STRING_CLASS> Sources; 
> 
> There's no API function returning an array of strings AFAIK, so this typedef
> isn't necessary (as the source code is device-independent there is no need to
> retrieve source code for each device, in contrast to the binaries).
>
> We could add this anyway for symmetry and convenience. I have done so in my
> mirror repository, see
> 
> http://repo.or.cz/w/opencl-headers/bugzilla-patches.git/commitdiff/3fd8a77b86af564eb442d5526b78119c8548631f
> 

I like this better way for symmetry as well. I always thought this was ugly: 

cl::Program::Sources source(1, std::make_pair(helloStr,strlen(helloStr)));

When it seems like the following should do just as well: 
cl::Program::Sources source(1, std::string(helloStr));
 
> > Also, on orthogonal note I looked over Sebastion's patch and the only issue I
> > have with it is that it specializes GetInfoHelper with the assumption that it
> > will only be used with CL_PROGRAM_BINARIES by hardcoding
> > CL_PROGRAM_BINARY_SIZES into it. Whatever we decide we should probably come up
> > with something a little more general in order to better support future
> > additions to the API more easily.
> 
> If it was guaranteed that all names like CL_PROGRAM_BINARIES and
> CL_PROGRAM_BINARY_SIZES always relate like CL_PROGRAM_BINARIES -
> CL_PROGRAM_BINARY_SIZES = 1, we could simply replace
> 
> cl_int err = GetInfoHelper<Func, VECTOR_CLASS< ::size_t> >::get(f,
> CL_PROGRAM_BINARY_SIZES, &sizes);
> 
> with
> 
> cl_int err = GetInfoHelper<Func, VECTOR_CLASS< ::size_t> >::get(f, name-1,
> &sizes);
> 
> But I guess that's too much a hack, or?

Ick, if there's no way you can think of to generalize GetHelperInfo we can just overload getInfo<CL_PROGRAM_BINARIES> directly with the following: 

template<>
inline VECTOR_CLASS<char *> cl::Program::getInfo<CL_PROGRAM_BINARIES>(cl_int* err) const
{
  ...
} 

Explicit is always better than implicit. :-)
Comment 9 Sebastian Schuberth 2011-01-17 16:07:22 PST
> I like this better way for symmetry as well. I always thought this was ugly: 
> 
> cl::Program::Sources source(1, std::make_pair(helloStr,strlen(helloStr)));

That's indeed ugly. And now that you say it, I realize that my current patch introduces new "Sources" and "Binaries" typedefs in addition to the ones in cl::Program, which is confusing.

Phew, seems to me this issue is generating a little more work than I expected. IMHO we should use the typedefs as suggested by you and introduced in my patch, probably move them to cl::Program, replacing the current ones, and make the constructors in cl::Program work with them. Do you agree?

> > But I guess that's too much a hack, or?
> 
> Ick, if there's no way you can think of to generalize GetHelperInfo we can just
> overload getInfo<CL_PROGRAM_BINARIES> directly with the following: 

Hmm, I'll think about that tomorrow :-)
Comment 10 Benedict Gaster 2011-01-17 16:14:10 PST
Would it not make more sense to remove the typedefs and introduce classes for binary and sources and then overload the constructors to support the nicer path and the backwards compatibility path?
Comment 11 Sebastian Schuberth 2011-01-17 16:37:10 PST
It would make more sense in terms of backwards compatibility, but to be honest, I don't care about backwards compatibility. cl.hpp already is pretty bloated, IMHO, and I'm not keen on making it worse. Else people will probably tend to start using more uncompromising and thus cleaner solutions like clutil [1].

[1] http://code.google.com/p/clutil/
Comment 12 Benedict Gaster 2011-01-17 16:39:38 PST
While I don't mind binaries changing semantics I am not going to allow sources to break backwards compatability and so we are going to have to use a class or leave it unchanged! Sorry.
Comment 13 Brian Cole 2011-01-17 18:41:23 PST
(In reply to comment #12)
> While I don't mind binaries changing semantics I am not going to allow sources
> to break backwards compatability and so we are going to have to use a class or
> leave it unchanged! Sorry.

Something more concrete about what we could do with Sources:

class Sources : public VECTOR_CLASS<std::pair<const char*, ::size_t> >
{
public:
    Sources(const std::string &src)
         : VECTOR_CLASS<std::pair<const char*, ::size_t> >(1, 
                                                           std::make_pair(src.c_str(), 
                                                                          src.size()))
    { }
}

The central annoyance is that the memory is managed outside the object for this. So the following would actually crash:

const char *src = "...";
cl::Program::Sources sources(std::string(src));

So it's maybe not worth it to change into a class at all. And then by symmetric argument, should 'Binaries' be changed?
Comment 14 Sebastian Schuberth 2011-01-18 05:06:58 PST
Created attachment 69 [details]
Introduce a wrapper class for program sources and binaries

How about this patch for a wrapper class for program sources and binaries? If you agree, I'll rebase my patch for the original issue onto it.
Comment 15 Brian Cole 2011-01-18 11:52:11 PST
(In reply to comment #14)
> Created attachment 69 [details]
> Introduce a wrapper class for program sources and binaries
> 
> How about this patch for a wrapper class for program sources and binaries? If
> you agree, I'll rebase my patch for the original issue onto it.

The same reason why this patch is not backwards compatible, is the same reason why it would be preferable. That is who responsible for memory management of the held pointers.
Comment 16 Sebastian Schuberth 2011-01-19 05:21:39 PST
(In reply to comment #15)

> The same reason why this patch is not backwards compatible, is the same reason
> why it would be preferable. That is who responsible for memory management of
> the held pointers.

Hmm, unless I'm overlooking something, both before and after the patch the caller is responsible for the memory management of the source code strings. In my patch the pointed-to memory gets copied, so the caller still can (and should) free its pointers.
Comment 17 Brian Cole 2011-01-19 19:34:44 PST
(In reply to comment #16)
> (In reply to comment #15)
> 
> > The same reason why this patch is not backwards compatible, is the same reason
> > why it would be preferable. That is who responsible for memory management of
> > the held pointers.
> 
> Hmm, unless I'm overlooking something, both before and after the patch the
> caller is responsible for the memory management of the source code strings. In
> my patch the pointed-to memory gets copied, so the caller still can (and
> should) free its pointers.

Not entirely true. Since the user could have relied on being able to pull the pointers back out of the Sources object later for the deletion. 

So code written like this will be broken: 

char *src = new char[size];
ReadSourceFromSomewhere(src);

cl::Program::Sources srcs; 
srcs.push_back(make_pair(src, strlen(src) + 1));

cl::Program program(context, srcs);
delete srcs.front().first;

No one is going to win an award for such ugly code, but that doesn't mean we can break it. 

This is also the downside of the C++ bindings being a header, people can look at the implementation and do evil things.
Comment 18 DaleT 2012-04-30 02:27:03 PDT
(In reply to comment #7)
> Created attachment 68 [details]
> Proposed patch for getInfo<CL_PROGRAM_BINARIES>

The code in this patch uses the vector resize function, which is not available in the cl::vector class. Not sure if this problem belongs here or if the cl::vector class should be patched.