Skip to content

[flang] Refine handling of NULL() actual to non-optional allocatable … #116126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

klausler
Copy link
Contributor

…dummy

We presently allow a NULL() actual argument to associate with a non-optional dummy allocatable argument only under INTENT(IN). This is too strict, as it precludes the case of a dummy argument with default intent. Continue to require that the actual argument be definable under INTENT(OUT) and INTENT(IN OUT), and (contra XLF) interpret NULL() as being an expression, not a definable variable, even when it is given an allocatable MOLD.

Fixes #115984.

@klausler klausler requested a review from DanielCChen November 14, 2024 00:18
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:semantics labels Nov 14, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 14, 2024

@llvm/pr-subscribers-flang-semantics

Author: Peter Klausler (klausler)

Changes

…dummy

We presently allow a NULL() actual argument to associate with a non-optional dummy allocatable argument only under INTENT(IN). This is too strict, as it precludes the case of a dummy argument with default intent. Continue to require that the actual argument be definable under INTENT(OUT) and INTENT(IN OUT), and (contra XLF) interpret NULL() as being an expression, not a definable variable, even when it is given an allocatable MOLD.

Fixes #115984.


Full diff: https://github.com/llvm/llvm-project/pull/116126.diff

4 Files Affected:

  • (modified) flang/include/flang/Common/Fortran-features.h (+2-2)
  • (modified) flang/lib/Common/Fortran-features.cpp (+2)
  • (modified) flang/lib/Semantics/check-call.cpp (+28-23)
  • (modified) flang/test/Semantics/call27.f90 (+15-1)
diff --git a/flang/include/flang/Common/Fortran-features.h b/flang/include/flang/Common/Fortran-features.h
index 74edbe44fdbb1c..78ba3f0d330292 100644
--- a/flang/include/flang/Common/Fortran-features.h
+++ b/flang/include/flang/Common/Fortran-features.h
@@ -38,7 +38,7 @@ ENUM_CLASS(LanguageFeature, BackslashEscapes, OldDebugLines,
     DistinguishableSpecifics, DefaultSave, PointerInSeqType, NonCharacterFormat,
     SaveMainProgram, SaveBigMainProgramVariables,
     DistinctArrayConstructorLengths, PPCVector, RelaxedIntentInChecking,
-    ForwardRefImplicitNoneData, NullActualForAllocatable,
+    NullActualForAllocatable, ForwardRefImplicitNoneData,
     ActualIntegerConvertedToSmallerKind, HollerithOrCharacterAsBOZ,
     BindingAsProcedure, StatementFunctionExtensions,
     UseGenericIntrinsicWhenSpecificDoesntMatch, DataStmtExtensions,
@@ -72,7 +72,7 @@ ENUM_CLASS(UsageWarning, Portability, PointerToUndefinable,
     PreviousScalarUse, RedeclaredInaccessibleComponent, ImplicitShared,
     IndexVarRedefinition, IncompatibleImplicitInterfaces, BadTypeForTarget,
     VectorSubscriptFinalization, UndefinedFunctionResult, UselessIomsg,
-    MismatchingDummyProcedure)
+    MismatchingDummyProcedure, NullActualForDefaultIntentAllocatable)
 
 using LanguageFeatures = EnumSet<LanguageFeature, LanguageFeature_enumSize>;
 using UsageWarnings = EnumSet<UsageWarning, UsageWarning_enumSize>;
diff --git a/flang/lib/Common/Fortran-features.cpp b/flang/lib/Common/Fortran-features.cpp
index fff796e42552a5..719ef2b0291ff8 100644
--- a/flang/lib/Common/Fortran-features.cpp
+++ b/flang/lib/Common/Fortran-features.cpp
@@ -80,8 +80,10 @@ LanguageFeatureControl::LanguageFeatureControl() {
   warnUsage_.set(UsageWarning::VectorSubscriptFinalization);
   warnUsage_.set(UsageWarning::UndefinedFunctionResult);
   warnUsage_.set(UsageWarning::UselessIomsg);
+  warnUsage_.set(UsageWarning::NullActualForDefaultIntentAllocatable);
   // New warnings, on by default
   warnLanguage_.set(LanguageFeature::SavedLocalInSpecExpr);
+  warnLanguage_.set(LanguageFeature::NullActualForAllocatable);
 }
 
 // Ignore case and any inserted punctuation (like '-'/'_')
