[Mesa-dev] [PATCH 2/5] clover: Fix not setting build log if the build succeeds.
Tom Stellard
tom at stellard.net
Mon Jun 23 11:54:56 PDT 2014
On Sat, Jun 21, 2014 at 06:33:17PM +0200, Francisco Jerez wrote:
> Tom Stellard <thomas.stellard at amd.com> writes:
>
> > From: Matt Arsenault <arsenm2 at gmail.com>
> >
> > If there were only warnings, they would not be added to the log.
> > Also fixes valgrind use after free errors.
> > ---
> > src/gallium/state_trackers/clover/core/compiler.hpp | 3 ++-
> > src/gallium/state_trackers/clover/core/error.hpp | 2 +-
> > src/gallium/state_trackers/clover/core/program.cpp | 11 +++++++----
> > src/gallium/state_trackers/clover/llvm/invocation.cpp | 14 +++++++-------
> > 4 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/gallium/state_trackers/clover/core/compiler.hpp b/src/gallium/state_trackers/clover/core/compiler.hpp
> > index 49cd022..3ce132f 100644
> > --- a/src/gallium/state_trackers/clover/core/compiler.hpp
> > +++ b/src/gallium/state_trackers/clover/core/compiler.hpp
> > @@ -32,7 +32,8 @@ namespace clover {
> > module compile_program_llvm(const compat::string &source,
> > pipe_shader_ir ir,
> > const compat::string &target,
> > - const compat::string &opts);
> > + const compat::string &opts,
> > + std::string &log_out);
> >
>
> This doesn't work. I'm afraid you need to use compat::string on the
> compiler interface because the C++98 and C++11 versions of std::string
> are not guaranteed to be binary compatible. This mess will go away once
> we can drop support for the non-C++11 versions of LLVM. Have a look at
> the attached patch for the memory management issues with compat::string.
>
Even with your patch, I'm still having trouble getting this to work. What
is the correct pattern here? I know I need to use compat::string in the function
signature, but what type should I pass to the compile_program_llvm() function from
program::build()? A std::string a compat::string, something else?
-Tom
> And maybe rename the output argument to "r_log" as is usual everywhere
> else in clover?
>
> > module compile_program_tgsi(const compat::string &source);
> > }
> > diff --git a/src/gallium/state_trackers/clover/core/error.hpp b/src/gallium/state_trackers/clover/core/error.hpp
> > index 28459f3..9802195 100644
> > --- a/src/gallium/state_trackers/clover/core/error.hpp
> > +++ b/src/gallium/state_trackers/clover/core/error.hpp
> > @@ -66,7 +66,7 @@ namespace clover {
> >
> > class build_error : public error {
> > public:
> > - build_error(const compat::string &log) :
> > + build_error(const compat::string &log = "") :
>
> Can you rename the argument to "what" as it's no longer going to hold
> the compilation log?
>
> > error(CL_BUILD_PROGRAM_FAILURE, log) {
> > }
> > };
> > diff --git a/src/gallium/state_trackers/clover/core/program.cpp b/src/gallium/state_trackers/clover/core/program.cpp
> > index 3aaa652..91ee553 100644
> > --- a/src/gallium/state_trackers/clover/core/program.cpp
> > +++ b/src/gallium/state_trackers/clover/core/program.cpp
> > @@ -52,15 +52,18 @@ program::build(const ref_vector<device> &devs, const char *opts) {
> >
> > _opts.insert({ &dev, opts });
> >
> > + std::string build_log;
> > +
> > try {
> > auto module = (dev.ir_format() == PIPE_SHADER_IR_TGSI ?
> > compile_program_tgsi(_source) :
> > compile_program_llvm(_source, dev.ir_format(),
> > - dev.ir_target(), build_opts(dev)));
> > + dev.ir_target(), build_opts(dev),
> > + build_log));
> > _binaries.insert({ &dev, module });
> > -
> > - } catch (build_error &e) {
> > - _logs.insert({ &dev, e.what() });
> > + _logs.insert({ &dev, build_log });
> > + } catch (const build_error &) {
> > + _logs.insert({ &dev, build_log });
> > throw;
> > }
> > }
> > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > index 48810bd..0dc1f50 100644
> > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > @@ -120,12 +120,11 @@ namespace {
> > compile(llvm::LLVMContext &llvm_ctx, const std::string &source,
> > const std::string &name, const std::string &triple,
> > const std::string &processor, const std::string &opts,
> > - clang::LangAS::Map& address_spaces) {
> > + clang::LangAS::Map& address_spaces, std::string &log_out) {
> >
> > clang::CompilerInstance c;
> > clang::EmitLLVMOnlyAction act(&llvm_ctx);
> > - std::string log;
> > - llvm::raw_string_ostream s_log(log);
> > + llvm::raw_string_ostream s_log(log_out);
> > std::string libclc_path = LIBCLC_LIBEXECDIR + processor + "-"
> > + triple + ".bc";
> >
> > @@ -220,10 +219,10 @@ namespace {
> >
> > // Compile the code
> > if (!c.ExecuteAction(act))
> > - throw build_error(log);
> > + throw build_error();
> >
> > // Get address spaces map to be able to find kernel argument address space
> > - memcpy(address_spaces, c.getTarget().getAddressSpaceMap(),
> > + memcpy(address_spaces, c.getTarget().getAddressSpaceMap(),
> > sizeof(address_spaces));
> >
> > return act.takeModule();
> > @@ -386,7 +385,8 @@ module
> > clover::compile_program_llvm(const compat::string &source,
> > enum pipe_shader_ir ir,
> > const compat::string &target,
> > - const compat::string &opts) {
> > + const compat::string &opts,
> > + std::string &log_out) {
> >
> > std::vector<llvm::Function *> kernels;
> > size_t processor_str_len = std::string(target.begin()).find_first_of("-");
> > @@ -400,7 +400,7 @@ clover::compile_program_llvm(const compat::string &source,
> > // The input file name must have the .cl extension in order for the
> > // CompilerInvocation class to recognize it as an OpenCL source file.
> > llvm::Module *mod = compile(llvm_ctx, source, "input.cl", triple, processor,
> > - opts, address_spaces);
> > + opts, address_spaces, log_out);
> >
> > find_kernels(mod, kernels);
> >
> > --
> > 1.8.1.4
>
> From 38c86060d51559596c1a852e1504fe9d46c289ae Mon Sep 17 00:00:00 2001
> From: Francisco Jerez <currojerez at riseup.net>
> Date: Sat, 21 Jun 2014 18:06:07 +0200
> Subject: [PATCH] clover: Have compat::string allocate its own memory.
>
> ---
> src/gallium/state_trackers/clover/api/kernel.cpp | 4 +++-
> src/gallium/state_trackers/clover/util/compat.hpp | 8 ++++----
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/kernel.cpp b/src/gallium/state_trackers/clover/api/kernel.cpp
> index 96cf302..05cc392 100644
> --- a/src/gallium/state_trackers/clover/api/kernel.cpp
> +++ b/src/gallium/state_trackers/clover/api/kernel.cpp
> @@ -58,7 +58,9 @@ clCreateKernelsInProgram(cl_program d_prog, cl_uint count,
>
> if (rd_kerns)
> copy(map([&](const module::symbol &sym) {
> - return desc(new kernel(prog, compat::string(sym.name),
> + return desc(new kernel(prog,
> + std::string(sym.name.begin(),
> + sym.name.end()),
> range(sym.args)));
> }, syms),
> rd_kerns);
> diff --git a/src/gallium/state_trackers/clover/util/compat.hpp b/src/gallium/state_trackers/clover/util/compat.hpp
> index e68d9df..28601e8 100644
> --- a/src/gallium/state_trackers/clover/util/compat.hpp
> +++ b/src/gallium/state_trackers/clover/util/compat.hpp
> @@ -72,7 +72,7 @@ namespace clover {
> vector(const vector &v) : p(alloc(v.n, v.p, v.n)), n(v.n) {
> }
>
> - vector(iterator p, size_type n) : p(alloc(n, p, n)), n(n) {
> + vector(const_iterator p, size_type n) : p(alloc(n, p, n)), n(n) {
> }
>
> template<typename C>
> @@ -263,13 +263,13 @@ namespace clover {
> size_t offset;
> };
>
> - class string : public vector_ref<const char> {
> + class string : public vector<char> {
> public:
> - string(const char *p) : vector_ref(p, std::strlen(p)) {
> + string(const char *p) : vector(p, std::strlen(p)) {
> }
>
> template<typename C>
> - string(const C &v) : vector_ref(v) {
> + string(const C &v) : vector(v) {
> }
>
> operator std::string() const {
> --
> 1.9.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list