Re: [PATCH] Harden tests that deal with memory regions

[Pedro Alves <palves@redhat.com>]

On 01/26/2017 05:37 PM, Luis Machado wrote:
> On 01/26/2017 07:17 AM, Pedro Alves wrote:
>> On 01/23/2017 09:24 PM, Luis Machado wrote:
>>
>>> 2017-01-23  Luis Machado  <lgustavo@codesourcery.com>
>>>
>>>     * lib/gdb-memory.exp: New file.
>>
>> Do we need "gdb-" in the file name?
>>
>> What other procedures to you envision being placed here?  Should
>> this have "regions" in the file name, like "memory-regions.exp"?
>> The file's intro comment talks about memory regions.
>>
> 
> I guess we don't really need the gdb prefix. I originally envisioned
> this particular file storing all proc's dealing with memory checks and
> manipulation (though i ended up describing it in a different way).

So can you drop it?

> I wanted to avoid having to add more helper functions to lib/gdb.exp.
> But maybe it wouldn't be a big problem? My instinct is to modularize it.

Sure, I'm not arguing against modularizing.  Only against calling it "memory",
but describing it as "memory ranges".  I'm arguing for picking one
and being consistent throughout.

> Either way is fine with me though, lib/gdb.exp or lib/memory.exp.
> 
>>>     * lib/gdb.exp: Load gdb-memory.exp
>>
>> Missing period.
>>
> 
> Thanks.
> 
>>> --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
>>> +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
>>> @@ -137,6 +137,9 @@ if ![get_function_bounds "main" main_lo main_hi] {
>>>      return -1
>>>  }
>>>
>>> +# Delete all memory regions.
>>> +delete_memory_regions
>>> +
>>
>> The comment as-is practically just reads the function name
>> in English.  The important detail missing here
>> is "target-supplied".  So:
>>
>>  # Delete all target-supplied memory regions.
>>  delete_memory_regions
>>
>> Likewise in the other spot.
>>
> 
> On second thought, i've pulled these comments from the test files. The
> updated proc documentation should be enough. What do you think?

I don't think so.  The important detail is that you call it
_here_, right after starting the target to get rid of
any target-supplied memory region.  While the procedure could
be called at any other point, to delete user-defined regions,
even.

>>> +
>>> +# This file holds functions and data dealing with memory regions
>>> manipulation.
>>> +
>>> +# Deletes all the memory regions GDB currently knows about.
>>> +
>>> +proc delete_memory_regions {} {
> 
> I've added the target-supplied bit to this as well.

That doesn't sound right.  The procedure deletes all memory
regions, either target-supplied, or user defined.

Thanks,
Pedro Alves