[Flang] Revise the fix in PR #81087 to get specific procedure from a potential generic name.

[DanielCChen]

This PR is to revise the fix in PR #81087 to handle the case that @psteinfeld brought up. Both test cases in PR #81087 now pass with this PR.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

Author: Daniel Chen (DanielCChen)

Changes

This PR is to revise the fix in PR #81807 to handle the case that @psteinfeld brought up. Both test cases in PR #81807 now pass with this PR.

---

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

2 Files Affected:

• (modified) flang/include/flang/Semantics/symbol.h (+13)
• (modified) flang/lib/Semantics/resolve-names.cpp (+4-2)

diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
index 4535a92ce3dd8e..0577cba9587d55 100644
--- a/flang/include/flang/Semantics/symbol.h
+++ b/flang/include/flang/Semantics/symbol.h
@@ -786,6 +786,9 @@ class Symbol {
   inline Symbol &GetUltimate();
   inline const Symbol &GetUltimate() const;
 
+  // Get the specific procedure from a potential generic symbol.
+  inline const Symbol *GetUltimateGeneric() const;
+
   inline DeclTypeSpec *GetType();
   inline const DeclTypeSpec *GetType() const;
   void SetType(const DeclTypeSpec &);
@@ -985,6 +988,16 @@ inline const Symbol &Symbol::GetUltimate() const {
   }
 }
 
+inline const Symbol *Symbol::GetUltimateGeneric() const {
+  if (this->has<GenericDetails>())
+    return this;
+  if (const auto *details{detailsIf<UseDetails>()})
+    return details->symbol().GetUltimateGeneric();
+  if (const auto *details{detailsIf<HostAssocDetails>()})
+    return details->symbol().GetUltimateGeneric();
+  return nullptr;
+}
+
 inline DeclTypeSpec *Symbol::GetType() {
   return const_cast<DeclTypeSpec *>(
       const_cast<const Symbol *>(this)->GetType());
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 36deab969456d0..0e21f3dabb6a01 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -5644,12 +5644,14 @@ void DeclarationVisitor::Post(const parser::ProcInterface &x) {
     NoteInterfaceName(*name);
   }
 }
+
 void DeclarationVisitor::Post(const parser::ProcDecl &x) {
   const auto &name{std::get<parser::Name>(x.t)};
   const Symbol *procInterface{nullptr};
   if (interfaceName_) {
-    procInterface = interfaceName_->symbol->has<GenericDetails>()
-        ? interfaceName_->symbol->get<GenericDetails>().specific()
+    const Symbol *ultimateGeneric{interfaceName_->symbol->GetUltimateGeneric()};
+    procInterface = ultimateGeneric
+        ? ultimateGeneric->get<GenericDetails>().specific()
         : interfaceName_->symbol;
   }
   auto attrs{HandleSaveName(name.source, GetAttrs())};

[jeanPerier]

I am afraid this might be pushing the problem further and that the issue would still occur if the renaming was done on a generic. I think we cannot easily change the procedure interface symbol in semantics without risking breaking the module file generation in case of renaming.

File 1:

module fmod
    INTERFACE inner
      MODULE PROCEDURE  inner
    END INTERFACE
contains
  subroutine inner(n)
    print *, n
  end subroutine
end module
module fmod1
  use fmod,proc1=>inner
  procedure(proc1),pointer :: p1
end module

File 2:

Program test
  Use fmod1
  p1 => proc1
  Call p1(343)
End Program

Would also raise the bogus semantic errors with this patch because the generated fmod1 module file also "bypass" the renaming in its procedure statement.

I think we should instead change the approach from #80738 and deal with the fact that the interface symbol may have generic details. Maybe adding a case for GenericDetails in Symbol::GetType() would be the best approach:

llvm-project/flang/include/flang/Semantics/symbol.h

Line 1016 in e79ad7b

},

I would run that through @klausler when he is back next week though to make sure there no code out there that expect Symbol::GetType to be null for generics.

[psteinfeld]

I have verified that this change passes all of our internal tests.

[DanielCChen]

I have verified that this change passes all of our internal tests.

Thanks!

[DanielCChen]

Program test
Use fmod1
p1 => proc1
Call p1(343)
End Program

Thanks for the review. Sure, I added @klausler as a reviewer and will wait for his input.

[DanielCChen]

@jeanPerier Your suggestion of "adding a case for GenericDetails in Symbol::GetType() " works for all cases that have been listed in all related PRs. Thanks for the suggestion! I will update this PR to change to that.

[DanielCChen]

@jeanPerier and @klausler By glaring through symbol.h, it seems RankImpl and Corank might also missing the GenericDetails case.

[psteinfeld]

I've tested again with your latest changes, and all looks good.

[DanielCChen]

I've tested again with your latest changes, and all looks good.

Thank you very much!

[jeanPerier]

it seems RankImpl and Corank might also missing the GenericDetails case.

Thanks for pushing the reasoning here, yes, I think you are correct, this also needs to be updated if go that path. But I am not entirely sure this is the only and best path (see next comment).

module generic_iface
    interface somefunc
      module procedure  somefunc
    end interface
contains
  function somefunc()
    integer :: somefunc(2)
    somefunc = [1,2]
  end function
end module
subroutine bug(p)
  use generic_iface,proc=>somefunc
  procedure(proc) :: p
  print *, p()
end subroutine

Would otherwise crash in lowering because expr.getRank() is inconsistent with the rank that is later found when lowering p().

[jeanPerier]

As mentioned in the previous comment, maybe there is a way to still update the interface symbol as you did in #81087 and #80738 and deal with the module file renaming issue. It seems module files may use a custom variable format to deal with this case with function calls.

module generic_iface
    interface somefunc
      module procedure  somefunc
    end interface
contains
  pure integer function somefunc()
    somefunc = 4
  end function
end module
module m
contains
  function foo()
    use generic_iface, only: renamed=>somefunc
    character(renamed()) :: foo
    bug = "hello"
  end function
end module

 use m
  print *, foo()
end

The module file m looks like:

!mod$ v1 sum:b9a6a09789c9dad6
module m              
contains                        
function foo()    
use generic_iface,only:generic_iface$generic_iface$somefunc=>somefunc
character(generic_iface$generic_iface$somefunc(),1)::foo
end              
end

This currently trigger a bogus error "Invalid specification expression: reference to impure function 'somefunc'" when the program is compiled in its own file, so this may require some more work, but at least the renaming is not an issue in this case.

@klausler, how is the renamed() generic call rewritten to generic_iface$generic_iface$somefunc() in the module file? Would it be possible to do the same with a procedure(renamed) statement where the host associated symbol has been replaced by the specific symbol from the module?

[DanielCChen]

I opened another Issue #82267 that the BIND(C) attribute got lost when the generic name is present. It might affect the fix of this PR.

[DanielCChen]

This one could be related to too. Issue #82390

[DanielCChen]

Just to confirm that PR #82837 fixed both code in #81087 and #80738. However, both @jeanPerier 's code in the PR still fail.

[DanielCChen]

@jeanPerier and @klausler , PR #82837 fixed the code in this PR. but the two cases that Jean brought up in this PR still fail. Do you prefer I close this PR and open a new one for those two failures or keep it open to track those?

[jeanPerier]

@DanielCChen, I opened issue #83836 with the reproducer, I think this PR can be closed for more clarity.

[DanielCChen]

Thanks @jeanPerier . Close this PR as suggested.