April 2016 - petsc-dev - mailman3.cels.anl.gov

Re: [petsc-dev] PETSc blame digest (next) 2016-04-08
by Michael Lange 08 Apr '16

08 Apr '16

Hi Matt, I just pushed the branch mlange/fix-plex-fluent-reader that re-enables the plex-fluent test cases and hopefully fixes these warnings. Can you please double-check this and merge to next/master? Thanks, Michael On 08/04/16 15:00, PETSc checkBuilds wrote: > > Dear PETSc developer, > > This email contains listings of contributions attributed to you by > `git blame` that caused compiler errors or warnings in PETSc automated > testing. Follow the links to see the full log files. Please attempt to fix > the issues promptly or let us know at petsc-dev(a)mcs.anl.gov if you are unable > to resolve the issues. > > Thanks, > The PETSc development team > > ---- > > warnings attributed to commit https://bitbucket.org/petsc/petsc/commits/3f6dc66 > Plex-Fluent: Add coordinates to the generated Plex > > src/dm/impls/plex/plexfluent.c:408 > [http://ftp.mcs.anl.gov/pub/petsc/nightlylogs/archive/2016/04/08/build_next_…] > /sandbox/petsc/petsc.clone/src/dm/impls/plex/plexfluent.c:408:27: warning: Array access (from variable 'coordsIn') results in a null pointer dereference > > ---- > > warnings attributed to commit https://bitbucket.org/petsc/petsc/commits/f732056 > Plex-Fluent: Build Plex topology from face connectivity > > src/dm/impls/plex/plexfluent.c:322 > [http://ftp.mcs.anl.gov/pub/petsc/nightlylogs/archive/2016/04/08/build_next_…] > /sandbox/petsc/petsc.clone/src/dm/impls/plex/plexfluent.c:322:27: warning: Array access (from variable 'faces') results in a null pointer dereference > > ---- > To opt-out from receiving these messages - send a request to petsc-dev(a)mcs.anl.gov.

1 0

Re: [petsc-dev] Correct syntax for DMDA ownership ranges in petsc4py
by Lawrence Mitchell 08 Apr '16

08 Apr '16

> On 8 Apr 2016, at 14:35, Chris Eldred <chris.eldred(a)gmail.com> wrote: > > Great! How would I go about testing that change- I have been using the > petsc4py version that comes with Firedrake, so what is the correct way > to modify petsc4py and rebuild it? Just modify the file in the repo and pip install from that directory again. cd path/to/petsc4py_dir pip install --upgrade . Lawrence

1 0

Re: [petsc-dev] Correct syntax for DMDA ownership ranges in petsc4py
by Chris Eldred 08 Apr '16

08 Apr '16

Great! How would I go about testing that change- I have been using the petsc4py version that comes with Firedrake, so what is the correct way to modify petsc4py and rebuild it? On Fri, Apr 8, 2016 at 10:01 AM, Lawrence Mitchell <lawrence.mitchell(a)imperial.ac.uk> wrote: > >> On 8 Apr 2016, at 08:29, Chris Eldred <chris.eldred(a)gmail.com> wrote: >> >> >> Yes I have tried all possible variants: >> [(8,8,4),] >> ([8,8,4],) >> [[8,8,4],] >> ((8,8,4),) >> and they all fail with the same >> TypeError: Expected tuple, got list >> >> However, I think I see what is happening now. In line 156 >> cdef object ranges = list(ownership_ranges) >> it looks like ranges is being defined as a list, rather than a tuple. >> So maybe there should be a cast from list to tuple in the return >> statement? > > That looks right, since the return type of asOwnerShipRanges is declared as tuple, but as you note, the return variable has type list. > > Lawrence -- Chris Eldred Postdoctoral Fellow, LAGA, University of Paris 13 PhD, Atmospheric Science, Colorado State University, 2015 DOE Computational Science Graduate Fellow (Alumni) B.S. Applied Computational Physics, Carnegie Mellon University, 2009 chris.eldred(a)gmail.com

1 0

Re: [petsc-dev] Need help with Fortran code/compiler issue
by Lawrence Mitchell 08 Apr '16

08 Apr '16

