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

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

[Andreas Arnez]

As reported in PR 17808, a test case with a forged (invalid) core file
can crash GDB with an assertion failure.  In that particular case the
prstatus of an i386 core file looks like that from an AMD64 core file,
i.e., it is larger than GDB would expect.

The patch replaces the assertion by a warning and skips the invalid
core file register section.  In this way it is guaranteed that no
bogus register values are read from the badly formatted section.

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.

gdb/ChangeLog:

	PR corefiles/17808
	* i386-tdep.c (i386_supply_gregset): Instead of yielding an
	internal error on unexpected input buffer size, ignore the data
	and emit a warning.

---
 gdb/i386-tdep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 7d174c4..d02aaf2 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3727,7 +3727,12 @@ i386_supply_gregset (const struct regset *regset, struct regcache *regcache,
   const gdb_byte *regs = gregs;
   int i;
 
-  gdb_assert (len == tdep->sizeof_gregset);
+  if (len != tdep->sizeof_gregset)
+    {
+      /* Buffer has unknown size: assume wrong format.  */
+      warning (_("Bad size of general register section"));
+      return;
+    }
 
   for (i = 0; i < tdep->gregset_num_regs; i++)
     {
-- 
1.7.9.5

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Thu, 08 Jan 2015 17:16:20 +0100, 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.

This patch is about 'assert' vs. 'if' so I find this paragraph outside of the
topic of this thread/regression.


Here is a testcase for the assertion crash regression.


Thanks,
Jan

[-- Attachment #2: 2 --]
[-- Type: text/plain, Size: 4413 bytes --]

gdb/testsuite/ChangeLog
2015-01-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR corefiles/17808
	* gdb.arch/i386-biarch-core.core.bz2.uu: New file.
	* gdb.arch/i386-biarch-core.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
new file mode 100644
index 0000000..4221e99
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
@@ -0,0 +1,28 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+begin 600 i386-biarch-core.core.bz2
+M0EIH.3%!629361`P1\P`!)?_____\9'@"8Q)6P380'9@'&#`0D@``"``%(``
+M@`#`"!<(L`%F"(:$GH13::F-)M&D&U,AD:`--#)M0&FT0XR9--,)D9`P(Q-&
+M",(-&F``02)%38HT]0T`&AH```'H@``T^>9T*(,("&)SE`>`9@+GP=[,N)KB
+M'I8BL(L]N5TCY\%V]/?DB.BN*UZ'U@]TN7-]UJ5\_%0QTT<*086#%MHT7XVJ
+M9D"+C!"2*L:8D1XPD!`--M@*XT1H5RFYN&)(!0P0#:`I:;2;$5M&\*9"0@%:
+MK@X[T()M)9N7`D$VA!^63)%,;@8LT`(7\K&[7G;U:"B6'!GG+46ALOZF.2F-
+M!@>C*%86X$-]C2`KE;HG)UL(913VR2G]0BD:J=Z_`G@S,`W%.8RMS-#5P:J0
+MAJ2\8&X?@DE;UF68QHM<,D`('::J65/S:PAG*R-09["8DBI)'V]Y.[(/AM*L
+M"X_O^V;%FY.S6Q]FM=D37>5F,%4-F1ZF#,CFJVU;H*^IT<(%<V`.32$`JU["
+/G`68?\7<D4X4)`0,$?,`
+`
+end
diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
new file mode 100644
index 0000000..0dcb256
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -0,0 +1,59 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test ability to load an elf64-i386 core file.  The provided core file was
+# elf64-x8664 one but it got binary patched to i386:
+# Elf32_Ehdr.e_machine @0x12..0x13
+# Elf64_Ehdr.e_machine @0x12..0x13
+# #define EM_386           3              /* Intel 80386 */
+# #define EM_X86_64       62              /* AMD x86-64 architecture */
+# patch @0x12: 0x3E -> 0x03
+
+standard_testfile
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386-biarch-core test."
+    return
+}
+
+set corebz2uufile ${srcdir}/${subdir}/${testfile}.core.bz2.uu
+set corefile ${objdir}/${subdir}/${testfile}.core
+# Entry point of the original executable.
+set address 0x400078
+
+if {[catch "system \"uudecode -o - ${corebz2uufile} | bzip2 -dc >${corefile}\""] != 0} {
+    untested "failed uudecode or bzip2"
+    return -1
+}
+file stat ${corefile} corestat
+if {$corestat(size) != 102400} {
+    untested "uudecode or bzip2 produce invalid result"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Wrongly built GDB complains by:
+# "..." is not a core dump: File format not recognized
+# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
+# This is just a problem of the test care, real-world elf64-i386 file will have
+# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
+# objcopy as it corrupts the core file beyond all recognition.
+# "\r\nCore was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal 11, Segmentation fault\\.\r\n.*"
+gdb_test "core-file ${corefile}" ".*" "core-file"
+
+gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

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

[Andreas Arnez]

On Thu, Jan 08 2015, Jan Kratochvil wrote:

> On Thu, 08 Jan 2015 17:16:20 +0100, 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.
>
> This patch is about 'assert' vs. 'if' so I find this paragraph outside of the
> topic of this thread/regression.

Right, it seems a bit off-topic.  What I really mean is that "this
behavior *still* deviates from the default policy", and I just wanted to
make sure that we all agree to that.  Anyway, I will remove this
paragraph from the commit message.

Any other comments?  After adjusting the commit message, is the patch
then OK to apply?

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

[Pedro Alves]

> 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.

Thanks,
Pedro Alves

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

[Mark Kettenis]

> 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.

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

[Pedro Alves]

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

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

[Andreas Arnez]

On Fri, Jan 09 2015, Pedro Alves wrote:

>> 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));

Right, these should be handled as well.  Once we agree on how to handle
them, I can provide an updated patch.

> 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.

Good point.  Or maybe instead of adding the expected size to the
callback we could allow the regset supply functions to pass back their
"recognized" size?  For instance:


diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 0750506..3ad491d 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3720,14 +3720,14 @@ i386_value_to_register (struct frame_info *frame, int regnum,
 
 void
 i386_supply_gregset (const struct regset *regset, struct regcache *regcache,
-		     int regnum, const void *gregs, size_t len)
+		     int regnum, const void *gregs, size_t *len)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   const struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   const gdb_byte *regs = gregs;
   int i;
 
-  gdb_assert (len == tdep->sizeof_gregset);
+  *len = tdep->sizeof_gregset;
 
   for (i = 0; i < tdep->gregset_num_regs; i++)
     {


(And then, for symmetry, the regset collect functions could also
indicate how much data they produced, i.e., how large the resulting
register section should be.  But I'm not sure whether there is currently
any application for this.)

A more "lazy" approach would be to assume that the "recognized" size
always equals the "minimum" size.  Then we could emit the warning like
this:

  if (size < min_size)
    {
      warning (_("Section `%s' in core file too small."), section_name);
      return;
    }
  else if (size != min_size)
    warning (_("Section `%s' in core file has unexpected size."),
             section_name);

This is certainly less flexible, but might be good enough.

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

[Mark Kettenis]

> 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 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.

The asserts should probably be changed into >= whatever happens.

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

[Pedro Alves]

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

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

[Mark Kettenis]

> Date: Fri, 09 Jan 2015 20:11:04 +0000
> From: 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?

Yes, the x86 "FPU" register set certainly is an example I had in mind.
It all started when SSE was introduced.

There are also some BSD's where during the transition from a.out to
ELF the floating-point registers were seperated out into their own
section.  In that case the section actually shrunk and the minmum size
was adjusted.

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

[Andreas Arnez]

On Fri, Jan 09 2015, Mark Kettenis wrote:

>> Date: Fri, 09 Jan 2015 20:11:04 +0000
>> From: Pedro Alves <palves@redhat.com>
>> 
>> ...
>> 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?
>
> Yes, the x86 "FPU" register set certainly is an example I had in mind.
> It all started when SSE was introduced.

Right, the floating point register set has grown over time.  The good
thing is that x86 Linux platforms represent the different FPU regset
versions with different target descriptions.  (As determined from a core
file with the core_read_description gdbarch method.)  In this way the
regset size passed to the iterator callback could match the "expected"
size, not just the minimum.  However, the size is currently not
calculated at all for ".reg-xstate".  I think this should be fixed,
probably by using X86_XSTATE_SIZE.

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

Hi,

now when the fix is checked-in even the testcase could be.

OK for check-in?


Jan


On Thu, 08 Jan 2015 17:43:27 +0100, Jan Kratochvil wrote:
[...]
Here is a testcase for the assertion crash regression.


Thanks,
Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 4413 bytes --]

gdb/testsuite/ChangeLog
2015-01-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR corefiles/17808
	* gdb.arch/i386-biarch-core.core.bz2.uu: New file.
	* gdb.arch/i386-biarch-core.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
new file mode 100644
index 0000000..4221e99
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
@@ -0,0 +1,28 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+begin 600 i386-biarch-core.core.bz2
+M0EIH.3%!629361`P1\P`!)?_____\9'@"8Q)6P380'9@'&#`0D@``"``%(``
+M@`#`"!<(L`%F"(:$GH13::F-)M&D&U,AD:`--#)M0&FT0XR9--,)D9`P(Q-&
+M",(-&F``02)%38HT]0T`&AH```'H@``T^>9T*(,("&)SE`>`9@+GP=[,N)KB
+M'I8BL(L]N5TCY\%V]/?DB.BN*UZ'U@]TN7-]UJ5\_%0QTT<*086#%MHT7XVJ
+M9D"+C!"2*L:8D1XPD!`--M@*XT1H5RFYN&)(!0P0#:`I:;2;$5M&\*9"0@%:
+MK@X[T()M)9N7`D$VA!^63)%,;@8LT`(7\K&[7G;U:"B6'!GG+46ALOZF.2F-
+M!@>C*%86X$-]C2`KE;HG)UL(913VR2G]0BD:J=Z_`G@S,`W%.8RMS-#5P:J0
+MAJ2\8&X?@DE;UF68QHM<,D`('::J65/S:PAG*R-09["8DBI)'V]Y.[(/AM*L
+M"X_O^V;%FY.S6Q]FM=D37>5F,%4-F1ZF#,CFJVU;H*^IT<(%<V`.32$`JU["
+/G`68?\7<D4X4)`0,$?,`
+`
+end
diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
new file mode 100644
index 0000000..0dcb256
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -0,0 +1,59 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test ability to load an elf64-i386 core file.  The provided core file was
+# elf64-x8664 one but it got binary patched to i386:
+# Elf32_Ehdr.e_machine @0x12..0x13
+# Elf64_Ehdr.e_machine @0x12..0x13
+# #define EM_386           3              /* Intel 80386 */
+# #define EM_X86_64       62              /* AMD x86-64 architecture */
+# patch @0x12: 0x3E -> 0x03
+
+standard_testfile
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386-biarch-core test."
+    return
+}
+
+set corebz2uufile ${srcdir}/${subdir}/${testfile}.core.bz2.uu
+set corefile ${objdir}/${subdir}/${testfile}.core
+# Entry point of the original executable.
+set address 0x400078
+
+if {[catch "system \"uudecode -o - ${corebz2uufile} | bzip2 -dc >${corefile}\""] != 0} {
+    untested "failed uudecode or bzip2"
+    return -1
+}
+file stat ${corefile} corestat
+if {$corestat(size) != 102400} {
+    untested "uudecode or bzip2 produce invalid result"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Wrongly built GDB complains by:
+# "..." is not a core dump: File format not recognized
+# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
+# This is just a problem of the test care, real-world elf64-i386 file will have
+# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
+# objcopy as it corrupts the core file beyond all recognition.
+# "\r\nCore was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal 11, Segmentation fault\\.\r\n.*"
+gdb_test "core-file ${corefile}" ".*" "core-file"
+
+gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

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

[Pedro Alves]

On 02/05/2015 08:37 AM, Jan Kratochvil wrote:
> Hi,
> 

Thanks for the test case.

> now when the fix is checked-in even the testcase could be.
> 
> OK for check-in?

It's not obvious to me why is the file uuencoded.  What's the
reason for that?  I can think of a reason, but I don't want to
guess.

> +# Wrongly built GDB complains by:
> +# "..." is not a core dump: File format not recognized
> +# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
> +# This is just a problem of the test care, real-world elf64-i386 file will have
> +# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
> +# objcopy as it corrupts the core file beyond all recognition.
> +# "\r\nCore was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal 11, Segmentation fault\\.\r\n.*"

Hmm, this line is commented out, but there's no explanation of why
that is.  Is that a left over that was intended to be used in the
gdb_test below?  As is, it seems like the gdb_test below will PASS
even on buggy GDB?

> +gdb_test "core-file ${corefile}" ".*" "core-file"
> +
> +gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

Thanks,
Pedro Alves

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On Thu, 05 Feb 2015 10:47:49 +0100, Pedro Alves wrote:
> It's not obvious to me why is the file uuencoded.  What's the
> reason for that?

I do not find it convenient to check in a binary, while GIT can handle that
diff+patch do not.

If you prefer a different encoding method I do not mind.


> > +# Wrongly built GDB complains by:
> > +# "..." is not a core dump: File format not recognized
> > +# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
> > +# This is just a problem of the test care, real-world elf64-i386 file will have
> > +# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
> > +# objcopy as it corrupts the core file beyond all recognition.
> > +# "\r\nCore was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal 11, Segmentation fault\\.\r\n.*"
> 
> Hmm, this line is commented out, but there's no explanation of why
> that is.  Is that a left over that was intended to be used in the
> gdb_test below?  As is, it seems like the gdb_test below will PASS
> even on buggy GDB?

The goal of this testcase is the final test:
	x/i 0x400078
	   0x400078:    hlt    
	(gdb) PASS: gdb.arch/i386-biarch-core.exp: .text is readable

If the core-file command fails to load the core file data it will get caught
by this final test anyway.

I really have no idea after 6 years why the expect string is commented out
there.  Therefore I have put in into the gdb_test command now together with
the new warning:
	(gdb) core-file /home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.arch/i386-biarch-core.core^M
	[New LWP 6901]^M
	warning: Unexpected size of section `.reg/6901' in core file.^M
	Failed to read a valid object file image from memory.^M
	Core was generated by `./bad'.^M
	Program terminated with signal SIGSEGV, Segmentation fault.^M
	warning: Unexpected size of section `.reg/6901' in core file.^M
	#0  0x00000200 in ?? ()^M
	(gdb) PASS: gdb.arch/i386-biarch-core.exp: core-file
->
	gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"



Thanks,
Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 4481 bytes --]

gdb/testsuite/ChangeLog
2015-02-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR corefiles/17808
	* gdb.arch/i386-biarch-core.core.bz2.uu: New file.
	* gdb.arch/i386-biarch-core.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
new file mode 100644
index 0000000..4221e99
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2.uu
@@ -0,0 +1,28 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+begin 600 i386-biarch-core.core.bz2
+M0EIH.3%!629361`P1\P`!)?_____\9'@"8Q)6P380'9@'&#`0D@``"``%(``
+M@`#`"!<(L`%F"(:$GH13::F-)M&D&U,AD:`--#)M0&FT0XR9--,)D9`P(Q-&
+M",(-&F``02)%38HT]0T`&AH```'H@``T^>9T*(,("&)SE`>`9@+GP=[,N)KB
+M'I8BL(L]N5TCY\%V]/?DB.BN*UZ'U@]TN7-]UJ5\_%0QTT<*086#%MHT7XVJ
+M9D"+C!"2*L:8D1XPD!`--M@*XT1H5RFYN&)(!0P0#:`I:;2;$5M&\*9"0@%:
+MK@X[T()M)9N7`D$VA!^63)%,;@8LT`(7\K&[7G;U:"B6'!GG+46ALOZF.2F-
+M!@>C*%86X$-]C2`KE;HG)UL(913VR2G]0BD:J=Z_`G@S,`W%.8RMS-#5P:J0
+MAJ2\8&X?@DE;UF68QHM<,D`('::J65/S:PAG*R-09["8DBI)'V]Y.[(/AM*L
+M"X_O^V;%FY.S6Q]FM=D37>5F,%4-F1ZF#,CFJVU;H*^IT<(%<V`.32$`JU["
+/G`68?\7<D4X4)`0,$?,`
+`
+end
diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
new file mode 100644
index 0000000..350e417
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -0,0 +1,58 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test ability to load an elf64-i386 core file.  The provided core file was
+# elf64-x8664 one but it got binary patched to i386:
+# Elf32_Ehdr.e_machine @0x12..0x13
+# Elf64_Ehdr.e_machine @0x12..0x13
+# #define EM_386           3              /* Intel 80386 */
+# #define EM_X86_64       62              /* AMD x86-64 architecture */
+# patch @0x12: 0x3E -> 0x03
+
+standard_testfile
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386-biarch-core test."
+    return
+}
+
+set corebz2uufile ${srcdir}/${subdir}/${testfile}.core.bz2.uu
+set corefile ${objdir}/${subdir}/${testfile}.core
+# Entry point of the original executable.
+set address 0x400078
+
+if {[catch "system \"uudecode -o - ${corebz2uufile} | bzip2 -dc >${corefile}\""] != 0} {
+    untested "failed uudecode or bzip2"
+    return -1
+}
+file stat ${corefile} corestat
+if {$corestat(size) != 102400} {
+    untested "uudecode or bzip2 produce invalid result"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Wrongly built GDB complains by:
+# "..." is not a core dump: File format not recognized
+# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
+# This is just a problem of the test care, real-world elf64-i386 file will have
+# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
+# objcopy as it corrupts the core file beyond all recognition.
+gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
+
+gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

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

[Pedro Alves]

On 02/14/2015 03:12 PM, Jan Kratochvil wrote:
> On Thu, 05 Feb 2015 10:47:49 +0100, Pedro Alves wrote:
>> It's not obvious to me why is the file uuencoded.  What's the
>> reason for that?
> 
> I do not find it convenient to check in a binary, while GIT can handle that
> diff+patch do not.
> 
> If you prefer a different encoding method I do not mind.

I'd rather just not uuencode the binary, dropping the dependency
on uudecode.

I think this is the first time we're adding a binary core dump
to the testsuite.  I think elfutils (a GPLv3 project)
just puts test core binaries directly in git, right?

Thanks,
Pedro Alves

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Tue, 17 Feb 2015 13:56:45 +0100, Pedro Alves wrote:
> I'd rather just not uuencode the binary, dropping the dependency
> on uudecode.
> 
> I think this is the first time we're adding a binary core dump
> to the testsuite.  I think elfutils (a GPLv3 project)
> just puts test core binaries directly in git, right?

Yes, it does.  It has the same problem of reviews by mail.

But while reading through GIT for this patch I have found there is 'git apply'
so the binary patches are manageable.  (I was using only 'git am' before.)

Considering the patch approved now, posting just as a preview of a binary
patch - from 'git diff --binary HEAD'.


Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 3680 bytes --]

gdb/testsuite/ChangeLog
2015-02-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR corefiles/17808
	* gdb.arch/i386-biarch-core.core.bz2: New file.
	* gdb.arch/i386-biarch-core.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2 b/gdb/testsuite/gdb.arch/i386-biarch-core.core.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..8b823f791d4dc4fa87c0b56138e8deaf8a898dda
GIT binary patch
literal 420
zcmV;V0bBk;T4*^jL0KkKSr9Ns%m4(J|NsC0@sZ#Oj7eJr*g$q*9ALmgNB{sJ02F`#
zfB?V<7YMKcW(bCao`h3rsf{Mlq#IKqk)RDUGHpO<v_p)UG}8%@kT4??MhL<U8ejlH
zB1KJ#H1!Pt8X5oq0qB4LH2LOqD1!(HVsn%SfMx>c!QRZcn&KXoBCv}+xm_dY!FKfb
z<cR35D_)1z4|KV6eb%LX{8Ta1M+!lOgBIE}UyZ6}K#PnJk}AfSksdIR5Dhli3gbj*
zS1Gx;Vn_uH5DlOyX|$UWTSoAvLP7yrt`0lUf^8+6mjXdHgddhnkxXs|EYJcM^0B*K
zcJ*i|mK+)9Ek&WS{-!x8jRps!C{`BWLw$`PE0ww@CtC<*6!ys}{X!`ksouW=cr!2!
z#W{?v%+S@rs*r}HykKr0f=OG}Wthf`Trxli9j2;TQ}b&GXDcI6XRw%(DoG!2c{{QX
zhSIDHkMH|t#ha6}TOVe%*%MvmW-wI^nI5JL$mXkUTcEG0(ZU6DU=B?o0IOcYoCTPF
O#oUoj6eI);5%T~Ca;~=k

literal 0
HcmV?d00001

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
new file mode 100644
index 0000000..612b4d8
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -0,0 +1,58 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test ability to load an elf64-i386 core file.  The provided core file was
+# elf64-x8664 one but it got binary patched to i386:
+# Elf32_Ehdr.e_machine @0x12..0x13
+# Elf64_Ehdr.e_machine @0x12..0x13
+# #define EM_386           3              /* Intel 80386 */
+# #define EM_X86_64       62              /* AMD x86-64 architecture */
+# patch @0x12: 0x3E -> 0x03
+
+standard_testfile
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386-biarch-core test."
+    return
+}
+
+set corebz2file ${srcdir}/${subdir}/${testfile}.core.bz2
+set corefile ${objdir}/${subdir}/${testfile}.core
+# Entry point of the original executable.
+set address 0x400078
+
+if {[catch "system \"bzip2 -dc ${corebz2file} >${corefile}\""] != 0} {
+    untested "failed bzip2"
+    return -1
+}
+file stat ${corefile} corestat
+if {$corestat(size) != 102400} {
+    untested "bzip2 produces invalid result"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Wrongly built GDB complains by:
+# "..." is not a core dump: File format not recognized
+# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
+# This is just a problem of the test care, real-world elf64-i386 file will have
+# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
+# objcopy as it corrupts the core file beyond all recognition.
+gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
+
+gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

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

[Jan Kratochvil]

On Tue, 17 Feb 2015 17:56:29 +0100, Jan Kratochvil wrote:
> But while reading through GIT for this patch I have found there is 'git apply'
> so the binary patches are manageable.  (I was using only 'git am' before.)

And 'git apply --index' even does 'git add' for the new files as I have found.


> Considering the patch approved now, posting just as a preview of a binary
> patch - from 'git diff --binary HEAD'.

Checked in:
	97a0c6972eb9eb730df3817a95f351545a8f7cac


Jan

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

[Yao Qi]

On 17/02/15 16:56, Jan Kratochvil wrote:
> +# Wrongly built GDB complains by:
> +# "..." is not a core dump: File format not recognized
> +# As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
> +# This is just a problem of the test care, real-world elf64-i386 file will have
> +# 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
> +# objcopy as it corrupts the core file beyond all recognition.
> +gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"

Hi Jan,
this new test fails on i686 buildbot slaves,

(gdb) core-file 
/home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core
"/home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core" 
is not a core dump: File format not recognized
(gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file

See more in https://sourceware.org/ml/gdb-testers/2015-q1/msg02825.html

I can reproduce these fails on i686-pc-linux-gnu GDB on my
x864_64-linux.  Could you take a look?

-- 
Yao (齐尧)

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

Hi Yao,

On Tue, 14 Jul 2015 10:52:33 +0200, Yao Qi wrote:
> this new test fails on i686 buildbot slaves,
> 
> (gdb) core-file /home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core
> "/home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core"
> is not a core dump: File format not recognized
> (gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file

There are two problems:

(1) The testcase did not really test if elf64-i386 is supported by GDB (BFD).
That was OK for a Fedora testcase but I forgot about it when submitting it
upstream.

I haven't really verified if the GNU target is elf64-little but it seems so,
no other one seems suitable from:
	elf32-x86-64
	elf64-big
	elf64-k1om
	elf64-l1om
	elf64-little
	elf64-x86-64
	pei-x86-64


(2) The output of the "core-file" command itself can be arbitrary as the
elf64-i386 file with x86_64 registers is really broken; but that does not
matter much, important is the following test whether core file memory is
readable.
	./configure --enable-64-bit-bfd
	(gdb) core-file /home/jkratoch/redhat/gdb-test-build32-plus64/gdb/testsuite/gdb.arch/i386-biarch-core.core^M
	warning: Couldn't find general-purpose registers in core file.^M
	Failed to read a valid object file image from memory.^M
	warning: Couldn't find general-purpose registers in core file.^M
	#0  <unavailable> in ?? ()^M
	(gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file
	x/i 0x400078^M
	   0x400078:    hlt    ^M
	(gdb) PASS: gdb.arch/i386-biarch-core.exp: .text is readable

OK for check-in a fix for (1) and (2) in this patch?


Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 1879 bytes --]

2015-07-14  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.arch/i386-biarch-core.exp: Replace istarget
	by "complete set gnutarget". Remove expectation for the "core-file"
	command.

diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
index 60d049b..9e05869 100644
--- a/gdb/testsuite/gdb.arch/i386-biarch-core.exp
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -23,9 +23,20 @@
 
 standard_testfile
 
-if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
-    verbose "Skipping i386-biarch-core test."
-    return
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+set test "complete set gnutarget"
+gdb_test_multiple "complete set gnutarget " $test {
+    -re "set gnutarget elf64-little\r\n(.*\r\n)?$gdb_prompt $" {
+	pass $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass $test
+	untested ".text is readable"
+	return
+    }
 }
 
 set corebz2file ${srcdir}/${subdir}/${testfile}.core.bz2
@@ -43,16 +54,12 @@ if {$corestat(size) != 102400} {
     return -1
 }
 
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-
 # Wrongly built GDB complains by:
 # "..." is not a core dump: File format not recognized
 # As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
 # This is just a problem of the test case, real-world elf64-i386 file will have
 # 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
 # objcopy as it corrupts the core file beyond all recognition.
-gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
+gdb_test "core-file ${corefile}" ".*" "core-file"
 
 gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

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

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> (1) The testcase did not really test if elf64-i386 is supported by GDB (BFD).
> That was OK for a Fedora testcase but I forgot about it when submitting it
> upstream.
>
> I haven't really verified if the GNU target is elf64-little but it seems so,
> no other one seems suitable from:
> 	elf32-x86-64
> 	elf64-big
> 	elf64-k1om
> 	elf64-l1om
> 	elf64-little
> 	elf64-x86-64
> 	pei-x86-64

Hi Jan,
Why can't we use istarget here?  I thought we still check
istarget "x86_64-*-*", no?

>
> (2) The output of the "core-file" command itself can be arbitrary as the
> elf64-i386 file with x86_64 registers is really broken; but that does not
> matter much, important is the following test whether core file memory is
> readable.

"that does not matter much" mean if internal error isn't triggered, any
output is acceptable, right?  and the purpose of following test "x/i $address"
is to verify this (internal error not triggered)?

Bug 17808 describes that GDB gets internal error when it loads in
i386-biarch-core.core.

> 	./configure --enable-64-bit-bfd
> 	(gdb) core-file /home/jkratoch/redhat/gdb-test-build32-plus64/gdb/testsuite/gdb.arch/i386-biarch-core.core^M
> 	warning: Couldn't find general-purpose registers in core file.^M
> 	Failed to read a valid object file image from memory.^M
> 	warning: Couldn't find general-purpose registers in core file.^M
> 	#0  <unavailable> in ?? ()^M
> 	(gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file
> 	x/i 0x400078^M
> 	   0x400078:    hlt    ^M
> 	(gdb) PASS: gdb.arch/i386-biarch-core.exp: .text is readable
>


>  # Wrongly built GDB complains by:
>  # "..." is not a core dump: File format not recognized
>  # As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
>  # This is just a problem of the test case, real-world elf64-i386 file will have
>  # 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
>  # objcopy as it corrupts the core file beyond all recognition.

As you said, the output of command "core-file" doesn't matter much, we
need to update the comments here.

> -gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
> +gdb_test "core-file ${corefile}" ".*" "core-file"

>  
>  gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

We also need comment here to explain the purpose this "x/i $address" test.

-- 
Yao (齐尧)

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

[Jan Kratochvil]

On Wed, 15 Jul 2015 18:14:01 +0200, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> > (1) The testcase did not really test if elf64-i386 is supported by GDB (BFD).
> > That was OK for a Fedora testcase but I forgot about it when submitting it
> > upstream.
> >
> > I haven't really verified if the GNU target is elf64-little but it seems so,
> > no other one seems suitable from:
> > 	elf32-x86-64
> > 	elf64-big
> > 	elf64-k1om
> > 	elf64-l1om
> > 	elf64-little
> > 	elf64-x86-64
> > 	pei-x86-64
> 
> Hi Jan,
> Why can't we use istarget here?

I do not know much dejagnu but I expect 'istarget' tests against the site.exp
'target_triplet' content which is set to the primary GDB target
(--target=...).

GDB is normally never configured for primary target elf64-i386, I think BFD
does not know such explicit target, it gets recognized as elf64-little.

In fact many testfiles of the GDB testsuite are wrong as they require
'istarget' (therefore primary GDB target) even for just loading arch specific
files which would be sufficient with secondary target (--enable-targets=...)
support.


> I thought we still check istarget "x86_64-*-*", no?

This my new patch removes this 'istarget' check as it is IMO unrelated to what
we need to test.  Although you are right we do 'x/i' and test for 'hlt' so
I think we should test also for available 'set architecture i386'.
We could also test by 'x/bx' instead of 'x/i' to avoid such additional
test/requirement.


> > (2) The output of the "core-file" command itself can be arbitrary as the
> > elf64-i386 file with x86_64 registers is really broken; but that does not
> > matter much, important is the following test whether core file memory is
> > readable.
> 
> "that does not matter much" mean if internal error isn't triggered, any
> output is acceptable, right?

Yes.

> and the purpose of following test "x/i $address"
> is to verify this (internal error not triggered)?

I did not think specifically about internal error but I agree.  After all the
core file should be loaded which is tested by readability of a core file's
segment.


> >  # Wrongly built GDB complains by:
> >  # "..." is not a core dump: File format not recognized
> >  # As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
> >  # This is just a problem of the test case, real-world elf64-i386 file will have
> >  # 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
> >  # objcopy as it corrupts the core file beyond all recognition.
> 
> As you said, the output of command "core-file" doesn't matter much, we
> need to update the comments here.

I think the comments above are useful to understand why it does not behave as
sanely as one would expect (=the real world case for loading kdump i386 kernel
core files).

So to add another part of the comment?
	# The output therefore does not matter much, just we should not get
	# GDB internal error.

Although this whole feature is becoming marginal as i386 kernels in enterprise
usage (=kdump) have AFAIK mostly disappeared.


> > -gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
> > +gdb_test "core-file ${corefile}" ".*" "core-file"
> 
> >  
> >  gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"
> 
> We also need comment here to explain the purpose this "x/i $address" test.

Such a comment?
	# Test readability of a core file segment memory.


Jan

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

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> I think the comments above are useful to understand why it does not behave as
> sanely as one would expect (=the real world case for loading kdump i386 kernel
> core files).
>
> So to add another part of the comment?
> 	# The output therefore does not matter much, just we should not get
> 	# GDB internal error.

It looks good to me.

>
> Although this whole feature is becoming marginal as i386 kernels in enterprise
> usage (=kdump) have AFAIK mostly disappeared.
>
>
>> > -gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of
>> > section `\\.reg/6901' in core file\\.\r\n.*Core was generated by
>> > \[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV,
>> > Segmentation fault\\.\r\n.*" "core-file"
>> > +gdb_test "core-file ${corefile}" ".*" "core-file"
>> 
>> >  
>> >  gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[
>> > \t\]*" ".text is readable"
>> 
>> We also need comment here to explain the purpose this "x/i $address" test.
>
> Such a comment?
> 	# Test readability of a core file segment memory.

Sorry, I should be more clear.  Let me ask in another way, why do we
need "x/i $address" test?  Without the patch fixing PR 17808, GDB should
crash on loading core-file, and we tested that.  Why do we do this test
and test whether ".text" is readable or not?

-- 
Yao (齐尧)

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

[Jan Kratochvil]

On Thu, 16 Jul 2015 16:15:00 +0200, Yao Qi wrote:
> Sorry, I should be more clear.  Let me ask in another way, why do we
> need "x/i $address" test?  Without the patch fixing PR 17808, GDB should
> crash on loading core-file, and we tested that.  Why do we do this test
> and test whether ".text" is readable or not?

Because this testcase comes from a different bug from 2009:
	https://bugzilla.redhat.com/show_bug.cgi?id=457187
	http://pkgs.fedoraproject.org/cgit/gdb.git/commit/?id=94cd124608bf0dd359cb48a710800d72c21b30c3

That bug has been fixed in the meantime but the same testcase was reproducing
this new different bug - internal error regression - so I submitted it.

We can remove the "x/i $address" test but it was useful for the previous bug
from 2009 as that time the internal error regression did not happen, just the
core file was not recognized (which would not be detected by the proposed
ignoring of the "core-file" command output) and so the core file was not
available.  That can be tested by the "x/i $address" test.


But we could be upstreaming much more Fedora testcases which I do not plan to.
Fedora contains many testcases - 49
	grep '^#=' gdb.spec|sed 's/[:+].*//'|sort|uniq -c
	49 #=fedoratest
for various bugs already fixed upstream.  But given there isn't enough work
resources to upstream even Fedora fixes (hacks) I have never much attempted to
upstream all the testcases there.
	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/
(Some/few of the testcases are also a different form of the upstreamed variant
of the same testcase kept to be really sure no regressions are needlessly
missed in Fedora/RHEL.)


Jan

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

[Yao Qi]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> Because this testcase comes from a different bug from 2009:
> 	https://bugzilla.redhat.com/show_bug.cgi?id=457187
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/commit/?id=94cd124608bf0dd359cb48a710800d72c21b30c3
>
> That bug has been fixed in the meantime but the same testcase was reproducing
> this new different bug - internal error regression - so I submitted
> it.

I see, that is clear to me now.

>
> We can remove the "x/i $address" test but it was useful for the previous bug
> from 2009 as that time the internal error regression did not happen, just the
> core file was not recognized (which would not be detected by the proposed
> ignoring of the "core-file" command output) and so the core file was not
> available.  That can be tested by the "x/i $address" test.
>

Yeah, I agree it is useful to keep this test there, but we need comments
on the purpose of such test, for example,

# Test bug https://bugzilla.redhat.com/show_bug.cgi?id=457187
gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"

>
> But we could be upstreaming much more Fedora testcases which I do not plan to.
> Fedora contains many testcases - 49
> 	grep '^#=' gdb.spec|sed 's/[:+].*//'|sort|uniq -c
> 	49 #=fedoratest
> for various bugs already fixed upstream.  But given there isn't enough work
> resources to upstream even Fedora fixes (hacks) I have never much attempted to
> upstream all the testcases there.
> 	http://pkgs.fedoraproject.org/cgit/gdb.git/tree/
> (Some/few of the testcases are also a different form of the upstreamed variant
> of the same testcase kept to be really sure no regressions are needlessly
> missed in Fedora/RHEL.)

I don't work on any distribution, so I don't know.

-- 
Yao (齐尧)

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 117 bytes --]

Hi Yao,

checked in the attached patch, all its parts have been communicated so it is
OK as is I hope.


Thanks,
Jan

[-- Attachment #2: Type: message/rfc822, Size: 6111 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Fix gdb.arch/i386-biarch-core.exp FAIL on i386.
Date: Thu, 16 Jul 2015 18:01:22 +0200

This new test fails on i686 buildbot slaves,

(gdb) core-file /home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core
"/home/gdb-buildbot-2/fedora-x86-64-2/fedora-i686/build/gdb/testsuite/gdb.arch/i386-biarch-core.core"
is not a core dump: File format not recognized
(gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file

There are two problems:

(1) The testcase did not really test if elf64-i386 is supported by GDB (BFD).
That was OK for a Fedora testcase but I forgot about it when submitting it
upstream.

I haven't really verified if the GNU target is elf64-little but it seems so,
no other one seems suitable from:
	elf32-x86-64
	elf64-big
	elf64-k1om
	elf64-l1om
	elf64-little
	elf64-x86-64
	pei-x86-64

(2) The output of the "core-file" command itself can be arbitrary as the
elf64-i386 file with x86_64 registers is really broken; but that does not
matter much, important is the following test whether core file memory is
readable.
	./configure --enable-64-bit-bfd
	(gdb) core-file /home/jkratoch/redhat/gdb-test-build32-plus64/gdb/testsuite/gdb.arch/i386-biarch-core.core^M
	warning: Couldn't find general-purpose registers in core file.^M
	Failed to read a valid object file image from memory.^M
	warning: Couldn't find general-purpose registers in core file.^M
	#0  <unavailable> in ?? ()^M
	(gdb) FAIL: gdb.arch/i386-biarch-core.exp: core-file
	x/i 0x400078^M
	   0x400078:    hlt    ^M
	(gdb) PASS: gdb.arch/i386-biarch-core.exp: .text is readable

I do not know much dejagnu but I expect 'istarget' tests against the site.exp
'target_triplet' content which is set to the primary GDB target
(--target=...).

GDB is normally never configured for primary target elf64-i386, I think BFD
does not know such explicit target, it gets recognized as elf64-little.

In fact many testfiles of the GDB testsuite are wrong as they require
'istarget' (therefore primary GDB target) even for just loading arch specific
files which would be sufficient with secondary target (--enable-targets=...)
support.

This my new patch removes this 'istarget' check as it is IMO unrelated to what
we need to test.  Although you are right we do 'x/i' and test for 'hlt' so
I think we should test also for available 'set architecture i386'.
We could also test by 'x/bx' instead of 'x/i' to avoid such additional
test/requirement.

This testcase comes from a different bug from 2009:
	https://bugzilla.redhat.com/show_bug.cgi?id=457187
	http://pkgs.fedoraproject.org/cgit/gdb.git/commit/?id=94cd124608bf0dd359cb48a710800d72c21b30c3

That bug has been fixed in the meantime but the same testcase was reproducing
this new different bug - internal error regression - so I submitted it.

We can remove the "x/bx $address" test but it was useful for the previous bug
from 2009 as that time the internal error regression did not happen, just the
core file was not recognized (which would not be detected by the proposed
ignoring of the "core-file" command output) and so the core file was not
available.  That can be tested by the "x/bx $address" test.

gdb/testsuite/ChangeLog
2015-07-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.arch/i386-biarch-core.exp: Replace istarget
	by "complete set gnutarget". Remove expectation for the "core-file"
	command.
---
 gdb/testsuite/ChangeLog                     |  6 ++++++
 gdb/testsuite/gdb.arch/i386-biarch-core.exp | 29 ++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 6175620..520f606 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-07-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.arch/i386-biarch-core.exp: Replace istarget
+	by "complete set gnutarget". Remove expectation for the "core-file"
+	command.
+
 2015-07-15  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Revert the previous commit:
diff --git a/gdb/testsuite/gdb.arch/i386-biarch-core.exp b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
index 60d049b..bc82287 100644
--- a/gdb/testsuite/gdb.arch/i386-biarch-core.exp
+++ b/gdb/testsuite/gdb.arch/i386-biarch-core.exp
@@ -23,9 +23,20 @@
 
 standard_testfile
 
-if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
-    verbose "Skipping i386-biarch-core test."
-    return
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+set test "complete set gnutarget"
+gdb_test_multiple "complete set gnutarget " $test {
+    -re "set gnutarget elf64-little\r\n(.*\r\n)?$gdb_prompt $" {
+	pass $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass $test
+	untested ".text is readable"
+	return
+    }
 }
 
 set corebz2file ${srcdir}/${subdir}/${testfile}.core.bz2
@@ -43,16 +54,16 @@ if {$corestat(size) != 102400} {
     return -1
 }
 
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-
 # Wrongly built GDB complains by:
 # "..." is not a core dump: File format not recognized
 # As the provided test core has 64bit PRSTATUS i386 built GDB cannot parse it.
 # This is just a problem of the test case, real-world elf64-i386 file will have
 # 32bit PRSTATUS.  One cannot prepare elf64-i386 core file from elf32-i386 by
 # objcopy as it corrupts the core file beyond all recognition.
-gdb_test "core-file ${corefile}" "\r\nwarning: Unexpected size of section `\\.reg/6901' in core file\\.\r\n.*Core was generated by `\[^\r\n\]*'\\.\r\nProgram terminated with signal SIGSEGV, Segmentation fault\\.\r\n.*" "core-file"
+# The output therefore does not matter much, just we should not get GDB
+# internal error.
+gdb_test "core-file ${corefile}" ".*" "core-file"
 
-gdb_test "x/i $address" "\r\n\[ \t\]*$address:\[ \t\]*hlt\[ \t\]*" ".text is readable"
+# Test if at least the core file segments memory has been loaded.
+# https://bugzilla.redhat.com/show_bug.cgi?id=457187
+gdb_test "x/bx $address" "\r\n\[ \t\]*$address:\[ \t\]*0xf4\[ \t\]*" ".text is readable"
-- 
2.1.0