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

[Tom Tromey <tom@tromey.com>]

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

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.

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

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.

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?

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?

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

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.

Tom