This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fix ICE for passing a label for non alternate return arguments
ClosedPublic

Authored by unterumarmung on Apr 18 2022, 8:45 AM.

Details

Summary

When we pass an alternate return specifier to a regular (not an asterisk)
dummy argument, flang would throw an internal compiler error of
derefencing a null pointer.
To avoid the ICE, a check was added.

Diff Detail

Event Timeline

unterumarmung created this revision.Apr 18 2022, 8:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
unterumarmung requested review of this revision.Apr 18 2022, 8:45 AM

Thanks for posting this @unterumarmung! Makes sense to me, but I'd rather defer to somebody with more experience in this area. Let me add a couple more reviewers!

Thanks @unterumarmung for this patch. I have a couple of minor comments.

I have two questions in general. I. can understand if you would like to skip these questions.
-> Are these issues from some internal testsuite?
-> Is this hobby work or official contribution?

flang/lib/Semantics/check-call.cpp
678–684

Isn't this error applicable to DummyProcedure too?

681

Nit: Should it be Alternate return label instead of label?

683

Nit: Sometimes we prefer not to use early returns. Please wait for another opinion.

unterumarmung added inline comments.Apr 20 2022, 3:45 AM
flang/lib/Semantics/check-call.cpp
678–684

I thought about it too! But there's already an error for this code:

fortran
module m
    contains
      subroutine s3(v)
        integer, intent(in) :: v
      end subroutine s3
      subroutine s(x)
        procedure(s3) :: x
      end
      subroutine s2
        call s(*42)
        42 stop
      end
  end module m
error: Semantic errors in a.f90
./a.f90:10:9: error: Assumed-type argument may not be forwarded as procedure dummy argument 'x='
          call s(*42)
          ^^^^^^^^^^^

Yet, the error message is not totally correct. Should I add the check to DummyProcedure also?

681

Nice suggestion, will fix

Rebased, removed useless restorer, address review comment

flang/lib/Semantics/check-call.cpp
678–684

Yes, i think it is useful here as well.

683

Can you remove the early return by using a nested if?

Rebased, addressed review comments

unterumarmung added inline comments.Apr 22 2022, 7:30 AM
flang/lib/Semantics/check-call.cpp
678–684

Done

683

Fixed

LGTM. Thanks for this patch. In case there are comments from other reviewers, please wait for a day before you submit.

flang/lib/Semantics/check-call.cpp
678

Nit: can't -> cannot

This revision is now accepted and ready to land.Apr 25 2022, 5:27 AM

Rebased, fixed the nit

This revision was landed with ongoing or failed builds.May 4 2022, 2:35 AM
This revision was automatically updated to reflect the committed changes.