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

Kamil Rytarowski
2020-03-20T16:59:42Z

[-- 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 --]