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

[Francisco Jerez]

Vedran Miletić  writes:

> On 07/13/2016 12:49 AM, Francisco Jerez wrote:
>> You can just replace the current implementation of tokenize(), it's not
>> used for anything else other than splitting compiler arguments AFAIK.
>>
>
> Done, patch incoming.
>
>>> Also, I would really like to progress on getting this merged in some
>>> way, so any general feedback is also welcome.
>>>
>>
>> You've got some more feedback in my previous reply it doesn't look like
>> you've taken into account yet, it would be nice to see those addressed
>> before we get your patch merged.
>>
>
> Indeed. Is [1] what you had in mind?
>

I was referring to the other code suggestions I pointed out in this message:

https://lists.freedesktop.org/archives/mesa-dev/2016-June/119419.html

Thanks a lot though for filing a Khronos bug report for this.

> Regards,
> Vedran
>
> [1] https://www.khronos.org/bugzilla/show_bug.cgi?id=1514
>
> -- 
> Vedran Miletić
> vedran.miletic.net


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

OpenCL apps can quote arguments they pass to the OpenCL compiler, most
commonly include paths containing spaces.

If the Clang OpenCL compiler was called via a shell, the shell would
split the arguments with respect to to quotes and then remove quotes
before passing the arguments to the compiler. Since we call Clang as a
library, we have to split the argument with respect to quotes and then
remove quotes before passing the arguments.

v2: move to tokenize(), remove throwing of CL_INVALID_COMPILER_OPTIONS
---
 src/gallium/state_trackers/clover/llvm/util.hpp | 37 ++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/gallium/state_trackers/clover/llvm/util.hpp 
