[Mesa-dev] [PATCH] clover: Pass unquoted compiler arguments to Clang

[Francisco Jerez]

Vedran Miletić <vedran at miletic.net> writes:

> OpenCL apps can quote arguments they pass to the OpenCL compiler, most
> commonly include paths containing spaces. If the OpenCL compiler was
> called via a shell, the shell would remove (single or double) quotes
> before passing the argument to the compiler. Since we call Clang as a
> library, we have to remove quotes before passing the argument.

Sigh, it's a shame that the OpenCL spec doesn't specify what format the
option string should have, whether quotes have any special
interpretation at all, whether escape sequences are supported, etc.
Might be worth reporting the problem at Khronos -- Or have you found any
evidence in the spec that the parsing logic you implement below is the
correct one?

> ---
>  .../state_trackers/clover/llvm/invocation.cpp      | 41 +++++++++++++++++++---
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index e2cadda..d389a76 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -147,12 +147,43 @@ namespace {
>  
>        // Parse the compiler options:
>        std::vector<std::string> opts_array;

The code below is complex enough (and largely unrelated to the rest of
compile_llvm()) that it would be worth putting it into a separate
function -- How about 'vector<string> quoted_tokenize(const string &)' or
something similar?

> -      std::istringstream ss(opts);
> +      std::ostringstream builder;
> +
> +      // OpenCL programs can pass a single or double quoted argument, most
> +      // frequently include path. This is useful so that the path containing
> +      // spaces is treated as a single argument, but we should anyhow unquote
> +      // quoted arguments before passing them to the compiler.
> +      // We do not want to avoid using std::string::replace here, as include
> +      // path can contain quotes in file names.
> +      bool escape_next = false;
> +      bool skip_space = false;

I believe this is just 'in_quote_single || in_quote_double'.  I would
feel slightly more confident of the logic below (one less bit of state
mutated by induction across loop iterations) if you dropped this
induction variable and wrote 'in_quote_single || in_quote_double' in
closed form in the code below instead.

> +      bool in_quote_double = false;
> +      bool in_quote_single = false;
> +      for (auto pos = std::begin(opts); pos != std::end(opts); ++pos) {

You could just do something like 'for (auto c : opts) {'.

> +         if (escape_next) {
> +            builder.put(*pos);
> +            escape_next = false;
> +         } else if (*pos == '\\') {
> +            escape_next = true;
> +         } else if (*pos == '"' && !in_quote_single) {
> +            in_quote_double = !in_quote_double;
> +            skip_space = !skip_space;
> +         } else if (*pos == '\'' && !in_quote_double) {
> +            in_quote_single = !in_quote_single;
> +            skip_space = !skip_space;
> +         } else if (*pos == ' ' && !skip_space && builder.tellp() > 0) {
> +            opts_array.emplace_back(builder.str());
> +            builder.str("");
> +         } else if (*pos != ' ' || skip_space) {
> +            builder.put(*pos);
> +         }

Because the last two conditions are fully disjoint you could swap them
and then simplify the last one, like:

|         } else if (*pos != ' ' || skip_space) {
|            builder.put(*pos);
|         } else if (builder.tellp() > 0) {
|            opts_array.emplace_back(builder.str());
|            builder.str("");
|         }

(Note that by doing this you get rid of one of the two occurrences of
skip_space which will be useful if you take my suggestion to drop it).

> +      }
> +      if (builder.tellp() > 0) {
> +         opts_array.emplace_back(builder.str());
> +      }

We usually omit redundant loop and conditional block braces when the
body is a single statement.

>  
> -      while (!ss.eof()) {
> -         std::string opt;
> -         getline(ss, opt, ' ');
> -         opts_array.push_back(opt);
> +      if (in_quote_double || in_quote_single) {

Same here.

BTW, this gives me the impression we could have done the same thing in
at most two lines of code by using boost::tokenizer -- We don't depend
on Boost right now though so I guess I don't have any better ideas in
mind than roughly what you have done here.

> +         throw error(CL_INVALID_COMPILER_OPTIONS);
>        }
>  
>        opts_array.push_back(name);
> -- 
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160604/b15399ea/attachment.sig>