From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10903 invoked by alias); 19 Sep 2019 21:50:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 10895 invoked by uid 89); 19 Sep 2019 21:50:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-13.1 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Sep 2019 21:50:15 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id A412BAE89; Thu, 19 Sep 2019 21:50:13 +0000 (UTC) Subject: Re: [PATCH][gdb/testsuite] Introduce gdb_test_ext To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <20190919111322.GA29391@delia> <20190919161846.GC4962@embecosm.com> <62b20c8f-6792-c17e-621a-946002df6df9@suse.de> <20190919192423.GF4962@embecosm.com> From: Tom de Vries Openpgp: preference=signencrypt Message-ID: <2f49f106-bbc5-43af-968d-e37928f6ede8@suse.de> Date: Thu, 19 Sep 2019 21:50:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190919192423.GF4962@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00384.txt.bz2 On 19-09-19 21:24, Andrew Burgess wrote: > * Tom de Vries [2019-09-19 21:01:01 +0200]: > >> On 19-09-19 18:18, Andrew Burgess wrote: >>> * Tom de Vries [2019-09-19 13:13:23 +0200]: >>> >>>> Hi, >>>> >>>> In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp >>>> to be unsupported" we replace a gdb_test: >>>> ... >>>> gdb_test "print l" " = ${l}" \ >>>> "${prefix}; print old l, expecting ${l}" >>>> ... >>>> with a gdb_test_multiple: >>>> ... >>>> set supported 1 >>>> set test "${prefix}; print old l, expecting ${l}" >>>> gdb_test_multiple "print l" "$test" { >>>> -re " = \r\n$gdb_prompt $" { >>>> unsupported $test >>>> set supported 0 >>>> } >>>> -re " = ${l}\r\n$gdb_prompt $" { >>>> pass $test >>>> } >>>> } >>>> ... >>>> in order to handle the UNSUPPORTED case. >>>> >>>> This has the drawback that we have to be explicit about the gdb_prompt, and >>>> move the gdb_test arguments around to fit the gdb_test_multiple format. >>>> >>>> Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows >>>> extension, allowing us to rewrite the gdb_test_multiple above in a form >>>> resembling the original gdb_test: >>>> ... >>>> set supported 1 >>>> gdb_test_ext "print l" " = ${l}" \ >>>> "${prefix}; print old l, expecting ${l}" \ >>>> -- [list "unsupported" " = " "set supported 0"] >>> >>> I've also thought about this sort of problem in the past, and would >>> like to propose a similar, but slightly different solution. >>> >>> My idea is more like a trimmed down gdb_test_multiple, so for your >>> example above you would write this: >>> >>> gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" { >>> " = ${l}" { >>> pass $gdb_test_ext_name >>> } >>> " = " { >>> unsupported $gdb_test_ext_name >>> set supported 0 >>> } >>> } >>> >>> You don't put '-re' before each pattern, this is because they aren't >>> full patterns, gdb_test_ext will be extending them. >>> >>> Unlike your solution the 'pass' case is not created automatically, you >>> need to write it yourself, so maybe that's a negative. The advantages >>> I see of this approach is that there's not special handling for >>> different "types" of alternatives as in your original code, the action >>> block can contain anything 'unsupported', 'fail', etc. Plus it's >>> formatted as a code body, which I like. >>> >> >> The solution as I proposed it doesn't limit itself to require each >> alternative to be handled as either supported or pass or fail or >> somesuch. It just adds a means to extend gdb_test using a keyword that >> determines how the keyword arguments are handled. > > I don't really understand what you're trying to say here, sorry. > Looking at the code it still appears that each case would need to be > added specifically, so if tomorrow I want a 'fail' case, I'd need to > add it to lib/gdb.exp - or am I not understanding? > I thought you were arguing that it was not possible handle things generically with my proposed solution (which is not the case, as I tried to demonstrate by adding the "generic" method), so I tried to argue against that. But I guess I misunderstood you there. So confirmed, adding a fail case requires adding to gdb_test_ext in gdb.exp. [ Although in the latest version you could handle it using the "generic" method, but I'd prefer to add a "fail" method instead. ] >> >> So, I've added the style you propose here as "generic", and rewrote one >> of the two places I update in store.exp using the "generic" style for >> demonstration purposes. >> >> I envision the usage like this: you'd usually use "unsupported" or >> similar to skip all the repetitive code and focus on the bits that >> actually contain content, and for special cases where that doesn't fit >> you'd use "generic". You can go further and add a "freeform" or some >> such where you'd have to write out the entire regexp. >> >> The nice thing is that you can add keywords and corresponding handling >> as you go, whereas the gdb_test_multiple-like solution you propose only >> has one way of handling its arguments, which of course does makes things >> consistent and clear, but is not very extensible. > > I'm still not seeing which arguments can only be handled in one way. > Could you give an example maybe? Well, you gave this example: ... gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" { " = ${l}" { pass $gdb_test_ext_name } " = " { unsupported $gdb_test_ext_name set supported 0 } } ... It has (ignoring the first two) two arguments, one containing the 'pass' and one containing the 'unsupported'. Each is handled in the exact same way. > I do see that you're suggestion > improves the existing test - removing the need to set an extra 'test' > variable, and not having to add $gdb_prompt, but when all is said and > done, the pattern is still just a pattern, and the code is still just > code - how is this any different to gdb_test_multiple? > It's much less verbose, and eliminates as much as possible code that is commonly occurring, in order to avoid errors and omissions there. >> >>> One other thing which I've wanted for _ages_ is to avoid having to set >>> the test name into a separate variable, which your solution offers >>> too. The solution I offer is '$gdb_test_ext_name', this variable is >>> set auto-magically by the call to 'gdb_test_ext, and is available in >>> all of the action bodies for calls to pass/fail/unsupported/etc. >>> >> >> Nice trick, I've copied that for usage in the "generic" method. >> >> So, WDYT? > > I'm still not convinced. > > On further thought, I actually think there's no need for an extra > function at all, we can get all the benefit (as I see it) by possibly > updating gdb_test_multiple. I'm travelling right now so can't code > this up, but I think a solution that does something like this: > > gdb_test_multiple "command" "test name" { > -re "full regexp here$gdb_prompt" { > pass $gdb_test_multiple_name > } > -output "pattern without prompt" { > fail $gdb_test_multiple_name > } > } > > So using '-re' and '-output' to specialise the behaviour of > gdb_test_multiple, and adding in the $gdb_test_multiple_name variable. > > When I get back to my desk I'll try to code this up. > Agreed, I think it makes sense to fold that into gdb_test_multiple. > I'm know the above isn't going to satisfy you though - it's basically > an iteration on what I already proposed Yeah, I think we're looking for different things here: less verbosity at the cost of specialization vs. generality at the cost of more verbosity. > - maybe you could expand on > the benefits of you solution a bit more. > Sure. The benefits of my solution as I see them: - uses gdb_test semantics for args before '--', minimizing the effort to rewrite from gdb_test to gdb_test_ext - maximally eliminates repetitive code, to prevent error and omissions in there and reduce verbosity - extensible. Each method is handled completely independent, and new methods can be added as needed. Thanks, - Tom