Re: [PATCH][gdb] Fix warning in foreach_arch selftests

[Simon Marchi <simark@simark.ca>]

On 6/1/22 12:36, Tom de Vries via Gdb-patches wrote:
> On 6/1/22 16:32, Pedro Alves wrote:
>> On 2022-06-01 11:41, Tom de Vries via Gdb-patches wrote:
>>> Hi,
>>>
>>> When running the selftests, I run into:
>>> ...
>>> $ gdb -q -batch -ex "maint selftest"
>>>    ...
>>> Running selftest execute_cfa_program::aarch64:ilp32.
>>> warning: A handler for the OS ABI "GNU/Linux" is not built into this
>>> configuration of GDB.  Attempting to continue with the default aarch64:ilp32
>>> settings.
>>> ...
>>> and likewise for execute_cfa_program::i8086 and
>>> execute_cfa_program::ia64-elf32.
>>>
>>> The warning can easily be reproduced outside the selftests by doing:
>>> ...
>>> $ gdb -q -batch -ex "set arch aarch64:ilp32"
>>> ...
>>> and can be prevented by first doing "set osabi none".
>>>
>>> Fix the warning by setting osabi to none while doing selftests that iterate
>>> over all architectures.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Any comments?
>>>
>>> Thanks,
>>> - Tom
>>>
>>> [gdb] Fix warning in foreach_arch selftests
>>>
>>> ---
>>>   gdb/osabi.c         | 13 +++++++++++++
>>>   gdb/osabi.h         |  6 ++++++
>>>   gdb/selftest-arch.c | 15 ++++++++++++++-
>>>   3 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/osabi.c b/gdb/osabi.c
>>> index bbd7635532f..c3f221df969 100644
>>> --- a/gdb/osabi.c
>>> +++ b/gdb/osabi.c
>>> @@ -633,6 +633,19 @@ set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
>>>       internal_error (__FILE__, __LINE__, _("Updating OS ABI failed."));
>>>   }
>>>   +void
>>> +set_osabi (const char *arg)
>>> +{
>>> +  set_osabi_string = arg;
>>> +  set_osabi (NULL, 0, NULL);
>>> +}
>>> +
>>> +const char *
>>> +get_osabi ()
>>> +{
>>> +  return set_osabi_string;
>>> +}
>>
>> Missing usual "see foo.h" comments.
>>
> 
> Hi,
> 
> thanks for the review.
> 
> Done.
> 
>>> +
>>>   static void
>>>   show_osabi (struct ui_file *file, int from_tty, struct cmd_list_element *c,
>>>           const char *value)
>>> diff --git a/gdb/osabi.h b/gdb/osabi.h
>>> index be016732cbc..eb5d88699e7 100644
>>> --- a/gdb/osabi.h
>>> +++ b/gdb/osabi.h
>>> @@ -89,4 +89,10 @@ const char *osabi_triplet_regexp (enum gdb_osabi osabi);
>>>   void generic_elf_osabi_sniff_abi_tag_sections (bfd *, asection *,
>>>                              enum gdb_osabi *);
>>>   +/* Set osabi to ARG.  */
>>> +extern void set_osabi (const char *arg);
>>> +
>>> +/* Return current osabi setting.  */
>>> +extern const char *get_osabi ();
>>> +
>>>   #endif /* OSABI_H */
>>> diff --git a/gdb/selftest-arch.c b/gdb/selftest-arch.c
>>> index f434da718d5..a631f52e31e 100644
>>> --- a/gdb/selftest-arch.c
>>> +++ b/gdb/selftest-arch.c
>>> @@ -66,12 +66,25 @@ foreach_arch_test_generator (const std::string &name,
>>>         auto test_fn
>>>       = ([=] ()
>>>          {
>>> +         /* Prevent warnings when setting architecture with current osabi
>>> +        settings, like:
>>> +          A handler for the OS ABI "GNU/Linux" is not built into this
>>> +          configuration of GDB.  Attempting to continue with the
>>> +          default aarch64:ilp32 settings.  */
>>> +         const char *save_osabi = get_osabi ();
>>> +         set_osabi ("none");
>>> +
>>
>> A bit of an odd API to have to pass the string name in.  I'd think an API that
>> takes an enum gdb_osabi would be better, as that way you don't have to worry
>> about either passing the wrong string, or even worry about whether it's the
>> string contents that counts (whether the function internally uses strcmp),
>> or the string's address (the function internally compares pointers, assuming
>> the only strings that will be passed down are the ones in the command's enum
>> strings array).
>>
> 
> Done.
> 
> 
>>>            struct gdbarch_info info;
>>>            info.bfd_arch_info = bfd_scan_arch (arch);
>>>            struct gdbarch *gdbarch = gdbarch_find_by_info (info);
>>>            SELF_CHECK (gdbarch != NULL);
>>> +
>>>            function (gdbarch);
>>> -         reset ();
>>> +
>>> +         SCOPE_EXIT {
>>> +           reset ();
>>> +           set_osabi (save_osabi);
>>> +         };
>>
>> Please format as:
>>
>>          SCOPE_EXIT
>>                 {
>>              reset ();
>>              set_osabi (save_osabi);
>>            };
>>
>> And you'll also need to move this to right after the set_osabi("none") call.  Doing it
>> here is too late.  SCOPE_EXIT creates a RAII object on the stack, which needs to exist
>> before any of the code that may throw right after set_osabi("none").
>>
> 
> Done, thanks for catching that.
> 
> How does this look?
> 
> Thanks,
> - Tom
> 

Hi Tom,

This introduces some failures for me:

Running selftest print_one_insn::A6.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::A7.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::ARC600.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::ARC601.^M
Running selftest print_one_insn::ARC700.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::ARCv2.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::EM.^M
Self test failed: Cannot access memory at address 0x0^M
Running selftest print_one_insn::HS.^M
Self test failed: Cannot access memory at address 0x0^M
...
FAIL: gdb.gdb/unittest.exp: no executable loaded: maintenance selftest, failed none

Simon