[flang-commits] [PATCH] D123927: [flang] Do not ICE on recursive function definition in function result

[Peter Klausler via Phabricator via flang-commits]

klausler added inline comments.


================
Comment at: flang/include/flang/Semantics/symbol.h:698
+  inline const DeclTypeSpec *GetTypeImpl(VisitedSymbols &visited) const;
+  inline int RankImpl(VisitedSymbols &visited) const {
+    if (visited.contains(this)) {
----------------
unterumarmung wrote:
> klausler wrote:
> > Your changes make Symbol::Rank() and Symbol::GetType() more expensive to call and liable to allocate/deallocate memory for all programs.  Would it be sufficient to just use an integer argument to limit the call depth?  That would avoid a crash in erroneous tests without affecting compilation for correct programs.
> That's a great idea! But, to be honest, I'm having a hard time coming up with an initial value for such argument. Is there a limit for number of nested calls of these functions for correct programs? Or, maybe, we should use some //big enough// value that //probably// is not encountered in correct programs, yet lets us avoid the stack overflow?
There is not a limit in the standard, but something like 100 should work.  And then you should crash with `CHECK()` instead of returning a rank of 0 or a null type.


================
Comment at: flang/lib/Semantics/check-declarations.cpp:1789
     if (!details) {
+      if (generic.test(Symbol::Flag::Function)) {
+        Characterize(generic);
----------------
unterumarmung wrote:
> klausler wrote:
> > When `generic` is not `GenericDetails`, what is it?
> Well, the `generic` was written not by me. If I understand correctly, `generic` is a `Symbol` that meant to have the `GenericDetails`.
Yes, but you're going out of your way to pass it to Characterize(), so I assume that you've encountered this case in a test somewhere.  If you keep this call here, why does it apply only to functions?  Fortran has generic subroutines as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123927/new/

https://reviews.llvm.org/D123927