Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fortran/gfortran] Initial support to override DejaGNU annotations #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tarunprabhu
Copy link
Contributor

The DejaGNU annotations in the gfortran test suite sometimes need to be overridden, for instance in cases where the one of the compilers generates a compile error for some non-standard extension while the other does not. This PR adds support to override the some annotations, such as forcing a test to pass with flang where it would fail in gfortran.

For now, only some of the behavior can be overridden, but this could be extended in the future. The documentation was updated to reflect this (some cleanup of the documentation was also carried out). Similarly, the static test configuration generation script was updated and cleaned up.

An override file is also provided for two tests that were disabled in 27a6a3a since they are expected to pass in flang. The static test configuration was also updated to reflect this change.

The DejaGNU annotations in the gfortran test suite sometimes need to be
overridden, for instance in cases where the one of the compilers generates a
compile error for some non-standard extension while the other does not. This
PR adds support to override the some annotations, such as forcing a test to
pass with flang where it would fail in gfortran.

For now, only some of the behavior can be overridden, but this could be
extended in the future. The documentation was updated to reflect this (some
cleanup of the documentation was also carried out). Similarly, the static
test configuration generation script was updated and cleaned up.

An override file is also provided for two tests that were disabled in
27a6a3a since they are expected to pass in flang. The static test
configuration was also updated to reflect this change.
Fortran/gfortran/README.md Outdated Show resolved Hide resolved
@kkwli
Copy link
Contributor

kkwli commented Oct 27, 2024

For the disabled tests, it shows up as NOEXE in the json file, is it expected? I am wondering if it can have a different code as missing executable can mean compilation error.

    {   
      "code": "NOEXE",
      "elapsed": 0.0127410888671875,
      "name": "test-suite :: Fortran/gfortran/regression/gfortran-regression-execute-regression__bessel_1_f90.test",
      "output": "Executable '/home/kli/build-test-suite/Fortran/gfortran/regression/gfortran-regression-execute-regression__bessel_1_f90' is missing"
    },  

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tarunprabhu
Copy link
Contributor Author

For the disabled tests, it shows up as NOEXE in the json file, is it expected?

NOEXE is definitely not expected for bessel_1.f90. Does it fail to build on AIX?

@kiranchandramohan
Copy link
Contributor

Is this the first use of yaml in the testsuite? Any particular reason you chose yaml?

@tarunprabhu
Copy link
Contributor Author

I didn't check if there were other uses of yaml in the test suite. I chose it for a few reasons:

  1. A parser for YAML is readily available for Python. The parser is only needed if one is regenerating the tests.cmake files. It will only be needed by those who maintain the test suite. Others who simply need to run the gfortran tests do not need it.
  2. The "override" files are intended to be human editable and readable. It would therefore have been useful to support comments.
  3. We don't need something like tablegen because we don't need to generate anything from the override files. The contents of the file affects the parsing of the DejaGNU annotations from the source files (see gfortran/utils/update-test-config.py).

YAML was the first format that came to mind that I assumed would be sufficiently familiar to most developers. If there are other formats that are more appropriate, I am happy to switch to one of those.

@kiranchandramohan
Copy link
Contributor

YAML was the first format that came to mind that I assumed would be sufficiently familiar to most developers. If there are other formats that are more appropriate, I am happy to switch to one of those.

No need to change. I was just wondering whether something in the Dejagnu test system could be used for this purpose. I am not aware of any.

@tarunprabhu
Copy link
Contributor Author

@kkwli,

I think there might be a bug in the way the disabled tests are being handled. I will look into it and keep you posted.

@tarunprabhu
Copy link
Contributor Author

@kkwli,

You should not be seeing bessel_1.f90 in the test suite report at all if you correctly disabled it. I just double-checked by adding this to Fortran/gfortran/regression/override.yaml

"bessel_1.f90":
  disabled_on: ["x86_64-*-*"]

The test was correctly disabled and never ran on my X86 machine. I assume that you had something equivalent in override.yaml with a different target triple. It is possible that the triple was not correctly matched in CMake. I suspect that the problem might be this line in Fortran/gfortran/CMakeLists.txt

set(triple "${arch}-unknown-${sys}")

If you set the second element of the triple to something other than *, this would fail to match. This is something that should be fixed, but it is better to do it in a separate PR.

Could you try setting the second element of the triple to * in your override file and see if that changes things?

@tarunprabhu
Copy link
Contributor Author

@kkwli

Did the suggested change to the target triple work for you? It's somewhat orthogonal to this PR and if that is the problem, I can fix it in a separate PR. But I'd like to get this in before fixing other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants