This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PR 8507 - Remote watchpoint limit really large


On Monday 29 December 2008 12:22:56, Mark Kettenis wrote:
> > From: Pedro Alves <pedro@codesourcery.com>

> > This patch fixes it to show:
> > 
> >  (gdb) set remote hardware-watchpoint-limit -1
> >  (gdb) show remote hardware-watchpoint-limit
> >  The maximum number of target hardware watchpoints is unlimited.
> > 

> > There's no current enum var_types for this integer usage, where
> > 0 really means zero, and negatives mean unlimited, so I've added one.
> 
> Hmm, I'm not sure I like the name.  

I didn't like it either, but it seemed better than the var_nuzinteger
I had started with.

> How about var_zuinteger, for zeroable unsigned Integer?

Hmm, I was trying to convey the idea that negatives mean unlimited,
as the watchpoint/breakpoint limits command explicitly mentions
that.  But, it's just a name, I'm not married to any.

> Interesting enough, the existing usage of var_zinteger in alpha-tdep.c
> and mips-tdep.c looks like it should really be var_zuinteger too.  I
> bet there are more cases.

Yeah, I had noticed those, but I thought I'd propose changing them
after fixing this particular remote command.  Notice that one of those
settings gets broken in a multi-arch GDB that includes both mips and
alpha support, as both -tdep.c files register the same command, but
handle different static variables.  A single setting should be
common-ized, or both settings should be prefixed with mips/alpha
respectively.

> I'm also not sure your implementation will actually work.  Especially
> the statement:
> 
> > +	      *(int *) c->var = UINT_MAX;
> 
> looks very suspicious to me.  I suspect you are having problems
> converting the variables set by this command from 'int' to 'unsigned
> int'.  

Outch.  Talk about dumbness.  Well it worked here, but it's undefined.
I was thinking of setting it to -1 for "unlimited", but for some
reason, ended up not doing it, as it was less typing.  Let's forget I
suggested that.  :-)

I just grepped through the sources for 'w_zinteger', 'w_integer' and
'w_uinteger', to catch add_setshow_XXX, and collected the following
table of all integer settings, excluding the "set debug ..."-like
variables.  I was surprised, as I thought there were many more.

Note that:

 integer does 0 -> INT_MAX conversion on a "set".
 uinteger does 0 -> UINT_MAX conversion on a "set".
 zinteger does nothing.

 uu - some smallish integer
 UU - some integer

 (Best viewed without wrapping enabled)

