Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Thomas Koenig]

Hi Janus,


and in that
case I would argue that, beyond being "not useful", it's even illegal,
so why not throw a hard error, if we can infer at compile-time that
the target is non-contiguous?


Problem is, we cannot infer this from the tests done.
We would also have to add a test if the array is empty
or that it contains only a single element, and that (I think)
is a) impossible in the general case, and b) not worth the bother.


I'm not sure I understand which cases you're worried about here. Maybe
you can give an example?


   real, dimension(5,5), target :: a
   real, dimension(:,:), pointer, contiguous :: ap
   ap => a(4::2,4::2)

points to a single element, which is (by definition) contiguous.


yes, I see that your current patch mishandles this case.


It does _not_ mishandle that case.  It issues a warning for a
very dubious practice. The warning can be turned off at will.

Issuing a hard error whould be mishandling this case.



But why
should it be impossible to detect that there is only a single element?
If the triplet and the array bounds are constant, it is certainly
possible.


Too much work into supporting a corner case of a dubious practice.  I'm
not going to do it.


There are also a few other checks that I missed, for
example

   subroutine foo(a)
   real, dimension(:), target, intent(inout) :: a
   real, dimension(:), contiguous :: ap
   ap => a

I will also address this in a future version of the patch.


How do you want to do that?


Warn in all cases.

Because the code writer can not how if the array passed to the
subroutine is contigous (use case of a library), it is not
possible for him to know if the user will do this correctly or
not.  The correct way to write this subroutine then is

subroutine foo(a)
  real, dimension(:), target, intent(inout), contiguous :: a
  real, dimension(:), contiguous :: ap
  ap => a

when gfortran will someday complain if the dummy argument
is associated with a non-contiguous entity.

Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Janus Weil]

Hi Thomas,

 and in that
 case I would argue that, beyond being "not useful", it's even illegal,
 so why not throw a hard error, if we can infer at compile-time that
 the target is non-contiguous?
>>>
>>> Problem is, we cannot infer this from the tests done.
>>> We would also have to add a test if the array is empty
>>> or that it contains only a single element, and that (I think)
>>> is a) impossible in the general case, and b) not worth the bother.
>>
>> I'm not sure I understand which cases you're worried about here. Maybe
>> you can give an example?
>
>   real, dimension(5,5), target :: a
>   real, dimension(:,:), pointer, contiguous :: ap
>   ap => a(4::2,4::2)
>
> points to a single element, which is (by definition) contiguous.

yes, I see that your current patch mishandles this case. But why
should it be impossible to detect that there is only a single element?
If the triplet and the array bounds are constant, it is certainly
possible.


>> In any case, I think your test case is a bit short, so I extended it
>> somewhat (see attachment) and found two cases along the way where your
>> patch throws a warning but shouldn't:
>>
>> r => x(::-1)
>
> This one is _not_ contiguous; contiguos implies stride==1.

Ok, apparently I did not read the F08 standard carefully. I guess the
mention of the "array element order" in chapter 5.3.7 forbids the
negative stride (which means reversed order).


>> Apart from the two mishandled cases above, one other thing comes to my
>> mind: It might be a good idea to apply your checks not only to pointer
>> assignments, but also to dummy arguments (passing a non-contiguous
>> array to a contiguous dummy pointer), where the same rules should
>> apply.
>
> This is true.
>
> There are also a few other checks that I missed, for
> example
>
>   subroutine foo(a)
>   real, dimension(:), target, intent(inout) :: a
>   real, dimension(:), contiguous :: ap
>   ap => a
>
> I will also address this in a future version of the patch.

How do you want to do that? I don't see a way to tell at compile time
if 'a' here is contiguous (like in many other cases). I guess one
would ultimately need a runtime check. Or do you want to warn
unconditionally because it 'might' be non-contiguous?

Cheers,
Janus


2017-08-29 20:13 GMT+02:00 Thomas Koenig :
> Hi Janus,
>
> I think an unconditional warning is OK
> in this case because
>
> - Assigning to a pointer from an obvious non-contiguous target
> is not useful at all, that I can see



 I guess you're talking about a *contiguous* pointer here,
>>>
>>>
>>> Correct.
>>>
 and in that
 case I would argue that, beyond being "not useful", it's even illegal,
 so why not throw a hard error, if we can infer at compile-time that
 the target is non-contiguous?
