[Patch] Another small memattr fix.

[Patch] Another small memattr fix.

[Don Howard]

The following patch fixes a buglet involving memory regions.  

(gdb) mem 0 4 32 wo
(gdb) mem 0xfffffff0 0x100000000 32 wo
invalid memory region: low >= high


2002-06-12  Don Howard  <dhoward@redhat.com>

	* memattr.c (create_mem_region): Permit max addr+1 for upper bound
	of memory regions.


Index: gdb/memattr.c
===================================================================
RCS file: /cvs/src/src/gdb/memattr.c,v
retrieving revision 1.11
diff -p -u -w -r1.11 memattr.c
--- gdb/memattr.c       12 May 2002 04:20:05 -0000      1.11
+++ gdb/memattr.c       12 Jun 2002 23:16:20 -0000
@@ -47,7 +47,7 @@ create_mem_region (CORE_ADDR lo, CORE_AD
   struct mem_region *n, *new;

   /* lo == hi is a useless empty region */
-  if (lo >= hi)
+  if (lo > hi-1)
     {
       printf_unfiltered ("invalid memory region: low >= high\n");


-- 
dhoward@redhat.com
gdb engineering

Re: [Patch] Another small memattr fix.

[Andrew Cagney]

> -  if (lo >= hi)
> +  if (lo > hi-1)

What happens with:

   mem 0xfffffff0 0

it looks like a testsuite addition (memattr.exp?) is needed to pin down 
all the edge cases - signed vs unsigned addresses etc.  (I assume you 
checked the change in).

Andrew

Re: [Patch] Another small memattr fix.

[Don Howard]

On Wed, 12 Jun 2002, Andrew Cagney wrote:

> > -  if (lo >= hi)
> > +  if (lo > hi-1)
> 
> What happens with:
> 
>    mem 0xfffffff0 0
> 
> it looks like a testsuite addition (memattr.exp?) is needed to pin down 
> all the edge cases - signed vs unsigned addresses etc.  (I assume you 
> checked the change in).
> 

No, I've not checked it in yet.  

-- 
dhoward@redhat.com
gdb engineering

Re: [Patch] Another small memattr fix.

[Kevin Buettner]

Don,

Could you explain this change in more detail?

> -  if (lo >= hi)
> +  if (lo > hi-1)

I.e, with the exception of the boundary conditions (max int, min int),
aren't these two equivalent when lo and hi are integers?

Kevin

Re: [Patch] Another small memattr fix.

[Don Howard]

On Fri, 14 Jun 2002, Kevin Buettner wrote:

> Don,
> 
> Could you explain this change in more detail?
> 
> > -  if (lo >= hi)
> > +  if (lo > hi-1)
> 
> I.e, with the exception of the boundary conditions (max int, min int),
> aren't these two equivalent when lo and hi are integers?
> 

Yes they are equivalent -- I was trying to address the boundary
conditions.  (Obviously this approch is wrong, wrong, wrong)

Memory regions are specified with an inclusive lower bound and an
exclusive upper bound, so

mem 0 4 wo 
mem 4 8 ro

means that addresses 0x0, 0x1, 0x2, 0x3 are writable and 0x4, 0x5, 0x6 0x7
are readable.  

The exclusive upper bound causes trouble when trying to address max int, 
as it wraps :

(Assuming 32 bit addresses)

mem 0xfffffff0 0x100000000 ==> 0xfffffff0 0x0  /* woops */



The strings are arbitrary expressions and are converted to address via
parse_and_eval_address(), which does not flag overflow:

mem_command (char *args, int from_tty)
{
  CORE_ADDR lo, hi;
  char *tok;
  struct mem_attrib attrib;

  if (!args)
    error_no_arg ("No mem");

  tok = strtok (args, " \t");
  if (!tok)
    error ("no lo address");
  lo = parse_and_eval_address (tok);

  tok = strtok (NULL, " \t");
  if (!tok)
    error ("no hi address");
  hi = parse_and_eval_address (tok);

mabe parse_and_eval_address could detect overflow and throw an error().

Another possiblity is that the interface could be changed, making the
upper bound inclusive also. 

Any preferences?




-- 
dhoward@redhat.com
gdb engineering

Re: [Patch] Another small memattr fix.

[Kevin Buettner]

On Jun 14, 12:44pm, Don Howard wrote:

> The strings are arbitrary expressions and are converted to address via
> parse_and_eval_address(), which does not flag overflow:
> 
> mem_command (char *args, int from_tty)
> {
>   CORE_ADDR lo, hi;
>   char *tok;
>   struct mem_attrib attrib;
> 
>   if (!args)
>     error_no_arg ("No mem");
> 
>   tok = strtok (args, " \t");
>   if (!tok)
>     error ("no lo address");
>   lo = parse_and_eval_address (tok);
> 
>   tok = strtok (NULL, " \t");
>   if (!tok)
>     error ("no hi address");
>   hi = parse_and_eval_address (tok);
> 
> mabe parse_and_eval_address could detect overflow and throw an error().

Maybe I'm missing something, but it seems to me that you're still left
with the problem of how to represent the maximum address + 1.  (Throwing
an error doesn't really help, does it?)

> Another possiblity is that the interface could be changed, making the
> upper bound inclusive also. 

This sounds better.

So, on a 16 bit machine, you could say

    mem 0xf000 0xffff ro

to indicate that the top 4096 bytes are read-only.

Kevin

Re: [Patch] Another small memattr fix.

[Andrew Cagney]

> On Jun 14, 12:44pm, Don Howard wrote:
> 
> 
>> The strings are arbitrary expressions and are converted to address via
>> parse_and_eval_address(), which does not flag overflow:
>> 
>> mem_command (char *args, int from_tty)
>> {
>>   CORE_ADDR lo, hi;
>>   char *tok;
>>   struct mem_attrib attrib;
>> 
>>   if (!args)
>>     error_no_arg ("No mem");
>> 
>>   tok = strtok (args, " \t");
>>   if (!tok)
>>     error ("no lo address");
>>   lo = parse_and_eval_address (tok);
>> 
>>   tok = strtok (NULL, " \t");
>>   if (!tok)
>>     error ("no hi address");
>>   hi = parse_and_eval_address (tok);
>> 
>> mabe parse_and_eval_address could detect overflow and throw an error().

On real hardware, addresses overflow causes it to wrap.  The problem of 
signed vs unsigned addresses is also lurking in there as well.

 From memory there is a tabled proposal to add a CORE_ADDR alu object so 
that CORE_ADDR arrithmetic is correct.

> Maybe I'm missing something, but it seems to me that you're still left
> with the problem of how to represent the maximum address + 1.  (Throwing
> an error doesn't really help, does it?)
> 
> 
>> Another possiblity is that the interface could be changed, making the
>> upper bound inclusive also. 
> 
> 
> This sounds better.
> 
> So, on a 16 bit machine, you could say
> 
>     mem 0xf000 0xffff ro
> 
> to indicate that the top 4096 bytes are read-only.

I can think of three alternatives:

	[base, bound)
	[base, bound]
	[base, base+size-1)

The first one is what the doco says and has been there for a while so I 
don't think that changing it is a good idea.

Internally, I suspect base+size-1 is the best representation.  However, 
for the user interface, is there anything that really says that:

	mem 0xfffffff0 0

is either illegal or poorly defined?  Perhaphs allow that and not stuff 
like:

	mem 0xfffffff0 0xffffffff0

Andrew

Re: [Patch] Another small memattr fix.

[Don Howard]

On Fri, 14 Jun 2002, Andrew Cagney wrote:

> > On Jun 14, 12:44pm, Don Howard wrote:
> > 
> > 
> >> The strings are arbitrary expressions and are converted to address via
> >> parse_and_eval_address(), which does not flag overflow:
> >> 
> >> mem_command (char *args, int from_tty)
> >> {
> >>   CORE_ADDR lo, hi;
> >>   char *tok;
> >>   struct mem_attrib attrib;
> >> 
> >>   if (!args)
> >>     error_no_arg ("No mem");
> >> 
> >>   tok = strtok (args, " \t");
> >>   if (!tok)
> >>     error ("no lo address");
> >>   lo = parse_and_eval_address (tok);
> >> 
> >>   tok = strtok (NULL, " \t");
> >>   if (!tok)
> >>     error ("no hi address");
> >>   hi = parse_and_eval_address (tok);
> >> 
> >> mabe parse_and_eval_address could detect overflow and throw an error().
> 





> On real hardware, addresses overflow causes it to wrap.  The problem of 
> signed vs unsigned addresses is also lurking in there as well.
> 
>  From memory there is a tabled proposal to add a CORE_ADDR alu object so 
> that CORE_ADDR arrithmetic is correct.
> 
> > Maybe I'm missing something, but it seems to me that you're still left
> > with the problem of how to represent the maximum address + 1.  (Throwing
> > an error doesn't really help, does it?)
> > 
> > 
> >> Another possiblity is that the interface could be changed, making the
> >> upper bound inclusive also. 
> > 
> > 
> > This sounds better.
> > 
> > So, on a 16 bit machine, you could say
> > 
> >     mem 0xf000 0xffff ro
> > 
> > to indicate that the top 4096 bytes are read-only.
> 
> I can think of three alternatives:
> 
> 	[base, bound)
> 	[base, bound]
> 	[base, base+size-1)
> 
> The first one is what the doco says and has been there for a while so I 
> don't think that changing it is a good idea.
> 
> Internally, I suspect base+size-1 is the best representation.  However, 
> for the user interface, is there anything that really says that:
> 
> 	mem 0xfffffff0 0
>
> is either illegal or poorly defined?  


The fact that the first bound is inclusive and the second is exclusive
implies that to me. Also, the current implemntation enforces it.


How's this: let the parser find the size of the region for us:

labs (parse_and_evaluate_long (tok1 " - " tok2));


That seems to avoid the max int problem.  Then we can use base and size
as the internal representation.




-- 
dhoward@redhat.com
gdb engineering

Re: [Patch] Another small memattr fix.

[Andrew Cagney]

>> I can think of three alternatives:
>> 
>> 	[base, bound)
>> 	[base, bound]
>> 	[base, base+size-1)

Try [base, base+(size-1)]

(and the paren are important :-)

>> 
>> The first one is what the doco says and has been there for a while so I 
>> don't think that changing it is a good idea.
>> 
>> Internally, I suspect base+size-1 is the best representation.  However, 
>> for the user interface, is there anything that really says that:
>> 
>> 	mem 0xfffffff0 0
>>
>> is either illegal or poorly defined?  
> 
> 
> 
> The fact that the first bound is inclusive and the second is exclusive
> implies that to me. Also, the current implemntation enforces it.

Don, sorry, I'm not sure what you mean here.


> How's this: let the parser find the size of the region for us:
> 
> labs (parse_and_evaluate_long (tok1 " - " tok2));

I think it is better to evaluate low/high and then compute the range 
directly.

I wouldn't trust the above expression to always do what you want.

> That seems to avoid the max int problem.  Then we can use base and size
> as the internal representation.

No matter what is done I think there will be an edge condition.  For 
instance:

[base, base+(size-1)]

doesn't work very well when base==0 :-)

Andrew

Re: [Patch] Another small memattr fix.

[Don Howard]

On Sat, 15 Jun 2002, Andrew Cagney wrote:

> 
> >> I can think of three alternatives:
> >> 
> >> 	[base, bound)
> >> 	[base, bound]
> >> 	[base, base+size-1)
> 
> Try [base, base+(size-1)]
> 
> (and the paren are important :-)
> 
> >> 
> >> The first one is what the doco says and has been there for a while so I 
> >> don't think that changing it is a good idea.
> >> 
> >> Internally, I suspect base+size-1 is the best representation.  However, 
> >> for the user interface, is there anything that really says that:
> >> 
> >> 	mem 0xfffffff0 0
> >>
> >> is either illegal or poorly defined?  
> > 
> > 
> > 
> > The fact that the first bound is inclusive and the second is exclusive
> > implies that to me. Also, the current implemntation enforces it.
> 
> Don, sorry, I'm not sure what you mean here.

The first bound == lower bound, second == upper (in the current
implementation).  The first/lower is included in the range, while the 
second/upper is excluded.

I suppose it's gdb could allow the upper or the low in either position, 
but this would be susceptible to the overflow problem.


> > How's this: let the parser find the size of the region for us:
> > 
> > labs (parse_and_evaluate_long (tok1 " - " tok2));
> 
> I think it is better to evaluate low/high and then compute the range 
> directly.
> 

This is where the maxint problem shows up, and is precisely what I was 
trying to avoid.  There is no way to detect overflow.  


> I wouldn't trust the above expression to always do what you want.

When would it not?  Maybe the string should be:

"(" tok1 ") - (" tok2 ")"  ?

Is there something about the expression parser that I am missing?

> 
> > That seems to avoid the max int problem.  Then we can use base and size
> > as the internal representation.
> 
> No matter what is done I think there will be an edge condition.  For 
> instance:
> 
> [base, base+(size-1)]
> 
> doesn't work very well when base==0 :-)

and size == 0, so don't permit size == 0.

> 
> Andrew
> 
> 

-- 
dhoward@redhat.com
gdb engineering