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

[Andrew Burgess <aburgess@redhat.com>]

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.

Thanks,
Andrew