[PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

Both dwarf2read.c (checkproducer) and utils.c (producer_is_gcc_ge_4)
implemented a GCC producer parser that tried to extract the major and minor
version of GCC. Merge them into one GCC producer parser used by both. Also
allow digits in the identifier after "GNU " such as used by GCC5 like:
"GNU C11 5.0.0 20150123 (experimental) -mtune=generic -march=x86-64 -gdwarf-5"

gdb/ChangeLog:

	* dwarf2read.c (checkproducer): Call producer_is_gcc.
	* utils.c (producer_is_gcc_ge_4): Likewise.
	(producer_is_gcc): New function.
	* utils.h (producer_is_gcc): New declaration.

No regressions on x86_64-unknown-linux-gnu with gcc 4.8.2 and 5.0.0.

OK to push?

Thanks,

Mark

---

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 9d765c5..acc296b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -12153,7 +12153,7 @@ static void
 check_producer (struct dwarf2_cu *cu)
 {
   const char *cs;
-  int major, minor, release;
+  int major, minor;
 
   if (cu->producer == NULL)
     {
@@ -12166,22 +12166,10 @@ check_producer (struct dwarf2_cu *cu)
 	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
 	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
     }
-  else if (strncmp (cu->producer, "GNU ", strlen ("GNU ")) == 0)
+  else if ((major = producer_is_gcc (cu->producer, &minor)) > 0)
     {
-      /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
-
-      cs = &cu->producer[strlen ("GNU ")];
-      while (*cs && !isdigit (*cs))
-	cs++;
-      if (sscanf (cs, "%d.%d.%d", &major, &minor, &release) != 3)
-	{
-	  /* Not recognized as GCC.  */
-	}
-      else
-	{
-	  cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
-	  cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
-	}
+      cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
+      cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
     }
   else if (strncmp (cu->producer, "Intel(R) C", strlen ("Intel(R) C")) == 0)
     cu->producer_is_icc = 1;
diff --git a/gdb/utils.c b/gdb/utils.c
index a9a3082..909476b 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3258,36 +3258,8 @@ make_bpstat_clear_actions_cleanup (void)
 int
 producer_is_gcc_ge_4 (const char *producer)
 {
-  const char *cs;
   int major, minor;
-
-  if (producer == NULL)
-    {
-      /* For unknown compilers expect their behavior is not compliant.  For GCC
-	 this case can also happen for -gdwarf-4 type units supported since
-	 gcc-4.5.  */
-
-      return -1;
-    }
-
-  /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
-
-  if (strncmp (producer, "GNU ", strlen ("GNU ")) != 0)
-    {
-      /* For non-GCC compilers expect their behavior is not compliant.  */
-
-      return -1;
-    }
-  cs = &producer[strlen ("GNU ")];
-  while (*cs && !isdigit (*cs))
-    cs++;
-  if (sscanf (cs, "%d.%d", &major, &minor) != 2)
-    {
-      /* Not recognized as GCC.  */
-
-      return -1;
-    }
-
+  major = producer_is_gcc (producer, &minor);
   if (major < 4)
     return -1;
   if (major > 4)
@@ -3295,6 +3267,36 @@ producer_is_gcc_ge_4 (const char *producer)
   return minor;
 }
 
+/* Returns the major version number if the given PRODUCER string is GCC and
+   sets the MINOR version.  Returns -1 if the given PRODUCER is NULL or it
+   isn't GCC.  */
+int
+producer_is_gcc (const char *producer, int *minor)
+{
+  const char *cs;
+  int major;
+
+  if (producer != NULL && strncmp (producer, "GNU ", strlen ("GNU ")) == 0)
+    {
+      /* Skip any identifier after "GNU " - such as "C11" "C++" or "Java".
+	 A full producer string might look like:
+	 "GNU C 4.7.2"
+	 "GNU Fortran 4.8.2 20140120 (Red Hat 4.8.2-16) -mtune=generic ..."
+	 "GNU C++14 5.0.0 20150123 (experimental)"
+      */
+      cs = &producer[strlen ("GNU ")];
+      while (*cs && !isspace (*cs))
+        cs++;
+      if (*cs && isspace (*cs))
+        cs++;
+      if (sscanf (cs, "%d.%d", &major, minor) == 2)
+	return major;
+    }
+
+  /* Not recognized as GCC.  */
+  return -1;
+}
+
 /* Helper for make_cleanup_free_char_ptr_vec.  */
 
 static void
diff --git a/gdb/utils.h b/gdb/utils.h
index e58260c..6724d7c 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -302,6 +302,7 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);
 #endif
 
 extern int producer_is_gcc_ge_4 (const char *producer);
+extern int producer_is_gcc (const char *producer, int *minor);
 
 extern int myread (int, char *, int);
 
-- 
1.8.3.1

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Joel Brobecker]

