[Mesa-dev] [PATCH v2 04/22] clover: Disallow creating libraries from other libraries

[Pierre Moreau]

On 2018-01-23 — 15:16, Francisco Jerez wrote:
> Pierre Moreau <pierre.morrow at free.fr> writes:
> 
> > On 2018-01-23 — 14:04, Francisco Jerez wrote:
> >> Pierre Moreau <pierre.morrow at free.fr> writes:
> >> 
> >> > If creating a library, do not allow non-compiled object in it, as
> >> > executables are not allowed, and libraries would make it really hard to
> >> > enforce the "-enable-link-options" flag.
> >> >
> >> > Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
> >> > ---
> >> >  src/gallium/state_trackers/clover/api/program.cpp | 19 ++++++++++++++++---
> >> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/src/gallium/state_trackers/clover/api/program.cpp b/src/gallium/state_trackers/clover/api/program.cpp
> >> > index 6044179587..8f0b103a4d 100644
> >> > --- a/src/gallium/state_trackers/clover/api/program.cpp
> >> > +++ b/src/gallium/state_trackers/clover/api/program.cpp
> >> > @@ -251,9 +251,13 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
> >> >  namespace {
> >> >     ref_vector<device>
> >> >     validate_link_devices(const ref_vector<program> &progs,
> >> > -                         const ref_vector<device> &all_devs) {
> >> > +                         const ref_vector<device> &all_devs,
> >> > +                         const std::string &opts) {
> >> >        std::vector<device *> devs;
> >> >  
> >> > +      const std::string flag = "-create-library";
> 
> This seems to be unused.
> 
> >> > +      const bool create_library = opts.find("-create-library") != std::string::npos;
> >> > +
> >> >        for (auto &dev : all_devs) {
> >> >           const auto has_binary = [&](const program &prog) {
> >> >              const auto t = prog.build(dev).binary_type();
> >> > @@ -261,10 +265,19 @@ namespace {
> >> >                     t == CL_PROGRAM_BINARY_TYPE_LIBRARY;
> >> >           };
> >> >  
> >> > +         // If creating a library, do not allow non-compiled object in it, as
> >> > +         // executables are not allowed, and libraries would make it really
> >> > +         // hard to enforce the "-enable-link-options".
> >> > +         if (create_library && any_of([&](const program &prog) {
> >> > +                  const auto t = prog.build(dev).binary_type();
> >> > +                  return t != CL_PROGRAM_BINARY_TYPE_COMPILED_OBJECT;
> >> > +               }, progs))
> >> > +            throw error(CL_INVALID_OPERATION);
> >> > +
> >> 
> >> Do you have any spec quote justifying this?  "Hard" is hardly a
> >> justification for emitting an unexpected API error ;).  If doing such a
> >> thing would rely on unimplemented behavior somewhere else it would
> >> probably be cleaner to assert-fail wherever an implementation is missing
> >> rather than returning an API error under conditions not expected by the
> >> application.
> >
> > Re-reading the spec, I am quite sure that libraries can’t be made of libraries.
> > From Section 5.6.5.1:
> >
> >> The following options can be specified when creating a library of compiled binaries.
> >>
> >> -create-library
> >>     Create a library of compiled binaries specified in input_programs argument to
> >>     clLinkProgram
> >
> 
> Fair enough, can you add this sentence as a spec quote to the comment
> above?  With that taken into account:

And I’ll also remove the sentence about it being ”hard”. :-)

Thank you very much for all the reviews! Much appreciated!

> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 
> > and earlier on, in Section 5.6.3 when defining clLinkProgram, it makes the
> > distinction between compiled binaries, libraries, and executables:
> >
> >> input_programs is an array of program objects that are compiled binaries or
> >> libraries that are to be linked to create the program executable.
> >
> > (both extracts come from the 1.2 specification)
> >
> >> 
> >> >           // According to the CL 1.2 spec, when "all programs specified [..]
> >> >           // contain a compiled binary or library for the device [..] a link is
> >> >           // performed",
> >> > -         if (all_of(has_binary, progs))
> >> > +         else if (all_of(has_binary, progs))
> >> >              devs.push_back(&dev);
> >> >  
> >> >           // otherwise if "none of the programs contain a compiled binary or
> >> > @@ -290,7 +303,7 @@ clLinkProgram(cl_context d_ctx, cl_uint num_devs, const cl_device_id *d_devs,
> >> >     auto prog = create<program>(ctx);
> >> >     auto devs = validate_link_devices(progs,
> >> >                                       (d_devs ? objs(d_devs, num_devs) :
> >> > -                                      ref_vector<device>(ctx.devices())));
> >> > +                                      ref_vector<device>(ctx.devices())), opts);
> >> >  
> >> >     validate_build_common(prog, num_devs, d_devs, pfn_notify, user_data);
> >> >  
> >> > -- 
> >> > 2.16.0