This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Disable Fortran FM509 test when -std=legacy is not supported
ClosedPublic

Authored by DavidSpickett on Mar 9 2022, 6:36 AM.

Details

Summary

This test requires -std=legacy which is not supported by flang.
Currently we check if the flag is supported and add if so,
but run the test anyway if not supported.

This has been failing on our bots for a long time but was not
noticed due to some issues reporting NOEXE failures.

https://lab.llvm.org/buildbot/#/builders/179/builds/3132

Executable Missing Tests (1):

test-suite :: Fortran/UnitTests/fcvs21_f95/FM509.test

This fixes llvm_singlesource to allow you to pass a list
of source files (which used to work when it was a macro).

(this was done for multisource in cecca649c310cfa78076ef96dadea315ac176efa)

If the flag is not supported we remove the test from the list
of source files. This means it's disabled for flang and
enabled for gfortran.

Event Timeline

DavidSpickett created this revision.Mar 9 2022, 6:36 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
DavidSpickett requested review of this revision.Mar 9 2022, 6:36 AM

@DavidSpickett , many thanks for looking into this!

I think that it would be worth clarifying (perhaps in CMake) that this test compiles fine with gfortran -std=legacy FM509.f and that (as far as I know) there are no intentions to add such flag in LLVM Flang. Also - how about running the test suite with gfortran? If that's to be supported then perhaps the test should be only XFAIL'ed for Flang?

AFAIK, @naromero77 no longer works in this area. I included @Meinersbur as a reviewer as. I know that Michael helped Nick when adding these tests, so might have some suggestions too.

Make it more clear that the test is valid if you have -std=legacy
but that's not planned to be possible with flang.

Also - how about running the test suite with gfortran?

I assume that somewhere in gnu land this suite is run with gfortran but yes I don't see why you couldn't take this exact copy of the tests and run them with it. Ideally we'd preserve that.

The GCC torture tests do have skip lists I could adapt that to this: https://reviews.llvm.org/D115994

Instead of removing the test file just don't add it if the flag
is not supported.

DavidSpickett retitled this revision from [test-suite] Remove Fortran FM509 test to [test-suite] Disable Fortran FM509 test when -std=legacy is not supported.Mar 10 2022, 2:08 AM
DavidSpickett edited the summary of this revision. (Show Details)
awarzynski accepted this revision.Mar 10 2022, 3:06 AM

Haha, great minds @DavidSpickett ! Just when I was to comment that this would be my preferred approach :) Thanks for doing this!

This makes a lot of sense and IMO can be merged as is. My comments are nits.

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt
39–40
cmake/modules/SingleMultiSource.cmake
17 ↗(On Diff #414309)
18 ↗(On Diff #414309)
29–30 ↗(On Diff #414309)
35 ↗(On Diff #414309)

It took me a while to grasp what's going on here, so I've suggested expanding the comment :) WDYT?

This revision is now accepted and ready to land.Mar 10 2022, 3:06 AM
DavidSpickett added inline comments.Mar 10 2022, 3:24 AM
cmake/modules/SingleMultiSource.cmake
35 ↗(On Diff #414309)

I feel like I evolved to a higher plane once I understood cmake functions :)

So yes I'll expand this.

Update comments.

DavidSpickett marked 5 inline comments as done.Mar 10 2022, 4:11 AM

Turns out it the "if (DEFINED Source" did work I just managed to miss the other users of it and none of them did anything on AArch64. Caused at least one failure on armv7 so I've reverted and will fix tomorrow.

Reland worked, test is now disabled for flang builds.

Fortran/UnitTests/fcvs21_f95/CMakeLists.txt