New language support : Vala

New language support : Vala

[Abderrahim KITOUNI]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

Hi all,

Here is a patch I wrote last summer that adds support for a new language to gdb.
It's called Vala [1], it's a high level programming language (like C# or Java)
and compiles to GObject C.

Here is a brief changelog :

buildsym.c : deduce the language from filename (like f2c).
dwarf2read.c : use that information (it's only used for stabs right now).
c-lang.[ch] : make c_emit_char public.
eval.c : use current language's value_of_this to evaluate OP_THIS.
stack.c : don't print _tmp* variables in vala
symtab.c, gdbtypes.c, linespec.c, valops.c: add some vala logic.
defs.h, symfile.c : add the new language.
vala-lang.[ch], vala-print.c : new files.

I didn't write a parser, I just used the java one. The attached patch
is made with gdb 6.8

[1] http://live.gnome.org/Vala

Regards,
Abderrahim

P.S. I don't have a copyright assignment yet.

[-- Attachment #2: gdb-vala.patch.gz --]
[-- Type: application/x-gzip, Size: 10107 bytes --]

Re: New language support : Vala

[Tom Tromey]

>>>>> "Abderrahim" == Abderrahim KITOUNI <a.kitouni@gmail.com> writes:

Abderrahim> Here is a patch I wrote last summer that adds support for
Abderrahim> a new language to gdb.  It's called Vala [1], it's a high
Abderrahim> level programming language (like C# or Java) and compiles
Abderrahim> to GObject C.

Fun!

Abderrahim> I didn't write a parser, I just used the java one. The
Abderrahim> attached patch is made with gdb 6.8

I took a quick look through the patch and there are definitely things
in there that won't apply today.  So, I suggest updating to CVS gdb
and resubmitting.

Also, I noticed a fair amount of code not conforming to GNU standards
-- missing spaces, spaces in the wrong places, comments that are not
full sentences; IOW, the usual sorts of nits.  This will all come up
in any eventual review, so I'd recommend taking a stab at fixing these
beforehand.

Any new file needs a copyright header.

All new functions ought to have an introductory comment explaining
their purpose, arguments, and return value.

For the stack.c change, I suggest a new language function that returns
true if the symbol ought to be printed.  Other languages can always
return 1.

I'm also not so sure about the valops.c change or the gdbtypes.c
change.  In general I think explicit checks of the current language
ought to be avoided in generic code.

I wonder whether some of this is better done in Python.  For instance,
perhaps specialized value-printing stuff could be done using a Python
pretty-printer.  (This code isn't in gdb CVS yet, but is coming
soon... and you can use it today by checking out from Archer.)  My
thinking here is that this might benefit all glib users, not just
Vala.  But this is just an idea, I won't insist on it.

Alternatively, I wonder whether some of the generic changes could be
made unnecessary by having a real Vala parser.

This patch needs a ChangeLog entry.

I think the overall direction of the patch seems reasonable.

Abderrahim> P.S. I don't have a copyright assignment yet.

I'll send you email off-list to get you started.

Tom

Re: New language support : Vala

[Abderrahim KITOUNI]

2009/2/7 Tom Tromey <tromey@redhat.com>:
> I took a quick look through the patch and there are definitely things
> in there that won't apply today.  So, I suggest updating to CVS gdb
> and resubmitting.
I'll try to do it.
>
> Also, I noticed a fair amount of code not conforming to GNU standards
> -- missing spaces, spaces in the wrong places, comments that are not
> full sentences; IOW, the usual sorts of nits.  This will all come up
> in any eventual review, so I'd recommend taking a stab at fixing these
> beforehand.
I'll try to see (I thought it was ok).
>
> Any new file needs a copyright header.
I didn't know what to put in, I've sent it for a preliminary review.
>
> All new functions ought to have an introductory comment explaining
> their purpose, arguments, and return value.
ok
>
> For the stack.c change, I suggest a new language function that returns
> true if the symbol ought to be printed.  Other languages can always
> return 1.
I just didn't want to do big changes.
>
> I'm also not so sure about the valops.c change or the gdbtypes.c
> change.  In general I think explicit checks of the current language
> ought to be avoided in generic code.
OK, I'll try to put these in separate changes (eventually adding
functions to language definitions)
>
> I wonder whether some of this is better done in Python.  For instance,
> perhaps specialized value-printing stuff could be done using a Python
> pretty-printer.  (This code isn't in gdb CVS yet, but is coming
> soon... and you can use it today by checking out from Archer.)  My
> thinking here is that this might benefit all glib users, not just
> Vala.  But this is just an idea, I won't insist on it.
Maybe.
>
> Alternatively, I wonder whether some of the generic changes could be
> made unnecessary by having a real Vala parser.
I don't think so, but I'll try to minimize them.
I'll try to break the patch up into several little patches, that will
ease reviewing.

Regards,
Abderrahim

Re : New language support : Vala

[Abderrahim KITOUNI]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

Hi,

Here is the first part of the patch, comprising what I think is
changes that don't affect anything else, I'll post the remaining
things later.

About the generic changes, most of them seem to be c++ specific
things, so maybe we have to put them in language structures instead.

Regards,
Abderrahim

[-- Attachment #2: gdb-vala-1.patch.gz --]
[-- Type: application/x-gzip, Size: 5884 bytes --]

Re: Re : New language support : Vala

[Tom Tromey]

>>>>> "Abderrahim" == Abderrahim KITOUNI <a.kitouni@gmail.com> writes:

Abderrahim> Here is the first part of the patch, comprising what I
Abderrahim> think is changes that don't affect anything else, I'll
Abderrahim> post the remaining things later.

Thanks.  Here's a detailed review, mostly about various formatting
nits and the like.  We're sticklers for proper formatting; and the GNU
style is a bit weird and unfamiliar to most folks.  I think there are
one or two more substantive comments in here as well.  Overall this
patch is looking pretty good.

It is worth mentioning where you built and tested this.  Also, you may
want to consider test suite additions for the vala support...

I don't know whether your FSF paperwork has gone through.  I don't
have a way to look that up any more; maybe someone else could find
out.

+	* gdb/buildsym.c (start_subfile): Deduce language from filename for Vala
+	aswell (as it's compiled to C).

That first line looks too long.  "aswell" is missing a space.
In general you don't need to explain the why of a change -- e.g., the
parenthetical comment is not needed.

+	* gdb/c-lang.c (c_emit_char): Make it non static so it can be used from
+	Vala.

Or, e.g., just "Now non-static" is fine here.

+	vala-lang.c vala-print.c\

Missing space before the \.

+	vala-lang.o vala-print.o\

Likewise.

-	  || subfile->next->language == language_fortran))
+	  || subfile->next->language == language_fortran
+	  || subfile->next->language  == language_vala))

Extra space before the ==.

vala-lang.c, vala-lang.h, and vala-print.c all need copyright headers.
You can copy one from just about any other .c file in gdb; just make
sure to update the first line (which describes the purpose of the
file) and the years.

+static CORE_ADDR vala_skip_trampoline (struct frame_info *, CORE_ADDR);
+static struct value *vala_value_of_this (int);
+static struct type *vala_lookup_transparent_type (const char *);
+static char *vala_demangle (const char *, int);

I think our current recommended style is to try to order new code so
that forward declarations are not needed for static functions (except
in the case of mutual recursion, of course).

This affects vala-print.c as well.

+/* defined in c-lang.c */
+extern const struct op_print c_op_print_tab[];

Two notes here.

First, in the GNU style, comments are full sentences, starting with a
capital letter and ending with a period followed by 2 spaces.  I
mention this because there are a number of comments that need
updating.

Second, I think it is better to simply drop the comment and move this
declaration to c-lang.h.

+const struct language_defn vala_language_defn = {
+  "vala",			/* Language name */

Move this toward the end of the file (avoiding the need for forward
declarations).  Also the "{" goes on its own line; see c-lang.c.

+extern void
+_initialize_vala_language (void)
+{
+  add_language (&vala_language_defn);
+}

By convention the _initialize function comes at the end of the file.

Also, drop the "extern" -- this applies in several places,
e.g. vala-print.c.  There's no need to have extern on a definition.

+static char *
+vala_demangle (const char *mangled, int options)

The comment before this function should describe the arguments and
the expected return values.

Other functions in this file need header comments as well.

+  char *class_name = strdup ("");

Use xstrdup in gdb.

+  part = strtok (name, "_");

I think you have to #include "gdb_string.h" to portably use strtok in
gdb.

I am not sure if you want to be using safe-ctype.h or ctype.h.

+      if (part + strlen (part) < name + strlen (mangled) &&
+	  (streq (part, "new") || streq (part, "real")))
+	{
+	  /* skip class name + '_' + "new" or "real" */
+	  method_name = strdup (mangled + (part - name) + 1 + strlen (part));
+	}

When there is a single statement in the body of an if (or else or
while or whatever), the GNU style omits the braces.  This shows up in
a few places.

+  if (streq (func_name + (strlen (func_name) - 9), "_get_type") ||
+      streq (func_name + (strlen (func_name) - 4), "_ref") ||
+      streq (func_name + (strlen (func_name) - 6), "_unref"))

Operators, in this case ||, go at the start of a line, not the end.
This applies in a few places, e.g. vala_value_print.

You may want to hoist those repeated strlen calls into a local
variable.

+/* Returns the real (runtime) type of val */

In the GNU style, when a comment refers to the value of a variable,
the variable's name is capitalized.

+  target_read_string (value_as_address (name), &type_name, 100, NULL);

That '100' looks odd.  It would probably be better to use the new
string-reading code that Thiago put in; see valprint.c:read_string.

Also, I think the code should properly handle errors.

+#define VALA_TYPE_NAME(type) ((TYPE_TAG_NAME (type) != NULL &&		\
+			       TYPE_TAG_NAME (type)[0] == '_') ?	\
+			      TYPE_TAG_NAME (type) + 1 : TYPE_TAG_NAME (type))

