From: Kamil Rytarowski <n54@gmx.com>
To: Simon Marchi <simark@simark.ca>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Implement IP_STAT on NetBSD
Date: Mon, 13 Apr 2020 21:43:15 +0200 [thread overview]
Message-ID: <4671cd7b-b081-1aad-88c3-f40aaefa38f5@gmx.com> (raw)
In-Reply-To: <64ef8889-0782-a541-6efa-3c37ed06b372@simark.ca>
On 13.04.2020 21:16, Simon Marchi wrote:
> On 2020-04-13 2:19 p.m., Kamil Rytarowski wrote:
>> NetBSD ships with a compatibility layer in a /proc virtual file system
>> and delivers optionally /proc/<pid>/stat. In case of missing
>> /proc/<pid>/stat fallback to emulating it with sysctl(3) APIs.
>>
>> gdb/ChangeLog:
>>
>> * nbsd-nat.c: Include "sys/resource.h" and "machine/vmparam.h".
>> (nbsd_pid_to_kinfo_proc2, nbsd_pid_to_rlimit_cur)
>> (nbsd_pid_to_statstr): New.
>> (nbsd_nat_target::info_proc): Add do_stat.
>> ---
>> gdb/ChangeLog | 7 ++
>> gdb/nbsd-nat.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 285 insertions(+)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 7d91abf6334..5ff76e85cb3 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2020-04-13 Kamil Rytarowski <n54@gmx.com>
>> +
>> + * nbsd-nat.c: Include "sys/resource.h" and "machine/vmparam.h".
>> + (nbsd_pid_to_kinfo_proc2, nbsd_pid_to_rlimit_cur)
>> + (nbsd_pid_to_statstr): New.
>> + (nbsd_nat_target::info_proc): Add do_stat.
>> +
>> 2020-04-12 Kamil Rytarowski <n54@gmx.com>
>>
>> * nbsd-nat.c (nbsd_nat_target::info_proc): Add IP_MINIMAL and
>> diff --git a/gdb/nbsd-nat.c b/gdb/nbsd-nat.c
>> index 5eaf9dec8af..b2a05d63122 100644
>> --- a/gdb/nbsd-nat.c
>> +++ b/gdb/nbsd-nat.c
>> @@ -28,6 +28,8 @@
>> #include <sys/types.h>
>> #include <sys/ptrace.h>
>> #include <sys/sysctl.h>
>> +#include <sys/resource.h>
>> +#include <machine/vmparam.h>
>>
>> /* Return the name of a file that can be opened to get the symbols for
>> the child process identified by PID. */
>> @@ -58,6 +60,134 @@ nbsd_pid_to_cwd (int pid)
>> return buf;
>> }
>>
>> +/* Return the kinfo_proc2 structure for the process identified by PID. */
>> +
>> +static bool
>> +nbsd_pid_to_kinfo_proc2(pid_t pid, struct kinfo_proc2 *ki)
>
> Space.
>
OK
>> +{
>> + gdb_assert (ki != nullptr);
>> +
>> + size_t size = sizeof (*ki);
>> + int mib[6] = {CTL_KERN, KERN_PROC2, KERN_PROC_PID, pid,
>> + static_cast<int> (size), 1};
>> + return sysctl (mib, ARRAY_SIZE (mib), ki, &size, NULL, 0);
>
> That looks counter-intuitive. sysctl returns 0 on success, which translates to
> false as a bool. WHen a function returns a bool it is more typical that true
> means success. This happens lower too.
>
OK
>> +}
>> +
>> +/* Return the RLIMIT value for the process identified by PID. */
>> +
>> +static bool
>> +nbsd_pid_to_rlimit_cur(pid_t pid, rlim_t *rlim, int type)
>
> Space.
>
OK
>> +{
>> + gdb_assert (rlim != nullptr);
>> +
>> + size_t size = sizeof (*rlim);
>> + int mib[5] = {CTL_PROC, pid, PROC_PID_LIMIT, type, PROC_PID_LIMIT_TYPE_SOFT};
>> + return sysctl (mib, ARRAY_SIZE (mib), rlim, &size, NULL, 0);
>> +}
>> +
>> +/* Return the emulated /proc/#/stat string for the process identified by PID.
>> + NetBSD keeps the /proc filesystem that is optional and not recommended for
>> + new software, thus this call emulates it with sysctl(3). */
>> +
>> +static gdb::unique_xmalloc_ptr<char>
>> +nbsd_pid_to_statstr (int pid)
>> +{
>> + struct kinfo_proc2 ki;
>> + if (nbsd_pid_to_kinfo_proc2 (pid, &ki))
>> + return nullptr;
>> +
>> + struct rusage cru = {};
>> + /* We can check only the current process. */
>> + if (getpid() == pid)
>
> Space.
>
OK
>> @@ -433,6 +568,149 @@ nbsd_nat_target::info_proc (const char *args, enum info_proc_what what)
>> else
>> warning (_("unable to fetch virtual memory map"));
>> }
>> + if (do_stat)
>> + {
>> + /* First, try with Linux-compat /proc/<pid>/stat. */
>> + char filename[100];
>> + xsnprintf (filename, sizeof filename, "/proc/%d/stat", pid);
>
> Use
>
> std::string filename = string_printf (...);
>
OK. Meanwhile linux-tdep.c could be upgraded by a Linux maintainer.
>> + gdb::unique_xmalloc_ptr<char> statstr
>> + = target_fileio_read_stralloc (NULL, filename);
>> +
>> + /* Then fallback to emulating /proc/<pid>/stat with sysctl(3). */
>> + if (statstr == nullptr)
>> + statstr = nbsd_pid_to_statstr (pid);
>
> I find it a bit strange to take the route of formatting the contents of kinfo_proc2,
> only to parse it here. I think it would have been more natural to do:
>
> - If /proc/#/stat is available: string -> struct some_internal_struct -> print function
> - If /proc/#/stat is available: kinfo_proc2 -> struct some_internal_struct -> print function
>
This compat shim is only to emulate Linux... it's possible to follow the
FreeBSD case and just print local kinfo_proc2 as-is (that is fully
OS-specific).
What do you think?
The same question applies to IP_STATUS.
>> +
>> + if (statstr)
>
> != nullptr
>
OK
>> + {
>> + const char *p = statstr.get ();
>> +
>> + printf_filtered (_("Process: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> +
>> + p = skip_spaces (p);
>> + if (*p == '(')
>> + {
>> + /* ps command also relies on no trailing fields
>> + ever contain ')'. */
>> + const char *ep = strrchr (p, ')');
>> + if (ep != NULL)
>> + {
>> + printf_filtered ("Exec file: %.*s\n",
>> + (int) (ep - p - 1), p + 1);
>> + p = ep + 1;
>> + }
>> + }
>> +
>> + p = skip_spaces (p);
>> + if (*p)
>> + printf_filtered (_("State: %c\n"), *p++);
>> +
>> + if (*p)
>> + printf_filtered (_("Parent process: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Process group: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Session id: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("TTY: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("TTY owner process group: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> +
>> + if (*p)
>> + printf_filtered (_("Flags: %s\n"),
>> + hex_string (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Minor faults (no memory page): %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Minor faults, children: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Major faults (memory page faults): %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Major faults, children: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("utime: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("stime: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("utime, children: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("stime, children: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("jiffies remaining in current "
>> + "time slice: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("'nice' value: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("jiffies until next timeout: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("jiffies until next SIGALRM: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("start time (jiffies since "
>> + "system boot): %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Virtual memory size: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Resident set size: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("rlim: %s\n"),
>> + pulongest (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Start of text: %s\n"),
>> + hex_string (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("End of text: %s\n"),
>> + hex_string (strtoulst (p, &p, 10)));
>> + if (*p)
>> + printf_filtered (_("Start of stack: %s\n"),
>> + hex_string (strtoulst (p, &p, 10)));
>> +#if 0 /* Don't know how architecture-dependent the rest is...
>> + Anyway the signal bitmap info is available from "status". */
>
> Either figure out how to print this correctly, or let's just leave it out.
>
This comment was copied as-is from gdb/linux-tdep.c.
> Simon
>
next prev parent reply other threads:[~2020-04-13 19:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-13 18:19 Kamil Rytarowski
2020-04-13 19:16 ` Simon Marchi
2020-04-13 19:43 ` Kamil Rytarowski [this message]
2020-04-13 20:11 ` Simon Marchi
2020-04-13 20:26 ` Kamil Rytarowski
2020-04-13 20:43 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4671cd7b-b081-1aad-88c3-f40aaefa38f5@gmx.com \
--to=n54@gmx.com \
--cc=gdb-patches@sourceware.org \
--cc=simark@simark.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox