[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)) {
----------------
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


================
Comment at: flang/lib/Semantics/check-declarations.cpp:1789
     if (!details) {
+      if (generic.test(Symbol::Flag::Function)) {
+        Characterize(generic);
----------------
klausler wrote:
> 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.
Well, if we delete this chunk of code and run the test, we will get an ICE in the lowering part of the compiler because we do not call the `Characterize` on `foo` or `r` (from the test) because they do not have `GenericDetails`. And because there's not any other compile errors, the execution of compiler successfully goes to the lowering, where it tries to generate `Procedure` for `foo`  via `Characterize` and crashes there. I added the code here specifically for functions, to enable at least some semantic checks for the `generic` symbol.  

This is not the best solution and I'd be glad to consider your (or anyone else's) advices how to make it better, if you have any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123927