This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Windows DLL support update.


Daniel Jacobowitz wrote:

I have one concern; one of us should fix the problem Ulrich pointed out with my core changes:


Ooouups, I totally forgot seeing it.


On Tue, Jun 19, 2007 at 09:05:51PM +0200, Ulrich Weigand wrote:
One thing I don't quite like is this:

+    case TARGET_OBJECT_LIBRARIES:
+      if (core_vec->xfer_shared_libraries != NULL)
+   return core_vec->xfer_shared_libraries (ops, object, annex,
readbuf,
+                                          writebuf, offset, len);
I had understood the core_fns method of providing a core file target
to
be deprecated, and in fact I just recently got rid of it for AIX in
favour
of the gdbarch_regset_from_core_section callback ...   I'd prefer
this to
be a gdbarch callback (which would also support core file
cross-debugging).
I hadn't even thought about it. Yes, you're right.


OK, I'll take a stab at it, although an interface suggestion would be nice.

Should it be a callback with a similar interface
to xfer_shared_libraries and to be called in core_xfer_partial
(the callback is expected to fill a TARGET_OBJECT_LIBRARIES,
 similar to what you had, but implemented as a gdbarch callback),
or do you think it should be somethink different?

I'm giving the first option a try, and while looking at it, I've
implemented gdbarch_regset_from_core_section for
i386/win32/Cygwin, meaning core file cross debugging is on
the way for Cygwin.

Also, we added #include's so the Makefile needs an update.


Ooouups 2. Will do.


+/* Returns 1 if ADDR is within the cygwin1.dll text segment, returns 0
+   otherwise.  */
+static int
+inside_cygwin (CORE_ADDR addr)
+{
+  if (cygwin_load_start == 0)
+    {
+      struct so_list *so;
+
+      for (so = master_so_list (); so; so = so->next)

Nothing outside of the solib implementations calls master_so_list. I think that's best; can you use ALL_OBJFILES here instead?


It would not work with 'set auto-solib-add 0'. In that case, the so_list from the master list has an open associated bfd to look at, but ALL_OBJFILES doesn't report cygwin1.dll, because the symbols haven't been read yet. This means that in that case, the user would see the internal Cygwin exceptions as SIGSEGVs. That would be a functionality regression.

Perhaps the cygwin1.dll .text range recording could also be
implemented with a callback (observer?) that gets called
on every solib loaded instead of iterating through all
loaded libs.

Let me explain why I was doing it differently from how it was
done before, because we may come to the conclusion this is
the wrong way to do it.

The current win32-nat.c records the cygwin1.dll
.text load start/end addresses when imediatelly when the
dll event is reported.  This range is then
used to filter any Windows exception that occurs inside Cygwin,
since they will be handled internally by Cygwin - the user should
never see them.  Same thing happens with the IsBadXxxPtr family
of win32 functions.  They trigger an access violation by design,
so the debugger although has a chance to see it, shouldn't
normally pass it to the user when they are used.  It is
desirable to also have this filtering funcionality on the
gdbserver side.  The function in the patch implements the base
for the filtering pretty much decoupled from the rest of the
win32-nat.c code.

Instead of teaching gdbserver about these things, we could move
these bits into win32-tdep.c, or i386-cygwin-tdep.c, and install
a gdbarch callback to be called from/near handle_inferior_event.
The callback could then decide to ignore the SIGTRAP, for
instance.

Alternativelly, we could extend the qSymbol mechanism, and have
gdbserver know how to ignore the exceptions itself.  qSymbol
allows gdbserver to know the address of a symbol, but the
IsBadxxPtr case needs a start-end function range, because the
exception happens *inside* that function.  The cygwin1.dll .text
start/end could be solved if we add __cygwin_text_start__ and
__cygwin_text_end__ symbols to cygwin1.dll.  The
advantage of this approach is that an app triggering
lots of internal cygwin exceptions, or calling IsBadXxxPtr in a
tight loop would perform much faster under gdbserver, as the
ignore addresses could be cached, instead of passing a SIGTRAP
to gdb every time, just to have it ignored.  I'm not sure it
justifies the extra complexity, though.

The gdb-only method side has the advantage that doesn't
need any protocol change, is simpler, and possibly more
extensible/flexible, and can share the same user
commands (set cygwin-exceptions x).

Of course for all this to work gdb needs to see
a copy of the same version of the dlls installed in
the target.  Not sure about licensing problems with
OS components...  Perhaps the best way would be to
require dbghelp.dll (MSFTs symbol reader API) on
gdbserver, and use it to get at the symbols.  Then all
this becomes moot.

Anyways, a lot to avoid the master_so_list call. :)

If desired, I'll revert to the old method of getting
at the cygwin1.dll ranges on dll load, and come back to
it after this patch is in.

+  if (data->module_count == 0)
+    {
+#if 0
+      /* TODO: What if the user supplied exec and/or
+	 symbol files on the command line?  */
+      /* The first module is the .exe itself.  */
+      symbol_file_add_main (module_name, 0);
+#endif
+    }
+  else
+    {
+      struct so_list *so = win32_make_so (module_name, base_addr);
+      solib_to_xml (so, data->obstack);
+      win32_free_so (so);
+    }
+  data->module_count++;

Let's not add #if 0 / TODO. I think we can drop this code, how about you?


Ahh, it worked - I was hoping somebody would comment on that. I'll remove it. :)

Cheers,
Pedro Alves







Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]