Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big

[Pedro Alves <palves at redhat dot com>]

On 01/09/2015 04:59 PM, Mark Kettenis wrote:
>> Date: Fri, 09 Jan 2015 16:27:12 +0000
>> From: Pedro Alves <palves@redhat.com>
>>
>>> Any other comments?
>>
>> Do we need to do the same in other places?  This grep seems to suggest yes:
>>
>> $ grep assert * | grep sizeof | grep regset
>> amd64obsd-tdep.c:  gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FXSAVE);
>> amd64-tdep.c:  gdb_assert (len == tdep->sizeof_fpregset);
>> amd64-tdep.c:  gdb_assert (len == tdep->sizeof_fpregset);
>> i386obsd-tdep.c:  gdb_assert (len >= tdep->sizeof_gregset + I387_SIZEOF_FSAVE);
>> i386-tdep.c:  gdb_assert (len == tdep->sizeof_gregset);
>> i386-tdep.c:  gdb_assert (len == tdep->sizeof_gregset);
>> i386-tdep.c:  gdb_assert (len == tdep->sizeof_fpregset);
>> i386-tdep.c:  gdb_assert (len == tdep->sizeof_fpregset);
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips_elf_gregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips_elf_gregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips_elf_fpregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips_elf_fpregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips64_elf_gregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips64_elf_gregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips64_elf_fpregset_t));
>> mips-linux-tdep.c:  gdb_assert (len == sizeof (mips64_elf_fpregset_t));
>> mn10300-linux-tdep.c:  gdb_assert (len == sizeof (mn10300_elf_gregset_t));
>> mn10300-linux-tdep.c:  gdb_assert (len == sizeof (mn10300_elf_fpregset_t));
>> mn10300-linux-tdep.c:  gdb_assert (len == sizeof (mn10300_elf_gregset_t));
>>
>> On 01/08/2015 04:16 PM, Andreas Arnez wrote:
>>> Note that this behavior deviates from the default policy: In general, if
>>> some future kernel adds new registers to a register set, then a GDB
>>> unaware of this extension would read the known subset and just ignore
>>> the unknown bytes.
>>
>> That's a good point.
>>
>> get_core_register_section checks the section size already:
>>
>> get_core_register_section (struct regcache *regcache,
>> 			   const struct regset *regset,
>> 			   const char *name,
>> 			   int min_size,
>> 			   int which,
>> 			   const char *human_name,
>> 			   int required)
>> {
>> ...
>>   size = bfd_section_size (core_bfd, section);
>>   if (size < min_size)
>>     {
>>       warning (_("Section `%s' in core file too small."), section_name);
>>       return;
>>     }
>> ...
>>
>> Should we remove all those asserts, and make it the
>> job of get_core_register_section to warn if the section
>> size is bigger than expected?  We may need to pass
>> the "expected" section size to the callback, in addition
>> to the "minimum" size though.
> 
> The code is designed to allow these sections to grow such that the OS
> kernel can add more registers without breaking GDB.

Not sure what you're disagreeing with.  My comment is in that direction
too (And Andreas' comment I'm quoting).  That is, get_core_register_section
would warn, but still continue processing the section.

The current code clearly does not work that way, given the assertions.

Thanks,
Pedro Alves