>>>
>>>
>>> Problem is, we cannot infer this from the tests done.
>>> We would also have to add a test if the array is empty
>>> or that it contains only a single element, and that (I think)
>>> is a) impossible in the general case, and b) not worth the bother.
>>
>>
>> I'm not sure I understand which cases you're worried about here. Maybe
>> you can give an example?
>
>
>
>   real, dimension(5,5), target :: a
>   real, dimension(:,:), pointer, contiguous :: ap
>   ap => a(4::2,4::2)
>
> points to a single element, which is (by definition) contiguous.
>
>> In any case, I think your test case is a bit short, so I extended it
>> somewhat (see attachment) and found two cases along the way where your
>> patch throws a warning but shouldn't:
>>
>> r => x(::-1)
>
>
> This one is _not_ contiguous; contiguos implies stride==1.
>
>> r => x2(2:3,1)
>
>
> Correct, I will correct that one.
>
>> Apart from the two mishandled cases above, one other thing comes to my
>> mind: It might be a good idea to apply your checks not only to pointer
>> assignments, but also to dummy arguments (passing a non-contiguous
>> array to a contiguous dummy pointer), where the same rules should
>> apply.
>
>
> This is true.
>
> There are also a few other checks that I missed, for
> example
>
>   subroutine foo(a)
>   real, dimension(:), target, intent(inout) :: a
>   real, dimension(:), contiguous :: ap
>   ap => a
>
> I will also address this in a future version of the patch. This will
> take a bit of time, though.
>
> Regards
>
> Thomas

Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Thomas Koenig]

Hi Janus,


I think an unconditional warning is OK
in this case because

- Assigning to a pointer from an obvious non-contiguous target
is not useful at all, that I can see



I guess you're talking about a *contiguous* pointer here,


Correct.


and in that
case I would argue that, beyond being "not useful", it's even illegal,
so why not throw a hard error, if we can infer at compile-time that
the target is non-contiguous?


Problem is, we cannot infer this from the tests done.
We would also have to add a test if the array is empty
or that it contains only a single element, and that (I think)
is a) impossible in the general case, and b) not worth the bother.


I'm not sure I understand which cases you're worried about here. Maybe
you can give an example?



  real, dimension(5,5), target :: a
  real, dimension(:,:), pointer, contiguous :: ap
  ap => a(4::2,4::2)

points to a single element, which is (by definition) contiguous.


In any case, I think your test case is a bit short, so I extended it
somewhat (see attachment) and found two cases along the way where your
patch throws a warning but shouldn't:

r => x(::-1)


This one is _not_ contiguous; contiguos implies stride==1.


r => x2(2:3,1)


Correct, I will correct that one.


Apart from the two mishandled cases above, one other thing comes to my
mind: It might be a good idea to apply your checks not only to pointer
assignments, but also to dummy arguments (passing a non-contiguous
array to a contiguous dummy pointer), where the same rules should
apply.


This is true.

There are also a few other checks that I missed, for
example

  subroutine foo(a)
  real, dimension(:), target, intent(inout) :: a
  real, dimension(:), contiguous :: ap
  ap => a

I will also address this in a future version of the patch. This will
take a bit of time, though.

Regards

Thomas

Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Janus Weil]

Hi Thomas,

>>> I think an unconditional warning is OK
>>> in this case because
>>>
>>> - Assigning to a pointer from an obvious non-contiguous target
>>>is not useful at all, that I can see
>>
>>
>> I guess you're talking about a *contiguous* pointer here,
>
> Correct.
>
>> and in that
>> case I would argue that, beyond being "not useful", it's even illegal,
>> so why not throw a hard error, if we can infer at compile-time that
>> the target is non-contiguous?
>
> Problem is, we cannot infer this from the tests done.
> We would also have to add a test if the array is empty
> or that it contains only a single element, and that (I think)
> is a) impossible in the general case, and b) not worth the bother.

I'm not sure I understand which cases you're worried about here. Maybe
you can give an example?

In any case, I think your test case is a bit short, so I extended it
somewhat (see attachment) and found two cases along the way where your
patch throws a warning but shouldn't:

r => x(::-1)
r => x2(2:3,1)

The first one is a stride of minus one, which is valid and contiguous
AFAICS. The second one is obviously also contiguous.


>> As explained above, I'd vote for an error (or at least a conditional
>> warning, so that it can be disabled, like most(?) other gfortran
>> warnings).
>
> Attached is a version which makes this a warning enabled by -Wall;
> this should be enough to give people a heads-up.
>
> I have introduced most of your comments into the revised patch.

Thank you, looks much better already.