I think it would be good if all the macros in vala-lang.h had comments
explaining their purpose and arguments.

+#define VALA_TYPE_IS_CLASS(type) (TYPE_FIELDS (type) != NULL &&		\
+				  (streq (TYPE_FIELD_NAME(type, 0), "parent_instance") \
+				   || streq (TYPE_FIELD_NAME(type, 0), "g_type_instance")))
+#define VALA_TYPE_BASE_CLASS(class) (check_typedef(TYPE_FIELD_TYPE (class, 0)))
+#define VALA_TYPE_HAS_PRIV(type) (TYPE_NFIELDS (type) >= 2 &&		\
+				  streq (TYPE_FIELD(type,1).name, "priv"))

These all are missing spaces before "(" in the bodies.

+extern void
+vala_print_typedef (struct type *type, struct symbol *new_symbol,
+		    struct ui_file *stream)
+{
+
+};

It seems strange to have an empty function for this.
If this is what you intended, it deserves a comment explaining why.
Otherwise, I suggest removing it and using the C function.

Tom

Re : New language support : Vala

[Abderrahim KITOUNI]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

2009/2/24, Tom Tromey <tromey@redhat.com>:
> It is worth mentioning where you built and tested this.  Also, you may
> want to consider test suite additions for the vala support...
I built this on a (Debian) GNU/Linux x86, if someone finds a problem
on another platform, I'll be happy to fix it. I'll try to send another
patch with tests later.
>
> I don't know whether your FSF paperwork has gone through.  I don't
> have a way to look that up any more; maybe someone else could find
> out.
Yes, I received the letter.
[...]
> vala-lang.c, vala-lang.h, and vala-print.c all need copyright headers.
> You can copy one from just about any other .c file in gdb; just make
> sure to update the first line (which describes the purpose of the
> file) and the years.
I just forgot to add this (and print_typedef)
[...]
> I am not sure if you want to be using safe-ctype.h or ctype.h.
I don't know myself, I just need toupper (and isupper in the linespec patch).
[...]
> +  target_read_string (value_as_address (name), &type_name, 100, NULL);
>
> That '100' looks odd.  It would probably be better to use the new
> string-reading code that Thiago put in; see valprint.c:read_string.
I didn't find a better solution (read_string returns a gdb_byte* and I don't
know how to convert it).
>
> Also, I think the code should properly handle errors.
In vala_real_type ? Yes, but I didn't find a way to do it, testing for null
pointers isn't sufficient.