> On 7 Apr 2016, at 19:46, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > > src/sys/classes/bag/f2003-src/fsrc/bagenum.F has the code fragment > > Subroutine PetscBagRegisterEnum(bag,addr,FArray,def,n,h,ierr) > use,intrinsic :: iso_c_binding > implicit none > > PetscBag bag > character(*) n,h > character(*) FArray(*) > PetscEnum :: def > PetscErrorCode,intent(out) :: ierr > PetscReal addr(*) > > Type(C_Ptr),Dimension(:),Pointer :: CArray > character(kind=c_char),pointer :: nullc => null() > PetscInt :: i,Len > Character(kind=C_char,len=256),Dimension(:),Pointer::list1 > > do i=1,256 > if (len_trim(Farray(i)) .eq. 0) then > Len = i-1 > goto 100 > endif > if (len_trim(Farray(i)) .gt. 255) then > ierr = PETSC_ERR_ARG_OUTOFRANGE > return > endif > enddo > ierr = PETSC_ERR_ARG_OUTOFRANGE > return > > 100 continue > > Allocate(list1(Len),stat=ierr); > Allocate(CArray(Len+1),stat=ierr) > do i=1,Len > list1(i) = trim(FArray(i))//C_NULL_CHAR > enddo > > CArray = (/(c_loc(list1(i)),i=1,Len),c_loc(nullc)/) > call PetscBagRegisterEnumPrivate(bag,addr,CArray,def,n,h,ierr) > DeAllocate(CArray) > DeAllocate(list1) > End Subroutine > > which produces the following warning with one Fortran compiler: > > /Users/petsc/petsc.clone-4/src/sys/objects/f2003-src/fsrc/optionenum.F:35:0: > > CArray = (/(c_loc(list1(i)),i=1,Len),c_loc(nullc)/) > ^ > Warning: 'list1.data' may be used uninitialized in this function [-Wmaybe-uninitialized] > > Do any Fortran programmers know if the code is correct or if this is an incorrect warning from the compiler? This is occurring because the return value from allocate(list1(Len),stat=ierr) is not being checked and so list1 may be NULL later on. This squashes the warning for me: Allocate(list1(Len),stat=ierr) if (ierr .ne. 0) then ierr = PETSC_ERR_MEM return endif One should presumably do the same after the allocation of CArray as well. It looks like the same change would apply in optionenum.F Cheers, Lawrence

1 0

Re: [petsc-dev] Correct syntax for DMDA ownership ranges in petsc4py
by Lawrence Mitchell 08 Apr '16

08 Apr '16

> On 8 Apr 2016, at 08:29, Chris Eldred <chris.eldred(a)gmail.com> wrote: > > > Yes I have tried all possible variants: > [(8,8,4),] > ([8,8,4],) > [[8,8,4],] > ((8,8,4),) > and they all fail with the same > TypeError: Expected tuple, got list > > However, I think I see what is happening now. In line 156 > cdef object ranges = list(ownership_ranges) > it looks like ranges is being defined as a list, rather than a tuple. > So maybe there should be a cast from list to tuple in the return > statement? That looks right, since the return type of asOwnerShipRanges is declared as tuple, but as you note, the return variable has type list. Lawrence

1 0

Re: [petsc-dev] Correct syntax for DMDA ownership ranges in petsc4py
by Chris Eldred 08 Apr '16

08 Apr '16

