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

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

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.
  

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
  

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?
  

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
  

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.