Re: [patch] initial OpenCL C language support

[Joel Brobecker <brobecker@adacore.com>]

> 2010-10-26  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* Makefile.in (SFILES): Add opencl-lang.c.
> 	(COMMON_OBS): Add opencl-lang.o.
> 	* opencl-lang.c: New File
> 	* defs.h (enum language): Add language_opencl.
> 	* dwarf2read.c (read_file_scope): Handle DW_AT_producer for the
> 	IBM XL C OpenCL compiler.
> 	* c-lang.h: Include "parser-defs.h".
> 	(evaluate_subexp_c): Declare.
> 	* c-lang.c (evaluate_subexp_c): Remove the static qualifier.
> 	(c_op_print_tab): Add declaration.
> 	* eval.c (binop_promote): Handle language_opencl.
> 	* c-exp.y: Lookup the primitive types instead of referring to the
> 	builtins.
> 
> testsuite/ChangeLog:
> 
> 2010-10-26  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* Makefile.in (ALL_SUBDIRS): Add gdb.opencl.
> 	* configure.ac (AC_OUTPUT): Add gdb.opencl/Makefile.
> 	* configure: Regenerate.
> 	* gdb.opencl/Makefile.in: New File.
> 	* gdb.opencl/datatypes.exp: Likewise.
> 	* gdb.opencl/datatypes.cl: Likewise.
> 	* gdb.opencl/operators.exp: Likewise.
> 	* gdb.opencl/operators.cl: Likewise.
> 	* gdb.opencl/vec_comps.exp: Likewise.
> 	* gdb.opencl/vec_comps.cl: Likewise.
> 	* gdb.opencl/convs_casts.exp: Likewise.
> 	* gdb.opencl/convs_casts.cl: Likewise.
> 	* lib/opencl.exp: Likewise.
> 	* lib/opencl_hostapp.c: Likewise.
> 	* lib/opencl_kernel.cl: Likewise.
> 	* lib/cl_util.c: Likewise.
> 	* lib/cl_util.c: Likewise.
> 	* gdb.base/default.exp (set language): Add "opencl" to the list of
> 	languages.

I have a few comments, and I feel like most of the changes except maybe
the ones in c-exp.y can go in without a followup review.

> Index: src/gdb/opencl-lang.c
[...]
> +#include <string.h>
> +#include "defs.h"

The include of <string.h> directly should be replaced by an include of
"gdb_string.h". Also, the first include should always be "defs.h".

> +/* Returns the corresponding OpenCL vector type from the given type code,
> +   the length of the element type, the unsigned flag and the amount of
> +   elements (n).  */
> +static struct type *
> +lookup_opencl_vector_type (struct gdbarch *gdbarch, enum type_code code,
> +			   unsigned int el_length, unsigned int flag_unsigned,
> +			   int n)

It's really nice to see someone who documents its code.  Just a tiny nit:
Our coding style require an empty line after the comment, before the
function definition starts (there are other instances of this later,
which I did not point out).

> +/* Returns whether array contains duplicates or not within the first n number
> +   of elements.  */

Another little style nit that I'd love to see fixed in your submission,
even if I'm not personally too fussy about, is to use capital letters
when referring to function parameters.  For instance, your comment above
could be adjusted to:

> /* Returns whether the array ARR contains duplicates or not within
>    the first N elements.  */

Usually, we even write this as:

  /* Returns non-zero if the array ARR contains duplicates within
     the first N elements.  */

> +  /* The number of indicies.  */
                      "indices"
> +  /* The element indicies themselves.  */
                    ^^^^^^^^

> +  /* Assume elsize aligned offset.  */
> +  gdb_assert (offset % elsize == 0);
> +  offset /= elsize;
     ^^^^^^^^^^^^^^^^^ ????

> +  for (i = offset; i < n; i++)
> +    {
> +      struct value *from_elm_val = allocate_value (eltype);
> +      struct value *to_elm_val = value_subscript (c->val, c->indices[i]);
> +      memcpy (value_contents_writeable (from_elm_val),

Another GDB coding style rule: Empty line after local declarations...
Most of the time, you are already following that rule, but there are
a few instance where the empty line is missing.

> +  /* Multiple components of the vector are requested which means the
> +     resulting type is a vector as well.  */
> +  else
> +    {

That's just a personal comment, so feel free to follow or not: I would
move the comment inside the else block.  It would make it more obvious
that the comment only applies to that case.  And it would also avoid
slightly splitting the else part from the associated if block.

> +    case BINOP_NOTEQUAL:
> +      ret = ! value_equal (val1, val2);
              ^^^^^ extra space

> +	      /* Throw an error if arg2 or arg3 aren't vecors.  */
                                                       ^^^^^^ vectors
> +# Increase timeout
> +set timeout 60
> +verbose "Timeout set to $timeout seconds" 2

Is that really necessary? (just curious)

> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +
> +# Load the OpenCL app
> +gdb_load ${bin}

Did you know about clean_restart? This is also documented in the testsuite
cookbook.

> +gdb_test "p sizeof(bool)" "\\\$$decimal = 4"

Small remark - the way you wrote the these tests is completely correct,
but most of us don't bother matching the $N part of the output, and
write these kinds of test as:

    gdb_test "p sizeof(bool)" " = 4"

> +gdb_run_cmd

See `Running the Example Program in GDB' in the GDB Testsuite cookbook
(http://sourceware.org/gdb/wiki/GDBTestcaseCookbook): you should add
a test that "swallows" the output from the run command.

> +/* Prints OpenCL information to stdout. */

Missing second space after the period.

> +/* Reads a given file into the memory.
> +   @param filename The file name of the file to be read
> +   @param size Returns the size of the file in bytes
> +   @return a pointer the data (NULL if the does not exist)

The style of the documentation does not follow the GDB style...

> +  /* Print OpenCL informations  */
> +  /*print_clinfo ();*/

This commented-out code should be removed.

> +  /* init data */

IWBN if we followed the GNU Coding Standards for comments, and in
particular if the sentence ended with a period.

>  	|	SHORT INT_KEYWORD
> -			{ $$ = parse_type->builtin_short; }
> +			{ $$ = lookup_signed_typename (parse_language,
> +						       parse_gdbarch,
> +						       "short"); }

Just wondering why this change is necessary (?). Can you explain a
little more?

> +extern struct value * evaluate_subexp_c (struct type *expect_type,
                       ^^ no space after the '*'

-- 
Joel