Re: [PATCH v2 10/10] Add minimal and functional NetBSD/amd64 gdbserver

Kamil Rytarowski
2020-09-08T00:06:53Z

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

On 07.09.2020 21:58, Simon Marchi wrote:
> I did a superficial review, that's as much as I can, since I can't easily test this. I also don't
> know gdbserver by heart, so I can't tell off the top of my head if some method implementation doesn't
> match what's expected, for example.
>
> So I checked for obvious mistakes (didn't find any) and coding standards (looks pretty good). Since
> this is new code, I am not too worried, it won't break any existing use case. The worst that can happen
> is that there are bugs and we fix them later.
>

Thank you for the review! Right, there could be bugs and there is room
for improvement and adding new features. Nonetheless, this code already
works.

> On 2020-09-03 8:29 p.m., Kamil Rytarowski wrote:
>> Implement the following functionality: create_inferior,
>> post_create_inferior, attach, kill, detach, mourn, join, thread_alive,
>> resume, wait, fetch_registers, store_registers, read_memory, write_memory,
>> request_interrupt, supports_read_auxv, read_auxv,
>> supports_hardware_single_step, sw_breakpoint_from_kind,
>> supports_z_point_type, insert_point, remove_point,
>> stopped_by_sw_breakpoint, supports_qxfer_siginfo, qxfer_siginfo,
>> supports_stopped_by_sw_breakpoint, supports_non_stop,
>> supports_multi_process, supports_fork_events, supports_vfork_events,
>> supports_exec_events, supports_disable_randomization,
>> supports_qxfer_libraries_svr4, qxfer_libraries_svr4,
>> supports_pid_to_exec_file, pid_to_exec_file, thread_name,
>> supports_catch_syscall.
>>
>> The only CPU architecture supported: x86_64.
>>
>> Implement only support for hardware assisted single step and
>> software breakpoint.
>>
>> Implement support only for regular X86 registers, thus no FPU.
>>
>> gdbserver/ChangeLog:
>>
>> * netbsd-low.cc: Add.
>> * netbsd-low.h: Likewise.
>> * netbsd-x86_64-low.cc: Likewise.
>
> Let's name this new file netbsd-amd64-low.cc. We use amd64 everywhere, so it's
> better to stay consistent.
>

OK

>> +/* Implement the fetch_registers target_ops method. */
>> +
>> +void
>> +netbsd_process_target::fetch_registers (struct regcache *regcache, int regno)
>> +{
>> + struct netbsd_regset_info *regset = netbsd_target_regsets;
>> + ptid_t inferior_ptid = ptid_of (current_thread);
>> +
>> + while (regset->size >= 0)
>> + {
>> + char *buf = (char *) xmalloc (regset->size);
>> + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf,
>> + inferior_ptid.lwp ());
>> + if (res == -1)
>> + perror_with_name (("ptrace"));
>> + regset->store_function (regcache, buf);
>> + free (buf);
>
> The perror would leak `buf`. Use some automatic memory management type.
>

OK

>> + regset++;
>> + }
>> +}
>> +
>> +/* Implement the store_registers target_ops method. */
>> +
>> +void
>> +netbsd_process_target::store_registers (struct regcache *regcache, int regno)
>> +{
>> + struct netbsd_regset_info *regset = netbsd_target_regsets;
>> + ptid_t inferior_ptid = ptid_of (current_thread);
>> +
>> + while (regset->size >= 0)
>> + {
>> + char *buf = (char *)xmalloc (regset->size);
>> + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf,
>> + inferior_ptid.lwp ());
>> + if (res == -1)
>> + perror_with_name (("ptrace"));
>> + if (res == 0)
>> + {
>> + /* Then overlay our cached registers on that. */
>> + regset->fill_function (regcache, buf);
>> + /* Only now do we write the register set. */
>> + res = ptrace (regset->set_request, inferior_ptid.pid (), buf,
>> + inferior_ptid.lwp ());
>> + }
>> + free (buf);
>
> Same here.
>

OK

>> +/* Extract &phdr and num_phdr in the inferior. Return 0 on success. */
>> +
>> +template <typename T>
>> +int get_phdr_phnum_from_proc_auxv (const pid_t pid,
>> + CORE_ADDR *phdr_memaddr, int *num_phdr)
>> +{
>> + typedef typename std::conditional<sizeof(T) == sizeof(int64_t),
>> + Aux64Info, Aux32Info>::type auxv_type;
>> + const size_t auxv_size = sizeof (auxv_type);
>> + const size_t auxv_buf_size = 128 * sizeof (auxv_type);
>> +
>> + char *auxv_buf = (char *) xmalloc (auxv_buf_size);
>> +
>> + netbsd_read_auxv (pid, nullptr, auxv_buf, auxv_buf_size);
>> +
>> + *phdr_memaddr = 0;
>> + *num_phdr = 0;
>> +
>> + for (char *buf = auxv_buf; buf < (auxv_buf + auxv_buf_size); buf += auxv_size)
>> + {
>> + auxv_type *const aux = (auxv_type *) buf;
>> +
>> + switch (aux->a_type)
>> + {
>> + case AT_PHDR:
>> + *phdr_memaddr = aux->a_v;
>> + break;
>> + case AT_PHNUM:
>> + *num_phdr = aux->a_v;
>> + break;
>> + }
>> +
>> + if (*phdr_memaddr != 0 && *num_phdr != 0)
>> + break;
>> + }
>> +
>> + xfree (auxv_buf);
>
> Same here.
>

OK

>> +struct regcache;
>> +struct target_desc;
>> +
>> +/* Some information relative to a given register set. */
>> +
>> +struct netbsd_regset_info
>> +{
>> + /* The ptrace request needed to get/set registers of this set. */
>> + int get_request, set_request;
>> + /* The size of the register set. */
>> + int size;
>> + /* Fill the buffer BUF from the contents of the given REGCACHE. */
>> + void (*fill_function) (struct regcache *regcache, char *buf);
>> + /* Store the register value in BUF in the given REGCACHE. */
>> + void (*store_function) (struct regcache *regcache, const char *buf);
>
> I find the distinction between "fill" and "store" unclear, it doesn't really tell
> which direction it's going (looking at the types helps though). fill the regcache?
> fill the buffer? store in the regcache? store in the buffer?
>
> It's the same with the regcache::supply_regset and regcache::collect_regset methods
> in GDB... I never know what supply and collect mean. Are they from the point of view
> of the regset? Or from the point of view of the user of the regset?
>
> What about:
>
> void (*buf_to_regcache) (struct regcache *regcache, char *buf);
> void (*regcache_to_buf) (struct regcache *regcache, const char *buf);
>
> You could add the `_function` suffix if you prefer, though I don't think it's necessary.
>

I will leave the naming as it is in this patch as it exists in Linux and
some generic code. I have got no opinion on renaming, if we go for it,
it should happen independently.

> Simon
>

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