diff mbox

Merge GCC producer parsers. Allow digits in identifiers.

Message ID 1423070384.4947.47.camel@bordewijk.wildebeest.org
State New
Headers show

Commit Message

Mark Wielaard Feb. 4, 2015, 5:19 p.m. UTC 
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.

Comments

Joel Brobecker Feb. 4, 2015, 6:05 p.m. UTC | #1 
> 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,
Mark Wielaard Feb. 4, 2015, 7:59 p.m. UTC | #2 
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
Mark Wielaard Feb. 10, 2015, 8:29 p.m. UTC | #3 
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
Patrick Palka Feb. 10, 2015, 11:15 p.m. UTC | #4 
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.
diff mbox

Patch

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);