|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| set/show command          | var type | ideal valid range          |  set unlimited? | comments?                                                                 |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| backtrace limit           | integer  | ]0, +uu]                   |               0 |                                                                           |
| remoteaddresssize         | integer  | ]0, bitsof(ULONGEST)       |                 | 0 means use target arch address size, but prints                          |
|                           |          |                            |                 | as "unlimited".  What's an unlimited address BTW?                         |
|                           |          |                            |                 | There's is no way to go back to 0 (use target arch size),                 |
|                           |          |                            |                 | as "set remoteaddresssize 0" sets the internal variable to INT_MAX        |
|                           |          |                            |                 |                                                                           |
| listsize                  | integer  | ]0, +inf]                  |               0 |                                                                           |
| remotetimeout             | integer  | -1, [0, +UU]               |              -1 | -1 means forever (unlimited), 0 is like poll (though                      |
|                           |          |                            |                 | that's unuseful) and GDB lets you say "set ... 0", but                    |
|                           |          |                            |                 | internally sets to INT_MAX, and displays as "unlimited", which is wrong   |
| max-user-call-depth       | integer  | [smallish integer, +inf]   |               0 |                                                                           |
|                           |          |                            |                 |                                                                           |
| historysize               | integer  | [0, +inf]                  | < 0, 0, INT_MAX | setting to < 0 automatically sets to                                      |
|                           |          |                            |                 | INT_MAX (unlimited).  There's no way to specify no-history,               |
|                           |          |                            |                 | as useful as that would be.  This could be a candidate                    |
|                           |          |                            |                 | for the new proposed type.                                                |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| input-radix               | uinteger | [2, +inf]                  |                 | OMG, contrary to claims, "set intput-radix 0" *is* accepted,              |
|                           |          |                            |                 | because of 0->UINT_MAX.  Missed that yesterday somehow, when I fixed "1". |
|                           |          |                            |                 |                                                                           |
| output-radix              | uinteger |                            |                 | OMG, set output-radix 0 is accepted, due to 0->UINT_MAX.                  |
|                           |          |                            |                 | We should only accept 16, 10, 8.                                          |
|                           |          |                            |                 |                                                                           |
| max-symbolic-offset       | uinteger | [0, +inf]                  |               0 | No way to set 0, as in no way to have no symbolic offsets?                |
| width                     | uinteger | [0, +inf]                  |               0 | no wrap == unlimited                                                      |
| height                    | uinteger | [0, +inf]                  |               0 | 0 means "no height"/disable pagination, which                             |
|                           |          |                            |                 | is a kind of "unlimited".                                                 |
| elements                  | uinteger |                            |               0 | help: "set print elements 0 causes there to be no limit."                 |
| repeats                   | uinteger |                            |                 | help: "set print repeats 0 causes all                                     |
|                           |          |                            |                 | elements to be individually printed.", yet GDB displays 0 == "unlimited", |
|                           |          |                            |                 | does this make sense?                                                     |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| heuristic-fence-post      | zinteger | [0, some smallish integer] |                 | Comments mention that it would be ideal if 0 and "unlimited"              |
|                           |          |                            |                 | were both valid setttings.                                                |
|                           |          |                            |                 |                                                                           |
| remotebaud                | zinteger |                            |                 | -1 is the default, (uint)-1 will show up in the UI...                     |
| maint dwarf max-cache-age | zinteger |                            |                 | 0 means disabled cache.  There are no negative ages, yet we allow it.     |
| stop-on-solib-events      | zinteger | [0, 1]                     |                 | should be boolean, or better yet, a "catchpoint" instead                  |
| watchdog                  | zinteger | [0, +inf]                  |               0 | 0 means no watchdog, or, "unlimited"                                      |
|                           |          |                            |                 | timeout, depending on how you think about it                              |
|                           |          |                            |                 |                                                                           |
| hardware-watchpoint-limit | zinteger | [0, +inf]                  |             < 0 | 0 is "no support", < 0 unlimited                                          |
| hardware-breakpoint-limit | zinteger | [0, +inf]                  |             < 0 | 0 is "no support", < 0 unlimited                                          |
| com1base, com1irq, com... | zinteger | [0, +inf]                  |                 |                                                                           |
| annotate                  | zinteger | [0, +uu]                   |                 |                                                                           |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|

There are several interesting issues here:

 - A few integer or uinteger variables translate 0->(U)INT_MAX, and
   show "unlimited" when it doesn't make sense to.

 - Most commands don't want negative values; those that do, want
   it for a special meaning.

 - zinteger variables hold "int", but print as "unsigned int".

   Just found PR8185, which mentions this.

 - There are three zinteger commands that want distinct 0, and
   "unlimited":

    heuristic-fence-post, hardware-watchpoint-limit, hardware-breakpoint-limit

   Two actually specify < 0 as unlimited:

    hardware-watchpoint-limit, hardware-breakpoint-limit

 - There several buggy cases.

 - Most commands ideally would validate set input range, but they
   don't.

> I actually think it would make sense to treat var_uinteger and 
> var_zuinteger variables as 'int' ans set them to INT_MAX to mean
> unlimited.  I don't think the loss of range is a big loss.  For all
> practical purposes INT_MAX is just as much infinty as UINT_MAX.

I see that most (all?) current commands don't care about > INT_MAX,
though, there are cases where it doesn't make seem hard to imagine
wanting it, say for commands similar to "com1base".  It doesn't seem
hard to imagine wanting an unsigned variable without "unlimited".

Here's a draft of the types I think could handle most cases better:

| type       | internal type | print as     | how to set unlimited | internal conversion |
|------------+---------------+--------------+----------------------+---------------------|
| integer    | int           | int          | 0                    | 0 -> INT_MAX        |
| uinteger   | unsigned int  | unsigned int | 0                    | 0 -> UINT_MAX       |
| zinteger   | int           | int          | -                    | -                   |
| zuinteger  | unsigned int  | unsigned int | -                    | -                   |
|------------+---------------+--------------+----------------------+---------------------|
| nuzinteger | int           | int          | < 0                  | <0 -> -1            |

For others not covered, where we want to print something not numeric
and != "unlimited", we'd have to handle this FIXME in cli-setshow.c:

      /* FIXME: cagney/2005-02-10: Need to split this in half: code to
	 convert the value into a string (esentially the above); and
	 code to print the value out.  For the latter there should be
	 MI and CLI specific versions.  */

Alternatively:

| type       | internal type | print as     | how to set unlimited | internal conversion |
|------------+---------------+--------------+----------------------+---------------------|
| integer    | int           | int          | 0                    | 0 -> INT_MAX        |
| uinteger   | unsigned int  | unsigned int | 0                    | 0 -> UINT_MAX       |
| zinteger   | int           | int          | <0                   | <0 -> INT_MAX       |
| zuinteger  | unsigned int  | unsigned int | -                    | -                   |
| zninteger  | int           | int          | -                    | -                   |

(It's likelly that zninteger wouldn't be needed)

I think you were proposing something like:

| type      | internal type | print as | how to set unlimited | internal conversion |
|-----------+---------------+----------+----------------------+---------------------|
| integer   | int           | int      |                    0 | 0 -> INT_MAX        |
| uinteger  | int           | int      |                   <0 | <0 -> INT_MAX       |
| zinteger  | int           | int      |              INT_MAX | -                   |
| zuinteger | int           | int      |                   <0 | <0 -> INT_MAX       |

I'm not sure I understood what you were proposing correctly.  I'm also not sure if this
handles all cases easily or at all; several commands would have to be adjusted.

Something else I just noticed: in integer or uinteger variables, the 0
-> (U)INT_MAX conversion for "unlimited" is only done on a "set",
which has this effect on commands that default to 0:

 (gdb) show remoteaddresssize
 The maximum size of the address (in bits) in a memory packet is 0.
 (gdb) set remoteaddresssize 0
 (gdb) show remoteaddresssize
 The maximum size of the address (in bits) in a memory packet is unlimited.

 What's an "unlimited" address size again?  :-)


Tricky bits here ...

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]