> gdb/ChangeLog:
> 
> 	* dwarf2read.c (checkproducer): Call producer_is_gcc.
> 	* utils.c (producer_is_gcc_ge_4): Likewise.
> 	(producer_is_gcc): New function.
> 	* utils.h (producer_is_gcc): New declaration.
> 
> No regressions on x86_64-unknown-linux-gnu with gcc 4.8.2 and 5.0.0.

Nice cleanup, thank you!

OK to push. One remark below, but I'll OK it nonetheless.

>  check_producer (struct dwarf2_cu *cu)
>  {
>    const char *cs;
> -  int major, minor, release;
> +  int major, minor;
>  
>    if (cu->producer == NULL)
>      {
> @@ -12166,22 +12166,10 @@ check_producer (struct dwarf2_cu *cu)
>  	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
>  	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
>      }
> -  else if (strncmp (cu->producer, "GNU ", strlen ("GNU ")) == 0)
> +  else if ((major = producer_is_gcc (cu->producer, &minor)) > 0)

I think you're going to get an ARI warning, here, because you're
assigning a variable inside a condition, and we usually don't want that.

Looking at the function, it seems to me that the current design of
the function is a little strange: Named "producer_is_gcc", it'd expect
the return value to be a boolean. So, if the function returned that
and had an extra parameter for getting the major, the issue would
not arise:

        else if (producer_is_gcc (cu->producer, &major, &minor))

I think you chose this approach out of consistency with
producer_is_gcc_ge_4, which is why I'm OK with the current patch.
But if I were to look into this, what I would do replace the two
calls to this function with calls to your new function.

I might also allow major/minor to be NULL to spare users the need
to create variables that they will not need afterwards.

So, to sumarize, push as is, and then let's clean this up (you don't
have to do the cleanup, if you don't want to, but let me know if you
pass, as I'd like to take over if you do).

>      {
> -      /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
> -
> -      cs = &cu->producer[strlen ("GNU ")];
> -      while (*cs && !isdigit (*cs))
> -	cs++;
> -      if (sscanf (cs, "%d.%d.%d", &major, &minor, &release) != 3)
> -	{
> -	  /* Not recognized as GCC.  */
> -	}
> -      else
> -	{
> -	  cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
> -	  cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
> -	}
> +      cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
> +      cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
>      }
>    else if (strncmp (cu->producer, "Intel(R) C", strlen ("Intel(R) C")) == 0)
>      cu->producer_is_icc = 1;
> diff --git a/gdb/utils.c b/gdb/utils.c
> index a9a3082..909476b 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3258,36 +3258,8 @@ make_bpstat_clear_actions_cleanup (void)
>  int
>  producer_is_gcc_ge_4 (const char *producer)
>  {
> -  const char *cs;
>    int major, minor;
> -
> -  if (producer == NULL)
> -    {
> -      /* For unknown compilers expect their behavior is not compliant.  For GCC
> -	 this case can also happen for -gdwarf-4 type units supported since
> -	 gcc-4.5.  */
> -
> -      return -1;
> -    }
> -
> -  /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
> -
> -  if (strncmp (producer, "GNU ", strlen ("GNU ")) != 0)
> -    {
> -      /* For non-GCC compilers expect their behavior is not compliant.  */
> -
> -      return -1;
> -    }
> -  cs = &producer[strlen ("GNU ")];
> -  while (*cs && !isdigit (*cs))
> -    cs++;
> -  if (sscanf (cs, "%d.%d", &major, &minor) != 2)
> -    {
> -      /* Not recognized as GCC.  */
> -
> -      return -1;
> -    }
> -
> +  major = producer_is_gcc (producer, &minor);
>    if (major < 4)
>      return -1;
>    if (major > 4)
> @@ -3295,6 +3267,36 @@ producer_is_gcc_ge_4 (const char *producer)
>    return minor;
>  }
>  
> +/* Returns the major version number if the given PRODUCER string is GCC and
> +   sets the MINOR version.  Returns -1 if the given PRODUCER is NULL or it
> +   isn't GCC.  */
> +int
> +producer_is_gcc (const char *producer, int *minor)
> +{
> +  const char *cs;
> +  int major;
> +
> +  if (producer != NULL && strncmp (producer, "GNU ", strlen ("GNU ")) == 0)
> +    {
> +      /* Skip any identifier after "GNU " - such as "C11" "C++" or "Java".
> +	 A full producer string might look like:
> +	 "GNU C 4.7.2"
> +	 "GNU Fortran 4.8.2 20140120 (Red Hat 4.8.2-16) -mtune=generic ..."
> +	 "GNU C++14 5.0.0 20150123 (experimental)"
> +      */
> +      cs = &producer[strlen ("GNU ")];
> +      while (*cs && !isspace (*cs))
> +        cs++;
> +      if (*cs && isspace (*cs))
> +        cs++;
> +      if (sscanf (cs, "%d.%d", &major, minor) == 2)
> +	return major;
> +    }
> +
> +  /* Not recognized as GCC.  */
> +  return -1;
> +}
> +
>  /* Helper for make_cleanup_free_char_ptr_vec.  */
>  
>  static void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index e58260c..6724d7c 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -302,6 +302,7 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);
>  #endif
>  
>  extern int producer_is_gcc_ge_4 (const char *producer);
> +extern int producer_is_gcc (const char *producer, int *minor);
>  
>  extern int myread (int, char *, int);
>  
> -- 
> 1.8.3.1


