Message ID | 1487859197-6269-1-git-send-email-lgustavo@codesourcery.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 26767 invoked by alias); 23 Feb 2017 14:13:26 -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 26756 invoked by uid 89); 23 Feb 2017 14:13:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=H*M:send, our X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Feb 2017 14:13:24 +0000 Received: from svr-orw-fem-04.mgc.mentorg.com ([147.34.97.41]) by relay1.mentorg.com with esmtp id 1cgu94-0004eU-Pu from Luis_Gustavo@mentor.com for gdb-patches@sourceware.org; Thu, 23 Feb 2017 06:13:22 -0800 Received: from Opsys.world.mentorg.com (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.3.224.2; Thu, 23 Feb 2017 06:13:21 -0800 From: Luis Machado <lgustavo@codesourcery.com> To: <gdb-patches@sourceware.org> Subject: [PATCH] Restrict gdb.base/gcore-relro-pie.exp to native/linux targets Date: Thu, 23 Feb 2017 08:13:17 -0600 Message-ID: <1487859197-6269-1-git-send-email-lgustavo@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Luis Machado
Feb. 23, 2017, 2:13 p.m. UTC
This particular test doesn't work correctly on aarch64-elf for a couple reasons. First, the test is supposed to crash and bare-metal targets are not always ready to handle such exceptions like Linux does. In our case the board just keeps cycling within an ISR function. Second, core file generation is not guaranteed/not supposed to work with bare-metal targets. If it does, it may just be luck. Therefore the following patch restricts this particular test to only native and Linux targets, like some other core file tests. I see other core file tests that are not restricted to native/linux, but i don't see failures due to those. It doesn't mean they are correct in their assumptions though. I could fix those up in a single batch if it is deemed appropriate. Otherwise i'm happy with fixing only this one. gdb/testsuite/ChangeLog: 2017-02-23 Luis Machado <lgustavo@codesourcery.com> * gdb.base/gcore-relro-pie.exp: Restrict to native/linux targets. --- gdb/testsuite/gdb.base/gcore-relro-pie.exp | 5 +++++ 1 file changed, 5 insertions(+)
Comments
On 02/23/2017 02:13 PM, Luis Machado wrote: > This particular test doesn't work correctly on aarch64-elf for a couple > reasons. > > First, the test is supposed to crash and bare-metal targets are not always > ready to handle such exceptions like Linux does. In our case the board just > keeps cycling within an ISR function. Hmm? AFAICS, the testcase is generating the core with gdb_gcore_cmd. How on earth is that causing this cycling? Either I'm missing something, or the reason for the cycling is something else, like for example, the fact that the test is compiled with flags (-fpie -pie Wl,-z,relro) that might generate a binary that does not work correctly, or strip is broken for that target, or something like that. Thanks, Pedro Alves
On 02/23/2017 08:24 AM, Pedro Alves wrote: > On 02/23/2017 02:13 PM, Luis Machado wrote: >> This particular test doesn't work correctly on aarch64-elf for a couple >> reasons. >> >> First, the test is supposed to crash and bare-metal targets are not always >> ready to handle such exceptions like Linux does. In our case the board just >> keeps cycling within an ISR function. > > Hmm? AFAICS, the testcase is generating the core with gdb_gcore_cmd. > How on earth is that causing this cycling? I tend to see this particular target stopping later in the source file compared to x86. I had an (incorrect) assumption it was dereferencing NULL when running to break_here, but it doesn't seem to be the case. It stops right at what would cause the crash: (gdb) b break_here Breakpoint 2 at 0x80000a20 (gdb) disass break_here Dump of assembler code for function break_here: 0x0000000080000a1c <+0>: mov x0, #0x0 0x0000000080000a20 <+4>: str wzr, [x0] It seems something is off with startup code/stack pointer then. Back to the drawing board. I still think it doesn't make much sense to run these tests if we're not sure gcore will support them. They may run a few early tests/setup tests, but that won't translate into meaningful PASSes. But i'm ok keeping it as-is if others think the early test PASSes are useful.
On 02/23/2017 02:47 PM, Luis Machado wrote: > I still think it doesn't make much sense to run these tests if we're not > sure gcore will support them. I don't understand what you're saying. We can't be sure up front. The "gcore" that is run is GDB's "gcore" command. If that doesn't work, gdb_gcore_cmd calls unsupported, and the rest of the testcase is skipped. > They may run a few early tests/setup > tests, but that won't translate into meaningful PASSes. But i'm ok > keeping it as-is if others think the early test PASSes are useful. Looks like it's been useful to catch a startup code problem. ;-) Thanks, Pedro Alves
On 02/23/2017 08:54 AM, Pedro Alves wrote: > On 02/23/2017 02:47 PM, Luis Machado wrote: > >> I still think it doesn't make much sense to run these tests if we're not >> sure gcore will support them. > > I don't understand what you're saying. We can't be sure up > front. The "gcore" that is run is GDB's "gcore" command. If that > doesn't work, gdb_gcore_cmd calls unsupported, and the rest of the > testcase is skipped. > The point is that we are indeed sure this isn't supported, unless we officially support core files on bare-metal targets (i don't think we do). See below. >> They may run a few early tests/setup >> tests, but that won't translate into meaningful PASSes. But i'm ok >> keeping it as-is if others think the early test PASSes are useful. > > Looks like it's been useful to catch a startup code problem. ;-) Don't you agree this is a clear sign we are really testing something else other than core file support? It just means a proper standalone test doesn't exist to catch such a problem and we got lucky crashing when doing a core file test on a target that doesn't support it. It is not like the test was designed to catch this. I find it a bit messy. A proper test would attempt to run pie executables to completion (and i can contribute that to verify this particular problem). I just don't like the concept of running a test that is unsupported on a particular target (core files) only to test a side-effect during pre-test/setup phases. It only adds to artificial PASSes that don't necessarily translate to robustness. But i understand if this is not acceptable. I just want to clean this target up, and seeing core file tests being executed is just confusing. :-)
On 02/23/2017 03:10 PM, Luis Machado wrote: > On 02/23/2017 08:54 AM, Pedro Alves wrote: >> On 02/23/2017 02:47 PM, Luis Machado wrote: >> >>> I still think it doesn't make much sense to run these tests if we're not >>> sure gcore will support them. >> >> I don't understand what you're saying. We can't be sure up >> front. The "gcore" that is run is GDB's "gcore" command. If that >> doesn't work, gdb_gcore_cmd calls unsupported, and the rest of the >> testcase is skipped. >> > > The point is that we are indeed sure this isn't supported, unless we > officially support core files on bare-metal targets (i don't think we > do). There's been talks / patches about adding that for years. Unfortunately it hasn't happened yet, but I think it'd be quite reasonable to have. > See below. Note your patch did not skip the test on bare metal targets. That's not even what the subject says. You've skipped it everywhere but Linux. It's not quite the same thing. > >>> They may run a few early tests/setup >>> tests, but that won't translate into meaningful PASSes. But i'm ok >>> keeping it as-is if others think the early test PASSes are useful. >> >> Looks like it's been useful to catch a startup code problem. ;-) > > Don't you agree this is a clear sign we are really testing something > else other than core file support? Not really. It's a sign that the test is missing a predicate, but it's not "cores work", IMO. The testcase has a few requirements. At least: #1 - PIEs make sense for this target. #2 - gcore works To me this is a clear sign a predicate for #1 is missing. We already detect #2. Other tests build PIEs too. IOW, IMO, all this talk about core files is a distraction from the real issue. But then I wonder why does compilation, linking, loading all succeed without something realizing the binary can't run on this target. It may well be that PIEs do make sense for this target. It's not uncommon to call things "bare-metal" when they're really pretty sophisticated RTOSs. > It just means a proper standalone > test doesn't exist to catch such a problem and we got lucky crashing > when doing a core file test on a target that doesn't support it. It is > not like the test was designed to catch this. > > I find it a bit messy. A proper test would attempt to run pie > executables to completion (and i can contribute that to verify this > particular problem). How would that help, if it'd crash and loop the board too? > I just don't like the concept of running a test that is unsupported on a > particular target (core files) only to test a side-effect during > pre-test/setup phases. It only adds to artificial PASSes that don't > necessarily translate to robustness. > > But i understand if this is not acceptable. I just want to clean this > target up, and seeing core file tests being executed is just confusing. :-) Thanks, Pedro Alves
On 02/23/2017 09:43 AM, Pedro Alves wrote: > There's been talks / patches about adding that for years. > Unfortunately it hasn't happened yet, but I think it'd > be quite reasonable to have. I find it reasonable too, but it is not here yet. > Not really. It's a sign that the test is missing a predicate, but > it's not "cores work", IMO. The testcase has a few requirements. > At least: > > #1 - PIEs make sense for this target. > #2 - gcore works > > To me this is a clear sign a predicate for #1 is missing. > We already detect #2. Other tests build PIEs too. > > IOW, IMO, all this talk about core files is a distraction > from the real issue. > > But then I wonder why does compilation, linking, loading all > succeed without something realizing the binary can't run on > this target. It may well be that PIEs do make sense for this > target. It's not uncommon to call things "bare-metal" when > they're really pretty sophisticated RTOSs. This is just a stack pointer startup problem, so not a matter of supporting PIE's or not. > How would that help, if it'd crash and loop the board too? > The test would need to be tweaked. The assumption that these targets can handle a crash is not always valid as it is with OS-based targets. I remember i addressed problems with these assumptions in the past. It is similar to assuming reading 0x0 would error out or that it is a bad address. We are much more aware of that problem these days. Unfortunately not a lot of bare-metal/RTOS testing going on for gdb at the moment. I think even QEMU shields these tests from problems sometimes. Anyway, i know what i have to do to get this FAIL fixed! The discussion about how focused/robust a particular gdb test is/needs to be can be rather long. At this point i don't think there is a lot of benefit from that, but maybe a worthy discussion in the future about how to improve gdb's testsuite, or not!
diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.exp b/gdb/testsuite/gdb.base/gcore-relro-pie.exp index 28321fc..fd255e4 100644 --- a/gdb/testsuite/gdb.base/gcore-relro-pie.exp +++ b/gdb/testsuite/gdb.base/gcore-relro-pie.exp @@ -19,6 +19,11 @@ standard_testfile +if {![isnative] || ![istarget "*-*-linux*"]} { + untested "skipping core file test" + return 0 +} + if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug additional_flags=-fpie "ldflags=-pie -Wl,-z,relro"}]} { return -1 }