[RFA 0/3] Fix various bugs found by static analysis

[RFA 0/3] Fix various bugs found by static analysis

[Tom Tromey]

PR gdb/20613 is a meta-bug describing various small gdb bugs found by
static analysis.

This short series fixes most of the bugs in gdb (a couple of the bugs
were elsewhere in the tree).

The two gdb bugs not fixed are:

* PR 20658, add -Wimplicit-fallthrough.  There's another series
  addressing much, but not all, of this.  This will probably come in
  by default anyway once GCC 7 is released.

* PR 20654, incorrect code in java_value_print.  Now that gcj has been
  removed, I think it's probably better to simply remove the Java
  language support.  If this sounds ok, let me know, and I can provide
  a patch.

Built and regtested on x86-64 Fedora 24.

Tom

[RFA 3/3] PR remote/20655 - small fix in handle_tracepoint_bkpts

[Tom Tromey]

handle_tracepoint_bkpts has two parallel "if"s.  This changes the
second one to check ipa_error_tracepoint, which seems to be what was
intended.

2016-10-03  Tom Tromey  <tom@tromey.com>

	PR remote/20655:
	* tracepoint.c (handle_tracepoint_bkpts): Check
	ipa_error_tracepoint, not ipa_stopping_tracepoint.
---
 gdb/gdbserver/ChangeLog    | 6 ++++++
 gdb/gdbserver/tracepoint.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 5c2cca9..afb0f78 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2016-10-03  Tom Tromey  <tom@tromey.com>
+
+	PR remote/20655:
+	* tracepoint.c (handle_tracepoint_bkpts): Check
+	ipa_error_tracepoint, not ipa_stopping_tracepoint.
+
 2016-09-30  Yao Qi  <yao.qi@linaro.org>
 
 	PR gdbserver/20627
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index c07e525..7700ad1 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -4534,7 +4534,7 @@ handle_tracepoint_bkpts (struct thread_info *tinfo, CORE_ADDR stop_pc)
 	    trace_debug ("lib stopped due to full buffer.");
 	  if (ipa_stopping_tracepoint)
 	    trace_debug ("lib stopped due to tpoint");
-	  if (ipa_stopping_tracepoint)
+	  if (ipa_error_tracepoint)
 	    trace_debug ("lib stopped due to error");
 	}
 
-- 
2.7.4

Re: [RFA 3/3] PR remote/20655 - small fix in handle_tracepoint_bkpts

[Yao Qi]

On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
> handle_tracepoint_bkpts has two parallel "if"s.  This changes the
> second one to check ipa_error_tracepoint, which seems to be what was
> intended.
>
> 2016-10-03  Tom Tromey  <tom@tromey.com>
>
>         PR remote/20655:
>         * tracepoint.c (handle_tracepoint_bkpts): Check
>         ipa_error_tracepoint, not ipa_stopping_tracepoint.

Patch is good to me.

-- 
Yao (齐尧)

[RFA 2/3] PR gdb/20653 - small cleanup in string_to_explicit_location

[Tom Tromey]

This bug points out that string_to_explicit_location compares a char*
against '\0'; whereas comparing against NULL is more normal.

2016-10-03  Tom Tromey  <tom@tromey.com>

	PR breakpoints/20653:
	* location.c (string_to_explicit_location): Use NULL, not '\0'.
---
 gdb/ChangeLog  | 5 +++++
 gdb/location.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 434f750..7a7ba1c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-10-03  Tom Tromey  <tom@tromey.com>
 
+	PR breakpoints/20653:
+	* location.c (string_to_explicit_location): Use NULL, not '\0'.
+
+2016-10-03  Tom Tromey  <tom@tromey.com>
+
 	PR symtab/20652:
 	* psymtab.c (psymbol_compare): Correctly compare "ginfo.value"
 	fields.
