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

[Pedro Alves <palves@redhat.com>]

On 01/09/2015 07:35 PM, Mark Kettenis wrote:
>> Date: Fri, 09 Jan 2015 17:19:14 +0000
>> From: Pedro Alves <palves@redhat.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.
> 
> It shouldn't warn if the sections is bigger that "expected", because
> in some cases the "expected" size is really the minimum supported
> size, where later versions of the OS added extra information.  At
> least not unconditionally.

I think we're saying the same thing, but what I'm calling "expected",
you're calling "maximum".  As in, consider the case where GDB
about a regset section that is supposed to have size A.  GDB is taught
about this, with "minimum" == A, and "expected/maximum" == A.  Later at
some point, a new variant of the machine appears with more registers, and
the regset is extended, to size B.  A GDB that only knows about A encounters
a core dump with B, and thus issues a warning (which suggests that either
more info is available that gdb doesn't grok, or the core is broken), but still
presents the A registers to the user.  Later, someone teaches GDB about B
registers, and at that point, "minimum" stays A, but "expected/maximum" is
set to B.  At some point, if the regset is extended further to C, a GDB
that knows about A and B warns when it sees C.  And on and on.  I think
we've already seen something like that with the x86 xsave regset?

> I can imagine extending the interface to also specify a maximum size
> and interpreting a maximum size of 0 as "no maximum".  Continiung
> after printing a warning if the section is larger than the maximum
> size probably makes sense.

Thanks,
Pedro Alves