This is an archive of the discontinued LLVM Phabricator instance.

[flang][f18] Add an additional check for semantic errors
AbandonedPublic

Authored by awarzynski on Apr 16 2021, 7:22 AM.

Details

Summary

Semantics::AnyFatalError may return true after
Fortran::semantics::BuildRuntimeDerivedTypeTables is run. That's
despite Semantics::perform having been run earlier and not reporting
any issues. This is demonstrated below:

semantics.perform()
// No errors here, i.e. `semantics.AnyFatalError()` returns `false`
BuildRuntimeDerivedTypeTables()
// New semantic errors are found, i.e. `semantics.AnyFatalError()` returns `true`

This patch udpates the "throwaway" driver so that the semantic errors
identified after BuildRuntimeDerivedTypeTables are correctly reported.

This change affects some tests - the corresponding input leads to
semantic errors that are only identified after
BuildRuntimeDerivedTypeTables is run. The generated output (which is
fed into FileCheck) still matches the expected output, but f18 returns
a non-zero code. As a temporary solution workaround for this, %f18 is
replaced with not %f18.

Diff Detail

Event Timeline

awarzynski requested review of this revision.Apr 16 2021, 7:22 AM
awarzynski created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 7:22 AM

I've noticed that there might be new semantic errors generated after BuildRuntimeDerivedTypeTables is run. Should we ignore or report them? The tests that trigger these errors look fine to me, so I'm not sure.

awarzynski added a project: Restricted Project.

Revert %flang_fc1 --> %f18 (leftover from experimenting)

semantics.perform()
// No errors here, i.e. `semantics.AnyFatalError()` returns `false`
BuildRuntimeDerivedTypeTables()
// New semantic errors are found, i.e. `semantics.AnyFatalError()` returns `false`

How do you know that more errors were found if AnyFatalError() is still false?

awarzynski edited the summary of this revision. (Show Details)Apr 16 2021, 9:56 AM
semantics.perform()
// No errors here, i.e. `semantics.AnyFatalError()` returns `false`
BuildRuntimeDerivedTypeTables()
// New semantic errors are found, i.e. `semantics.AnyFatalError()` returns `false`

How do you know that more errors were found if AnyFatalError() is still false?

That's a typo, apologies.

So AnyFatalError() is true after the derived type tables are built for the runtime? Were there additional Messages?

So AnyFatalError() is true after the derived type tables are built for the runtime? Were there additional Messages?

Yes. You can reproduce it with this file (extracted from offset01.f90):

subroutine s5(n)
   integer :: n
   type :: t1(l)
     integer, len :: l
     real :: a(l)
   end type
   type :: t2(l1, l2)
     integer, len :: l1
     integer, len :: l2
     real :: b(l1, l2)
   end type
   type(t1(n))   :: x1 !CHECK: x1 size=48 offset=
   type(t2(n,n)) :: x2 !CHECK: x2 size=56 offset=
 end

And here are the errors:

$ f18 -fdebug-dump-symbols offsets01.f90 > dump.txt
f18: Semantic errors in offsets01.f90
offsets01.f90:5:13: error: Specification expression 'int(int(int(n,kind=8),kind=4),kind=8)' is neither constant nor a length type parameter
      real :: a(l)
              ^
offsets01.f90:10:13: error: Specification expression 'int(int(int(n,kind=8),kind=4),kind=8)' is neither constant nor a length type parameter
      real :: b(l1, l2)
              ^
offsets01.f90:10:13: error: Specification expression 'int(int(int(n,kind=8),kind=4),kind=8)' is neither constant nor a length type parameter
      real :: b(l1, l2)
              ^
PeteSteinfeld accepted this revision.Apr 19 2021, 6:43 AM

All builds, tests, and looks good.

But I'm concerned that we may be sweeping problems under the rug with this change. @klausler, shouldn't all semantic errors be caught by the end of execution of semantics.perform()? If so, we should track down all of the errors revealed by data05.f90, offsets01.f90, offsets02.f90, and typeinfo01.f90 and make sure that they're handled in semantics.perform() and write tests for them.

This revision is now accepted and ready to land.Apr 19 2021, 6:43 AM
klausler added a comment.EditedApr 19 2021, 8:15 AM

All builds, tests, and looks good.

But I'm concerned that we may be sweeping problems under the rug with this change. @klausler, shouldn't all semantic errors be caught by the end of execution of semantics.perform()? If so, we should track down all of the errors revealed by data05.f90, offsets01.f90, offsets02.f90, and typeinfo01.f90 and make sure that they're handled in semantics.perform() and write tests for them.

All user-caused errors must be caught by the end of semantics (specifically, before lowering) and all messages created by then. It's okay to have the derived type table builder catch errors so long as it's always run.

In this case, though, the errors are bogus, so it's kind of a moot point.

It's okay to have the derived type table builder catch errors so long as it's always run.

Currently it's only run for -fdebug-dump-symbols. Should it be always run instead?

In this case, though, the errors are bogus, so it's kind of a moot point.

Do you recommend that we don't check these errors and that this change is not merged?

It's okay to have the derived type table builder catch errors so long as it's always run.

Currently it's only run for -fdebug-dump-symbols. Should it be always run instead?

That subroutine will have to always run when code is going to be lowered. So it will soon always have to run unless a syntax-only option is in effect.

In this case, though, the errors are bogus, so it's kind of a moot point.

Do you recommend that we don't check these errors and that this change is not merged?

These checks are valid, but they obviously can't be applied until you fix the bogus errors.

awarzynski abandoned this revision.Apr 20 2021, 8:44 AM

@PeteSteinfeld & @klausler , thank you for your comments - the context surrounding BuildRuntimeDerivedTypeTables is much clearer now.

I agree with both of you - it would be much better to fix the invalid errors generated by BuildRuntimeDerivedTypeTables first. As I'm unfamiliar with this part of the frontend, I would rather wait for somebody more experienced in this area to look into it.

This is now reported as 50040.

A fix for the bogus errors is coming soon.