Hey Matt, Yes I have tried all possible variants: [(8,8,4),] ([8,8,4],) [[8,8,4],] ((8,8,4),) and they all fail with the same TypeError: Expected tuple, got list However, I think I see what is happening now. In line 156 cdef object ranges = list(ownership_ranges) it looks like ranges is being defined as a list, rather than a tuple. So maybe there should be a cast from list to tuple in the return statement? -Chris On Thu, Apr 7, 2016 at 9:19 PM, Matthew Knepley <knepley(a)gmail.com> wrote: > On Thu, Apr 7, 2016 at 2:14 PM, Chris Eldred <chris.eldred(a)gmail.com> wrote: >> >> I think what happens on the 1st version is that the input list has >> len(3), and since the DMDA only has 1 dimension petsc4py throws the >> first ValueError. > > > Yes, you are right. > >> >> As far as I can tell, the code is expecting a list of lists: ie for >> each dimension it gets ranges[i] (where i is the index for the >> dimension) and casts it into an array. So I tried the 2nd version, >> which fails regardless of whether I use lists or tuples, and I don't >> understand why. > > > That function returns 'range' and the type is declared as tuple. Did you try > > ownership_ranges=((8,8,4),) > > Matt > >> >> >> -Chris >> >> On Thu, Apr 7, 2016 at 9:00 PM, Matthew Knepley <knepley(a)gmail.com> wrote: >> > On Thu, Apr 7, 2016 at 1:52 PM, Chris Eldred <chris.eldred(a)gmail.com> >> > wrote: >> >> >> >> Hey All, >> >> >> >> (I sent this to petsc-users a few days ago and didn't get any replies, >> >> figured I would try here as well...) >> >> >> >> Does anyone know the correct syntax for setting ownership ranges for >> >> DMDAs in petsc4py? For example, if I have a 1D DMDA with 20 vertices >> >> and 3 processes, and I want to distribute the vertices as [8,8,4], >> >> should the correct create call be: >> >> >> >> da = >> >> >> >> PETSc.DMDA().create(dim=1,dof=1,sizes=[20,],proc_sizes=[3,],boundary_type=[PETSc.DM.BoundaryType.PERIODIC,], >> >> stencil_type= PETSc.DMDA.StencilType.BOX, >> >> stencil_width=1,ownership_ranges=[8,8,4]) >> >> >> >> or >> >> >> >> da = >> >> >> >> PETSc.DMDA().create(dim=1,dof=1,sizes=[20,],proc_sizes=[3,],boundary_type=[PETSc.DM.BoundaryType.PERIODIC,], >> >> stencil_type= PETSc.DMDA.StencilType.BOX, >> >> stencil_width=1,ownership_ranges=[[8,8,4],]) >> >> >> >> For me, the 1st version fails with >> >> ValueError: number of dimensions 1 and number ownership ranges 3 >> >> (which I expected) >> >> but the 2nd version also fails with >> >> TypeError: Expected tuple, got list >> >> >> >> I have tried using tuples instead of lists and nothing changes. >> >> Creating without the ownership_ranges argument works fine as well. Is >> >> there something else I am missing? >> > >> > >> > Here is the code: >> > >> > >> > >> > https://bitbucket.org/petsc/petsc4py/src/b234a50085607b793fce93d5dc46307c0d… >> > >> > I do not understand why the 1st version fails. >> > >> > Matt >> > >> >> >> >> Thanks in advance for any assistance! >> >> >> >> Regards, >> >> -Chris >> >> >> >> >> >> -- >> >> Chris Eldred >> >> Postdoctoral Fellow, LAGA, University of Paris 13 >> >> PhD, Atmospheric Science, Colorado State University, 2015 >> >> DOE Computational Science Graduate Fellow (Alumni) >> >> B.S. Applied Computational Physics, Carnegie Mellon University, 2009 >> >> chris.eldred(a)gmail.com >> > >> > >> > >> > >> > -- >> > What most experimenters take for granted before they begin their >> > experiments >> > is infinitely more interesting than any results to which their >> > experiments >> > lead. >> > -- Norbert Wiener >> >> >> >> -- >> Chris Eldred >> Postdoctoral Fellow, LAGA, University of Paris 13 >> PhD, Atmospheric Science, Colorado State University, 2015 >> DOE Computational Science Graduate Fellow (Alumni) >> B.S. Applied Computational Physics, Carnegie Mellon University, 2009 >> chris.eldred(a)gmail.com > > > > > -- > What most experimenters take for granted before they begin their experiments > is infinitely more interesting than any results to which their experiments > lead. > -- Norbert Wiener -- Chris Eldred Postdoctoral Fellow, LAGA, University of Paris 13 PhD, Atmospheric Science, Colorado State University, 2015 DOE Computational Science Graduate Fellow (Alumni) B.S. Applied Computational Physics, Carnegie Mellon University, 2009 chris.eldred(a)gmail.com

1 0

Re: [petsc-dev] technical C question
by Jeff Hammond 07 Apr '16

07 Apr '16

