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

Pedro Alves
2015-01-09T20:11:35Z

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