Page MenuHomePhabricator

[flang] Fix CheckSpecificationExpr handling of associated names
ClosedPublic

Authored by klausler on Nov 10 2020, 2:55 PM.

Details

Summary

Avoid a spurious error message about a dummy procedure reference
in a specification expression by restructuring the handling of
use-associated and host-associated symbols.

Diff Detail

Event Timeline

klausler created this revision.Nov 10 2020, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 2:55 PM
klausler requested review of this revision.Nov 10 2020, 2:55 PM
PeteSteinfeld accepted this revision.Nov 10 2020, 3:11 PM

All builds, tests, and looks good.

This revision is now accepted and ready to land.Nov 10 2020, 3:11 PM
This revision was landed with ongoing or failed builds.Nov 10 2020, 4:19 PM
This revision was automatically updated to reflect the committed changes.

Hi @klausler , thank you for working on this!

Sadly this is causing our public buildbots to fail:

I'm guessing that this patch wasn't tested with BUILD_SHARED_LIBS=ON? I always try to find and apply a fix for this kind of things, but in this case I'm struggling. IIUC, FortranSemantics is missing from the list of dependencies of libflangEvaluate:

index a2fdc10896b..bbe7857cfc5 100644
--- a/flang/lib/Evaluate/CMakeLists.txt
+++ b/flang/lib/Evaluate/CMakeLists.txt
@@ -42,6 +42,7 @@ add_flang_library(FortranEvaluate
   FortranCommon
   FortranDecimal
   FortranParser
+  FortranSemantics
   ${LIBPGMATH}

   LINK_COMPONENTS

However, that introduces a circular dependency that CMake rejects:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "FortranEvaluate" of type SHARED_LIBRARY
    depends on "FortranSemantics" (weak)
  "FortranSemantics" of type SHARED_LIBRARY
    depends on "FortranEvaluate" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

See here: https://gitlab.kitware.com/cmake/cmake/-/issues/17905

Perhaps I'm missing something obvious? I'm not really familiar with this codebase. I will revert this for now. It's a relatively small patch and hopefully that will not block anyone.

Probably IsModule and IsSubmodule have to be made inline.

I think that the root cause of the build failure is the circular dependency: FortranSemantics <-> FortranEvaluate <-> FortranSemantics. Adding inline to IsModule and IsSubmodule fixes this particular build issue, but not the circular dependency.

Probably IsModule and IsSubmodule have to be made inline.

I think that's the case; will do.

I think that the root cause of the build failure is the circular dependency: FortranSemantics <-> FortranEvaluate <-> FortranSemantics. Adding inline to IsModule and IsSubmodule fixes this particular build issue, but not the circular dependency.

We know. The long-term solution seems to be to refactor the representational aspects of Semantics -- i.e., scopes and symbols -- into their own library, but that's a big job that nobody has time for. For now, we restrict Evaluate to depend on the headers of Semantics but not its binary library. (The other solution would be to combine Evaluate back into Semantics, of course.)