* [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big @ 2015-01-08 16:16 Andreas Arnez 2015-01-08 16:43 ` [testsuite patch] for: " Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Andreas Arnez @ 2015-01-08 16:16 UTC (permalink / raw) To: gdb-patches; +Cc: jan.kratochvil 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-08 16:16 [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big Andreas Arnez @ 2015-01-08 16:43 ` Jan Kratochvil 2015-01-09 9:47 ` Andreas Arnez 2015-02-05 7:38 ` ping: " Jan Kratochvil 0 siblings, 2 replies; 25+ messages in thread From: Jan Kratochvil @ 2015-01-08 16:43 UTC (permalink / raw) To: Andreas Arnez; +Cc: gdb-patches [-- 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" ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-08 16:43 ` [testsuite patch] for: " Jan Kratochvil @ 2015-01-09 9:47 ` Andreas Arnez 2015-01-09 16:45 ` Pedro Alves 2015-02-05 7:38 ` ping: " Jan Kratochvil 1 sibling, 1 reply; 25+ messages in thread From: Andreas Arnez @ 2015-01-09 9:47 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches 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? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 9:47 ` Andreas Arnez @ 2015-01-09 16:45 ` Pedro Alves 2015-01-09 16:59 ` Mark Kettenis 2015-01-09 19:27 ` Andreas Arnez 0 siblings, 2 replies; 25+ messages in thread From: Pedro Alves @ 2015-01-09 16:45 UTC (permalink / raw) To: Andreas Arnez, Jan Kratochvil; +Cc: gdb-patches > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 16:45 ` Pedro Alves @ 2015-01-09 16:59 ` Mark Kettenis 2015-01-09 17:19 ` Pedro Alves 2015-01-09 19:27 ` Andreas Arnez 1 sibling, 1 reply; 25+ messages in thread From: Mark Kettenis @ 2015-01-09 16:59 UTC (permalink / raw) To: palves; +Cc: arnez, jan.kratochvil, gdb-patches > 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 16:59 ` Mark Kettenis @ 2015-01-09 17:19 ` Pedro Alves 2015-01-09 19:35 ` Mark Kettenis 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2015-01-09 17:19 UTC (permalink / raw) To: Mark Kettenis; +Cc: arnez, jan.kratochvil, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 17:19 ` Pedro Alves @ 2015-01-09 19:35 ` Mark Kettenis 2015-01-09 20:11 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Mark Kettenis @ 2015-01-09 19:35 UTC (permalink / raw) To: palves; +Cc: mark.kettenis, arnez, jan.kratochvil, gdb-patches > 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 19:35 ` Mark Kettenis @ 2015-01-09 20:11 ` Pedro Alves 2015-01-09 20:30 ` Mark Kettenis 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2015-01-09 20:11 UTC (permalink / raw) To: Mark Kettenis; +Cc: arnez, jan.kratochvil, gdb-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 20:11 ` Pedro Alves @ 2015-01-09 20:30 ` Mark Kettenis 2015-01-12 14:30 ` Andreas Arnez 0 siblings, 1 reply; 25+ messages in thread From: Mark Kettenis @ 2015-01-09 20:30 UTC (permalink / raw) To: palves; +Cc: mark.kettenis, arnez, jan.kratochvil, gdb-patches > 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 20:30 ` Mark Kettenis @ 2015-01-12 14:30 ` Andreas Arnez 0 siblings, 0 replies; 25+ messages in thread From: Andreas Arnez @ 2015-01-12 14:30 UTC (permalink / raw) To: Mark Kettenis; +Cc: palves, jan.kratochvil, gdb-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-09 16:45 ` Pedro Alves 2015-01-09 16:59 ` Mark Kettenis @ 2015-01-09 19:27 ` Andreas Arnez 1 sibling, 0 replies; 25+ messages in thread From: Andreas Arnez @ 2015-01-09 19:27 UTC (permalink / raw) To: Pedro Alves; +Cc: Jan Kratochvil, gdb-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-01-08 16:43 ` [testsuite patch] for: " Jan Kratochvil 2015-01-09 9:47 ` Andreas Arnez @ 2015-02-05 7:38 ` Jan Kratochvil 2015-02-05 9:47 ` Pedro Alves 1 sibling, 1 reply; 25+ messages in thread From: Jan Kratochvil @ 2015-02-05 7:38 UTC (permalink / raw) To: gdb-patches; +Cc: Andreas Arnez [-- 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" ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-05 7:38 ` ping: " Jan Kratochvil @ 2015-02-05 9:47 ` Pedro Alves 2015-02-14 15:12 ` Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2015-02-05 9:47 UTC (permalink / raw) To: Jan Kratochvil, gdb-patches; +Cc: Andreas Arnez 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-05 9:47 ` Pedro Alves @ 2015-02-14 15:12 ` Jan Kratochvil 2015-02-17 12:56 ` Pedro Alves 0 siblings, 1 reply; 25+ messages in thread From: Jan Kratochvil @ 2015-02-14 15:12 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Andreas Arnez [-- 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" ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-14 15:12 ` Jan Kratochvil @ 2015-02-17 12:56 ` Pedro Alves 2015-02-17 16:56 ` Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Pedro Alves @ 2015-02-17 12:56 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Andreas Arnez 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-17 12:56 ` Pedro Alves @ 2015-02-17 16:56 ` Jan Kratochvil 2015-02-21 14:28 ` [commit] " Jan Kratochvil 2015-07-14 8:52 ` ping: " Yao Qi 0 siblings, 2 replies; 25+ messages in thread From: Jan Kratochvil @ 2015-02-17 16:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Andreas Arnez [-- 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" ^ permalink raw reply [flat|nested] 25+ messages in thread
* [commit] [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-17 16:56 ` Jan Kratochvil @ 2015-02-21 14:28 ` Jan Kratochvil 2015-07-14 8:52 ` ping: " Yao Qi 1 sibling, 0 replies; 25+ messages in thread From: Jan Kratochvil @ 2015-02-21 14:28 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Andreas Arnez 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-02-17 16:56 ` Jan Kratochvil 2015-02-21 14:28 ` [commit] " Jan Kratochvil @ 2015-07-14 8:52 ` Yao Qi 2015-07-14 18:07 ` Jan Kratochvil 1 sibling, 1 reply; 25+ messages in thread From: Yao Qi @ 2015-07-14 8:52 UTC (permalink / raw) To: Jan Kratochvil, Pedro Alves; +Cc: gdb-patches, Andreas Arnez 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 (é½å°§) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-14 8:52 ` ping: " Yao Qi @ 2015-07-14 18:07 ` Jan Kratochvil 2015-07-15 16:14 ` Yao Qi 0 siblings, 1 reply; 25+ messages in thread From: Jan Kratochvil @ 2015-07-14 18:07 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, gdb-patches, Andreas Arnez [-- 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" ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-14 18:07 ` Jan Kratochvil @ 2015-07-15 16:14 ` Yao Qi 2015-07-15 16:58 ` Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2015-07-15 16:14 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Yao Qi, Pedro Alves, gdb-patches, Andreas Arnez 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 (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-15 16:14 ` Yao Qi @ 2015-07-15 16:58 ` Jan Kratochvil 2015-07-16 14:15 ` Yao Qi 0 siblings, 1 reply; 25+ messages in thread From: Jan Kratochvil @ 2015-07-15 16:58 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, gdb-patches, Andreas Arnez 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-15 16:58 ` Jan Kratochvil @ 2015-07-16 14:15 ` Yao Qi 2015-07-16 14:37 ` Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2015-07-16 14:15 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Yao Qi, Pedro Alves, gdb-patches, Andreas Arnez 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 (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-16 14:15 ` Yao Qi @ 2015-07-16 14:37 ` Jan Kratochvil 2015-07-16 15:35 ` Yao Qi 0 siblings, 1 reply; 25+ messages in thread From: Jan Kratochvil @ 2015-07-16 14:37 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, gdb-patches, Andreas Arnez 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ping: [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-16 14:37 ` Jan Kratochvil @ 2015-07-16 15:35 ` Yao Qi 2015-07-16 16:10 ` [commit] " Jan Kratochvil 0 siblings, 1 reply; 25+ messages in thread From: Yao Qi @ 2015-07-16 15:35 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Yao Qi, Pedro Alves, gdb-patches, Andreas Arnez 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 (齐尧) ^ permalink raw reply [flat|nested] 25+ messages in thread
* [commit] [testsuite patch] for: [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big 2015-07-16 15:35 ` Yao Qi @ 2015-07-16 16:10 ` Jan Kratochvil 0 siblings, 0 replies; 25+ messages in thread From: Jan Kratochvil @ 2015-07-16 16:10 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, gdb-patches, Andreas Arnez [-- 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2015-07-16 16:10 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-08 16:16 [PATCH] [PR corefiles/17808] i386: Fix internal error when prstatus in core file is too big Andreas Arnez 2015-01-08 16:43 ` [testsuite patch] for: " Jan Kratochvil 2015-01-09 9:47 ` Andreas Arnez 2015-01-09 16:45 ` Pedro Alves 2015-01-09 16:59 ` Mark Kettenis 2015-01-09 17:19 ` Pedro Alves 2015-01-09 19:35 ` Mark Kettenis 2015-01-09 20:11 ` Pedro Alves 2015-01-09 20:30 ` Mark Kettenis 2015-01-12 14:30 ` Andreas Arnez 2015-01-09 19:27 ` Andreas Arnez 2015-02-05 7:38 ` ping: " Jan Kratochvil 2015-02-05 9:47 ` Pedro Alves 2015-02-14 15:12 ` Jan Kratochvil 2015-02-17 12:56 ` Pedro Alves 2015-02-17 16:56 ` Jan Kratochvil 2015-02-21 14:28 ` [commit] " Jan Kratochvil 2015-07-14 8:52 ` ping: " Yao Qi 2015-07-14 18:07 ` Jan Kratochvil 2015-07-15 16:14 ` Yao Qi 2015-07-15 16:58 ` Jan Kratochvil 2015-07-16 14:15 ` Yao Qi 2015-07-16 14:37 ` Jan Kratochvil 2015-07-16 15:35 ` Yao Qi 2015-07-16 16:10 ` [commit] " Jan Kratochvil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox