Re: [PATCH 00/25 V2] Make GDB builtin target descriptions more flexible

[Pedro Alves <palves@redhat.com>]

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