Re: [PATCHv5 3/3] gdbserver: pass osabi to GDB in more target descriptions

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Kevin Buettner <kevinb@redhat.com> writes:
>
>> On Sun, 27 Oct 2024 20:07:42 +0000
>> Andrew Burgess <aburgess@redhat.com> wrote:
>>
>>> Problem Description
>>> -------------------
>>> 
>>> On a Windows machine I built gdbserver, configured for the target
>>> 'x86_64-w64-mingw32', then on a GNU/Linux machine I built GDB with
>>> support for all target (--enable-targets=all).
>>> 
>>> On the Windows machine I start gdbserver with a small test binary:
>>> 
>>>   $ gdbserver 192.168.129.25:54321 C:\some\directory\executable.exe
>>> 
>>> On the GNU/Linux machine I start GDB without the test binary, and
>>> connect to gdbserver.
>>> 
>>> As I have not given GDB the test binary, my expectation is that GDB
>>> would connect to gdbserver and then download the file over the remote
>>> protocol, but instead I was presented with this message:
>>> 
>>>   (gdb) target remote 192.168.129.25:54321
>>>   Remote debugging using 192.168.129.25:54321
>>>   warning: C:\some\directory\executable.exe: No such file or directory.
>>>   0x00007ffa3e1e1741 in ?? ()
>>>   (gdb)
>>> 
>>> What I found is that if I told GDB where to find the binary, like
>>> this:
>>> 
>>>   (gdb) file target:C:/some/directory/executable.exe
>>>   A program is being debugged already.
>>>   Are you sure you want to change the file? (y or n) y
>>>   Reading C:/some/directory/executable.exe from remote target...
>>>   warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
>>>   Reading C:/some/directory/executable.exe from remote target...
>>>   Reading symbols from target:C:/some/directory/executable.exe...
>>>   (gdb)
>>> 
>>> then GDB would download the executable.
>>> 
>>> The Actual Issue
>>> ----------------
>>> 
>>> I tracked the problem down to exec_file_find (solib.c).  The remote
>>> target was passing an absolute Windows filename (beginning with "C:/"
>>> in this case), but in exec_file_find GDB was failing the
>>> IS_TARGET_ABSOLUTE_PATH call, and so was treating the filename as
>>> relative.
>>> 
>>> The IS_TARGET_ABSOLUTE_PATH call was failing because GDB thought that
>>> the file system kind was "unix", and as the filename didn't start with
>>> a "/" it assumed the filename was not absolute.
>>> 
>>> But I'm connecting to a Windows target and 'target-file-system-kind'
>>> was set to "auto", so GDB should be figuring out that the target
>>> file-system is "dos-based".
>>> 
>>> Looking in effective_target_file_system_kind (filesystem.c), we find
>>> that the logic of "auto" is delegated to the current gdbarch.  However
>>> in windows-tdep.c we see:
>>> 
>>>   set_gdbarch_has_dos_based_file_system (gdbarch, 1);
>>> 
>>> So if we are using a Windows gdbarch we should have "dos-based"
>>> filesystems.  What this means is that after connecting to the remote
>>> target GDB has selected the wrong gdbarch.
>>> 
>>> What's happening is that the target description sent back by the
>>> remote target only includes the x86-64 registers.  There's no
>>> information about which OS we're on.  As a consequence, GDB picks the
>>> first x86-64 gdbarch which can handle the provided register set, which
>>> happens to be a GNU/Linux gdbarch.
>>> 
>>> And indeed, there doesn't appear to be anywhere in gdbserver that sets
>>> the osabi on the target descriptions. Some target descriptions do have
>>> their osabi set when the description is created, e.g. in:
>>> 
>>>   gdb/arch/amd64.c	- Sets GNU/Linux osabi when appropriate.
>>>   gdb/arch/i386.c	- Likewise.
>>>   gdb/arch/tic6x.c	- Always set GNU/Linux osabi.
>>> 
>>> There are also some cases in gdb/features/*.c where the tdesc is set,
>>> but these locations are only called from GDB, not from gdbserver.
>>> 
>>> This means that many target descriptions are created without an osabi,
>>> gdbserver does nothing to fix this, and the description is returned to
>>> GDB without an osabi included.  This leaves GDB having to guess what
>>> the target osabi is, and in some cases, GDB can get this wrong.
>>> 
>>> Proposed Solution
>>> -----------------
>>> 
>>> I propose to change init_target_desc so that it requires an gdb_osabi
>>> to be passed in, this will then be used to set the target_desc osabi
>>> field.
>>> 
>>> I believe that within gdbserver init_target_desc is called for every
>>> target_desc, so this should mean that every target_desc has an
>>> opportunity to set the osabi to something sane.
>>> 
>>> I did consider passing the osabi into the code which creates the
>>> target_desc objects, but that would require updating far more code, as
>>> each target has its own code for creating target descriptions.
>>> The approach taken here requires minimal changes and forces every
>>> user of init_target_desc to think about what the correct osabi is.
>>> 
>>> In some cases, e.g. amd64, where the osabi is already set when the
>>> target_desc is created, the init_target_desc call will override the
>>> current value, however, we should always be replacing it with the same
>>> actual value.  i.e. if the target_desc is created with the osabi set
>>> to GNU/Linux, then this should only happen when gdbserver is built for
>>> GNU/Linux, in which case the init_target_desc should also be setting
>>> the osabi to GNU/Linux.
>>> 
>>> The Tricky Bits
>>> ---------------
>>> 
>>> Some targets, like amd64, use a features based approach for creating
>>> target_desc objects, there's a function in arch/amd64.c which creates
>>> a target_desc, adds features too it, and returns the new target_desc.
>>> This target_desc is then passed to an init_target_desc call within
>>> gdbserver.  This is the easy case to handle.
>>> 
>>> Then there are other targets which instead have a fixed set of xml
>>> files, each of which is converted into a .dat file, which is then used
>>> to generate a .cc file, which is compiled into gdbserver.  The
>>> generated .cc file creates the target_desc object and calls
>>> init_target_desc on it.  In this case though the target description
>>> that is sent to GDB isn't generated from the target_desc object, but
>>> is instead the contents of the fixed xml file.  For this case the
>>> osabi which we pass to init_target_desc should match the osabi that
>>> exists in the fixed xml file.
>>> 
>>> Luckily, in the previous commit I copied the osabi information from
>>> the fixed xml files into the .dat files.  So in this commit I have
>>> extended regdat.sh to read the osabi from the .dat file and use it in
>>> the generated init_target_desc call.
>>> 
>>> The problem with some of these .dat base targets is that their fixed
>>> xml files don't currently contain any osabi information, and the file
>>> names don't indicate that they are Linux only (despite them currently
>>> only being used from gdbserver for Linux targets), so I don't
>>> currently feel confident adding any osabi information to these files.
>>> An example would be features/rs6000/powerpc-64.xml.  For now I've just
>>> ignored these cases.  The init_target_desc will use GDB_OSABI_UNKNOWN
>>> which is the default.  This means that for these targets nothing
>>> changes from the current behaviour.  But many other targets do now
>>> pass the osabi back.  Targets that do pass the osabi back are
>>> improved with this commit.
>>> 
>>> Conclusion
>>> ----------
>>> 
>>> Now when I connect to the Windows remote the target description
>>> returned includes the osabi name.  With this extra information GDB
>>> selects the correct gdbarch object, which means that GDB understands
>>> the target has a "dos-based" file-system.  With that correct GDB
>>> understands that the filename it was given is absolute, and so fetches
>>> the file from the remote as we'd like.
>>
>> The approach that you've taken makes sense to me, but I'm not an
>> expert in this area.  Therefore, I don't feel comfortable giving an
>> "Approved-by" for this particular patch.  That said, I don't think it
>> makes sense for it to languish on the mailing list, so I recommend
>> that you approve it if there are no other comments within a week or
>> so.
>>
>> Reviewed-by: Kevin Buettner <kevinb@redhat.com>
>
> Thanks for the reviews.  I'll give this some additional time to see if
> anyone else wants to comment.

I've gone ahead and pushed this series now.

Thanks,
Andrew