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

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

[Tamar Christina]

Resending as plain text

From: Tamar Christina
Sent: Thursday, October 6, 2016 9:35 AM
To: tom@tromey.com; Yao Qi; keiths@redhat.com; gdb-patches@sourceware.org
Cc: nd
Subject: Re: [RFA 2/3] PR gdb/20653 - small cleanup in string_to_explicit_location
  

Hi All,


Any progress on getting this patch committed?
This issue is currently breaking our nightlies.


Cheers,
Tamar

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

[Tom Tromey]

>>>>> "Tamar" == Tamar Christina <Tamar.Christina@arm.com> writes:

Tamar> Any progress on getting this patch committed?
Tamar> This issue is currently breaking our nightlies.

I pushed the patch yesterday.
Did it cause some problem?  And if so, what?

Tom

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

[Tamar Christina]

> -----Original Message-----
> From: Tom Tromey [mailto:tom@tromey.com]
> Sent: 06 October 2016 16:16
> To: Tamar Christina
> Cc: tom@tromey.com; Yao Qi; keiths@redhat.com; gdb-
> patches@sourceware.org; nd
> Subject: Re: [RFA 2/3] PR gdb/20653 - small cleanup in
> string_to_explicit_location
> 
> >>>>> "Tamar" == Tamar Christina <Tamar.Christina@arm.com> writes:
> 
> Tamar> Any progress on getting this patch committed?
> Tamar> This issue is currently breaking our nightlies.
> 
> I pushed the patch yesterday.

Ah, great. Thanks, I see it in the queue for build tonight.

> Did it cause some problem?  And if so, what?
> 

No, the patch did not cause a problem. I have been waiting for the patch to
solve a problem. Without the patch compilation fails as we compile with
Warnings as errors.

Thanks,
Tamar

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

[Yao Qi]

On Mon, Oct 17, 2016 at 10:04 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On Thu, Oct 6, 2016 at 4:15 PM, Tom Tromey <tom@tromey.com> wrote:
>>>>>>> "Tamar" == Tamar Christina <Tamar.Christina@arm.com> writes:
>>
>> Tamar> Any progress on getting this patch committed?
>> Tamar> This issue is currently breaking our nightlies.
>>
>> I pushed the patch yesterday.
>
> Hi Tom,
> Could you push this patch in to 7.12 branch too?  Otherwise, we have a
> build failure on gdb 7.12 branch with gcc trunk,
>
> ../../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[2]: *** [location.o] Error 1
>

I pushed this commit to 7.12 branch, and update wiki page
https://www.sourceware.org/gdb/wiki/GDB_7.12_Release

-- 
Yao (齐尧)

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 (齐尧)

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

[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