diff --git a/gdb/location.c b/gdb/location.c
index 65116c7..8dce21a 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -524,7 +524,7 @@ string_to_explicit_location (const char **argp,
      character is an explicit location.  "-p" is reserved, though,
      for probe locations.  */
   if (argp == NULL
-      || *argp == '\0'
+      || *argp == NULL
       || *argp[0] != '-'
       || !isalpha ((*argp)[1])
       || ((*argp)[0] == '-' && (*argp)[1] == 'p'))
-- 
2.7.4

Re: [RFA 2/3] PR gdb/20653 - small cleanup in string_to_explicit_location

[Keith Seitz]

On 10/03/2016 02:54 PM, Tom Tromey wrote:
> This bug points out that string_to_explicit_location compares a char*
> against '\0'; whereas comparing against NULL is more normal.
> 
> 2016-10-03  Tom Tromey  <tom@tromey.com>
> 
> 	PR breakpoints/20653:
> 	* location.c (string_to_explicit_location): Use NULL, not '\0'.

While I cannot approve changes to this code, having been the
under-caffeinated person who wrote it all, I'd have to say that your
patch is correct.

Thank you for taking care of this! [And hopefully a maintainer agrees
with us. :-)]

Keith

Re: [RFA 2/3] PR gdb/20653 - small cleanup in string_to_explicit_location

[Yao Qi]

On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
> This bug points out that string_to_explicit_location compares a char*
> against '\0'; whereas comparing against NULL is more normal.
>
> 2016-10-03  Tom Tromey  <tom@tromey.com>
>
>         PR breakpoints/20653:
>         * location.c (string_to_explicit_location): Use NULL, not '\0'.

Patch is good to me.

GCC trunk can capture it by a warning like this,

../../binutils-gdb/gdb/location.c: In function ‘event_location*
string_to_explicit_location(const char**, const language_defn*, int)’:
../../binutils-gdb/gdb/location.c:527:19: error: ISO C++ forbids
comparison between pointer and integer [-fpermissive]
       || *argp == '\0'
                   ^~~~
make: *** [location.o] Error 1

-- 
Yao (齐尧)

[RFA 1/3] PR symtab/20652 - fix psymbol_compare

[Tom Tromey]

This fixes an oversight in psymbol_compare.

2016-10-03  Tom Tromey  <tom@tromey.com>

	PR symtab/20652:
	* psymtab.c (psymbol_compare): Correctly compare "ginfo.value"
	fields.
---
 gdb/ChangeLog | 6 ++++++
 gdb/psymtab.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ffbdc3d..434f750 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2016-10-03  Tom Tromey  <tom@tromey.com>
+
+	PR symtab/20652:
+	* psymtab.c (psymbol_compare): Correctly compare "ginfo.value"
+	fields.
+
 2016-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	PR gdb/20609 - attach of JIT-debug-enabled inf 7.11.1 regression
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index a39c6e2..edcaa8b 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1577,7 +1577,7 @@ psymbol_compare (const void *addr1, const void *addr2, int length)
   struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
   struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
 
