Merge GCC producer parsers. Allow digits in identifiers.

  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

---
  

> 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
  

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.