Here is a new version of the patch, I think I fixed everything (except the
read_string above), and another patch to deal with linespec parsing for Vala
(this one is relatively small).

Abderrahim

[-- Attachment #2: gdb-vala-rev2.patch.gz --]
[-- Type: application/x-gzip, Size: 7318 bytes --]

[-- Attachment #3: gdb-vala-linespec.patch.gz --]
[-- Type: application/x-gzip, Size: 1819 bytes --]

Re: Re : New language support : Vala

[Tom Tromey]

>>>>> "Abderrahim" == Abderrahim KITOUNI <a.kitouni@gmail.com> writes:

Sorry about the long delay on this.

As a general rule, please send a patch ping message after a week or
two without a response.  This is currently the best way to ensure that
your patch does not fall through the cracks.

Tom> I don't know whether your FSF paperwork has gone through.  I don't
Tom> have a way to look that up any more; maybe someone else could find
Tom> out.

Abderrahim> Yes, I received the letter.

Ok.  The important bit for us is knowing when the FSF says that you
are all set up.

Tom> +  target_read_string (value_as_address (name), &type_name, 100, NULL);

Tom> That '100' looks odd.  It would probably be better to use the new
Tom> string-reading code that Thiago put in; see valprint.c:read_string.

Abderrahim> I didn't find a better solution (read_string returns a
Abderrahim> gdb_byte* and I don't know how to convert it).

The result will be a string in the target encoding.  You can convert
it to the host encoding, assuming that is what you want, using the
functions declared in charset.h.

The patch is looking pretty good.  You'll need to update it to work
with cvs; some incompatible changes have gone in.

A few specific notes:

+  char *_name;

Don't start symbols with an underscore.

+  name = call_function_by_hand (g_type_name, 1, &gtype);

You may want to find a way to do this without an inferior call.
First, this won't work for core files.  Second, if the user puts a
breakpoint in g_type_name, something weird will happen.

+const struct op_print vala_op_print_tab[] =

Should be static.

+static struct field *
+vala_type_get_fields (struct type *type, int *nfields)
+{
+  int offset = 0;
+
+  if (TYPE_CODE (type) != TYPE_CODE_STRUCT)
+    /* error */
+    return NULL;

This can return NULL without setting *nfields, but...

+		  fields = vala_type_get_fields (real_type, &nfields);
+		  privfields = vala_type_get_private_fields (real_type, &nprivfields);
+		  if (nfields + nprivfields > 0)

... this code does not check that.

vala_type_get_private_fields has the same bug.

I didn't look to see if this condition is checked before the call.  If
so then you can just call internal_error or error rather than return
NULL.


+      if (VALA_TYPE_HAS_PRIV (type))
+	{
+	  offset = 2;
+	}

Remove the braces here.

+      else if (TYPE_CODE (real_type) == TYPE_CODE_INT &&
+	       streq (TYPE_NAME (real_type), "char"))

Put "&&" at the start of the line, not the end.

+	fputs_filtered (TYPE_CODE (type) == TYPE_CODE_STRUCT ?
+			"struct " : "enum ", stream);

Ditto the "?"

+      /* Don't print typedefs that just remove the '_'.  */
+      if (streq (SYMBOL_NATURAL_NAME (new_symbol), TYPE_TAG_NAME (type) + 1))
+	return;

It seems like this should actually check for the "_".

+  if (TYPE_CODE (type) == TYPE_CODE_PTR &&

"&&" position.

+      TYPE_CODE (type =
+		 check_typedef (TYPE_TARGET_TYPE (type))) == TYPE_CODE_STRUCT)

GNU style prohibits embedded assignments like this.

The linespec patch looks decent.  I am not sure we want to do it this
way, though.  I think a new language function would be preferable.

One nit:

+      result = strdup ("g_");

Should be xstrdup.

Also, the new style is to put a static function above its users, and
then not have a forward declaration for it.  This is simpler to
maintain.

Too bad the Python support isn't further along.  I'd like to see a new
language supported purely using Python; this one would have been a
good choice :-)

I know it is a pain, but this really needs test cases and
documentation -- documentation for the users, and test cases so the
gdb developers can test it occasionally.  ("Occasionally" since I
assume most of us won't have the vala compiler installed.)

Tom

Re: Re : New language support : Vala

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Tom Tromey <tromey@redhat.com>
> Date: Mon, 13 Apr 2009 18:28:37 -0600
> 
> Tom> I don't know whether your FSF paperwork has gone through.  I don't
> Tom> have a way to look that up any more; maybe someone else could find
> Tom> out.
> 
> Abderrahim> Yes, I received the letter.
> 
> Ok.  The important bit for us is knowing when the FSF says that you
> are all set up.

I see that Abderrahim's name is already on file in the FSF copyright
assignments list.

> +      TYPE_CODE (type =
> +		 check_typedef (TYPE_TARGET_TYPE (type))) == TYPE_CODE_STRUCT)
> 
> GNU style prohibits embedded assignments like this.

Only inside an `if', which this one is not:

       Try to avoid assignments inside `if'-conditions (assignments inside
    `while'-conditions are ok).  For example, don't write this:

	 if ((foo = (char *) malloc (sizeof *foo)) == 0)
	   fatal ("virtual memory exhausted");

    instead, write this:

	 foo = (char *) malloc (sizeof *foo);
	 if (foo == 0)
	   fatal ("virtual memory exhausted");

That doesn't necessarily mean I'm opposed to Tom's comment, though:
unnecessary use of this style makes the source somewhat harder to
read.

> I know it is a pain, but this really needs test cases and
> documentation -- documentation for the users, and test cases so the
> gdb developers can test it occasionally.  ("Occasionally" since I
> assume most of us won't have the vala compiler installed.)

I think such an important new feature will also need an entry for
NEWS.

Thanks.