Apart from the two mishandled cases above, one other thing comes to my
mind: It might be a good idea to apply your checks not only to pointer
assignments, but also to dummy arguments (passing a non-contiguous
array to a contiguous dummy pointer), where the same rules should
apply.


> So, here is the updated patch.  Regression-tested on
> powerpc-linux, make dvi and make pdf also passed.
> OK for trunk?

Not quite, but we're getting closer :)

Sorry for being such a nag ...

Cheers,
Janus
! { dg-do compile }
! { dg-additional-options "-Wcontiguous" }
program cont_01_neg
  implicit none
  real, pointer, contiguous :: r(:)
  real, pointer, contiguous :: r2(:,:)
  real, target :: x(45)
  real, target :: x2(5,9)
  integer :: i

  x = (/ (real(i),i=1,45) /)
  x2 = reshape(x,shape(x2))
  i = 4

  r => x(::1)
  r => x(::-1)  ! bogus warning here
  r => x(::3)   ! { dg-warning "Assignment to contiguous pointer" }
  r => x(::-3)  ! { dg-warning "Assignment to contiguous pointer" }
  r => x(::i)   ! { dg-warning "Assignment to contiguous pointer" }

  r2 => x2(:,2:)
  r2 => x2(1:,:)
  r2 => x2(2:,:)! { dg-warning "Assignment to contiguous pointer" }

  r2 => x2(:,:4)
  r2 => x2(:5,:)
  r2 => x2(:4,:)! { dg-warning "Assignment to contiguous pointer" }

  r2 => x2(:,2:3)
  r2 => x2(1:5,:)
  r2 => x2(2:3,:)   ! { dg-warning "Assignment to contiguous pointer" }

  r => x2(3,:)  ! { dg-warning "Assignment to contiguous pointer" }
  r => x2(:,3)

  r2 => x2(3:3,:)   ! { dg-warning "Assignment to contiguous pointer" }
  r2 => x2(:,3:3)

  r2 => x2(2:3:2,:) ! { dg-warning "Assignment to contiguous pointer" }
  r2 => x2(:,2:3:2) ! { dg-warning "Assignment to contiguous pointer" }

  r => x2(1,2:3)
  r => x2(2:3,1)! bogus warning here

end program

Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Thomas Koenig]

Hi Janus,





I think an unconditional warning is OK
in this case because

- Assigning to a pointer from an obvious non-contiguous target
   is not useful at all, that I can see


I guess you're talking about a *contiguous* pointer here,


Correct.


and in that
case I would argue that, beyond being "not useful", it's even illegal,
so why not throw a hard error, if we can infer at compile-time that
the target is non-contiguous?


Problem is, we cannot infer this from the tests done.
We would also have to add a test if the array is empty
or that it contains only a single element, and that (I think)
is a) impossible in the general case, and b) not worth the bother.




- Some language laywer will come up with the fact that it is,
   in fact, legal if the target is empty or has a single
   element only, so a hard error would be a rejects-valid.


Should be possible to detect and ignore such cases, shouldn't it?


Not in general.




However, I can also make this into a warning depending on
-Wall, if this is preferred.


As explained above, I'd vote for an error (or at least a conditional
warning, so that it can be disabled, like most(?) other gfortran
warnings).


Attached is a version which makes this a warning enabled by -Wall;
this should be enough to give people a heads-up.

I have introduced most of your comments into the revised patch.


Also I noticed that there is a function called
"gfc_is_simply_contiguous" in expr.c. Could that be useful for your
purpose? (Some of the code in there looks very similar to the logic
you're adding.)


There is a subtle distinction between "simply contiguous" where
the compiler _has_ to know at compile-time that the expression
is contigous (and failure to diagnose is is a compiler error)
and "contiguous" where the burden is on the programmer.
My patch is intended to be an aid to the programmer for
the "continuous" case.

Because of the "simple contiguous" thing, the function does
not quite match the requirements.

So, here is the updated patch.  Regression-tested on
powerpc-linux, make dvi and make pdf also passed.
OK for trunk?

Regards

Thomas

2017-08-27  Thomas Koenig  

PR fortran/49232
* expr.c (gfc_check_pointer_assign): Warn for
suspicious assignments with contiguous pointrs if
-Wcontiguous is given.
* lang.opt (Wcontiguous): New option, implied
by -Wall.
* invoke.texi: Document -Wcontiguous.

2017-08-27  Thomas Koenig  

PR fortran/49232
* gfortran.dg/contiguous_4.f90: New test.
Index: expr.c
===
--- expr.c	(Revision 251375)
+++ expr.c	(Arbeitskopie)
@@ -3802,6 +3802,54 @@
 	  }
 }
 
