Tom Stellard <[email protected]> writes:

> From: Matt Arsenault <[email protected]>
>
> 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.

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 <[email protected]>
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

Attachment: pgphsB_Zwc2UW.pgp
Description: PGP signature

_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to