From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21710 invoked by alias); 26 Oct 2010 19:58:03 -0000 Received: (qmail 21688 invoked by uid 22791); 26 Oct 2010 19:57:57 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_CP X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 26 Oct 2010 19:57:51 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9B9122BAC92; Tue, 26 Oct 2010 15:57:49 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id oAzxmf99+DCN; Tue, 26 Oct 2010 15:57:49 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 5CE472BAC30; Tue, 26 Oct 2010 15:57:48 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 00908F59A2; Tue, 26 Oct 2010 15:57:47 -0400 (EDT) Date: Tue, 26 Oct 2010 19:58:00 -0000 From: Joel Brobecker To: Ken Werner Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [patch] initial OpenCL C language support Message-ID: <20101026195747.GE2847@adacore.com> References: <201010221920.30046.ken@linux.vnet.ibm.com> <201010261505.07587.ken@linux.vnet.ibm.com> <201010261800.43899.ken@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201010261800.43899.ken@linux.vnet.ibm.com> User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-10/txt/msg00377.txt.bz2 > 2010-10-26 Ken Werner > > * 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 > > * 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 > +#include "defs.h" The include of 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