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

Tom Tromey
2020-03-20T15:36:24Z

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