New ARI warning Wed Oct 2 01:56:22 UTC 2019

Message ID 875zl7yvhy.fsf@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Oct. 2, 2019, 4:15 p.m. UTC
  >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>>> gdb/valprint.c:2077: code: %p: Do not use printf(%p), instead use printf(%s,paddr()) to dump a target address, or host_address_to_string() for a host address
>> gdb/valprint.c:2077:	  fprintf_filtered (stream, " %p[<repeats %u times>%p]",
>> 

Simon> Hmm, I think we can get rid of that %p rule (or improve it),
Simon> otherwise we will have a lot of noise.

How's this?

Tom

commit ce98317860cdcdb19426ddee3509ccceea79c7b1
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Oct 2 10:13:33 2019 -0600

    Let ARI allow gdb %p printf extensions
    
    As pointed out by Simon, this changes ARI to allow the gdb-specific %p
    printf extensions.
    
    gdb/ChangeLog
    2019-10-02  Tom Tromey  <tromey@adacore.com>
    
            * contrib/ari/gdb_ari.sh (%p): Allow gdb-specific %p extensions.
  

Comments

Eli Zaretskii Oct. 2, 2019, 5:01 p.m. UTC | #1
> From: Tom Tromey <tromey@adacore.com>
> Cc: GDB Administrator <gdbadmin@sourceware.org>,  gdb-patches@sourceware.org
> Date: Wed, 02 Oct 2019 10:15:05 -0600
> 
> >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> >>> gdb/valprint.c:2077: code: %p: Do not use printf(%p), instead use printf(%s,paddr()) to dump a target address, or host_address_to_string() for a host address
> >> gdb/valprint.c:2077:	  fprintf_filtered (stream, " %p[<repeats %u times>%p]",
> >> 
> 
> Simon> Hmm, I think we can get rid of that %p rule (or improve it),
> Simon> otherwise we will have a lot of noise.
> 
> How's this?
> 
> Tom
> 
> commit ce98317860cdcdb19426ddee3509ccceea79c7b1
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Wed Oct 2 10:13:33 2019 -0600
> 
>     Let ARI allow gdb %p printf extensions
>     
>     As pointed out by Simon, this changes ARI to allow the gdb-specific %p
>     printf extensions.

But %p is really not quite portable.  For starters, on some systems it
includes the 0x prefix, on others it doesn't.
  
Tom Tromey Oct. 2, 2019, 5:20 p.m. UTC | #2
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> But %p is really not quite portable.  For starters, on some systems it
Eli> includes the 0x prefix, on others it doesn't.

Ordinary uses of %p are still rejected with this patch.

Tom
  
Simon Marchi Oct. 2, 2019, 5:30 p.m. UTC | #3
On 2019-10-02 12:15 p.m., Tom Tromey wrote:
> diff --git a/gdb/contrib/ari/gdb_ari.sh b/gdb/contrib/ari/gdb_ari.sh
> index 4bd434c8fec..1556756a12c 100755
> --- a/gdb/contrib/ari/gdb_ari.sh
> +++ b/gdb/contrib/ari/gdb_ari.sh
> @@ -353,7 +353,7 @@ Do not use printf(\"%p\"), instead use printf(\"%s\",paddr()) to dump a \
>  target address, or host_address_to_string() for a host address"
>      category["%p"] = ari_code
>  }
> -/%p/ && !/%prec/ {
> +/%p[^][sF]/ && !/%prec/ {
>      fail("%p")
>  }
>  

My mind is a bit confused by that regex: aren't the [^] and [sF] considered
as two successive character sets?  If so, the caret character, which negates
the character set, applies to nothing.

Shouldn't there be some escaping involved?  Or is the language smart enough
to know that because the first ] wouldn't end a valid character set, it means
it's not actually the end of the character set?

Simon
  
Tom Tromey Oct. 2, 2019, 5:35 p.m. UTC | #4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +/%p[^][sF]/ && !/%prec/ {

Simon> My mind is a bit confused by that regex: aren't the [^] and [sF] considered
Simon> as two successive character sets?  If so, the caret character, which negates
Simon> the character set, applies to nothing.

I believe that in most regexp syntax, a "]" doesn't need to be quoted in
a character class, provided it is either the first character (like
"[]...]") or comes immediately after the "^" (like "[^]...]").

The gawk manual mentions this, though it does also recommend using
backslash, so maybe I should just do that.

(info "(gawk) Bracket Expressions")

       To include one of the characters '\', ']', '-', or '^' in a bracket
    expression, put a '\' in front of it.  For example:

         [d\]]

    matches either 'd' or ']'.  Additionally, if you place ']' right after
    the opening '[', the closing bracket is treated as one of the characters
    to be matched.


Tom
  
Simon Marchi Oct. 2, 2019, 5:39 p.m. UTC | #5
On 2019-10-02 1:35 p.m., Tom Tromey wrote:
> I believe that in most regexp syntax, a "]" doesn't need to be quoted in
> a character class, provided it is either the first character (like
> "[]...]") or comes immediately after the "^" (like "[^]...]").
> 
> The gawk manual mentions this, though it does also recommend using
> backslash, so maybe I should just do that.
> 
> (info "(gawk) Bracket Expressions")
> 
>        To include one of the characters '\', ']', '-', or '^' in a bracket
>     expression, put a '\' in front of it.  For example:
> 
>          [d\]]
> 
>     matches either 'd' or ']'.  Additionally, if you place ']' right after
>     the opening '[', the closing bracket is treated as one of the characters
>     to be matched.

Well, as  long as the regex does what it is meant to do, I am fine with it.

I just thought that it would be nice to have a small comment that explains
what it does, prevent using %p to print raw pointers, but allow the gdb
extensions.

Simon
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cb450e28730..e79612696e6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2019-10-02  Tom Tromey  <tromey@adacore.com>
+
+	* contrib/ari/gdb_ari.sh (%p): Allow gdb-specific %p extensions.
+
 2019-10-02  Tom Tromey  <tromey@adacore.com>
 
 	* NEWS: Add $_ada_exception entry.
diff --git a/gdb/contrib/ari/gdb_ari.sh b/gdb/contrib/ari/gdb_ari.sh
index 4bd434c8fec..1556756a12c 100755
--- a/gdb/contrib/ari/gdb_ari.sh
+++ b/gdb/contrib/ari/gdb_ari.sh
@@ -353,7 +353,7 @@  Do not use printf(\"%p\"), instead use printf(\"%s\",paddr()) to dump a \
 target address, or host_address_to_string() for a host address"
     category["%p"] = ari_code
 }
-/%p/ && !/%prec/ {
+/%p[^][sF]/ && !/%prec/ {
     fail("%p")
 }