[PATCHv2,4/5] gdb: Introduce new language field la_is_string_type_p

  This commit is preparation work for the next commit, and by itself
makes no user visible change to GDB.  I've split this work into a
separate commit in order to make code review easier.

This commit adds a new field 'la_is_string_type_p' to the language
struct, this predicate will return true if a type is a string type for
the given language.

Some languages already have a "is this a string" predicate that I was
able to reuse, while for other languages I've had to add a new
predicate.  In this case I took inspiration from the value printing
code for that language - what different conditions would result in
printing something as a string.

A default "is this a string" method has also been added that looks for
TYPE_CODE_STRING, this is the fallback I've used for a couple of
languages.

In this commit I add the new field and initialise it for each
language, however at this stage the new field is never used.

gdb/ChangeLog:

	* ada-lang.c (ada_language_defn): Initialise new field.
	* c-lang.c (c_is_string_type_p): New function.
	(c_language_defn): Initialise new field.
	(cplus_language_defn): Initialise new field.
	(asm_language_defn): Initialise new field.
	(minimal_language_defn): Initialise new field.
	* c-lang.h (c_is_string_type_p): Declare new function.
	* d-lang.c (d_language_defn): Initialise new field.
	* f-lang.c (f_is_string_type_p): New function.
	(f_language_defn): Initialise new field.
	* go-lang.c (go_is_string_type_p): New function.
	(go_language_defn): Initialise new field.
	* language.c (default_is_string_type_p): New function.
	(unknown_language_defn): Initialise new field.
	(auto_language_defn): Initialise new field.
	* language.h (struct language_defn) <la_is_string_type_p>: New
	member variable.
	(default_is_string_type_p): Declare new function.
	* m2-lang.c (m2_language_defn): Initialise new field.
	* objc-lang.c (objc_language_defn): Initialise new field.
	* opencl-lang.c (opencl_language_defn): Initialise new field.
	* p-lang.c (pascal_is_string_type_p): New function.
	(pascal_language_defn): Initialise new field.
	* rust-lang.c (rust_is_string_type_p): New function.
	(rust_language_defn): Initialise new field.
---
 gdb/ChangeLog     | 28 ++++++++++++++++++++++++++++
 gdb/ada-lang.c    |  1 +
 gdb/c-lang.c      | 35 +++++++++++++++++++++++++++++++++++
 gdb/c-lang.h      |  5 +++++
 gdb/d-lang.c      |  1 +
 gdb/f-lang.c      | 11 +++++++++++
 gdb/go-lang.c     | 11 +++++++++++
 gdb/language.c    | 16 ++++++++++++++++
 gdb/language.h    |  7 +++++++
 gdb/m2-lang.c     | 22 ++++++++++++++++++++++
 gdb/objc-lang.c   |  1 +
 gdb/opencl-lang.c |  1 +
 gdb/p-lang.c      | 11 +++++++++++
 gdb/rust-lang.c   | 21 +++++++++++++++++++++
 14 files changed, 171 insertions(+)
  

On 4/17/19 12:06 AM, Andrew Burgess wrote:
> This commit is preparation work for the next commit, and by itself
> makes no user visible change to GDB.  I've split this work into a
> separate commit in order to make code review easier.
> 
> This commit adds a new field 'la_is_string_type_p' to the language
> struct, this predicate will return true if a type is a string type for
> the given language.
> 
> Some languages already have a "is this a string" predicate that I was
> able to reuse, while for other languages I've had to add a new
> predicate.  In this case I took inspiration from the value printing
> code for that language - what different conditions would result in
> printing something as a string.
> 
> A default "is this a string" method has also been added that looks for
> TYPE_CODE_STRING, this is the fallback I've used for a couple of
> languages.
> 
> In this commit I add the new field and initialise it for each
> language, however at this stage the new field is never used.
> 

Thanks.  This nicely addresses one of my previous review comments.

I can't speak to whether the implementation for the different languages
are correct.  E.g., I'm curious from where you extracted the M2 and
the Rust bits.  Didn't seem like it was a refactoring job?

Formatting comments below.

> +/* Return true if TYPE is a string.  */
> +static bool

Empty line after comment.

> +static bool
> +m2_is_string_type_p (struct type *type)
> +{
> +  type = check_typedef (type);
> +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> +      && TYPE_LENGTH (type) > 0
> +      && TYPE_LENGTH (TYPE_TARGET_TYPE (type)) > 0)
> +    {
> +      struct type *elttype = check_typedef (TYPE_TARGET_TYPE (type));
> +
> +      if (TYPE_LENGTH (elttype) == 1 &&

&& goes on the next line.

> +	  (TYPE_CODE (elttype) == TYPE_CODE_INT
> +	   || TYPE_CODE (elttype) == TYPE_CODE_CHAR))
> +	return true;
> +    }
> +
> +  return false;
> +}
> +