+  /* Warn for assignments of contiguous pointers to targets which might
+ not be contiguous.  */
+
+  if (warn_contiguous && lhs_attr.contiguous)
+{
+  gfc_array_ref *ar;
+  int i;
+  bool do_warn = false;
+  
+  ar = gfc_find_array_ref (rvalue, true);
+  if (ar && ar->type == AR_SECTION)
+	{
+	  for (i = 0; i < ar->dimen; i++)
+	{
+	  /* Check for p => a(:,:,2).  */
+	  if (ar->dimen_type[i] == DIMEN_RANGE && ar->stride[i]
+		  && (ar->stride[i]->expr_type != EXPR_CONSTANT
+		  || (ar->stride[i]->expr_type == EXPR_CONSTANT
+			  && mpz_cmp_si (ar->stride[i]->value.integer, 1
+		{
+		  do_warn = true;
+		  break;
+		}
+	}
+	  if (!do_warn && ar->dimen > 1)
+	{
+	  /* Check for p => a(2:4,:) */
+	  for (i = 0; i < ar->dimen - 1; i++)
+		{
+		  if ((ar->start[i] && ar->as->lower[i]
+		   && gfc_dep_compare_expr (ar->start[i], ar->as->lower[i])
+		   != 0)
+		  || (ar->end[i] && ar->as->upper[i]
+			  && gfc_dep_compare_expr (ar->end[i], ar->as->upper[i])
+			  != 0))
+		{
+		  do_warn = true;
+		  break;
+		}
+		}
+	}
+	}
+  if (do_warn)
+	gfc_warning (OPT_Wcontiguous, "Assignment to contiguous pointer "
+		 "from possibly non-contiguous target at %L",
+		 >where);
+}
+
   /* Warn if it is the LHS pointer may lives longer than the RHS target.  */
   if (warn_target_lifetime
   && rvalue->expr_type == EXPR_VARIABLE
Index: invoke.texi
===
--- invoke.texi	(Revision 251375)
+++ invoke.texi	(Arbeitskopie)
@@ -145,7 +145,7 @@
 @xref{Error and Warning Options,,Options to request or suppress errors
 and warnings}.
 @gccoptlist{-Waliasing -Wall -Wampersand -Wargument-mismatch -Warray-bounds
--Wc-binding-type -Wcharacter-truncation @gol
+-Wc-binding-type -Wcharacter-truncation -Wcontiguous @gol
 -Wconversion -Wfunction-elimination -Wimplicit-interface @gol
 -Wimplicit-procedure -Wintrinsic-shadow -Wuse-without-only -Wintrinsics-std @gol
 -Wline-truncation -Wno-align-commons -Wno-tabs -Wreal-q-constant @gol
@@ -797,7 +797,7 @@
 This currently includes @option{-Waliasing}, @option{-Wampersand},
 @option{-Wconversion}, 

Re: [patch, fortran] Warn about suspicious assignment to contiguous pointers

[Janus Weil]

Hi Thomas,

> the attached patch warns about the dubious pointer assignments (see
> test case for details).

thanks for the patch! Sounds like a useful diagnostic.


> I think an unconditional warning is OK
> in this case because
>
> - Assigning to a pointer from an obvious non-contiguous target
>   is not useful at all, that I can see

I guess you're talking about a *contiguous* pointer here, and in that
case I would argue that, beyond being "not useful", it's even illegal,
so why not throw a hard error, if we can infer at compile-time that
the target is non-contiguous?


> - Some language laywer will come up with the fact that it is,
>   in fact, legal if the target is empty or has a single
>   element only, so a hard error would be a rejects-valid.

Should be possible to detect and ignore such cases, shouldn't it?


> However, I can also make this into a warning depending on
> -Wall, if this is preferred.

As explained above, I'd vote for an error (or at least a conditional
warning, so that it can be disabled, like most(?) other gfortran
warnings).


On top, some direct comments to your patch:

+  /* Warn for suspicious assignments like
+
+ pointer, real, dimension(:), contiguous :: p
+ real, dimension(10,10) :: a
+ p => a(:,:,2) or p => a(2:4,:)  */

I'm all for good and extensive comments, but instead of writing out
example code here, it might be more concise to just say something
like: "Check for assignments of contiguous pointers to non-contiguous
targets."

+  gfc_array_ref *ar;
+  int i;
+  bool do_warn;
+
+  do_warn = false;
+  ar = NULL;

Maybe add the initialization directly to the declaration here?

gfc_array_ref *ar = NULL;
bool do_warn = false;

The following could be replaced by "gfc_find_array_ref", I guess?

+  for (ref = rvalue->ref; ref; ref = ref->next)
+{
+  if (ref->type == REF_ARRAY)
+{
+  ar = >u.ar;
+  break;
+}
+}

Also I noticed that there is a function called
"gfc_is_simply_contiguous" in expr.c. Could that be useful for your
purpose? (Some of the code in there looks very similar to the logic
you're adding.)


Cheers,
Janus

[patch, fortran] Warn about suspicious assignment to contiguous pointers

[Thomas Koenig]

Hello world,

the attached patch warns about the dubious pointer assignments (see
test case for details).  I think an unconditional warning is OK
in this case because

- Assigning to a pointer from an obvious non-contiguous target
  is not useful at all, that I can see

- Some language laywer will come up with the fact that it is,
  in fact, legal if the target is empty or has a single
  element only, so a hard error would be a rejects-valid.

However, I can also make this into a warning depending on
-Wall, if this is preferred.

Regresson-tested. OK for trunk?

Regards

Thomas

2017-08-27  Thomas Koenig  

PR fortran/49232
* expr.c (gfc_check_pointer_assign): Warn for
suspicious assignments with contiguous pointers.

2017-08-27  Thomas Koenig  

PR fortran/49232
* gfortran.dg/contiguous_4.f90: New test.
Index: expr.c
===
--- expr.c	(Revision 239977)
+++ expr.c	(Arbeitskopie)
@@ -3764,6 +3764,66 @@ gfc_check_pointer_assign (gfc_expr *lvalue, gfc_ex
 	  }
 }
 
+  /* Warn for suspicious assignments like
+
+ pointer, real, dimension(:), contiguous :: p
+ real, dimension(10,10) :: a
+ p => a(:,:,2) or p => a(2:4,:)  */
+
+  if (lhs_attr.contiguous)
+{
+  gfc_array_ref *ar;
+  int i;
+  bool do_warn;
+  
+  do_warn = false;
+  ar = NULL;
+
+  for (ref = rvalue->ref; ref; ref = ref->next)
+	{
+	  if (ref->type == REF_ARRAY)
+	{
+	  ar = >u.ar;
+	  break;
+	}
+	}
+  if (ar && ar->type == AR_SECTION)
+	{
+
+	  for (i = 0; i < ar->dimen; i++)
+	{
+	  if (ar->dimen_type[i] == DIMEN_RANGE && ar->stride[i]
+		  && (ar->stride[i]->expr_type != EXPR_CONSTANT
+		  || (ar->stride[i]->expr_type == EXPR_CONSTANT
+			  && mpz_cmp_si (ar->stride[i]->value.integer, 1
+		{
+		  do_warn = true;
+		  break;
+		}
+	}
+	  if (!do_warn && ar->dimen > 1)
+	{
+	  for (i = 0; i < ar->dimen - 1; i++)
+		{
+		  if ((ar->start[i] && ar->as->lower[i]
+		   && gfc_dep_compare_expr (ar->start[i], ar->as->lower[i])
+		   != 0)
+		  || (ar->end[i] && ar->as->upper[i]
+			  && gfc_dep_compare_expr (ar->end[i], ar->as->upper[i])
+			  != 0))
+		{
+		  do_warn = true;
+		  break;
+		}
+		}
+	}
+	}
+  if (do_warn)
+	gfc_warning (0, "Assignment to contiguous pointer from "
+		 "possibly non-contiguous target at %L",
+		 >where);
+}
+
   /* Warn if it is the LHS pointer may lives longer than the RHS target.  */
   if (warn_target_lifetime
   && rvalue->expr_type == EXPR_VARIABLE
! { dg-do compile }
program cont_01_neg
  implicit none
  real, pointer, contiguous :: r(:)
  real, pointer, contiguous :: r2(:,:)
  real, target :: x(45)
  real, target :: x2(5,9)
  integer :: i

  x = (/ (real(i),i=1,45) /)
  x2 = reshape(x,shape(x2))
  r => x(::3)   ! { dg-warning "Assignment to contiguous pointer" }
  r2 => x2(2:,:) ! { dg-warning "Assignment to contiguous pointer" }
  r2 => x2(:,2:3)

end program