Re: [PATCH 1/3] Fix ppc64 single step over atomic sequence testcase

[Anton Blanchard <anton@samba.org>]

Hi Joel,

> Thanks for your contributions,
> 
> First, a quick question: Are you making these contributions on behalf
> of a corporation, or on your own? This is for copyright assignment
> purposes - I just want to make sure that we have one already on file.

Thanks for the review. I work for IBM and my paperwork should be on
file.

> > The current ppc64 single step over atomic sequence testcase is
> > broken. Convert the test into assembly and use stepi to step
> > through it.
> 
> It would be useful for you to say what exactly is broken, and on
> which platform. At least it seems to have been working for some
> people (at IBM).

Sorry the explanation was a bit terse. The testcase assumes it only
requires two "next"s to get through the test function:

set bp1 [gdb_get_line_number "lwarx"]
gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
  "Set the breakpoint at the start of the sequence"

gdb_test continue "Continuing.*Breakpoint $decimal.*" \
  "Continue until breakpoint"

gdb_test next ".*__asm __volatile.*" \
  "Step through the lwarx/stwcx sequence"

gdb_test next ".*return 0.*" \
  "Step through the ldarx/stdcx sequence"

If I run this testcase manually on Fedora 16 (gcc 4.6.2), it
actually takes 7 steps to get through it:

Breakpoint 2, main () at ./gdb.arch/ppc64-atomic-inst.c:27
27	  __asm __volatile ("1:     lwarx   %0,0,%2\n"              \
(gdb) next
32	                    : "b" (word_addr), "m" (*word_addr)     \
(gdb) next
27	  __asm __volatile ("1:     lwarx   %0,0,%2\n"              \
(gdb) next
39	                    : "=&b" (dword), "=m" (*dword_addr)     \
(gdb) next
35	  __asm __volatile ("1:     ldarx   %0,0,%2\n"              \
(gdb) next
40	                    : "b" (dword_addr), "m" (*dword_addr)   \
(gdb) next
35	  __asm __volatile ("1:     ldarx   %0,0,%2\n"              \
(gdb) next
43	  return 0;
(gdb) 

I'm not sure what is expected here, is "next" supposed to step all the
way through inline assembly? Perhaps it is, but it seems fragile to
depend on this high level behaviour.

Anton

> 
> > 2012-06-05  Anton Blanchard  <anton@samba.org>
> > 
> > 	* gdb.arch/ppc64-atomic-inst.c: Remove
> > 	* gdb.arch/ppc64-atomic-inst.s: New file
> > 	* gdb.arch/ppc64-atomic-inst.exp: Adapt for asm based
> > testcase 
> > 
> > Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> > ===================================================================
> > --- gdb.orig/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> > +++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> > @@ -27,7 +27,7 @@ if {![istarget "powerpc*"] || ![is_lp64_
> >  }
> >  
> >  set testfile "ppc64-atomic-inst"
> > -set srcfile ${testfile}.c
> > +set srcfile ${testfile}.s
> >  set binfile ${objdir}/${subdir}/${testfile}
> >  set compile_flags {debug quiet}
> >  
> > @@ -50,11 +50,18 @@ set bp1 [gdb_get_line_number "lwarx"]
> >  gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
> >    "Set the breakpoint at the start of the sequence"
> >  
> > +set bp2 [gdb_get_line_number "ldarx"]
> > +gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
> > +  "Set the breakpoint at the start of the sequence"
> > +
> >  gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> >    "Continue until breakpoint"
> >  
> > -gdb_test next ".*__asm __volatile.*" \
> > +gdb_test nexti "bne.*1b" \
> >    "Step through the lwarx/stwcx sequence"
> >  
> > -gdb_test next ".*return 0.*" \
> > -  "Step through the ldarx/stdcx sequence"
> > +gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> > +  "Continue until breakpoint"
> > +
> > +gdb_test nexti "bne.*1b" \
> > +  "Step through the lwarx/stwcx sequence"
> > Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
> > ===================================================================
> > --- gdb.orig/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
> > +++ /dev/null
> > @@ -1,44 +0,0 @@
> > -/* This file is part of GDB, the GNU debugger.
> > -
> > -   Copyright 2008-2012 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
> > <http://www.gnu.org/licenses/>.  */ -
> > -#include <stdio.h>
> > -
> > -int main()
> > -{
> > -  unsigned int word = 0;
> > -  unsigned int *word_addr = &word;
> > -  unsigned long dword = 0;
> > -  unsigned long *dword_addr = &dword;
> > -
> > -  __asm __volatile ("1:     lwarx   %0,0,%2\n"              \
> > -                    "       addi    %0,%0,1\n"              \
> > -                    "       stwcx.  %0,0,%2\n"              \
> > -                    "       bne-    1b"
> > \
> > -                    : "=&b" (word), "=m" (*word_addr)       \
> > -                    : "b" (word_addr), "m" (*word_addr)     \
> > -                    : "cr0", "memory");
> > \ -
> > -  __asm __volatile ("1:     ldarx   %0,0,%2\n"              \
> > -                    "       addi    %0,%0,1\n"              \
> > -                    "       stdcx.  %0,0,%2\n"              \
> > -                    "       bne-    1b"                     \
> > -                    : "=&b" (dword), "=m" (*dword_addr)     \
> > -                    : "b" (dword_addr), "m" (*dword_addr)   \
> > -                    : "cr0", "memory");                     \
> > -
> > -  return 0;
> > -}
> > Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
> > ===================================================================
> > --- /dev/null
> > +++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
> > @@ -0,0 +1,26 @@
> > +        .include "gdb.asm/common.inc"
> > +        .include "gdb.asm/powerpc64.inc"
> > +
> > +.global main
> > +gdbasm_declare main
> > +	li	0,0
> > +	addi	4,1,-8
> > +
> > +	stw	0,0(4)
> > +1:	lwarx	5,0,4
> > +	cmpwi	5,0
> > +	bne	2f
> > +	addi	5,5,1
> > +	stwcx.	5,0,4
> > +	bne	1b
> > +
> > +	std	0,0(4)
> > +2:	ldarx	5,0,4
> > +	cmpdi	5,0
> > +	bne	3f
> > +	addi	5,5,1
> > +	stdcx.	5,0,4
> > +	bne	1b
> > +
> > +3:	li	3,0
> > +	blr
>