The thing about amber test code - the coverage is extraordinarily
incomplete. If you really want to make changes like this, then please send
me the proposed files so I can locally take a hard look at them (just makes
my life easier). You could easily be breaking things that are subtle;
something like valgrind can be useful, but you still really need to be darn
sure that what it is suggesting does not violate critical logic (like when
you actually need something initialized, as opposed to an out-of-the-loop
initialization (not sure on that one, but I want to look at it hard)). This
shake stuff, by the way, has been apparently working without incident for at
least 6 years; some of it is unique code (no direct analog in sander) that I
optimized long ago - not saying that it has to be okay. The gb stuff is
pretty much lifted straight from sander, with minor mods, and I would expect
that if this 0-reference bug exists in pmemd, it exists in sander too.
Regards - Bob
----- Original Message -----
From: "Mark Williamson" <mjw.sdsc.edu>
To: "AMBER Developers Mailing List" <amber-developers.ambermd.org>
Sent: Friday, August 28, 2009 8:59 PM
Subject: [AMBER-Developers] PMEMD and valgrind
> Dear All,
>
> I've just been passing a uni proc build of PMEMD through the memory
> allocation checker tool Valgrind for unrelated reasons to the contents of
> this email and I've found some issues. I think I've fixed these (make
> test.pmemd is passing), but I just want to check If I have fixed them
> correctly. Feedback would be great :)
>
> PMEMD was built as follows from latest amber11 cvs co:
>
> ./configure linux_em64t gfortran nopar
>
> generated config.h modded as follows:
>
> F90_OPT_DBG = -g
> ...
> F90_OPT_DFLT = $(F90_OPT_DBG)
>
> Built using:
> make ; make install
>
> Tested on the gb_mb benchmark:
>
> cd amber11/benchmarks/gb_mb
> diff -r10.0 bench.gb_mb
> 28c28
> < /bin/rm restrt gbin mdinfo
> ---
> > #/bin/rm restrt gbin mdinfo
> ./bench.gb_mb
>
> ln -s gbin mdin
>
> Valgrind invoked as follows:
> valgrind --tool=memcheck -q ~/code/AMBER/amber11/exe/pmemd -O
>
>
> Issue 1)
> -=-=-=-=
>
> | WARNING: Stack usage limited by a hard resource limit of 536870912
> bytes!
> | If segment violations occur, get your sysadmin to increase the
> limit.
> ==26884== Conditional jump or move depends on uninitialised value(s)
> ==26884== at 0x464C83: __shake_mod__get_water_distances (shake.f90:647)
> ==26884== by 0x465C9D: __shake_mod__shake_setup (shake.f90:369)
> ==26884== by 0x47CDCE: MAIN__ (pmemd.f90:127)
> ==26884== by 0x4A4BED: main (in
> /server-home/mjw/code/AMBER/amber11/bin/pmemd)
> ==26884==
>
> atm_i = gbl_bond(j)%atm_i
> atm_j = gbl_bond(j)%atm_j
> parm_idx = gbl_bond(j)%parm_idx
>
> if (atm_i .eq. iof .and. atm_j .eq. ihf1) then !<--shake.f90:647
> roh1 = gbl_req(parm_idx)
> else if (atm_j .eq. iof .and. atm_i .eq. ihf1) then
> roh1 = gbl_req(parm_idx)
> else if (atm_i .eq. iof .and. atm_j .eq. ihf2) then
>
> I think this is fixed with:
>
> [16:59][bunny:0.12][mjw:src]$ cvs diff shake.fpp
> Index: shake.fpp
> ===================================================================
> RCS file: /home/amber_cvs/cvsroot/amber11/src/pmemd/src/shake.fpp,v
> retrieving revision 10.1
> diff -r10.1 shake.fpp
> 834a835,838
> > iof = 0
> > ihf1 = 0
> > ihf2 = 0
> >
> 840,842d843
> < iof = 0
> < ihf1 = 0
> < ihf2 = 0
>
>
>
> Issue 2
> -=-=-=-=
> ==15652== Invalid write of size 4
> ==15652== at 0x46E52A: __gb_ene_mod__gb_ene (gb_ene.f90:630)
> ==15652== by 0x472BED: __gb_force_mod__gb_force (gb_force.f90:143)
> ==15652== by 0x45E658: __runmd_mod__runmd (runmd.f90:344)
> ==15652== by 0x47D188: MAIN__ (pmemd.f90:194)
> ==15652== by 0x4A4C5D: main (in
> /server-home/mjw/code/AMBER/amber11/bin/pmemd)
>
>
> do jjv = jexcl, jexcl_last
> skipv(natex(jjv)) = .true. <---------gb_ene.f90:630
> end do
>
> This is happening because some values of natex() are actually zero, hence
> skipv(0) is being set to .true. This patch fixes it:
>
> Index: gb_ene.fpp
> ===================================================================
> RCS file: /home/amber_cvs/cvsroot/amber11/src/pmemd/src/gb_ene.fpp,v
> retrieving revision 10.1
> diff -r10.1 gb_ene.fpp
> 612c612,614
> < skipv(natex(jjv)) = .true.
> ---
> > if (natex(jjv) .ne. 0 ) then !natex(jjv) can contain zero values
> > skipv(natex(jjv)) = .true.
> > end if
>
>
> regards,
>
> Mark
>
> _______________________________________________
> AMBER-Developers mailing list
> AMBER-Developers.ambermd.org
> http://lists.ambermd.org/mailman/listinfo/amber-developers
>
_______________________________________________
AMBER-Developers mailing list
AMBER-Developers.ambermd.org
http://lists.ambermd.org/mailman/listinfo/amber-developers
Received on Wed Sep 02 2009 - 23:06:06 PDT
