[PATCH] gdb/testsuite: do not hard-code location indices in condbreak-multi-context.exp

[Aktemur, Tankut Baris]

On Monday, November 23, 2020 6:26 PM, Simon Marchi wrote:
> On 2020-10-30 11:06 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> > +proc find_location_contexts {} {
> > +    global loc_name loc_index bpnum1 fill
> > +    global decimal hex gdb_prompt
> > +
> > +    gdb_test_multiple "info breakpoint $bpnum1" "find_location_indices" {
> > +	-re "stop only if ${fill}\r\n" {
> > +	    exp_continue
> > +	}
> > +	-re "^${bpnum1}\.($decimal) ${fill} ${hex} in (A|Base|C)::func${fill}\r\n" {
> > +	    set index $expect_out(1,string)
> > +	    set name $expect_out(2,string)
> > +	    set loc_name($index) $name
> > +	    set loc_index($name) $index
> > +	    exp_continue
> > +	}
> > +	-re "$gdb_prompt $" {
> > +	    verbose -log "Loc names: $loc_name(1), $loc_name(2), $loc_name(3)"
> > +	    if {($loc_name(1) != $loc_name(2))
> > +		&& ($loc_name(1) != $loc_name(3))
> 
> Is the goal to check that all elements are unique?  If so, should you
> also check that $loc_name(2) != $loc_name(3)?  If/when we ever have more
> than three elements, it will become interesting to make two for loops
> that compare all pairs of elements.
> 
> > +		&& ($loc_name(1) != "")
> > +		&& ($loc_name(2) != "")
> > +		&& ($loc_name(3) != "")} {
> > +		pass $gdb_test_name
> > +	    } else {
> > +		fail $gdb_test_name
> > +	    }
> 
> Instead of
> 
>   if { something } {
>     pass foo
>   } else {
>     fail foo
>   }
> 
> You can use the gdb_assert proc:
> 
>   gdb_assert something foo
> 
> And to make it easier to narrow down the issue if that fails, I'd make
> separate calls to gdb_assert:
> 
>     gdb_assert { ![string equal $loc_name(1) $loc_name(2)] }
>     gdb_assert { ![string equal $loc_name(1) $loc_name(3)] }
>     gdb_assert { ![string equal $loc_name(2) $loc_name(3)] }
>     gdb_assert { [string length $loc_name(1)] > 0 }
>     gdb_assert { [string length $loc_name(2)] > 0 }
>     gdb_assert { [string length $loc_name(3)] > 0 }
> 
> Comparing strings with != would make quoting particularly bad in this
> case, that's why I used string equal instead.
> 
> > +	}
> > +    }
> > +}
> >
> > -proc test_info_break {suffix} {
> > -    global bpnum1 bpnum2 fill
> > +# Test the breakpoint location enabled states.  STATES is a list of
> > +# location states.  We assume STATES to contain the state for A, Base,
> > +# and C, and in this order.  E.g. {N* y n} for 'N*' at A::func, 'y' at
> > +# B::func, and 'n' at C::func, respectively.
> >
> > -    set bp_hit_info "${fill}(\r\n${fill}breakpoint already hit 1 time)?"
> > +proc check_bp_locations {bpnum states cond {msg ""}} {
> > +    global loc_name fill bpnum1 bpnum2
> 
> bpnum1 and bpnum2 are not used in this proc.
> 
> Otherwise, LGTM, thanks!
> 
> Simon


Thanks for the corrections.  Pushed with those fixes.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928