Re: [PATCH 1/2] Display ExceptionRecord for $_siginfo

["Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>]

 Am Freitag, 7. Februar 2020, 22:56:18 MEZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-01-17 10:31 a.m., Hannes Domani via gdb-patches wrote:
> > diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> > index 901e64263c..824ff4b322 100644
> > --- a/gdb/windows-nat.c
> > +++ b/gdb/windows-nat.c
> > @@ -236,6 +236,7 @@ static DEBUG_EVENT current_event;    /* The current debug event from
> >                        WaitForDebugEvent */
> >  static HANDLE current_process_handle;    /* Currently executing process */
> >  static windows_thread_info *current_thread;    /* Info on currently selected thread */
> > +static EXCEPTION_RECORD siginfo_er;    /* Contents of $_siginfo */
>
> Huh... I was going to say that it shouldn't be a global variable, but a per-inferior
> thing (or is it per-thread?), but pretty much all the state is already global...  so
> I guess it's fine.  I gather that the windows-nat does not support debugging multiple
> inferiors?  Same for win32-low in gdbserver?
>
> >
> >  /* Counts of things.  */
> >  static int exception_count = 0;
> > @@ -1166,6 +1167,8 @@ handle_exception (struct target_waitstatus *ourstatus)
> >    DWORD code = rec->ExceptionCode;
> >    handle_exception_result result = HANDLE_EXCEPTION_HANDLED;
> >
> > +  memcpy (&siginfo_er, rec, sizeof siginfo_er);
> > +
> >    ourstatus->kind = TARGET_WAITKIND_STOPPED;
> >
> >    /* Record the context of the current thread.  */
> > @@ -2862,6 +2865,7 @@ windows_nat_target::mourn_inferior ()
> >        CHECK (CloseHandle (current_process_handle));
> >        open_process_used = 0;
> >      }
> > +  siginfo_er.ExceptionCode = 0;
> >    inf_child_target::mourn_inferior ();
> >  }
> >
> > @@ -2994,6 +2998,28 @@ windows_xfer_shared_libraries (struct target_ops *ops,
> >    return len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;
> >  }
> >
> > +static enum target_xfer_status
> > +windows_xfer_siginfo (gdb_byte *readbuf, ULONGEST offset, ULONGEST len,
> > +              ULONGEST *xfered_len)
> > +{
> > +  if (!siginfo_er.ExceptionCode)
>
> siginfo_er.ExceptionCode != 0
>
> > +    return TARGET_XFER_E_IO;
> > +
> > +  if (!readbuf)
>
> readbuf == nullptr
>
> > +    return TARGET_XFER_E_IO;
> > +
> > +  if (offset > sizeof (siginfo_er))
> > +    return TARGET_XFER_E_IO;
> > +
> > +  if (offset + len > sizeof (siginfo_er))
> > +    len = sizeof (siginfo_er) - offset;
> > +
> > +  memcpy (readbuf, (char *) &siginfo_er + offset, len);
> > +  *xfered_len = len;
> > +
> > +  return TARGET_XFER_OK;
> > +}
> > +
> >  enum target_xfer_status
> >  windows_nat_target::xfer_partial (enum target_object object,
> >                    const char *annex, gdb_byte *readbuf,
> > @@ -3009,6 +3035,9 @@ windows_nat_target::xfer_partial (enum target_object object,
> >        return windows_xfer_shared_libraries (this, object, annex, readbuf,
> >                          writebuf, offset, len, xfered_len);
> >
> > +    case TARGET_OBJECT_SIGNAL_INFO:
> > +      return windows_xfer_siginfo (readbuf, offset, len, xfered_len);
> > +
> >      default:
> >        if (beneath () == NULL)
> >      {
> > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> > index 1fc2748581..50efa31709 100644
> > --- a/gdb/windows-tdep.c
> > +++ b/gdb/windows-tdep.c
> > @@ -153,6 +153,26 @@ static const int FULL_TIB_SIZE = 0x1000;
> >
> >  static bool maint_display_all_tib = false;
> >
> > +static struct gdbarch_data *windows_gdbarch_data_handle;
> > +
> > +struct windows_gdbarch_data
> > +  {
> > +    struct type *siginfo_type;
> > +  };
>
> Unindent the curly braces and the field:
>
> {
>   struct type *siginfo_type;
> }
>
> I know there are some structures formatted this way, but the new ones we add are
> aligned on column 0.

OK, I fill fix all these coding style problems and try to remember them.


> > +
> > +static void *
> > +init_windows_gdbarch_data (struct gdbarch *gdbarch)
> > +{
> > +  return GDBARCH_OBSTACK_ZALLOC (gdbarch, struct windows_gdbarch_data);
> > +}
> > +
> > +static struct windows_gdbarch_data *
> > +get_windows_gdbarch_data (struct gdbarch *gdbarch)
> > +{
> > +  return ((struct windows_gdbarch_data *)
> > +      gdbarch_data (gdbarch, windows_gdbarch_data_handle));
> > +}
> > +
> >  /* Define Thread Local Base pointer type.  */
> >
> >  static struct type *
> > @@ -648,6 +668,43 @@ windows_gdb_signal_to_target (struct gdbarch *gdbarch, enum gdb_signal signal)
> >    return -1;
> >  }
> >
> > +static struct type *
> > +windows_get_siginfo_type (struct gdbarch *gdbarch)
> > +{
> > +  struct windows_gdbarch_data *windows_gdbarch_data;
> > +  struct type *uint_type, *void_ptr_type;
> > +  struct type *siginfo_ptr_type, *siginfo_type;
> > +
> > +  windows_gdbarch_data = get_windows_gdbarch_data (gdbarch);
> > +  if (windows_gdbarch_data->siginfo_type != NULL)
> > +    return windows_gdbarch_data->siginfo_type;
> > +
> > +  uint_type = arch_integer_type (gdbarch, gdbarch_int_bit (gdbarch),
> > +                1, "unsigned int");
>
> You should be able to get this one from builtin_type (gdbarch)->builtin_unsigned_int.

Good to know.


> > +  void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
> > +
> > +  siginfo_type = arch_composite_type (gdbarch, "EXCEPTION_RECORD",
> > +                      TYPE_CODE_STRUCT);
> > +  siginfo_ptr_type = arch_pointer_type (gdbarch, gdbarch_ptr_bit (gdbarch),
> > +                    NULL, siginfo_type);
> > +
> > +  append_composite_type_field (siginfo_type, "ExceptionCode", uint_type);
> > +  append_composite_type_field (siginfo_type, "ExceptionFlags", uint_type);
> > +  append_composite_type_field (siginfo_type, "ExceptionRecord",
> > +                  siginfo_ptr_type);
> > +  append_composite_type_field (siginfo_type, "ExceptionAddress",
> > +                  void_ptr_type);
> > +  append_composite_type_field (siginfo_type, "NumberParameters", uint_type);
> > +  append_composite_type_field_aligned (siginfo_type, "ExceptionInformation",
> > +                      lookup_array_range_type (void_ptr_type,
> > +                                0, 14),
> > +                      TYPE_LENGTH (void_ptr_type));
>
>
> Shouldn't you use "DWORD" and other types named like what is found in the
> real structure, instead of plain "unsigned int"?  Like what is done in
> windows_get_tlb_type?
>
> As a user, I would expect that "ptype $_siginfo" shows me "DWORD" and not
> "unsigned int", don't you think?

Personally I didn't really care what the name of the type is,
but you are right, it probably is what users expect, so I will change the
type names accordingly.


Regards
Hannes Domani