-- 
Joel

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

On Thu, 2015-01-29 at 12:00 +0400, Joel Brobecker wrote:
> Looking at the function, it seems to me that the current design of
> the function is a little strange: Named "producer_is_gcc", it'd expect
> the return value to be a boolean. So, if the function returned that
> and had an extra parameter for getting the major, the issue would
> not arise:
> 
>         else if (producer_is_gcc (cu->producer, &major, &minor))
> 
> I think you chose this approach out of consistency with
> producer_is_gcc_ge_4, which is why I'm OK with the current patch.
> But if I were to look into this, what I would do replace the two
> calls to this function with calls to your new function.
> 
> I might also allow major/minor to be NULL to spare users the need
> to create variables that they will not need afterwards.
> 
> So, to sumarize, push as is, and then let's clean this up (you don't
> have to do the cleanup, if you don't want to, but let me know if you
> pass, as I'd like to take over if you do).

Thanks, I pushed as is. You observations are correct.
I'll refactor this as you suggest in a separate patch.

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

On Thu, 2015-01-29 at 13:43 +0100, Mark Wielaard wrote:
> On Thu, 2015-01-29 at 12:00 +0400, Joel Brobecker wrote:
> > Looking at the function, it seems to me that the current design of
> > the function is a little strange: Named "producer_is_gcc", it'd expect
> > the return value to be a boolean. So, if the function returned that
> > and had an extra parameter for getting the major, the issue would
> > not arise:
> > 
> >         else if (producer_is_gcc (cu->producer, &major, &minor))
> > 
> > I think you chose this approach out of consistency with
> > producer_is_gcc_ge_4, which is why I'm OK with the current patch.
> > But if I were to look into this, what I would do replace the two
> > calls to this function with calls to your new function.
> > 
> > I might also allow major/minor to be NULL to spare users the need
> > to create variables that they will not need afterwards.
> > 
> > So, to sumarize, push as is, and then let's clean this up (you don't
> > have to do the cleanup, if you don't want to, but let me know if you
> > pass, as I'd like to take over if you do).
> 
> Thanks, I pushed as is. You observations are correct.
> I'll refactor this as you suggest in a separate patch.

How about the following cleanup:

Change producer_is_gcc function return type to bool.