b/src/gallium/state_trackers/clover/llvm/util.hpp
index 8db6f20..c149542 100644
--- a/src/gallium/state_trackers/clover/llvm/util.hpp
+++ b/src/gallium/state_trackers/clover/llvm/util.hpp
@@ -42,11 +42,40 @@ namespace clover {
   inline std::vector
   tokenize(const std::string ) {
  std::vector ss;
- std::istringstream iss(s);
- std::string t;
+ std::ostringstream oss;
 
- while (getline(iss, t, ' '))
-ss.push_back(t);
+ // 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;
+ bool in_quote_double = false;
+ bool in_quote_single = false;
+ for (auto pos = std::begin(s); pos != std::end(s); ++pos) {
+if (escape_next) {
+   oss.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 && oss.tellp() > 0) {
+   ss.emplace_back(oss.str());
+   oss.str("");
+} else if (*pos != ' ' || skip_space) {
+   oss.put(*pos);
+}
+ }
+ if (oss.tellp() > 0) {
+ss.emplace_back(oss.str());
+ }
 
  return ss;
   }
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

On 07/13/2016 12:49 AM, Francisco Jerez wrote:

You can just replace the current implementation of tokenize(), it's not
used for anything else other than splitting compiler arguments AFAIK.



Done, patch incoming.


Also, I would really like to progress on getting this merged in some
way, so any general feedback is also welcome.



You've got some more feedback in my previous reply it doesn't look like
you've taken into account yet, it would be nice to see those addressed
before we get your patch merged.



Indeed. Is [1] what you had in mind?

Regards,
Vedran

[1] https://www.khronos.org/bugzilla/show_bug.cgi?id=1514

--
Vedran Miletić
vedran.miletic.net
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Vedran Miletić  writes:

> 06.06.2016 u 12:24, Vedran Miletić je napisao/la:
>> On 06/06/2016 02:04 AM, Francisco Jerez wrote:
>>> Vedran Miletić  writes:

 Aside from working just like NVIDIA and AMD proprietary stacks, no.

 However, what we do right now is most certainly broken. Consider the
 following argument

 -I"/foo bar/baz"

 which is going to be sent to Clang as two arguments broken over space,
 while it should be one.
>>>
>>> But the OpenCL spec doesn't impose any explicit requirements on the
>>> formatting of the option string (unless I'm missing something), so
>>> strictly speaking both interpretations are equally broken.  It wasn't my
>>> intention to argue against implementing the same behaviour as the
>>> proprietary CL stacks as temporary solution, but the fact that this is
>>> unspecified is certainly an OpenCL spec bug.
>>>
>>
>> Agree that the spec is incomplete. Additional problem is that Clang is
>> unable to handle the incorrectly split arguments, in which case Clang
>> IMHO does the correct thing. Mesa is right now breaking the user/app
>> supplied compiler args into an array before passing it to Clang and that
>> breaking is done in a wrong way.
>>
>> I am not claiming that it's Clover's job to unquote args, even though it
>> would be convenient for Clang if it was.
>>
>> Regards,
>> Vedran
>>
 We should split arguments correctly, and whether or not we remove
 quotes in the process or we expect Clang to handle them is the easier
 part of the job.

>>
>
> Francisco,
>
> I am rebasing this. Do you suggest to replace the code in tokenize() or 
> to add an additional boolean argument to it, e.g. respect_quoting and 
> then do an if(respect_quoting) branching inside?
>

You can just replace the current implementation of tokenize(), it's not
used for anything else other than splitting compiler arguments AFAIK.

> Also, I would really like to progress on getting this merged in some 
> way, so any general feedback is also welcome.
>

You've got some more feedback in my previous reply it doesn't look like
you've taken into account yet, it would be nice to see those addressed
before we get your patch merged.

> Regards,
> Vedran
>
> -- 
> Vedran Miletić
> vedran.miletic.net


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

06.06.2016 u 12:24, Vedran Miletić je napisao/la:

On 06/06/2016 02:04 AM, Francisco Jerez wrote:

Vedran Miletić  writes:


Aside from working just like NVIDIA and AMD proprietary stacks, no.

However, what we do right now is most certainly broken. Consider the
following argument

-I"/foo bar/baz"

which is going to be sent to Clang as two arguments broken over space,
while it should be one.


But the OpenCL spec doesn't impose any explicit requirements on the
formatting of the option string (unless I'm missing something), so
strictly speaking both interpretations are equally broken.  It wasn't my
intention to argue against implementing the same behaviour as the
proprietary CL stacks as temporary solution, but the fact that this is
unspecified is certainly an OpenCL spec bug.



Agree that the spec is incomplete. Additional problem is that Clang is
unable to handle the incorrectly split arguments, in which case Clang
IMHO does the correct thing. Mesa is right now breaking the user/app
supplied compiler args into an array before passing it to Clang and that
breaking is done in a wrong way.

I am not claiming that it's Clover's job to unquote args, even though it
would be convenient for Clang if it was.

Regards,
Vedran


We should split arguments correctly, and whether or not we remove
quotes in the process or we expect Clang to handle them is the easier
part of the job.





Francisco,

I am rebasing this. Do you suggest to replace the code in tokenize() or 
to add an additional boolean argument to it, e.g. respect_quoting and 
then do an if(respect_quoting) branching inside?


Also, I would really like to progress on getting this merged in some 
way, so any general feedback is also welcome.


Regards,
Vedran

--
Vedran Miletić
vedran.miletic.net
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

On 06/06/2016 02:04 AM, Francisco Jerez wrote:

Vedran Miletić  writes:


Aside from working just like NVIDIA and AMD proprietary stacks, no.

However, what we do right now is most certainly broken. Consider the
following argument

-I"/foo bar/baz"

which is going to be sent to Clang as two arguments broken over space,
while it should be one.


But the OpenCL spec doesn't impose any explicit requirements on the
formatting of the option string (unless I'm missing something), so
strictly speaking both interpretations are equally broken.  It wasn't my
intention to argue against implementing the same behaviour as the
proprietary CL stacks as temporary solution, but the fact that this is
unspecified is certainly an OpenCL spec bug.



Agree that the spec is incomplete. Additional problem is that Clang is 
unable to handle the incorrectly split arguments, in which case Clang 
IMHO does the correct thing. Mesa is right now breaking the user/app 
supplied compiler args into an array before passing it to Clang and that 
breaking is done in a wrong way.


I am not claiming that it's Clover's job to unquote args, even though it 
would be convenient for Clang if it was.


Regards,
Vedran


We should split arguments correctly, and whether or not we remove
quotes in the process or we expect Clang to handle them is the easier
part of the job.



--
Vedran Miletić
vedran.miletic.net
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Vedran Miletić  writes:

> On 06/04/2016 10:13 AM, Francisco Jerez wrote:
>> Vedran Miletić  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?
>>
>
> Aside from working just like NVIDIA and AMD proprietary stacks, no.
>
> However, what we do right now is most certainly broken. Consider the 
> following argument
>
> -I"/foo bar/baz"
>
> which is going to be sent to Clang as two arguments broken over space, 
> while it should be one.

But the OpenCL spec doesn't impose any explicit requirements on the
formatting of the option string (unless I'm missing something), so
strictly speaking both interpretations are equally broken.  It wasn't my
intention to argue against implementing the same behaviour as the
proprietary CL stacks as temporary solution, but the fact that this is
unspecified is certainly an OpenCL spec bug.

> We should split arguments correctly, and whether or not we remove
> quotes in the process or we expect Clang to handle them is the easier
> part of the job.
>
> Regards,
> Vedran
>
> -- 
> Vedran Miletić
> vedran.miletic.net


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Vedran Miletić]

On 06/04/2016 10:13 AM, Francisco Jerez wrote:

Vedran Miletić  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?



Aside from working just like NVIDIA and AMD proprietary stacks, no.

However, what we do right now is most certainly broken. Consider the 
following argument


-I"/foo bar/baz"

which is going to be sent to Clang as two arguments broken over space, 
while it should be one. We should split arguments correctly, and whether 
or not we remove quotes in the process or we expect Clang to handle them 
is the easier part of the job.


Regards,
Vedran

--
Vedran Miletić
vedran.miletic.net
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Vedran Miletić  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 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 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

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

[Vedran Miletić]

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.
---
 .../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 opts_array;
-  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;
+  bool in_quote_double = false;
+  bool in_quote_single = false;
+  for (auto pos = std::begin(opts); pos != std::end(opts); ++pos) {
+ 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);
+ }
+  }
+  if (builder.tellp() > 0) {
+ opts_array.emplace_back(builder.str());
+  }
 
-  while (!ss.eof()) {
- std::string opt;
- getline(ss, opt, ' ');
- opts_array.push_back(opt);
+  if (in_quote_double || in_quote_single) {
+ throw error(CL_INVALID_COMPILER_OPTIONS);
   }
 
   opts_array.push_back(name);
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev