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

[Daniil Dudkin via Phabricator via flang-commits]

unterumarmung 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:
> > 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.
> Ok, I'll try it and submit the updated patch if it works
Well, I do not think, that we should crash here. The whole point of the change was not to emit the internal compiler error, but proceed with `nullptr` or rank of 0 to give a readable error.
I added the updated version of recursion handling but with returning //invalid// values.
  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123927