Re: [PATCH] Use kinfo_getvmmap () on FreeBSD to enumerate memory regions.

[Pedro Alves <palves@redhat.com>]

On 03/06/2015 09:06 PM, John Baldwin wrote:
> On Friday, March 06, 2015 07:49:23 PM Pedro Alves wrote:
>> Hi John,
>>
>> Looks good to me, with a few nits/suggestions below.
>>
>> On 02/26/2015 09:44 PM, John Baldwin wrote:
>>> Use kinfo_getvmmap () from libutil on FreeBSD to enumerate memory
>>> regions in a running process instead of /proc/<pid>/map.  FreeBSD systems
>>> do not mount procfs by default, but kinfo_getvmmap () uses a sysctl that
>>> is always available.
>>>
>>> Skip memory regions for devices as well as regions an application has
>>> requested to not be dumped via the MAP_NOCORE flag to mmap () or
>>> MADV_NOCORE advice to madvise ().
>>
>> Note that GNU's coding conventions tell us to refer to functions by
>> name without the ()'s.
> 
> Ok.
> 
>>> gdb/ChangeLog:
>>> 	* configure.ac: AC_CHECK_LIB(util, kinfo_getvmmap).
>>> 	* configure: Regenerate.
>>> 	* config.in: Regenerate.
>>> 	* fbsd-nat.c (fbsd_find_memory_regions): Use kinfo_getvmmap to
>>> 	enumerate memory regions if present.
>>
>>  	* fbsd-nat.c [!HAVE_KINFO_GETVMMAP] (fbsd_read_mapping): Don't
>> 	define.
>>  	(fbsd_find_memory_regions): Use kinfo_getvmmap to
>>  	enumerate memory regions if present.
> 
> Ok.
> 
>>> ---
>>>
>>>  gdb/configure.ac |  5 +++++
>>>  gdb/fbsd-nat.c   | 56
>>>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files
>>>  changed, 61 insertions(+)
>>>
>>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>>> index 6ac8adb..b094164 100644
>>> --- a/gdb/configure.ac
>>> +++ b/gdb/configure.ac
>>> @@ -532,6 +532,11 @@ AM_ZLIB
>>>
>>>  # On HP/UX we may need libxpdl for dlgetmodinfo (used by solib-pa64.c).
>>>  AC_SEARCH_LIBS(dlgetmodinfo, [dl xpdl])
>>>
>>> +# On FreeBSD we may need libutil for kinfo_getvmmap (used by fbsd-nat.c).
>>> +AC_CHECK_LIB(util, kinfo_getvmmap,
>>> +  [AC_DEFINE(HAVE_KINFO_GETVMMAP, 1,
>>> +            [Define to 1 if your system has the kinfo_getvmmap function.
>>> ])]) +
>>
>> Isn't
>>
>>  AC_SEARCH_LIBS(kinfo_getvmmap, [util])
>>
>> pretty much the same?
> 
> Might be, I wasn't sure from reading the autoconf docs that this would add the 
> appropriate define.  I'll certainly try it and if it works I'm happier to use 
> the simpler form.
> 
>> (Note: please make sure to use pristine GNU autoconf 2.64 when generating
>> the files, to avoid spurious odd differences coming out.  Some distros
>> carry local patches that result in that, dunno about FreeBSD.)
> 
> Yes, I had to download my own copy to use when I tested this locally.
> 
>>> +  vmentl = kinfo_getvmmap (pid, &nitems);
>>> +  if (vmentl == NULL)
>>> +    perror_with_name (_("Couldn't fetch VM map entries."));
>>> +  cleanup = make_cleanup (free, vmentl);
>>
>> s/free/xfree/g.
> 
> I wasn't sure about this one.  kinfo_getvmmap() calls malloc() from libc 
> internally (so it isn't using xmalloc() to allocate the memory that is 
> returned).  Is it still correct to use xfree() instead of free() in that case?

Ah, I just saw bare "free" and didn't realize that it was kinfo_getvmmap
that malloc the memory.  Yeah, free is correct then.  Sorry for the noise.

> 
>  > @@ -136,4 +191,5 @@ fbsd_find_memory_regions (struct target_ops *self,
>>>
>>>    do_cleanups (cleanup);
>>>    return 0;
>>>
>>> +#endif
>>>
>>>  }
>>
>> I'd suggest splitting fbsd_find_memory_regions in two instead of the
>> big #if/#else/#endif, but that's just personal preference.
> 
> Do you mean an #if/#else/#endif around the entire function vs just the body or 
> do you mean something else?  If the former, I can easily do that (and collapse 
> down to on #if/#else/#endif with the prior conditionally-defined function).

Yeah, that.

Thanks,
Pedro Alves