Re: [PATCH 1/2] gdb: Allow target description to be dumped even when it is remote

[Pedro Alves <palves@redhat.com>]

On 6/11/20 3:05 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-06-11 12:33:08 +0100]:
> 
>> On 6/11/20 11:41 AM, Andrew Burgess wrote:
>>> The maintenance command 'maintenance print c-tdesc' can only print the
>>> target description if it was loaded from a local file, or if the local
>>> filename is passed to the maintenance command as an argument.
>>>
>>> Sometimes it would be nice to know what target description GDB was
>>> given by the remote, however, if I connect to a remote target and try
>>> this command I see this:
>>>
>>>   (gdb) maintenance print c-tdesc
>>>   The current target description did not come from an XML file.
>>>   (gdb)
>>>
>>> Which is not very helpful.
>>>
>>> This commit changes things so that if the description came from the
>>> remote end then GDB will use the fake filename 'target.xml' as the
>>> filename for the description, GDB will then create the C description
>>> of the target as if it was in a file 'target.xml'.
>>>
>>> I originally added this functionality so I could inspect the
>>> description passed to GDB by the remote target.  After using this for
>>> a while I realised that actually having GDB recreate the XML would be
>>> even better, so a later commit will add that functionality too.
>>>
>>> Still, given how small this patch is I thought it might be nice to
>>> include this in GDB anyway.
>>>
>>> While I was working on this anyway I've added filename command
>>> completion to this command.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* target-descriptions.c (maint_print_c_tdesc_cmd): Use fake
>>> 	filename for target descriptions that came from the target.
>>> 	(_initialize_target_descriptions): Add filename command completion
>>> 	for 'maint print c-tdesc'.
>>> ---
>>>  gdb/ChangeLog             |  7 +++++++
>>>  gdb/target-descriptions.c | 14 ++++++++++----
>>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
>>> index 20a3a640f4f..55ea416d69a 100644
>>> --- a/gdb/target-descriptions.c
>>> +++ b/gdb/target-descriptions.c
>>> @@ -1680,7 +1680,12 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
>>>      error (_("There is no target description to print."));
>>>  
>>>    if (filename == NULL)
>>> -    error (_("The current target description did not come from an XML file."));
>>> +    {
>>> +      printf_unfiltered (_("The current target description was fetched "
>>> +			   "from the target, using\n'target.xml' as a fake "
>>> +			   "filename.\n\n"));
>>
>> That "," makes it read a little bit ambiguously.  Try reading the sentence
>> without the "," to see what I mean:
>>
>>   The current target description was fetched from the target using
>>  'target.xml' as a fake filename.
>>
>> This can be read as GDB having read the remote fake 'target.xml' filename,
>> like:
>>   fetch_available_features_from_target ("target.xml", ops);
>> which is what it always does anyway...
>>
>> I'd suggest a hard period (and line break after the period) instead:
>>
>>   The current target description was fetched from the target.
>>   Using 'target.xml' as a fake filename.
>>
>> But, why do we need to provide a fake name at all?  Isn't the only use of
>> that filename to print it in the comment, here:
>>
>>  /* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
>>    Original: target.xml */
>>
>> How about just doing this:
> 
> No, the filename is used in a couple of other places too, object names
> are created based on the filename as well.  Still I took your idea and
> went with it.

Ah, missed that.

> 
> The patch below uses 'fetched from target' as the fake filename
> instead.  I needed one small extra change so that 'fetched from
> target' could become 'fetched_from_target' when creating C++ variable
> names, but otherwise this seem fine to me.

That fixes the case of a filename with spaces, regardless,
like "foo bar.xml", as in:

  (gdb) maint print c-tdesc foo bar.xml

Maybe mention that in the commit log.

> 
> How's this?

LGTM.

Thanks,
Pedro Alves