On Thursday, April 7, 2016, Matthew Knepley <knepley(a)gmail.com> wrote: > On Thu, Apr 7, 2016 at 7:15 PM, Barry Smith <bsmith(a)mcs.anl.gov > <javascript:_e(%7B%7D,'cvml','bsmith(a)mcs.anl.gov');>> wrote: > >> >> This works. >> >> unsigned int joe = index%PETSC_BITS_PER_BYTE; >> if (joe > 7) joe = 7; >> return (BT_idx = index/PETSC_BITS_PER_BYTE, >> BT_c = array[BT_idx], >> BT_mask = (char)(1 << joe), >> >> I think it is because clang is not smart enough to know that >> (index%PETSC_BITS_PER_BYTE) is always between 0 and 7. >> >> Hence it thinks that you might be doing something like 1 << 9 >> which is as I understand it undefined >> http://stackoverflow.com/questions/21909412/why-would-the-outcome-of-this-s… >> >> So do we pander to the clang analyzer or just leave the warning? I vote >> to leave the warning and not change the code. >> > > I believe Bill also encouraged us toward clean code rather than 0 > warnings. I like this as well. > > Some compilers allow you suppress warnings, potentially at file granularity via pragmas... > Matt > > >> Barry >> >> >> >> > On Apr 7, 2016, at 5:56 PM, Jed Brown <jed(a)jedbrown.org >> <javascript:_e(%7B%7D,'cvml','jed(a)jedbrown.org');>> wrote: >> > >> > Satish Balay <balay(a)mcs.anl.gov >> <javascript:_e(%7B%7D,'cvml','balay(a)mcs.anl.gov');>> writes: >> > >> >> On Thu, 7 Apr 2016, Jed Brown wrote: >> >> >> >>> Barry Smith <bsmith(a)mcs.anl.gov >> <javascript:_e(%7B%7D,'cvml','bsmith(a)mcs.anl.gov');>> writes: >> >>>> Should we caste to an unsigned PetscInt first then? >> >>> >> >>> It should be unsigned, yes. Does that fix the warning? >> >> >> >> Nope.. >> >> >> >> Commenting out the following line - or changing the argument thus is >> >> making a difference. >> >> >> >> diff --git a/src/mat/impls/baij/seq/baijfact.c >> b/src/mat/impls/baij/seq/baijfact.c >> >> index fea37cb..e2c210c 100644 >> >> --- a/src/mat/impls/baij/seq/baijfact.c >> >> +++ b/src/mat/impls/baij/seq/baijfact.c >> >> @@ -1081,7 +1081,7 @@ PetscErrorCode MatICCFactorSymbolic_SeqBAIJ(Mat >> fact,Mat A,IS perm,const MatFact >> >> ncols_upper++; >> >> } >> >> } >> >> - ierr = >> PetscIncompleteLLAdd(ncols_upper,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); >> >> + ierr = >> PetscIncompleteLLAdd(ncols,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); >> > >> > That looks like it changes the semantics. >> > >> > In any case, that file wasn't even mentioned in the message that Barry >> > shared. If it is indeed the same issue, then it would appear that the >> > static analyzer has determined that it is possible for index to be >> > negative. >> >> > > > -- > What most experimenters take for granted before they begin their > experiments is infinitely more interesting than any results to which their > experiments lead. > -- Norbert Wiener > -- Jeff Hammond jeff.science(a)gmail.com http://jeffhammond.github.io/

1 0

Re: [petsc-dev] technical C question
by Matthew Knepley 07 Apr '16

07 Apr '16

On Thu, Apr 7, 2016 at 7:15 PM, Barry Smith <bsmith(a)mcs.anl.gov> wrote: > > This works. > > unsigned int joe = index%PETSC_BITS_PER_BYTE; > if (joe > 7) joe = 7; > return (BT_idx = index/PETSC_BITS_PER_BYTE, > BT_c = array[BT_idx], > BT_mask = (char)(1 << joe), > > I think it is because clang is not smart enough to know that > (index%PETSC_BITS_PER_BYTE) is always between 0 and 7. > > Hence it thinks that you might be doing something like 1 << 9 which > is as I understand it undefined > http://stackoverflow.com/questions/21909412/why-would-the-outcome-of-this-s… > > So do we pander to the clang analyzer or just leave the warning? I vote > to leave the warning and not change the code. > I believe Bill also encouraged us toward clean code rather than 0 warnings. I like this as well. Matt > Barry > > > > > On Apr 7, 2016, at 5:56 PM, Jed Brown <jed(a)jedbrown.org> wrote: > > > > Satish Balay <balay(a)mcs.anl.gov> writes: > > > >> On Thu, 7 Apr 2016, Jed Brown wrote: > >> > >>> Barry Smith <bsmith(a)mcs.anl.gov> writes: > >>>> Should we caste to an unsigned PetscInt first then? > >>> > >>> It should be unsigned, yes. Does that fix the warning? > >> > >> Nope.. > >> > >> Commenting out the following line - or changing the argument thus is > >> making a difference. > >> > >> diff --git a/src/mat/impls/baij/seq/baijfact.c > b/src/mat/impls/baij/seq/baijfact.c > >> index fea37cb..e2c210c 100644 > >> --- a/src/mat/impls/baij/seq/baijfact.c > >> +++ b/src/mat/impls/baij/seq/baijfact.c > >> @@ -1081,7 +1081,7 @@ PetscErrorCode MatICCFactorSymbolic_SeqBAIJ(Mat > fact,Mat A,IS perm,const MatFact > >> ncols_upper++; > >> } > >> } > >> - ierr = > PetscIncompleteLLAdd(ncols_upper,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); > >> + ierr = > PetscIncompleteLLAdd(ncols,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); > > > > That looks like it changes the semantics. > > > > In any case, that file wasn't even mentioned in the message that Barry > > shared. If it is indeed the same issue, then it would appear that the > > static analyzer has determined that it is possible for index to be > > negative. > > -- What most experimenters take for granted before they begin their experiments is infinitely more interesting than any results to which their experiments lead. -- Norbert Wiener

1 0

Re: [petsc-dev] technical C question
by Barry Smith 07 Apr '16

07 Apr '16

This works. unsigned int joe = index%PETSC_BITS_PER_BYTE; if (joe > 7) joe = 7; return (BT_idx = index/PETSC_BITS_PER_BYTE, BT_c = array[BT_idx], BT_mask = (char)(1 << joe), I think it is because clang is not smart enough to know that (index%PETSC_BITS_PER_BYTE) is always between 0 and 7. Hence it thinks that you might be doing something like 1 << 9 which is as I understand it undefined http://stackoverflow.com/questions/21909412/why-would-the-outcome-of-this-s… So do we pander to the clang analyzer or just leave the warning? I vote to leave the warning and not change the code. Barry > On Apr 7, 2016, at 5:56 PM, Jed Brown <jed(a)jedbrown.org> wrote: > > Satish Balay <balay(a)mcs.anl.gov> writes: > >> On Thu, 7 Apr 2016, Jed Brown wrote: >> >>> Barry Smith <bsmith(a)mcs.anl.gov> writes: >>>> Should we caste to an unsigned PetscInt first then? >>> >>> It should be unsigned, yes. Does that fix the warning? >> >> Nope.. >> >> Commenting out the following line - or changing the argument thus is >> making a difference. >> >> diff --git a/src/mat/impls/baij/seq/baijfact.c b/src/mat/impls/baij/seq/baijfact.c >> index fea37cb..e2c210c 100644 >> --- a/src/mat/impls/baij/seq/baijfact.c >> +++ b/src/mat/impls/baij/seq/baijfact.c >> @@ -1081,7 +1081,7 @@ PetscErrorCode MatICCFactorSymbolic_SeqBAIJ(Mat fact,Mat A,IS perm,const MatFact >> ncols_upper++; >> } >> } >> - ierr = PetscIncompleteLLAdd(ncols_upper,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); >> + ierr = PetscIncompleteLLAdd(ncols,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); > > That looks like it changes the semantics. > > In any case, that file wasn't even mentioned in the message that Barry > shared. If it is indeed the same issue, then it would appear that the > static analyzer has determined that it is possible for index to be > negative.

1 0

Re: [petsc-dev] technical C question
by Jed Brown 07 Apr '16

07 Apr '16

Satish Balay <balay(a)mcs.anl.gov> writes: > On Thu, 7 Apr 2016, Jed Brown wrote: > >> Barry Smith <bsmith(a)mcs.anl.gov> writes: >> > Should we caste to an unsigned PetscInt first then? >> >> It should be unsigned, yes. Does that fix the warning? > > Nope.. > > Commenting out the following line - or changing the argument thus is > making a difference. > > diff --git a/src/mat/impls/baij/seq/baijfact.c b/src/mat/impls/baij/seq/baijfact.c > index fea37cb..e2c210c 100644 > --- a/src/mat/impls/baij/seq/baijfact.c > +++ b/src/mat/impls/baij/seq/baijfact.c > @@ -1081,7 +1081,7 @@ PetscErrorCode MatICCFactorSymbolic_SeqBAIJ(Mat fact,Mat A,IS perm,const MatFact > ncols_upper++; > } > } > - ierr = PetscIncompleteLLAdd(ncols_upper,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); > + ierr = PetscIncompleteLLAdd(ncols,cols,levels,cols_lvl,am,nlnk,lnk,lnk_lvl,lnkbt);CHKERRQ(ierr); That looks like it changes the semantics. In any case, that file wasn't even mentioned in the message that Barry shared. If it is indeed the same issue, then it would appear that the static analyzer has determined that it is possible for index to be negative.

1 0