Page MenuHomePhabricator

[flang] Include missing internal interfaces in .mod files
ClosedPublic

Authored by ekieri on Mar 15 2022, 1:43 PM.

Details

Summary

Interfaces which are internal to a procedure need to be included in
module files if (and only if) they are referenced in the interface of
the procedure. That is, they are needed if they are the interfaces of
dummy or return value procedures.

Fixes #53420

Diff Detail

Event Timeline

ekieri created this revision.Mar 15 2022, 1:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
ekieri requested review of this revision.Mar 15 2022, 1:43 PM
ekieri added a comment.EditedMar 15 2022, 1:45 PM

Right, here is a first attempt at fixing [1]. Please check in that my intent, as described in the above summary, is the desired behaviour.

[1] https://github.com/llvm/llvm-project/issues/53420

PeteSteinfeld requested changes to this revision.Mar 15 2022, 2:43 PM

I'm getting build errors because the variable subprogramDetails declared at line 1060 is never used.

This revision now requires changes to proceed.Mar 15 2022, 2:43 PM
ekieri updated this revision to Diff 415608.Mar 15 2022, 3:29 PM

Sorry about that ...

@ekieri, many thanks for doing this! This is not my area of expertise, so I will defer to Pete.

I've just noticed that the pre-merge CI is failing to apply your patch. Perhaps try rebasing on top of main?

ekieri updated this revision to Diff 415897.Mar 16 2022, 10:25 AM

Hi Andrzej, many thanks for all the encouragement! Yes, I added you more
like a cc (could rather have used that, I realise) as you did the
initial triage.

The CI log says git was unable to acquire a lock file, which did not
make me think it was a rebasing issue. But let's give it a try!

Hi Andrzej, many thanks for all the encouragement! Yes, I added you more
like a cc (could rather have used that, I realise) as you did the
initial triage.

The CI log says git was unable to acquire a lock file, which did not
make me think it was a rebasing issue. But let's give it a try!

Ah! I checked manually - your patch applies cleanly and all tests pass :) Looks like an infra bug - reported here: https://github.com/google/llvm-premerge-checks/issues/395

-Andrzej

PeteSteinfeld accepted this revision.Mar 16 2022, 11:38 AM

Thanks for doing this, Emil.

Aside from my small suggestion, everything builds and tests correctly and looks good.

flang/lib/Semantics/mod-file.cpp
1079–1081

This would be slightly more efficient as

needed = needed ||
  (details.isFunction() && hasInterface(&details.result()));
This revision is now accepted and ready to land.Mar 16 2022, 11:38 AM
ekieri updated this revision to Diff 415945.Mar 16 2022, 12:54 PM

Thanks for the suggestion, Pete!

ekieri marked an inline comment as done.Mar 16 2022, 1:12 PM
This revision was automatically updated to reflect the committed changes.