[OpenCL] Several delegates buggy

Posted Wednesday, 11 November, 2009 - 22:37 by nythrix

Project:	The Open Toolkit library
Version:	1.x-dev
Component:	Code
Category:	bug report
Priority:	normal
Assigned:	Unassigned
Status:	confirmed

public static int EnqueueCopyImage(
                   IntPtr command_queue,
                   IntPtr src_image,
                   IntPtr dst_image,
                   IntPtr** src_origin,
                   IntPtr** dst_origin,
                   IntPtr** region,
                   int num_events_in_wait_list,
                   IntPtr* event_wait_list,
                   IntPtr* @event );

cl.h contains:

extern CL_API_ENTRY cl_int CL_API_CALL
clEnqueueCopyImage(cl_command_queue     /* command_queue */,
                   cl_mem               /* src_image */,
                   cl_mem               /* dst_image */, 
                   const size_t *       /* src_origin[3] */,
                   const size_t *       /* dst_origin[3] */,
                   const size_t *       /* region[3] */, 
                   cl_uint              /* num_events_in_wait_list */,
                   const cl_event *     /* event_wait_list */,
                   cl_event *           /* event */) CL_API_SUFFIX__VERSION_1_0;

The specs:

cl_int clEnqueueCopyImage (cl_command_queue command_queue,
                          cl_mem src_image,
                          cl_mem dst_image,
                          const size_t src_origin[3],
                          const size_t dst_origin[3],
                          const size_t region[3],
                          cl_uint num_events_in_wait_list,
                          const cl_event *event_wait_list,
                          cl_event *event)

I strongly suggest sticking to the specs. An array is basically a pointer so adding another indirection is useless (if not erroneous). IMO the [3] in cl.h is just a hint and not part of the declaration.
This affects all clEnqueue* functions that take at least one cl_mem image argument.

Comments

Status:

open

» confirmed

The cl.h-to-XML converter is parsing this line

const size_t *       /* src_origin[3] */

const size_t *       src_origin[3]

which is then translated to

IntPtr**

(where size_t* -> IntPtr* and [3] -> *)

In other words, the issue is caused by mis-parsing the C header. I don't think this can be fixed without breaking other parts of the parser, so the solution is to use overrides.xml and correct those functions by hand.

I have inquired Khronos about OpenCL spec files but unfortunately it seems the headers are all hand-written.

Please post any other such issues you encounter!

That's exactly what I thought. However, I'm not blaming the generator itself. Until it gets some sort of AI, we're on our own :)

I'm posting a possible workaround for this issue. I have no knowledge of the generator so a review might be in place.

    <function name="EnqueueReadImage" extension="Core">
      <param name="origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>
 
    <function name="EnqueueWriteImage" extension="Core">
      <param name="origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>
 
    <function name="EnqueueCopyImage" extension="Core">
      <param name="src_origin"><type>IntPtr*</type></param>
      <param name="dst_origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>
 
    <function name="EnqueueCopyImageToBuffer" extension="Core">
      <param name="src_origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>
 
    <function name="EnqueueCopyBufferToImage" extension="Core">
      <param name="dst_origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>
 
    <function name="EnqueueMapImage" extension="Core">
      <param name="origin"><type>IntPtr*</type></param>
      <param name="region"><type>IntPtr*</type></param>
    </function>

Have you tried this, Fiddler? I tried putting those lines into ../Source/Bind/Specifications/CL10/overrides.xml and recreate the bindings but the generator doesn't like me.

When can we expect the CL/ES bindings move back into "official" OpenTK? Release 1.1? Further on down the roadmap?

Make sure the overrides are placed inside the <replace> section of overrides.xml and invoke the generator as follows:

Bind.exe -mode:cl -out:..\..\..\OpenTK\Source\Compute

This seems to work fine here. (Edit: update opentk trunk first to make comparisons between existing CL.cs and new CL.cs easier).

ES20 is already available in opentk-1.0, while CL should be part of opentk-1.1. ES10/ES11 depend on whether anyone displays interest for the APIs.

Ok, this workaround doesn't really work. Instead of replacing IntPtr** with IntPtr*, the two get merged together so I end up with a triple pointer. Not good.

Sounds like a bug in the generator, let me check what is going on here.

Ok I finally tracked down the bit of code that does the parsing. File ESCLParser.cs, lines 215-218:

// The second part (after the '|') matches parameters of the following formats:
// '[parameter type] [parameter name]', '[parameter type] [pointer] [parameter name]', 'const [parameter type][pointer] [parameter name]'
// where [parameter name] may be inside comments (/* ... */) and [pointer] is '', '*', '**', etc.
var get_param = new Regex(@"(\w+\s\(\*\w+\)\s*\(.*\)\s*(/\*.*?\*/|\w+)? | (const\s)?(\w+\s*)+\**\s*(/\*.*?\*/|\w+(\[.*?\])?)),?", RegexOptions.IgnorePatternWhitespace);

