[PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers

[Joel Brobecker]

> I spent a bit playing with this today.  See comments below.

Thank you!

> remote debugging when the remote target does not supply an explicit
> xml target description.  The only thing that can be done is add
> new registers at the end.  
> 
> So to fix this, we could also make windows_nat_target::{fetch,store}_registers
> check whether the requested register is within the mappings array.  We could
> do that by simply changing the 'mappings' global from a plain pointer to a
> gdb::array_view for example (which stores the array's size).  I suspect the
> current table based approach generates a little better code (the compiler may
> end up generating a table anyway from the switch/case in your patch, but it's
> not garanteed), though it shouldn't really matter in practice.
> 
> But see below.

I am OK with taking the bounded-buffer approach, as it would clearly
already be an improvement. I thought about something like that, but
using a C-oriented solution, which was not elegant, so discarded it
then. Your suggestion takes care of that aspect.

However, thinking further about this, we have this huge gap between
the majority of the registers, and the next ones. What if we wanted
one day to add more? We'd have to triple the size of the table and
keep it mostly empty. The counter point is that I tend to think that
this is probably unlikely.

> Since Windows doesn't expose these to userspace, I'm thinking that it
> doesn't make much sense to show the registers at all?  The (xml) target
> features are designed exactly such that if you don't have some
> optional feature (like org.gnu.gdb.i386.segments), then the corresponding
> registers don't exist at all in gdb.  
> 
> The attached patch implements that.

Makes sense to me indeed.

I tested your patch on x86_64-windows, and didn't detect any issues
with it, so I suggest we push that (see attached patch with comments
added and initial revision log provided as well -- hopefully a modest
thank you for sending the patch showing how it's done).

... and then decide whether we want to reorganize a bit the way
we get the index of each register in the CONTEXT structure. I would
say that we do want to do something. Perhaps, the path of least
resistance is to just change the mappings structure from a C array
to a gdb::array_view as you suggested. I may have a preference for
the approach I took, but it is a large diff, and it's not clear
whether it's going to be beneficial in the long run...

You pick! ;-) I'll take care of your comments if you chose the first
patch. I'll send a new one if you prefere the gdb::array_view approach.

Thanks Pedro!
-- 
Joel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-x86_64-windows-GDB-crash-due-to-fs_base-gs_base-regi.patch
Type: text/x-diff
Size: 9190 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20180626/d325eee9/attachment.bin>