Re: [PATCH] Implement "info proc mappings" for NetBSD

[Kamil Rytarowski <n54@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3411 bytes --]

On 20.03.2020 16:36, Tom Tromey wrote:
>>>>>> "Kamil" == Kamil Rytarowski <n54@gmx.com> writes:
> 
> Kamil> Define nbsd_nat_target::find_memory_regions and
> Kamil> nbsd_nat_target::info_proc. info_proc handles as of now only
> Kamil> the "mappings" command.
> 
> Thank you for the patch.
> 
> Kamil> +/* Retrieve all the memory regions in the specified process.  */
> Kamil> +
> Kamil> +static struct kinfo_vmentry *
> Kamil> +kinfo_get_vmmap(pid_t pid, size_t *size)
> 
> Space before paren.
> There are multiple instances of this in the patch.
> 

Done.

> Kamil> +{
> Kamil> +  int mib[5] = {CTL_VM, VM_PROC, VM_PROC_MAP, pid, sizeof(struct kinfo_vmentry)};
> Kamil> +
> Kamil> +  size_t length = 0;
> Kamil> +  if (sysctl(mib, ARRAY_SIZE (mib), NULL, &length, NULL, 0)) {
> 
> GNU style is to put the open brace on its own line.
> There are several cases of this as well.
> 

Done.

> Kamil> +  if (sysctl(mib, ARRAY_SIZE (mib), kiv, &length, NULL, 0)) {
> Kamil> +    *size = 0;
> Kamil> +    free(kiv);
> 
> xfree, though these days in gdb it's better to create and return a
> self-managing object, like gdb::unique_xmalloc_ptr<>.
> 

Done.

> Kamil> +
> Kamil> +int
> Kamil> +nbsd_nat_target::find_memory_regions (find_memory_region_ftype func,
> Kamil> +				      void *obfd)
> 
> I wouldn't name that parameter "obfd" but rather "data" or "user_data"
> or something like that.  The name "obfd" in gdb normally implies that it
> is a "BFD *", but this is just opaque data that's shared by the caller
> and the callback.
> 

I see. I prefer to keep this called obfd, as it matches the FreeBSD code
more closely. If we want to rename it, we shall do it also in FreeBSD.

> Kamil> diff --git a/gdb/nbsd-tdep.c b/gdb/nbsd-tdep.c
> [...]
> Kamil> +/* Flags in the 'kve_protection' field in struct kinfo_vmentry.  These
> Kamil> +   match the KVME_PROT_* constants in <sys/sysctl.h>.  */
> 
> What if they stop matching?
> 

Not a concern.

If that will ever change, it will be patched, same as the other existing
NetBSD support.

> I wonder if this code really ought to be in the tdep file.  I suppose
> so, if it will be used somehow when cross debugging... but will that
> happen?
> 

This code follows the structure of FreeBSD.

> If not then maybe putting it in the -nat file is better; and then the
> appropriate NetBSD headers could be used.
> 

I prefer to keep similarity with FreeBSD. If we want to change it,
FreeBSD shall be changed too.

> Kamil> +  printf_filtered (_("Mapped address spaces:\n\n"));
> Kamil> +  if (addr_bit == 64)
> Kamil> +    {
> Kamil> +      printf_filtered ("  %18s %18s %10s %10s %9s %s\n",
> Kamil> +		       "Start Addr",
> Kamil> +		       "  End Addr",
> Kamil> +		       "      Size", "    Offset", "Flags  ", "File");
> Kamil> +    }
> Kamil> +  else
> Kamil> +    {
> Kamil> +      printf_filtered ("\t%10s %10s %10s %10s %9s %s\n",
> Kamil> +		       "Start Addr",
> Kamil> +		       "  End Addr",
> Kamil> +		       "      Size", "    Offset", "Flags  ", "File");
> 
> Personally I like using ui-out tables instead of this kind of thing.
> 

I will keep this as is. If we want to change it, we shall change FreeBSD
in one go.

I don't have chance to work or test on FreeBSD myself so I defer
refactoring it into future.

Please see v2.

> Tom
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]