Re: [PATCH 3/4] Support 'info proc' for native FreeBSD processes.

Simon Marchi
2018-01-03T19:13:34Z

Hi John,

>> > +#ifdef HAVE_KINFO_GETVMMAP
>> > + if (do_mappings)
>> > + {
>> > + int nvment;
>> > + std::unique_ptr<struct kinfo_vmentry, free_deleter<struct kinfo_vmentry>>
>>
>> Is there a reason to have and use free_deleter rather than
>> gdb::unique_xmalloc_ptr?
>>
>> > + vmentl (kinfo_getvmmap (pid, &nvment));
>
> This function (kinfo_getvmmap) which is defined in the libutil library
> included in
> FreeBSD's base system calls malloc() internally, so the memory returned
> must be
> freed with free() rather than xfree(). This deleter is already used
> earlier in
> fbsd_find_memory_regions() for another call to kinfo_getvmmap() for
> the same reason.

But isn't xfree just a wrapper around free?

>> > +
>> > + if (vmentl)
>>
>> vmentl != NULL
>>
>> There are a few other instances of if (ptr) that should be changed to
>> if (ptr != NULL).
>
> Ok. The style in GDB doesn't seem consistent in this regard (I
> largely based this
> code on the 'info proc' implementation in linux-tdep.c which doesn't
> explicitly
> compare against NULL/nullptr (though I prefer the explicit comparison
> myself)).
> Also, I feel like we should use nullptr rather than NULL when working
> with "smart"
> pointer types like unique_ptr<> at least?

You are right, there is code that comes from an era where the coding
style wasn't as strictly enforced, it seems. I don't think we should go
fix coding style issues just for fun, but when we modify or copy
existing code, we should make it respect the style.

Thanks,

Simon