[patchv2,1/2] save breakpoints does not save disabled breakpoints correctly

  On Thu, 09 Oct 2014 15:55:52 +0200, Yao Qi wrote:
> This line is too long... IWBN

I would prefer no IWBN and rather this is or is not required for check-in
approval.  I am really aware of very many IWBN things to change in GDB.


> to convert gdb_test to gdb_test_sequence,
> so that the patterns are written for each line, and will be more clear.

Done.  But IMO it is a functionality regression as:

 * gdb_test_sequence permits arbitary number of lines of text between those
   lines being matched.  Former regex string did not allow it.
   This may make a difference if GDB regresses by printing some unexpected
   line after the breakpoint info line (like a "silent" line).

 * \[\r\n\]+ can be used to anchor the beginning of the pattern, in the sense
   of Perl regex ^ /m match.  At least I have found such cases in existing
   *.exp files so I used that.  Using ^ really does not work.
   
   But I am not aware how to do Perl regex $ /m match.  Using $ really does
   not work.  But this means that for example the trailing
     ( \\((host|target) evals\\))?
   on the line
     "\[\r\n\]+\[ \t\]+stop only if i == 1( \\((host|target) evals\\))?"
   originally made sense there but now it can be removed as it has no longer
   any functionality there - it will match now any trailing line garbage.


Jan
gdb/testsuite/
2014-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/save-bp.exp (info break): Use gdb_test_sequence.
  

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

These two patches look right to me.

>  * \[\r\n\]+ can be used to anchor the beginning of the pattern, in the sense
>    of Perl regex ^ /m match.  At least I have found such cases in existing
>    *.exp files so I used that.  Using ^ really does not work.
>    
>    But I am not aware how to do Perl regex $ /m match.  Using $ really does
>    not work.  But this means that for example the trailing
>      ( \\((host|target) evals\\))?
>    on the line
>      "\[\r\n\]+\[ \t\]+stop only if i == 1( \\((host|target) evals\\))?"
>    originally made sense there but now it can be removed as it has no longer
>    any functionality there - it will match now any trailing line garbage.

In this test case, ( \\((host|target) evals\\))? isn't needed in the
pattern.  What we test here is to save breakpoints into file and restore
them from file.  The contents saved in file are:

break save-bp.c:31
  condition $bpnum i == 1

the information about the place where the condition is evaluated isn't
saved, so we don't need to check.  Breakpoint save and restore has
nothing to do with where the condition is evaluated (host or target).  I
am fine to leave it here now.
  

On 10/09/2014 08:10 PM, Jan Kratochvil wrote:
> On Thu, 09 Oct 2014 15:55:52 +0200, Yao Qi wrote:

>> to convert gdb_test to gdb_test_sequence,
>> so that the patterns are written for each line, and will be more clear.
> 
> Done.  But IMO it is a functionality regression as:
> 
>  * gdb_test_sequence permits arbitary number of lines of text between those
>    lines being matched.  Former regex string did not allow it.
>    This may make a difference if GDB regresses by printing some unexpected
>    line after the breakpoint info line (like a "silent" line).

In places were we want to avoid that, and we want to write a pattern
that spawns multiple lines, we build the regex with join.  E.g.,
gdb.btrace/instruction_history.exp:

# test that we see the expected instructions
gdb_test "record instruction-history 3,7" [join [list \
  "3\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tje     0x\[0-9a-f\]+ <loop\\+\[0-9\]+>" \
  "4\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tdec    %eax" \
  "5\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tjmp    0x\[0-9a-f\]+ <loop\\+\[0-9\]+>" \
  "6\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tcmp    \\\$0x0,%eax" \
  "7\t   0x\[0-9a-f\]+ <loop\\+\[0-9\]+>:\tje     0x\[0-9a-f\]+ <loop\\+\[0-9\]+>\r" \
  ] "\r\n"]

This grep will find more cases:

  grep '"\\r\\n"' testsuite/* -rn

Thanks,
Pedro Alves