Thanks,
Pedro Alves
  

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Some languages already have a "is this a string" predicate that I was
Andrew> able to reuse, while for other languages I've had to add a new
Andrew> predicate.  In this case I took inspiration from the value printing
Andrew> code for that language - what different conditions would result in
Andrew> printing something as a string.

This looks essentially fine, but I had some questions.

Andrew> +bool
Andrew> +c_is_string_type_p (struct type *type)
Andrew> +{
Andrew> +  type = check_typedef (type);
Andrew> +  while (TYPE_CODE (type) == TYPE_CODE_REF)
Andrew> +    {
Andrew> +      type = TYPE_TARGET_TYPE (type);
Andrew> +      type = check_typedef (type);
Andrew> +    }
Andrew> +
Andrew> +  switch (TYPE_CODE (type))
Andrew> +    {
Andrew> +    case TYPE_CODE_ARRAY:
Andrew> +      {
Andrew> +	/* See if target type looks like a string.  */
Andrew> +	struct type *array_target_type = TYPE_TARGET_TYPE (type);
Andrew> +	return (TYPE_LENGTH (type) > 0
Andrew> +		&& TYPE_LENGTH (array_target_type) > 0
Andrew> +		&& c_textual_element_type (array_target_type, 0));
Andrew> +      }
Andrew> +    case TYPE_CODE_STRING:
Andrew> +      return true;

It seems to me that a "char *" should be considered a string in C;
and probably a "wchar_t *" as well.  Maybe see c-lang.c:classify_type.

Andrew> +/* Return true if TYPE is a string type.  */
Andrew> +static bool
Andrew> +rust_is_string_type_p (struct type *type)
Andrew> +{
Andrew> +  LONGEST low_bound, high_bound;
Andrew> +
Andrew> +  type = check_typedef (type);
Andrew> +  return ((TYPE_CODE (type) == TYPE_CODE_STRING)
Andrew> +	  || (TYPE_CODE (type) == TYPE_CODE_PTR
Andrew> +	      && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_ARRAY
Andrew> +		  && rust_u8_type_p (TYPE_TARGET_TYPE (TYPE_TARGET_TYPE (type)))
Andrew> +		  && get_array_bounds (TYPE_TARGET_TYPE (type), &low_bound,
Andrew> +				       &high_bound)))
Andrew> +	  || ((TYPE_CODE (type) == TYPE_CODE_UNION
Andrew> +	       || (TYPE_CODE (type) == TYPE_CODE_STRUCT
Andrew> +		   && !rust_enum_p (type)))
Andrew> +	      && rust_slice_type_p (type)
Andrew> +	      && strcmp (TYPE_NAME (type), "&str") == 0));

I didn't understand the reason for TYPE_CODE_UNION here.

Also, I think an array or slice of 'char' should probably be considered
a string in Rust.  See rust_chartype_p.

thanks,
Tom
  

* Pedro Alves <palves@redhat.com> [2019-04-18 18:07:55 +0100]:

> On 4/17/19 12:06 AM, Andrew Burgess wrote:
> > This commit is preparation work for the next commit, and by itself
> > makes no user visible change to GDB.  I've split this work into a
> > separate commit in order to make code review easier.
> > 
> > This commit adds a new field 'la_is_string_type_p' to the language
> > struct, this predicate will return true if a type is a string type for
> > the given language.
> > 
> > Some languages already have a "is this a string" predicate that I was
> > able to reuse, while for other languages I've had to add a new
> > predicate.  In this case I took inspiration from the value printing
> > code for that language - what different conditions would result in
> > printing something as a string.
> > 
> > A default "is this a string" method has also been added that looks for
> > TYPE_CODE_STRING, this is the fallback I've used for a couple of
> > languages.
> > 
> > In this commit I add the new field and initialise it for each
> > language, however at this stage the new field is never used.
> > 
> 
> Thanks.  This nicely addresses one of my previous review comments.
> 
> I can't speak to whether the implementation for the different languages
> are correct.  E.g., I'm curious from where you extracted the M2 and
> the Rust bits.  Didn't seem like it was a refactoring job?

It was mostly me looking for calls to LA_PRINT_STRING in each
languages value printing code then combining all of the conditions
required to reach these calls.

Could we refactor to make use of these new is_string functions within
the value printing code?  Sure, but I don't think it would be pretty.
Most languages base their value printing around a switch statement,
and also have additional format related conditions mixed in with the
logic.

Ideally, at least for this commit I'd prefer to no mess with the value
printing code more than I need too...

Thanks,
Andrew




> 
> Formatting comments below.
> 
> > +/* Return true if TYPE is a string.  */
> > +static bool
> 
> Empty line after comment.
> 
> > +static bool
> > +m2_is_string_type_p (struct type *type)
> > +{
> > +  type = check_typedef (type);
> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> > +      && TYPE_LENGTH (type) > 0
> > +      && TYPE_LENGTH (TYPE_TARGET_TYPE (type)) > 0)
> > +    {
> > +      struct type *elttype = check_typedef (TYPE_TARGET_TYPE (type));
> > +
> > +      if (TYPE_LENGTH (elttype) == 1 &&
> 
> && goes on the next line.
> 
> > +	  (TYPE_CODE (elttype) == TYPE_CODE_INT
> > +	   || TYPE_CODE (elttype) == TYPE_CODE_CHAR))
> > +	return true;
> > +    }
> > +
> > +  return false;
> > +}
> > +
> 
> Thanks,
> Pedro Alves
  

* Tom Tromey <tom@tromey.com> [2019-04-19 08:45:03 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> Some languages already have a "is this a string" predicate that I was
> Andrew> able to reuse, while for other languages I've had to add a new
> Andrew> predicate.  In this case I took inspiration from the value printing
> Andrew> code for that language - what different conditions would result in
> Andrew> printing something as a string.
> 
> This looks essentially fine, but I had some questions.
> 
> Andrew> +bool
> Andrew> +c_is_string_type_p (struct type *type)
> Andrew> +{
> Andrew> +  type = check_typedef (type);
> Andrew> +  while (TYPE_CODE (type) == TYPE_CODE_REF)
> Andrew> +    {
> Andrew> +      type = TYPE_TARGET_TYPE (type);
> Andrew> +      type = check_typedef (type);
> Andrew> +    }
> Andrew> +
> Andrew> +  switch (TYPE_CODE (type))
> Andrew> +    {
> Andrew> +    case TYPE_CODE_ARRAY:
> Andrew> +      {
> Andrew> +	/* See if target type looks like a string.  */
> Andrew> +	struct type *array_target_type = TYPE_TARGET_TYPE (type);
> Andrew> +	return (TYPE_LENGTH (type) > 0
> Andrew> +		&& TYPE_LENGTH (array_target_type) > 0
> Andrew> +		&& c_textual_element_type (array_target_type, 0));
> Andrew> +      }
> Andrew> +    case TYPE_CODE_STRING:
> Andrew> +      return true;
> 
> It seems to me that a "char *" should be considered a string in C;
> and probably a "wchar_t *" as well.  Maybe see c-lang.c:classify_type.

Thanks, I'll take a look at these cases.

> 
> Andrew> +/* Return true if TYPE is a string type.  */
> Andrew> +static bool
> Andrew> +rust_is_string_type_p (struct type *type)
> Andrew> +{
> Andrew> +  LONGEST low_bound, high_bound;
> Andrew> +
> Andrew> +  type = check_typedef (type);
> Andrew> +  return ((TYPE_CODE (type) == TYPE_CODE_STRING)
> Andrew> +	  || (TYPE_CODE (type) == TYPE_CODE_PTR
> Andrew> +	      && (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_ARRAY
> Andrew> +		  && rust_u8_type_p (TYPE_TARGET_TYPE (TYPE_TARGET_TYPE (type)))
> Andrew> +		  && get_array_bounds (TYPE_TARGET_TYPE (type), &low_bound,
> Andrew> +				       &high_bound)))
> Andrew> +	  || ((TYPE_CODE (type) == TYPE_CODE_UNION
> Andrew> +	       || (TYPE_CODE (type) == TYPE_CODE_STRUCT
> Andrew> +		   && !rust_enum_p (type)))
> Andrew> +	      && rust_slice_type_p (type)
> Andrew> +	      && strcmp (TYPE_NAME (type), "&str") == 0));
> 
> I didn't understand the reason for TYPE_CODE_UNION here.

Most of the _is_string functions were created by looking through the
language's value printing code looking for places where the language
identifies something as a string, then taking all of the conditions
that led to that point and placing them in the _is_string function.

For rust, rust_val_print_str is called from val_print_struct, which is
called for TYPE_CODE_STRUCT and TYPE_CODE_UNION, hence the above...

...but, after prompting, I took a closer look, and rust_slice_type_p
is only true for TYPE_CODE_STRUCT, which makes the union check
redundant - so its gone!

> 
> Also, I think an array or slice of 'char' should probably be considered
> a string in Rust.  See rust_chartype_p.

That sounds sensible, but .... I don't believe these things that you
describe are currently printed as strings.  Here's what I tried
(excuse my poor rust skills, maybe I'm not trying what you're
suggesting):

    $ cat str.rs
    #![allow(dead_code)]
    #![allow(unused_variables)]
    #![allow(unused_assignments)]
    
    fn main () {
       let str1: &str = "str1";
       let str2: String = "str2".to_string();
       let str3: [char; 4] = [ 's', 't', 'r', '3' ];
       let str4: &[char] = &str3;
    
       println!("hello world");	// Break here.
    }
    $ rustc -g -o str str.rs 
    $ gdb str
    <... snip ...>
    (gdb) b 11
    Breakpoint 1 at 0x3d3b: file str.rs, line 11.
    (gdb) r
    <... snip ...>
    Breakpoint 1, str::main () at str.rs:11
    11	   println!("hello world");	// Break here.
    (gdb) p str1
    $1 = "str1"
    (gdb) p str2
    $2 = alloc::string::String {vec: alloc::vec::Vec<u8> {buf: alloc::raw_vec::RawVec<u8, alloc::alloc::Global> {ptr: core::ptr::Unique<u8> {pointer: core::nonzero::NonZero<*const u8> (0x555555784a40 "str2\000"), _marker: core::marker::PhantomData<u8>}, cap: 4, a: alloc::alloc::Global}, len: 4}}
    (gdb) p str3
    $3 = [115 's', 116 't', 114 'r', 51 '3']
    (gdb) p str4
    $4 = &[char] {data_ptr: 0x7fffffffcd90 "s\000\000\000t\000\000\000r\000\000\000\063\000\000\000\xffffcd90\x7fff\004\000\000\000", length: 4}

So I think that currently only the '&str' (String slice?) is printed
as a string.

Like I said, my rust-foo is weak, so if you have an example of a
different type that is currently printed as a string then I'm happy to
fold this info a rust test and make any fixes for rust's _is_string
function as needed.

You'll have noticed (maybe?) that the original patch didn't include a
rust test at all.  This was because some of rusts value printing seems
a little broken right now.  For example, printing an array slice
(&str) variable works fine, but place this inside a struct and it no
longer works, for example:

  $ cat nested.rs
  #![allow(dead_code)]
  #![allow(unused_variables)]
  #![allow(unused_assignments)]
  
  pub struct Bad {
      pub field1: i32,
      pub field2: &'static str,
  }
  
  fn main () {
     let s = Bad { field1: 1, field2: "hello" };
     println!("hello world");	// Break here.
  }
  $ rustc -g -o nested nested.rs 
  $ gdb nested
  <... snip ...>
  (gdb) b 12
  Breakpoint 1 at 0x3d00: file nested.rs, line 12.
  (gdb) r
  <... snip ...>
  Breakpoint 1, nested::main () at nested.rs:12
  12	   println!("hello world");	// Break here.
  (gdb) p s
  $1 = nested::Bad {field1: 1, field2: <error reading variable>}
  (gdb) 

However, I fixed that with this patch:

    diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
    index 2fada465d65..597986a4b34 100644
    --- a/gdb/rust-lang.c
    +++ b/gdb/rust-lang.c
    @@ -378,6 +378,8 @@ val_print_struct (struct type *type, int embedded_offset,

       if (rust_slice_type_p (type) && strcmp (TYPE_NAME (type), "&str") == 0)
         {
    +      if (embedded_offset != 0)
    +	val = value_at_lazy (type, value_address (val) + embedded_offset);
           rust_val_print_str (stream, val, options);
           return;
         }

So, I think the summary is, I'm happy to fix rust_is_string_p to cover
any cases that currently print as a string, but I think that if
something _doesn't_ currently print as a string then rust_is_string_p
shouldn't identify it as a string - if it did then we'd end up
printing a structure at a depth when it should have been replaced with
ellipsis.

Thanks,
Andrew
  

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> ...but, after prompting, I took a closer look, and rust_slice_type_p
Andrew> is only true for TYPE_CODE_STRUCT, which makes the union check
Andrew> redundant - so its gone!

Thanks.

>> Also, I think an array or slice of 'char' should probably be considered
>> a string in Rust.  See rust_chartype_p.

Andrew> That sounds sensible, but .... I don't believe these things that you
Andrew> describe are currently printed as strings.

Thanks for looking into that.  Maybe it's just an oversight in the Rust
code, or maybe it's just uncommon to do this kind of thing.  Not your
problem :)

Andrew> You'll have noticed (maybe?) that the original patch didn't include a
Andrew> rust test at all.  This was because some of rusts value printing seems
Andrew> a little broken right now.  For example, printing an array slice
Andrew> (&str) variable works fine, but place this inside a struct and it no
Andrew> longer works, for example:

Thanks for noticing that.  I can deal with it, unless you want to.

Andrew> So, I think the summary is, I'm happy to fix rust_is_string_p to cover
Andrew> any cases that currently print as a string, but I think that if
Andrew> something _doesn't_ currently print as a string then rust_is_string_p
Andrew> shouldn't identify it as a string - if it did then we'd end up
Andrew> printing a structure at a depth when it should have been replaced with
Andrew> ellipsis.

Agreed.

Tom