diff --git a/flang/lib/Semantics/check-call.cpp b/flang/lib/Semantics/check-call.cpp
index a161d2bdf9dbb8..59bbd8b7da56a7 100644
--- a/flang/lib/Semantics/check-call.cpp
+++ b/flang/lib/Semantics/check-call.cpp
@@ -766,21 +766,21 @@ static void CheckExplicitDataArg(const characteristics::DummyDataObject &dummy,
       }
     } else if (actualIsNull) {
       if (dummyIsOptional) {
-      } else if (dummy.intent == common::Intent::In) {
-        // Extension (Intel, NAG, XLF): a NULL() pointer is an acceptable
-        // actual argument for an INTENT(IN) allocatable dummy, and it
-        // is treated as an unassociated allocatable.
-        if (context.ShouldWarn(
-                common::LanguageFeature::NullActualForAllocatable)) {
-          messages.Say(common::LanguageFeature::NullActualForAllocatable,
-              "Allocatable %s is associated with a null pointer"_port_en_US,
-              dummyName);
-        }
-      } else {
+      } else if (dummy.intent == common::Intent::Default &&
+          context.ShouldWarn(
+              common::UsageWarning::NullActualForDefaultIntentAllocatable)) {
         messages.Say(
-            "A null pointer may not be associated with allocatable %s without INTENT(IN)"_err_en_US,
+            "A null pointer should not be associated with allocatable %s without INTENT(IN)"_warn_en_US,
+            dummyName);
+      } else if (dummy.intent == common::Intent::In &&
+          context.ShouldWarn(
+              common::LanguageFeature::NullActualForAllocatable)) {
+        messages.Say(common::LanguageFeature::NullActualForAllocatable,
+            "Allocatable %s is associated with a null pointer"_port_en_US,
             dummyName);
       }
+      // INTENT(OUT) and INTENT(IN OUT) cases are caught elsewhere as being
+      // undefinable actual arguments.
     } else {
       messages.Say(
           "ALLOCATABLE %s must be associated with an ALLOCATABLE actual argument"_err_en_US,
@@ -1265,19 +1265,24 @@ static void CheckExplicitInterfaceArg(evaluate::ActualArgument &arg,
                 } else if (object.attrs.test(characteristics::DummyDataObject::
                                    Attr::Allocatable) &&
                     evaluate::IsNullPointer(*expr)) {
-                  if (object.intent == common::Intent::In) {
-                    // Extension (Intel, NAG, XLF); see CheckExplicitDataArg.
-                    if (context.ShouldWarn(common::LanguageFeature::
-                                NullActualForAllocatable)) {
-                      messages.Say(
-                          common::LanguageFeature::NullActualForAllocatable,
-                          "Allocatable %s is associated with NULL()"_port_en_US,
-                          dummyName);
-                    }
-                  } else {
+                  if (object.intent == common::Intent::Out ||
+                      object.intent == common::Intent::InOut) {
                     messages.Say(
-                        "NULL() actual argument '%s' may not be associated with allocatable %s without INTENT(IN)"_err_en_US,
+                        "NULL() actual argument '%s' may not be associated with allocatable dummy argument %s that is INTENT(OUT) or INTENT(IN OUT)"_err_en_US,
                         expr->AsFortran(), dummyName);
+                  } else if (object.intent == common::Intent::Default &&
+                      context.ShouldWarn(common::UsageWarning::
+                              NullActualForDefaultIntentAllocatable)) {
+                    messages.Say(common::UsageWarning::
+                                     NullActualForDefaultIntentAllocatable,
+                        "NULL() actual argument '%s' should not be associated with allocatable dummy argument %s without INTENT(IN)"_warn_en_US,
+                        expr->AsFortran(), dummyName);
+                  } else if (context.ShouldWarn(common::LanguageFeature::
+                                     NullActualForAllocatable)) {
+                    messages.Say(
+                        common::LanguageFeature::NullActualForAllocatable,
+                        "Allocatable %s is associated with %s"_port_en_US,
+                        dummyName, expr->AsFortran());
                   }
                 } else {
                   messages.Say(
diff --git a/flang/test/Semantics/call27.f90 b/flang/test/Semantics/call27.f90
index 062df6e45da890..135d6c06dcb4ac 100644
--- a/flang/test/Semantics/call27.f90
+++ b/flang/test/Semantics/call27.f90
@@ -1,12 +1,26 @@
 ! RUN: %python %S/test_errors.py %s %flang_fc1 -pedantic
 ! Catch NULL() actual argument association with allocatable dummy argument
 program test
-  !ERROR: NULL() actual argument 'NULL()' may not be associated with allocatable dummy argument 'a=' without INTENT(IN)
+  real, allocatable :: a
+  !ERROR: NULL() actual argument 'NULL()' may not be associated with allocatable dummy argument dummy argument 'a=' that is INTENT(OUT) or INTENT(IN OUT)
+  call foo0(null())
+  !WARNING: NULL() actual argument 'NULL()' should not be associated with allocatable dummy argument dummy argument 'a=' without INTENT(IN)
   call foo1(null())
   !PORTABILITY: Allocatable dummy argument 'a=' is associated with NULL()
   call foo2(null())
   call foo3(null()) ! ok
+  !ERROR: Actual argument associated with INTENT(IN OUT) dummy argument 'a=' is not definable
+  !BECAUSE: 'null(mold=a)' is a null pointer
+  call foo0(null(mold=a))
+  !WARNING: A null pointer should not be associated with allocatable dummy argument 'a=' without INTENT(IN)
+  call foo1(null(mold=a))
+  !PORTABILITY: Allocatable dummy argument 'a=' is associated with a null pointer
+  call foo2(null(mold=a))
+  call foo3(null(mold=a)) ! ok
  contains
+  subroutine foo0(a)
+    real, allocatable, intent(in out) :: a
+  end subroutine
   subroutine foo1(a)
     real, allocatable :: a
   end subroutine

Copy link
Contributor

@DanielCChen DanielCChen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some comments in the issue.
I think the warning message is still confusing.

Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should lowering now generate conditional deallocation after the call in case the dummy argument associated to NULL is allocated inside the function?

Currently, lowering does not do that (relying on the INTENT(IN) aspect). But with this patch, the following code will now compile (with a warning) and leak memory:

module m
contains
subroutine foo(x)
  real, allocatable :: x(:)
  allocate(x(10))
end subroutine
end module
  use m, only : foo
  do i=1,10
  call foo(null())
  end do
end

valgrind:

==2914349== HEAP SUMMARY:
==2914349==     in use at exit: 400 bytes in 10 blocks
==2914349==   total heap usage: 10 allocs, 0 frees, 400 bytes allocated
==2914349== 
==2914349== LEAK SUMMARY:
==2914349==    definitely lost: 400 bytes in 10 blocks

@DanielCChen
Copy link
Contributor

Should lowering now generate conditional deallocation after the call in case the dummy argument associated to NULL is allocated inside the function?

Currently, lowering does not do that (relying on the INTENT(IN) aspect). But with this patch, the following code will now compile (with a warning) and leak memory:

module m
contains
subroutine foo(x)
  real, allocatable :: x(:)
  allocate(x(10))
end subroutine
end module
  use m, only : foo
  do i=1,10
  call foo(null())
  end do
end

valgrind:

==2914349== HEAP SUMMARY:
==2914349==     in use at exit: 400 bytes in 10 blocks
==2914349==   total heap usage: 10 allocs, 0 frees, 400 bytes allocated
==2914349== 
==2914349== LEAK SUMMARY:
==2914349==    definitely lost: 400 bytes in 10 blocks

I would think this is user's responsibility.
I think they should also check if x is allocated or not before the allocate statement and deallocate before exit if the allocate is inside of foo.

@klausler
Copy link
Contributor Author

Daniel and I are trying to figure that out. The question hinges on whether NULL(MOLD=allocatable) is definable or not. If it is, then yes, we'll have to clean up after it. But this patch is valid if NULL(MOLD=allocatable) is not definable, as it would still be conforming to associate it with a dummy argument with default intent, so long as it is not defined.

(If the result of that NULL is definable, then we have to figure out its lifetime -- the standard is silent -- and what other contexts in which it may appear besides an actual argument. And this patch will need to change so that any intent is allowed.)

And this feature only works with XLF, and maybe we'll decide that it's not worth preserving.

@klausler
Copy link
Contributor Author

@DanielCChen ping -- should I merge this patch?

@DanielCChen
Copy link
Contributor

@DanielCChen ping -- should I merge this patch?

I haven't got clear response from the standard committee as some of the folks are on vacation.
At this point, I would rather to keep the behavior as is (not merge this patch) as I don't see much of the usage of allowing it as well as the issue that Jean pointed out.
If you prefer, I can close issue #115984 for now until I get definite answer from the committee.

@klausler
Copy link
Contributor Author

No rush. I'll leave this one hanging for now. I prefer the status quo myself as well (allow only for INTENT(IN)). You can also leave the bug report open.

@klausler klausler marked this pull request as draft December 17, 2024 16:08
@klausler klausler marked this pull request as ready for review February 25, 2025 18:25
…dummy

We presently allow a NULL() actual argument to associate with a non-optional
dummy allocatable argument only under INTENT(IN).  This is too strict, as
it precludes the case of a dummy argument with default intent.
Continue to require that the actual argument be definable under INTENT(OUT)
and INTENT(IN OUT), and (contra XLF) interpret NULL() as being an expression,
not a definable variable, even when it is given an allocatable MOLD.

Fixes llvm#115984.
Fixes llvm#125928.
@klausler
Copy link
Contributor Author

Ping; do you @DanielCChen have any objection to merging this fix?

Copy link
Contributor

@DanielCChen DanielCChen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thanks for the fix!

@klausler klausler merged commit 9a49a03 into llvm:main Feb 27, 2025
11 checks passed
@klausler klausler deleted the bug115984 branch February 27, 2025 22:27
cheezeburglar pushed a commit to cheezeburglar/llvm-project that referenced this pull request Feb 28, 2025
llvm#116126)

…dummy

We presently allow a NULL() actual argument to associate with a
non-optional dummy allocatable argument only under INTENT(IN). This is
too strict, as it precludes the case of a dummy argument with default
intent. Continue to require that the actual argument be definable under
INTENT(OUT) and INTENT(IN OUT), and (contra XLF) interpret NULL() as
being an expression, not a definable variable, even when it is given an
allocatable MOLD.

Fixes llvm#115984.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flang] Incorrect diagnose on NULL(mold) actual to allocatable dummy
4 participants