This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 00/25 V2] Make GDB builtin target descriptions more flexible
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, gdb-patches at sourceware dot org
- Date: Wed, 28 Jun 2017 20:06:45 +0100
- Subject: Re: [PATCH 00/25 V2] Make GDB builtin target descriptions more flexible
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 51205C0467D7
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 51205C0467D7
- References: <1497256916-4958-1-git-send-email-yao.qi@linaro.org>
Hi Yao,
In general, I'm happy with the direction of the series.
I was having a bit of trouble grokking the individual patches,
because of missing comments, etc. I expect this will all see
more polish in v3.
So diffing the whole series instead, here are some comments.
Nothing major, though I don't claim that I've grokked
everything, yet.
-- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -757,7 +757,7 @@ x86_linux_read_description (void)
{
have_ptrace_getfpxregs = 0;
have_ptrace_getregset = 0;
- return tdesc_i386_mmx_linux;
+ return i386_get_ipa_tdesc (X86_TDESC_MMX);
}
else
have_ptrace_getfpxregs = 1;
Seems odd that these functions have "_ipa_" in their
name when they're not IPA specific.
int
+i386_get_ipa_tdesc_idx (const struct target_desc *tdesc)
+{
+ for (int i = 0; i < X86_TDESC_LAST; i++)
+ {
+ if (tdesc == i386_tdescs[i])
+ return i;
+ }
+
+ return 0;
+}
Do you really want to return 0? That's a valid index:
enum x86_linux_tdesc {
X86_TDESC_MMX = 0,
...
@@ -3639,6 +3645,11 @@ captured_main (int argc, char *argv[])
startup_with_shell = false;
else if (strcmp (*next_arg, "--once") == 0)
run_once = 1;
+ else if (strcmp (*next_arg, "--self-test") == 0)
+ {
+ selftest = true;
+ break;
+ }
This "break" means that
$ gdbserver --self-self asdfausidpoasduifa
works. Is that expected? Might be useful to pass down
the remaining arguments to the self tests infrastructure,
but you're not doing that.
- Missing "m_" prefix in private fields.
- Missing comments throughout; function intros, struct fields, etc.
- Missing empty lines between functions in several places.
- Nit but I think you should be able to replace the
"std::pair" uses with unnamed structs, avoiding the
mostly meaningless "first" vs "second":
- std::pair<type1, type2> array_name[] = ...
+ struct { type1 field1; type2 field2; } array_name[] = ...
- Can we do something about the #ifdefery in the selftests framework.
Thanks,
Pedro Alves