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

Luis Machado
2017-01-26T18:06:29Z

On 01/26/2017 11:47 AM, Pedro Alves wrote:
> 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.
>

Great. I've picked lib/memory.exp as the name in v3.

>> 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.
>

I've reverted this in v3 and adjusted the comments to be more sensible
depending on the context of the call.

>>>> +
>>>> +# 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
>

Fixed.

Thanks,
Luis