gdb/ChangeLog:

        * utils.h (producer_is_gcc): Change return type to bool. Add major
        argument.
        * utils.c (producer_is_gcc): Likewise.
        (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
        * dwarf2read.c (check_producer): Likewise.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0d8026f..eb8541f 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -12293,7 +12293,7 @@ check_producer (struct dwarf2_cu *cu)
 	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
 	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
     }
-  else if ((major = producer_is_gcc (cu->producer, &minor)) > 0)
+  else if (producer_is_gcc (cu->producer, &major, &minor))
     {
       cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
       cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
diff --git a/gdb/utils.c b/gdb/utils.c
index 909476b..99b3874 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3259,7 +3259,8 @@ int
 producer_is_gcc_ge_4 (const char *producer)
 {
   int major, minor;
-  major = producer_is_gcc (producer, &minor);
+  if (! producer_is_gcc (producer, &major, &minor))
+    return -1;
   if (major < 4)
     return -1;
   if (major > 4)
@@ -3267,17 +3268,22 @@ producer_is_gcc_ge_4 (const char *producer)
   return minor;
 }
 
-/* Returns the major version number if the given PRODUCER string is GCC and
-   sets the MINOR version.  Returns -1 if the given PRODUCER is NULL or it
-   isn't GCC.  */
-int
-producer_is_gcc (const char *producer, int *minor)
+/* Returns true if the given PRODUCER string is GCC and sets the MAJOR
+   and MINOR versions when not NULL.  Returns false if the given PRODUCER
+   is NULL or it isn't GCC.  */
+bool
+producer_is_gcc (const char *producer, int *major, int *minor)
 {
   const char *cs;
-  int major;
 
   if (producer != NULL && strncmp (producer, "GNU ", strlen ("GNU ")) == 0)
     {
+      int maj, min;
+      if (major == NULL)
+	major = &maj;
+      if (minor == NULL)
+	minor = &min;
+
       /* Skip any identifier after "GNU " - such as "C11" "C++" or "Java".
 	 A full producer string might look like:
 	 "GNU C 4.7.2"
@@ -3289,7 +3295,7 @@ producer_is_gcc (const char *producer, int *minor)
         cs++;
       if (*cs && isspace (*cs))
         cs++;
-      if (sscanf (cs, "%d.%d", &major, minor) == 2)
+      if (sscanf (cs, "%d.%d", major, minor) == 2)
 	return major;
     }
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 6724d7c..d8afa79 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -21,6 +21,8 @@
 #ifndef UTILS_H
 #define UTILS_H
 
+#include <stdbool.h>
+
 #include "exceptions.h"
 
 extern void initialize_utils (void);
@@ -302,7 +304,7 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);
 #endif
 
 extern int producer_is_gcc_ge_4 (const char *producer);
-extern int producer_is_gcc (const char *producer, int *minor);
+extern bool producer_is_gcc (const char *producer, int *major, int *minor);
 
 extern int myread (int, char *, int);
 
-- 
1.8.3.1

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Joel Brobecker]

> How about the following cleanup:
> 
> Change producer_is_gcc function return type to bool.
> 
> gdb/ChangeLog:
> 
>         * utils.h (producer_is_gcc): Change return type to bool. Add major
>         argument.
>         * utils.c (producer_is_gcc): Likewise.
>         (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
>         * dwarf2read.c (check_producer): Likewise.

It looks really great, thanks for doing that!

I have few very minor nits to report (see below), and also I'm wincing
a bit at the use of type bool. This is the first use in GDB, and
while I don't see that as a problem, and will pre-approve this patch,
let's have this patch sit for a week to give people the opportunity
to comment before we push it. Even if people don't comment before
we put it in, and give good reasons for using "int" instead, it is
easy to fix it afterwards, and I will take care of that.

We might have to adjust our gnulib import to include the "stdbool"
module, though...

Thank you,
-- 
Joel

> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 0d8026f..eb8541f 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -12293,7 +12293,7 @@ check_producer (struct dwarf2_cu *cu)
>  	 combination.  gcc-4.5.x -gdwarf-4 binaries have DW_AT_accessibility
>  	 interpreted incorrectly by GDB now - GCC PR debug/48229.  */
>      }
> -  else if ((major = producer_is_gcc (cu->producer, &minor)) > 0)
> +  else if (producer_is_gcc (cu->producer, &major, &minor))
>      {
>        cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor < 6);
>        cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor < 3);
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 909476b..99b3874 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3259,7 +3259,8 @@ int
>  producer_is_gcc_ge_4 (const char *producer)
>  {
>    int major, minor;
> -  major = producer_is_gcc (producer, &minor);
> +  if (! producer_is_gcc (producer, &major, &minor))

Would you mind taking this opportunity for adding an empty line
after the declarations? It's a GDB Coding Style...

> +    return -1;
>    if (major < 4)
>      return -1;
>    if (major > 4)
> @@ -3267,17 +3268,22 @@ producer_is_gcc_ge_4 (const char *producer)
>    return minor;
>  }
>  
> -/* Returns the major version number if the given PRODUCER string is GCC and
> -   sets the MINOR version.  Returns -1 if the given PRODUCER is NULL or it
> -   isn't GCC.  */
> -int
> -producer_is_gcc (const char *producer, int *minor)
> +/* Returns true if the given PRODUCER string is GCC and sets the MAJOR
> +   and MINOR versions when not NULL.  Returns false if the given PRODUCER
> +   is NULL or it isn't GCC.  */
> +bool

Similar to above, we have a convention that function documentations
should be separated by an empty line from the implementation.

> +producer_is_gcc (const char *producer, int *major, int *minor)
>  {
>    const char *cs;
> -  int major;
>  
>    if (producer != NULL && strncmp (producer, "GNU ", strlen ("GNU ")) == 0)
>      {
> +      int maj, min;
> +      if (major == NULL)

Empty line after "int maj, min;"



> +	major = &maj;
> +      if (minor == NULL)
> +	minor = &min;
> +
>        /* Skip any identifier after "GNU " - such as "C11" "C++" or "Java".
>  	 A full producer string might look like:
>  	 "GNU C 4.7.2"
> @@ -3289,7 +3295,7 @@ producer_is_gcc (const char *producer, int *minor)
>          cs++;
>        if (*cs && isspace (*cs))
>          cs++;
> -      if (sscanf (cs, "%d.%d", &major, minor) == 2)
> +      if (sscanf (cs, "%d.%d", major, minor) == 2)
>  	return major;
>      }
>  
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 6724d7c..d8afa79 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -21,6 +21,8 @@
>  #ifndef UTILS_H
>  #define UTILS_H
>  
> +#include <stdbool.h>
> +
>  #include "exceptions.h"
>  
>  extern void initialize_utils (void);
> @@ -302,7 +304,7 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);
>  #endif
>  
>  extern int producer_is_gcc_ge_4 (const char *producer);
> -extern int producer_is_gcc (const char *producer, int *minor);
> +extern bool producer_is_gcc (const char *producer, int *major, int *minor);
>  
>  extern int myread (int, char *, int);
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

On Wed, 2015-02-04 at 22:05 +0400, Joel Brobecker wrote:
> > How about the following cleanup:
> > 
> > Change producer_is_gcc function return type to bool.
> > 
> > gdb/ChangeLog:
> > 
> >         * utils.h (producer_is_gcc): Change return type to bool. Add major
> >         argument.
> >         * utils.c (producer_is_gcc): Likewise.
> >         (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
> >         * dwarf2read.c (check_producer): Likewise.
> 
> It looks really great, thanks for doing that!
> 
> I have few very minor nits to report (see below), and also I'm wincing
> a bit at the use of type bool. This is the first use in GDB, and
> while I don't see that as a problem, and will pre-approve this patch,
> let's have this patch sit for a week to give people the opportunity
> to comment before we push it.

OK. I added the 3 empty lines (don't want ARI yelling at me) and will
push next week.

Thanks,

Mark

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

On Wed, 2015-02-04 at 20:59 +0100, Mark Wielaard wrote:
> On Wed, 2015-02-04 at 22:05 +0400, Joel Brobecker wrote:
> > > How about the following cleanup:
> > > 
> > > Change producer_is_gcc function return type to bool.
> > > 
> > > gdb/ChangeLog:
> > > 
> > >         * utils.h (producer_is_gcc): Change return type to bool. Add major
> > >         argument.
> > >         * utils.c (producer_is_gcc): Likewise.
> > >         (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
> > >         * dwarf2read.c (check_producer): Likewise.
> > 
> > It looks really great, thanks for doing that!
> > 
> > I have few very minor nits to report (see below), and also I'm wincing
> > a bit at the use of type bool. This is the first use in GDB, and
> > while I don't see that as a problem, and will pre-approve this patch,
> > let's have this patch sit for a week to give people the opportunity
> > to comment before we push it.
> 
> OK. I added the 3 empty lines (don't want ARI yelling at me) and will
> push next week.

I pushed this now.

Thanks,

Mark

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Patrick Palka]

On Tue, Feb 10, 2015 at 3:29 PM, Mark Wielaard <mjw@redhat.com> wrote:
> On Wed, 2015-02-04 at 20:59 +0100, Mark Wielaard wrote:
>> On Wed, 2015-02-04 at 22:05 +0400, Joel Brobecker wrote:
>> > > How about the following cleanup:
>> > >
>> > > Change producer_is_gcc function return type to bool.
>> > >
>> > > gdb/ChangeLog:
>> > >
>> > >         * utils.h (producer_is_gcc): Change return type to bool. Add major
>> > >         argument.
>> > >         * utils.c (producer_is_gcc): Likewise.
>> > >         (producer_is_gcc_ge_4): Adjust producer_is_gcc call.
>> > >         * dwarf2read.c (check_producer): Likewise.
>> >
>> > It looks really great, thanks for doing that!
>> >
>> > I have few very minor nits to report (see below), and also I'm wincing
>> > a bit at the use of type bool. This is the first use in GDB, and
>> > while I don't see that as a problem, and will pre-approve this patch,
>> > let's have this patch sit for a week to give people the opportunity
>> > to comment before we push it.
>>
>> OK. I added the 3 empty lines (don't want ARI yelling at me) and will
>> push next week.
>
> I pushed this now.
>
> Thanks,
>
> Mark

Now that producer_is_gcc() returns a bool, shouldn't the statement
'return -1' become 'return false;', and the statement 'return major;'
become 'return true;'?

As it stands the function now always returns true since both
'(bool)-1' and '(bool)major' evaluate to true, I think.

Re: [PATCH] Merge GCC producer parsers. Allow digits in identifiers.

[Mark Wielaard]

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

On Tue, Feb 10, 2015 at 06:15:44PM -0500, Patrick Palka wrote:
> Now that producer_is_gcc() returns a bool, shouldn't the statement
> 'return -1' become 'return false;', and the statement 'return major;'
> become 'return true;'?

Yes, of course. What a silly mistake. Thanks for catching that.

I pushed the attached as obvious.


[-- Attachment #2: 0001-gdb-producer_is_gcc-fix-bool-return-value.patch --]
[-- Type: text/plain, Size: 1188 bytes --]

From 9f615e3af0356052a475812cb5a4380a5fe51182 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 11 Feb 2015 00:45:31 +0100
Subject: [PATCH] gdb producer_is_gcc fix bool return value.

gdb/ChangeLog:

	* utils.c (producer_is_gcc): Return true or false.
---
 gdb/ChangeLog | 4 ++++
 gdb/utils.c   | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a23c2d8..fe61d24 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2015-02-11  Mark Wielaard  <mjw@redhat.com>
+
+	* utils.c (producer_is_gcc): Return true or false.
+
 2015-02-04  Mark Wielaard  <mjw@redhat.com>
 
 	* utils.h (producer_is_gcc): Change return type to bool. Add major
diff --git a/gdb/utils.c b/gdb/utils.c
index 2b54739..3ce5814 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3299,11 +3299,11 @@ producer_is_gcc (const char *producer, int *major, int *minor)
       if (*cs && isspace (*cs))
         cs++;
       if (sscanf (cs, "%d.%d", major, minor) == 2)
-	return major;
+	return true;
     }
 
   /* Not recognized as GCC.  */
-  return -1;
+  return false;
 }
 
 /* Helper for make_cleanup_free_char_ptr_vec.  */
-- 
2.1.0