Now, the problem lies with the "[parameter name] may be inside comments" assumption. It should be modified to "[parameter name] [warning]AND MORE[/warning] may be inside comments". These hairy bits can, surprisingly, change the parameter type.

the Fiddler wrote:

The cl.h-to-XML converter is parsing this line
const size_t * /* src_origin[3] */
as
const size_t * src_origin[3]

It didn't occurr to me before but this transition is a solid bug. Such a step is wrong because you're changing the API here.
The [param name] in the comment should be stripped out of everything except the first identificator-style word. If this breaks anything then there are more undiscovered bugs in the bindings.

Fiddler wrote:

Aehm, no. See above. Besides, this advice reminds me of a certain gigantic SW corporation :D

Edit:
Acutally it's simple as plastic: Under no circumstances shall the removal of a comment change the API...

Me wrote:

Instead of replacing IntPtr** with IntPtr*, the two get merged together so I end up with a triple pointer.

This issue is a completely different story that comes from another part of the generator: #1506: [Generator] Bad pointer type overriding..

Seems there could be a simple solution after all. Does this patch to 'converter' fix the issue?

Index: ESCLParser.cs
===================================================================
--- ESCLParser.cs	(revision 2574)
+++ ESCLParser.cs	(working copy)
@@ -237,18 +237,19 @@
                             let param_name =
                                 is_function_pointer ? tokens[1].TrimStart('(', '*').Split(')')[0] :
                                 (tokens.Last().Trim() != "*/" ? tokens.Last() : tokens[tokens.Length - 2]).Trim()
+							let is_param_name_in_comment = !is_function_pointer && tokens.Last().Trim == "*/" // If the parameter is commented out, don't let it affect 'indirection_level' (i.e. "float* /* foo[4] */", see bug http://www.opentk.com/node/1368).
                             let param_type =
                                 is_function_pointer ? "IntPtr" :
                                 (from t in tokens where t.Trim() != "const" && t.Trim() != "unsigned" select t).First().Trim()
                             let has_array_size = array_size.IsMatch(param_name)
                             let indirection_level =
                                 is_function_pointer ? 0 :
-                                (from c in param_name where c == '*' select c).Count() +
+                                is_param_name_in_comment ? 0 : (from c in param_name where c == '*' select c).Count() +
                                 (from c in param_type where c == '*' select c).Count() +
                                 (from t in tokens where t == "***" select t).Count() * 3 +
                                 (from t in tokens where t == "**" select t).Count() * 2 +
                                 (from t in tokens where t == "*" select t).Count() +
-                                (has_array_size ? 1 : 0)
+                                (has_array_size && !is_param_name_in_comment ? 1 : 0)
                             let pointers = new string[] { "*", "*", "*", "*" } // for adding indirection levels (pointers) to param_type
                             where tokens.Length > 1
                             select new

Apply, compile and run with converter -p cl -v 1.0 [path to cl.h]. (Edit: I'd test it myself, but monodevelop/gmcs don't want to cooperate right now).

One thing to keep in mind is that converter started as a simple (15 line) helper script I wrote for internal use. I never intended to release it to the public and I never intended it to make it complex or make it produce 100% correct results. If it has a bug that is simpler to fix by patching its output, so be it. (This doesn't seem to be the case here, after all).

Unfortunately, that wipes out every pointer type.

But this works:

Index: ESCLParser.cs
===================================================================
--- ESCLParser.cs	(revision 2576)
+++ ESCLParser.cs	(working copy)
@@ -208,6 +208,11 @@
 
                 var paramaters_string = Regex.Match(line, @"\(.*\)").Captures[0].Value.TrimStart('(').TrimEnd(')');
 
+                // This clears up commented param names that are accompanied
+                // by constructs that would modify the type when uncommented (i.e.: /* param[6] */)
+                // It prevents bugs such as these: http://www.opentk.com/node/1368
+                paramaters_string = Regex.Replace( paramaters_string, @"/\*+\s*(\w+)[\[\]\d\s,\w]*\*+/", @"$1" );
+
                 // This regex matches function parameters.
                 // The first part matches function pointers in the following format:
                 // '[return type] (*[function pointer name])([parameter list]) [parameter name]

A quick look into the generated signatures shows this is ok.

Home of the Open Toolkit library

[OpenCL] Several delegates buggy

Jump to:

Comments

Comment viewing options

#1

#2

#3

#4

#5

#6

#7

#8

#9

#10

Recent