-  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
+  return (memcmp (&sym1->ginfo.value, &sym2->ginfo.value,
                   sizeof (sym1->ginfo.value)) == 0
 	  && sym1->ginfo.language == sym2->ginfo.language
           && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
-- 
2.7.4

Re: [RFA 1/3] PR symtab/20652 - fix psymbol_compare

[Yao Qi]

On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
> This fixes an oversight in psymbol_compare.
>
> 2016-10-03  Tom Tromey  <tom@tromey.com>
>
>         PR symtab/20652:
>         * psymtab.c (psymbol_compare): Correctly compare "ginfo.value"
>         fields.

Patch is good to me.

-- 
Yao (齐尧)

Re: [RFA 1/3] PR symtab/20652 - fix psymbol_compare

[Pedro Alves]

On 10/03/2016 10:54 PM, Tom Tromey wrote:
> @@ -1577,7 +1577,7 @@ psymbol_compare (const void *addr1, const void *addr2, int length)
>    struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
>    struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
>  
> -  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
> +  return (memcmp (&sym1->ginfo.value, &sym2->ginfo.value,
>                    sizeof (sym1->ginfo.value)) == 0

I wonder whether using memcpy is really the correct thing
here, considering ginfo.value is a union.  I.e., thinking about
padding bits not part of the active member.  Maybe we're zero
initializing the whole value and it works in practice because
of that.

>  	  && sym1->ginfo.language == sym2->ginfo.language
>            && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)

Thanks,
Pedro Alves

Re: [RFA 1/3] PR symtab/20652 - fix psymbol_compare

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I wonder whether using memcpy is really the correct thing
Pedro> here, considering ginfo.value is a union.  I.e., thinking about
Pedro> padding bits not part of the active member.  Maybe we're zero
Pedro> initializing the whole value and it works in practice because
Pedro> of that.

Yes, psymbols are fully zero-initialized so that they can be entered
into the bcache.  See add_psymbol_to_bcache.

Tom

Re: [RFA 0/3] Fix various bugs found by static analysis

[Yao Qi]

On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
>
> * PR 20654, incorrect code in java_value_print.  Now that gcj has been
>   removed, I think it's probably better to simply remove the Java
>   language support.  If this sounds ok, let me know, and I can provide
>   a patch.
>

I am fine to remove java language support in GDB, but I'd like to hear
what other people think about this.

-- 
Yao (齐尧)

Re: [RFA 0/3] Fix various bugs found by static analysis

[Pedro Alves]

On 10/04/2016 04:14 PM, Yao Qi wrote:
> On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
>>
>> * PR 20654, incorrect code in java_value_print.  Now that gcj has been
>>   removed, I think it's probably better to simply remove the Java
>>   language support.  If this sounds ok, let me know, and I can provide
>>   a patch.
>>
> 
> I am fine to remove java language support in GDB, but I'd like to hear
> what other people think about this.
> 

I'm fine with removing gcj support, but then again I never really
needed it personally.

Thanks,
Pedro Alves

Re: [RFA 0/3] Fix various bugs found by static analysis

[Eli Zaretskii]

> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 4 Oct 2016 19:30:58 +0100
> 
> On 10/04/2016 04:14 PM, Yao Qi wrote:
> > On Mon, Oct 3, 2016 at 10:54 PM, Tom Tromey <tom@tromey.com> wrote:
> >>
> >> * PR 20654, incorrect code in java_value_print.  Now that gcj has been
> >>   removed, I think it's probably better to simply remove the Java
> >>   language support.  If this sounds ok, let me know, and I can provide
> >>   a patch.
> >>
> > 
> > I am fine to remove java language support in GDB, but I'd like to hear
> > what other people think about this.
> > 
> 
> I'm fine with removing gcj support, but then again I never really
> needed it personally.

Shouldn't we wait for some time?  I mean, gcj may have been removed,
but that doesn't mean all of its installations have been deleted, or
that no one out there uses the last release, and will use them for
some time.

How about just not making any significant maintenance efforts for it
from now on?

Re: [RFA 0/3] Fix various bugs found by static analysis

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Shouldn't we wait for some time?  I mean, gcj may have been removed,
Eli> but that doesn't mean all of its installations have been deleted, or
Eli> that no one out there uses the last release, and will use them for
Eli> some time.

Nobody has used it seriously in years.  GCC dropping it is just the
final chapter, but really it died long ago.  Fedora (not sure about
other distros) stopped shipping it a while ago and there's no critical
(or even useful) free software tool written using it.  So, I think it's
safe to delete now.  In the unlikely even that someone is using it, they
can debug with gdb 7.11.

Also, FWIW, it's not outrageously bad to debug gcj-compiled code as if
it were C++.  You lose a little bit (String printing primarily) and you
have to use C++ names for things, but I think otherwise it works ok.

Tom