From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44634 invoked by alias); 3 Mar 2018 06:27:43 -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 44621 invoked by uid 89); 3 Mar 2018 06:27:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 03 Mar 2018 06:27:40 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B4F7A1E059; Sat, 3 Mar 2018 01:27:38 -0500 (EST) Subject: Re: [PATCHv3 2/2] gdb: Initial baremetal riscv support To: Andrew Burgess , gdb-patches@sourceware.org Cc: Eli Zaretskii References: <430dd07a2ed6520ad17108eb156935ec86cec826.1520021226.git.andrew.burgess@embecosm.com> From: Simon Marchi Message-ID: <3e083125-4a39-04ee-45cc-0152ced1e259@simark.ca> Date: Sat, 03 Mar 2018 06:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <430dd07a2ed6520ad17108eb156935ec86cec826.1520021226.git.andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00086.txt.bz2 Hi Andrew, I took a deeper look at the test which I had just skimmed previously, I have a few comments on that. One general comment, I would suggest naming the test "infcall-nested-structs" to make it clear that it tests inferior function calls. On 2018-03-02 03:09 PM, Andrew Burgess wrote: > diff --git a/gdb/testsuite/gdb.base/nested-structs.exp b/gdb/testsuite/gdb.base/nested-structs.exp > new file mode 100644 > index 00000000000..c359acde48c > --- /dev/null > +++ b/gdb/testsuite/gdb.base/nested-structs.exp > @@ -0,0 +1,189 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2018 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 . > + > + > +# Some targets can't call functions, so don't even bother with this > +# test. > + > +if [target_info exists gdb,cannot_call_functions] { > + unsupported "this target can not call functions" > + continue > +} > + > +if [gdb_skip_float_test] { > + unsupported "this target doesn't support float" > + continue > +} > + > +set int_types { tc ts ti tl tll } > +set float_types { tf td tld } > +set complex_types { tfc tdc tldc } > + > +set compile_flags {debug} > +if [support_complex_tests] { > + lappend compile_flags "additional_flags=-DTEST_COMPLEX" > +} > + > +# Given N (0..25), return the corresponding alphabetic letter in lower > +# or upper case. This is ment to be i18n proof. ment -> meant. Also, I'm curious, what do you mean exactly by i18n proof? > + > +proc i2a { n } { > + return [string range "abcdefghijklmnopqrstuvwxyz" $n $n] > +} The i2a proc does not seem to be used (other than in I2A), so can you instead implement I2A directly and not have i2a? > + > +proc I2A { n } { > + return [string toupper [i2a $n]] > +} > + > +# Compile a variant of nested-structs.c using TYPES to specify the > +# types of the struct fields within the source. Run up to main. > +# Also updates the global "testfile" to reflect the most recent build. > + > +proc start_nested_structs_test { types } { > + global testfile > + global srcfile > + global binfile > + global subdir > + global srcdir > + global gdb_prompt gdb_prompt is unused in this proc. > + global compile_flags > + > + standard_testfile .c > + > + # Create the additional flags > + set flags $compile_flags > + > + set n 0 That "set n 0" can be removed. > + for {set n 0} {$n<[llength ${types}]} {incr n} { > + set m [I2A ${n}] > + set t [lindex ${types} $n] > + lappend flags "additional_flags=-Dt${m}=${t}" > + append testfile "-" "$t" > + } > + > + set binfile [standard_output_file ${testfile}] > + if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } { > + verbose -log "failed to compile" > + return 0 > + } > + > + # Start with a fresh gdb. > + gdb_exit > + gdb_start > + gdb_reinitialize_dir $srcdir/$subdir > + gdb_load ${binfile} Can you use "clean_restart ${binfile}" instead? > + > + # Make certain that the output is consistent > + gdb_test_no_output "set print sevenbit-strings" > + gdb_test_no_output "set print address off" > + gdb_test_no_output "set print pretty off" > + gdb_test_no_output "set width 0" > + gdb_test_no_output "set print elements 300" > + > + # Advance to main > + if { ![runto_main] } then { > + verbose -log "failed to run to main" The usual pattern is to put fail "can't run to main" if runto_main fails. Strangely, I don't think runto_main will cause a FAIL if it fails (it calls runto with no-message, which disables the FAILs). I see that the failure would be noticed because of the unresolved below, but let's follow the usual pattern, in case somebody decides to copy paste your code :). > + return 0 > + } > + > + return 1 > +} > + > +# Assuming GDB is stopped at main within a test binary, run some tests > +# passing structures, and reading return value structures. > + > +proc run_tests {} { > + global gdb_prompt > + > + foreach {name} {struct01 struct02 struct03 struct04} { > + gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1" > + > + set refval "" > + set test1 "fetch reference value for ${name}" > + gdb_test_multiple "p ref_val_${name}" "${test1}" { > + -re " = (.*)\r\n$gdb_prompt $" { > + pass "$test1" > + set refval $expect_out(1,string) > + } > + } I wonder if you can use the get_valueof proc from lib/gdb.exp here and below. It would save a few lines and make the code more straight to the point. > + > + set test2 "check return value ${name}" > + if { ${refval} != "" } { > + set answer "" > + set test3 "fetch return value for ${name}" > + gdb_test_multiple "p rtn_str_${name} ()" "$test3" { > + -re " = (.*)\r\n$gdb_prompt $" { > + pass $test3 > + set answer $expect_out(1,string) > + } > + } > + > + if { ${answer} == ${refval} } { > + pass $test2 > + } else { > + fail $test2 > + } When you have that pattern, you can use gdb_assert {${answer} == ${refval}} ${test2} > + } else { > + unresolved $test2 > + } > + } > +} > + > +# Set up a test prefix, compile the test binary, run to main, and then > +# run some tests. > + > +proc start_gdb_and_run_tests { types } { > + set prefix "types" > + > + foreach t $types { > + append prefix "-" "${t}" > + } > + > + with_test_prefix $prefix { > + if { ! [start_nested_structs_test $types] } { > + unresolved "failed to start test correctly" > + } else { > + run_tests > + } > + } > +} > + > +if [support_complex_tests] { > + foreach ta $complex_types { > + start_gdb_and_run_tests $ta > + } > +} > + > +foreach ta $float_types { > + start_gdb_and_run_tests $ta > +} Would it make sense to run the test with int_types only? foreach ta $int_types { start_gdb_and_run_tests $ta } If so, your test could be useful on targets that are marked as "skip_float_test". Instead of skipping this test entirely, they could run the int tests, and the tests that deal with float would be in an if ![gdb_skip_float_test] { ... } > + > +foreach ta $int_types { > + foreach tb $float_types { > + start_gdb_and_run_tests [list $ta $tb] > + } > +} > + > +foreach ta $float_types { > + foreach tb $int_types { > + start_gdb_and_run_tests [list $ta $tb] > + } > + > + foreach tb $float_types { > + start_gdb_and_run_tests [list $ta $tb] > + } > +} > Thanks, Simon