Message ID | m31smcp3b6.fsf@oc1027705133.ibm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 13246 invoked by alias); 9 Oct 2017 18:46:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 13236 invoked by uid 89); 9 Oct 2017 18:46:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=encouraged, H*o:GmbH, lift, feels X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 18:46:29 +0000 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v99IjIsq036408 for <gdb-patches@sourceware.org>; Mon, 9 Oct 2017 14:46:27 -0400 Received: from e06smtp10.uk.ibm.com (e06smtp10.uk.ibm.com [195.75.94.106]) by mx0b-001b2d01.pphosted.com with ESMTP id 2dgc1m8p4s-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for <gdb-patches@sourceware.org>; Mon, 09 Oct 2017 14:46:27 -0400 Received: from localhost by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for <gdb-patches@sourceware.org> from <arnez@linux.vnet.ibm.com>; Mon, 9 Oct 2017 19:46:26 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp10.uk.ibm.com (192.168.101.140) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 9 Oct 2017 19:46:24 +0100 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v99IkOSE25100332; Mon, 9 Oct 2017 18:46:24 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id CE36142041; Mon, 9 Oct 2017 19:42:08 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AD5964203F; Mon, 9 Oct 2017 19:42:08 +0100 (BST) Received: from oc1027705133.ibm.com (unknown [9.152.212.164]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Mon, 9 Oct 2017 19:42:08 +0100 (BST) From: Andreas Arnez <arnez@linux.vnet.ibm.com> To: Kevin Buettner <kevinb@redhat.com> Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] GDB test suite: Add helper for locating core files References: <1505760152-28775-1-git-send-email-arnez@linux.vnet.ibm.com> <1505760152-28775-2-git-send-email-arnez@linux.vnet.ibm.com> <20171007094545.1bba5c51@pinnacle.lan> Date: Mon, 09 Oct 2017 20:46:21 +0200 In-Reply-To: <20171007094545.1bba5c51@pinnacle.lan> (Kevin Buettner's message of "Sat, 7 Oct 2017 09:45:45 -0700") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 x-cbid: 17100918-0040-0000-0000-000003E107E2 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17100918-0041-0000-0000-000025E30830 Message-Id: <m31smcp3b6.fsf@oc1027705133.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-10-09_05:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710090272 X-IsSubscribed: yes |
Commit Message
Andreas Arnez
Oct. 9, 2017, 6:46 p.m. UTC
On Sat, Oct 07 2017, Kevin Buettner wrote: [...] > E.g. when I test your patch on my x86-64 linux box using the > following command... > > make check RUNTESTFLAGS="--target_board=native-gdbserver" > > ...I see 32 fewer passes than before and also one more known failure. > > Here are the passes that no longer occur when using your patch: [...] > Instead, several warnings are now printed instead: > > WARNING: Can not generate core dump on remote target. These warnings are newly introduced by the patch. They are meant to improve diagnostics when someone attempts to run the tests on a "real" remote target. I wanted to clearly document the fact that this is unsupported (and always was). Also, by documenting this restriction, maybe someone feels encouraged to lift it ;-) But it seems I went overboard, now also bailing out in case of native-gdbserver, which is unnecessary, since no extra handling is required for that. How to fix this, though? Rather than bailing out on "is_remote target", maybe we should check for "isnative" instead? See the delta-patch below. This should fix the problem with native-gdbserver and is probably not worse than before in other scenarios, so maybe it's good enough. WDYT? > If you can restore support for handling of remote core files, I'd very > much like to review this patch again. Thanks, I'd appreciate that. -- Andreas -- >8 -- Subject: [PATCH] Squash into "GDB test suite: Add helper for locating core files"
Comments
On Mon, 09 Oct 2017 20:46:21 +0200 Andreas Arnez <arnez@linux.vnet.ibm.com> wrote: > > Instead, several warnings are now printed instead: > > > > WARNING: Can not generate core dump on remote target. > > These warnings are newly introduced by the patch. Yes, I saw that. > They are meant to > improve diagnostics when someone attempts to run the tests on a "real" > remote target. I wanted to clearly document the fact that this is > unsupported (and always was). Also, by documenting this restriction, > maybe someone feels encouraged to lift it ;-) In the distant past, I used to run the testsuite against resource constrained linux machines often of a different architecture from the host I was running the tests from. These machines would run gdbserver built for that architecture. Now, I don't recall whether corefile support in the testsuite actually worked for those targets, but it at least seems possible due to the various invocations of remote_exec which are present (prior to your patch). Do you think you could restore those calls to remote_exec in your patch? Or do you know for a fact that they do not work? Kevin
On Wed, Oct 11 2017, Kevin Buettner wrote: > On Mon, 09 Oct 2017 20:46:21 +0200 > Andreas Arnez <arnez@linux.vnet.ibm.com> wrote: > >> > Instead, several warnings are now printed instead: >> > >> > WARNING: Can not generate core dump on remote target. >> >> These warnings are newly introduced by the patch. > > Yes, I saw that. > >> They are meant to >> improve diagnostics when someone attempts to run the tests on a "real" >> remote target. I wanted to clearly document the fact that this is >> unsupported (and always was). Also, by documenting this restriction, >> maybe someone feels encouraged to lift it ;-) > > In the distant past, I used to run the testsuite against resource > constrained linux machines often of a different architecture from the > host I was running the tests from. These machines would run gdbserver > built for that architecture. OK, in that scenario, core file test cases would need to be roughly executed in four steps: (1) Build executable. (2) Run executable on the remote machine. (3) Transfer core dump to the local host. (4) Start GDB on the local host. AFAIK, steps (2) and (3) have not been implemented for the GDB test suite so far. > Now, I don't recall whether corefile support in the testsuite actually > worked for those targets, but it at least seems possible due to the > various invocations of remote_exec which are present (prior to your > patch). Fairly unlikely. See, for instance, the beginning of corefile.exp: # are we on a target board if ![isnative] then { return } A check like this appears in all core dump test cases. Also note that the core_find command in gdb.exp uses expect's "system" command for invoking the executable: catch "system \"(cd ${coredir}; ulimit -c unlimited; ${binfile} ${arg}; true) >/dev/null 2>&1\"" Which means that this command line will be executed on the local machine and will fail in a remote setup. But you're right that there were remote_exec invocations; mostly for file handling such as: remote_exec build "mv $i $destcore" remote_exec build "mv $corefile $destcore" remote_exec build "rmdir $coredir" TBH, I don't quite understand their intention. In a native remote setup, "build" is the remote machine. Moving the core file around there doesn't make it appear on the local machine. And in a cross remote setup, "build" is the local machine, whereas the core dump should be generated on the remote machine. So I think these commands only work if host == build == target. Since I found this too misleading, I replaced them by local commands. > Do you think you could restore those calls to remote_exec in your > patch? Or do you know for a fact that they do not work? I'm pretty sure they don't. Thus I wanted to document this restriction more clearly. -- Andreas
On 10/09/2017 07:46 PM, Andreas Arnez wrote: > @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} { > # specified. Return that path name, or "" if no core file was found. > > proc find_core {binfile coredir {destcore ""}} { > - if {[is_remote target]} { > + if {![isnative]} { > warning "Can not access remote core file." > return "" > } This seems incorrect to me. "isnative" only checks if the build and target _triplets_ are the same. So foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate machine still returns isnative==true. I think the real problem is that the native-gdbserver board returns true to is_remote, when I think it shouldn't. Doing that alone results in fallout in the testsuite, of course. I'm seeing if fixing it is doable. Thanks, Pedro Alves
On Thu, Oct 12 2017, Pedro Alves wrote: > On 10/09/2017 07:46 PM, Andreas Arnez wrote: > >> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} { >> # specified. Return that path name, or "" if no core file was found. >> >> proc find_core {binfile coredir {destcore ""}} { >> - if {[is_remote target]} { >> + if {![isnative]} { >> warning "Can not access remote core file." >> return "" >> } > > This seems incorrect to me. "isnative" only checks > if the build and target _triplets_ are the same. So > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate > machine still returns isnative==true. Exactly, that's why I originally wrote is_remote instead. And I also wondered why the core dump tests check isnative. Does anyone run the testsuite on a native remote setup? > I think the real problem is that the native-gdbserver board > returns true to is_remote, when I think it shouldn't. > > Doing that alone results in fallout in the testsuite, of > course. I'm seeing if fixing it is doable. Right, this seems all a bit mixed up to me, similar to the confusing remote_file and remote_exec operations in core_find. -- Andreas
On Thu, 12 Oct 2017, Andreas Arnez wrote: > > This seems incorrect to me. "isnative" only checks > > if the build and target _triplets_ are the same. So > > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate > > machine still returns isnative==true. > > Exactly, that's why I originally wrote is_remote instead. And I also > wondered why the core dump tests check isnative. Does anyone run the > testsuite on a native remote setup? Well, I had cases where I did that, and offhand I can find two reasons: 1. You want to verify `gdbserver' itself rather than GDB, in which case you may even run it locally (i.e. where the remote target is really `localhost'). 2. You want to test a feature (e.g. an extra register set) only your target system has and it is too slow or unequipped to run DejaGNU itself. Maciej
On Fri, Oct 13 2017, Maciej W. Rozycki wrote: > On Thu, 12 Oct 2017, Andreas Arnez wrote: > >> > This seems incorrect to me. "isnative" only checks >> > if the build and target _triplets_ are the same. So >> > foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate >> > machine still returns isnative==true. >> >> Exactly, that's why I originally wrote is_remote instead. And I also >> wondered why the core dump tests check isnative. Does anyone run the >> testsuite on a native remote setup? > > Well, I had cases where I did that, and offhand I can find two reasons: > > 1. You want to verify `gdbserver' itself rather than GDB, in which case > you may even run it locally (i.e. where the remote target is really > `localhost'). This is the native-gdbserver setup, right? In that case the core dump tests should work... > 2. You want to test a feature (e.g. an extra register set) only your > target system has and it is too slow or unequipped to run DejaGNU > itself. ...and in this case they shouldn't. Do you remember whether you saw FAILs from corefile.exp and friends? -- Andreas
On 10/09/2017 07:46 PM, Andreas Arnez wrote: > On Sat, Oct 07 2017, Kevin Buettner wrote: > > [...] > >> E.g. when I test your patch on my x86-64 linux box using the >> following command... >> >> make check RUNTESTFLAGS="--target_board=native-gdbserver" >> >> ...I see 32 fewer passes than before and also one more known failure. >> >> Here are the passes that no longer occur when using your patch: > > [...] > >> Instead, several warnings are now printed instead: >> >> WARNING: Can not generate core dump on remote target. > > These warnings are newly introduced by the patch. They are meant to > improve diagnostics when someone attempts to run the tests on a "real" > remote target. I wanted to clearly document the fact that this is > unsupported (and always was). Also, by documenting this restriction, > maybe someone feels encouraged to lift it ;-) Wouldn't an UNTESTED or UNSUPPORTED be better? It's what we tend to do with other cases of unsupported/untested tests. Those convey what happened to the testscase, and are accounted for and an untested/unsupported count is shown at the end of a test run, while a count of warnings isn't. Thanks, Pedro Alves
On 10/12/2017 06:00 PM, Andreas Arnez wrote: > On Thu, Oct 12 2017, Pedro Alves wrote: > >> On 10/09/2017 07:46 PM, Andreas Arnez wrote: >> >>> @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} { >>> # specified. Return that path name, or "" if no core file was found. >>> >>> proc find_core {binfile coredir {destcore ""}} { >>> - if {[is_remote target]} { >>> + if {![isnative]} { >>> warning "Can not access remote core file." >>> return "" >>> } >> >> This seems incorrect to me. "isnative" only checks >> if the build and target _triplets_ are the same. So >> foo-linux-gnu gdb x foo-linux-gnu gdbserver on separate >> machine still returns isnative==true. > > Exactly, that's why I originally wrote is_remote instead. And I also > wondered why the core dump tests check isnative. Does anyone run the > testsuite on a native remote setup? I think most if not all isnative checks are stale/bogus, and likely were introduced at a time when cross debugging, including cross-core debugging wasn't that well supported. I'd be delighted to see them cleaned up and replaced with a more appropriate check. > Right, this seems all a bit mixed up to me, similar to the confusing > remote_file and remote_exec operations in core_find. I wonder whether it'd be equally simple to move in the opposite direction of making these operations use the right FOO "remote_exec FOO", making it easier to actually support remote core dump testing. (Maybe not by default, since uploading the cores may be slow, but with a board option/setting.) Thanks, Pedro Alves
On Fri, 13 Oct 2017, Andreas Arnez wrote: > >> Exactly, that's why I originally wrote is_remote instead. And I also > >> wondered why the core dump tests check isnative. Does anyone run the > >> testsuite on a native remote setup? > > > > Well, I had cases where I did that, and offhand I can find two reasons: > > > > 1. You want to verify `gdbserver' itself rather than GDB, in which case > > you may even run it locally (i.e. where the remote target is really > > `localhost'). > > This is the native-gdbserver setup, right? In that case the core dump > tests should work... Yes and indeed. > > 2. You want to test a feature (e.g. an extra register set) only your > > target system has and it is too slow or unequipped to run DejaGNU > > itself. > > ...and in this case they shouldn't. Do you remember whether you saw > FAILs from corefile.exp and friends? Nope, sorry. It was a while ago and I only looked for regressions with whatever I meant to verify and not preexisting failures anyway. Maciej
On Tue, 17 Oct 2017, Pedro Alves wrote: > >> E.g. when I test your patch on my x86-64 linux box using the > >> following command... > >> > >> make check RUNTESTFLAGS="--target_board=native-gdbserver" > >> > >> ...I see 32 fewer passes than before and also one more known failure. > >> > >> Here are the passes that no longer occur when using your patch: > > > > [...] > > > >> Instead, several warnings are now printed instead: > >> > >> WARNING: Can not generate core dump on remote target. > > > > These warnings are newly introduced by the patch. They are meant to > > improve diagnostics when someone attempts to run the tests on a "real" > > remote target. I wanted to clearly document the fact that this is > > unsupported (and always was). Also, by documenting this restriction, > > maybe someone feels encouraged to lift it ;-) > > Wouldn't an UNTESTED or UNSUPPORTED be better? It's what > we tend to do with other cases of unsupported/untested tests. I think UNSUPPORTED is the right status; UNTESTED is meant for missing tests really; see: UNTESTED A test was not run. This is a placeholder, used when there is no real test case yet. vs: UNSUPPORTED There is no support for the tested case. This may mean that a conditional feature of an operating system, or of a compiler, is not implemented. DejaGnu also uses this message when a testing environment (often a "bare board" target) lacks basic support for compiling or running the test case. For example, a test for the system subroutine gethostname would never work on a target board running only a boot monitor. (taken from <http://www.delorie.com/gnu/docs/dejagnu/dejagnu_6.html>). FWIW, Maciej
Hi Maciej, On 10/17/2017 07:19 PM, Maciej W. Rozycki wrote: > On Tue, 17 Oct 2017, Pedro Alves wrote: >> Wouldn't an UNTESTED or UNSUPPORTED be better? It's what >> we tend to do with other cases of unsupported/untested tests. > > I think UNSUPPORTED is the right status; UNTESTED is meant for missing > tests really; see: > > UNTESTED > A test was not run. This is a placeholder, used when there is no real > test case yet. > > vs: > > UNSUPPORTED > There is no support for the tested case. This may mean that a > conditional feature of an operating system, or of a compiler, is not > implemented. DejaGnu also uses this message when a testing > environment (often a "bare board" target) lacks basic support for > compiling or running the test case. For example, a test for the > system subroutine gethostname would never work on a target board > running only a boot monitor. > IMHO, this "placeholder" status of UNTESTED is pretty useless, at least for GDB. I don't recall ever adding such a placeholder testcase, and I don't think GDB uses it like that. I think a more useful distinction would be: - UNSUPPORTED to indicate that the feature isn't supported by GDB or the remote stub. E.g., in this case we'd use it if the GDB port does not support debugging core dumps at all. - UNTESTED to indicate that the feature is supported by GDB but the test wasn't run because it's not possible to run it, or we choose to not run it, in the current test environment, e.g., because of a board limitation. In this case, we skip core tests when the host or target boards are remote, even though the feature is supported by GDB (even with "target remote" and cross debugging, at least on some ports). We use UNTESTED in many places already meaning something like the above (instead of placeholder status), though I'm not aware of rationale written down anywhere. The above is just my intuition, and most of these untested/unsupported calls predate my involvement with GDB. Thanks, Pedro Alves
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 2c5e94d..091933a 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5850,7 +5850,7 @@ proc exec_in_dir {dir command} { # Return the core file's filename, or "", if none was found. proc run_and_get_core {binfile {arg ""}} { - if {[is_remote target]} { + if {![isnative]} { warning "Can not generate core dump on remote target." return "" } @@ -5883,7 +5883,7 @@ proc run_and_get_core {binfile {arg ""}} { # specified. Return that path name, or "" if no core file was found. proc find_core {binfile coredir {destcore ""}} { - if {[is_remote target]} { + if {![isnative]} { warning "Can not access remote core file." return "" }