Page MenuHomePhabricator

[Flang] Ported test_errors.sh to Python
ClosedPublic

Authored by ijan1 on Aug 5 2021, 8:21 AM.

Details

Summary

To enable Flang testing on Windows, shell scripts have to be ported to Python. In this patch the "test_errors.sh" script is ported to python ("test_errors.py"). The RUN line of existing tests was changed to make use of the python script.

Used python regex in place of awk/sed.

Diff Detail

Event Timeline

ijan1 created this revision.Aug 5 2021, 8:21 AM
ijan1 requested review of this revision.Aug 5 2021, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2021, 8:22 AM
ijan1 edited the summary of this revision. (Show Details)Aug 5 2021, 8:31 AM
ijan1 edited the summary of this revision. (Show Details)Aug 5 2021, 8:38 AM
ijan1 edited the summary of this revision. (Show Details)

There seems to be a problem in how import common not finding common.py in a sibling directory.

vsnprintf is named _vsnprintf by Microsoft. According to their docs, it should also be available as vsnprintf with

#include <stdio.h>
#include <stdarg.h>

but even Google test uses

#if GTEST_OS_WINDOWS
# define vsnprintf _vsnprintf
#endif  // GTEST_OS_WINDOWS

There seems to be a problem in how import common not finding common.py in a sibling directory.

vsnprintf is named _vsnprintf by Microsoft. According to their docs, it should also be available as vsnprintf with

#include <stdio.h>
#include <stdarg.h>

but even Google test uses

#if GTEST_OS_WINDOWS
# define vsnprintf _vsnprintf
#endif  // GTEST_OS_WINDOWS

That is only part of the problem - the code compiles without errors with vsnprintf in place - the Fortran compiler uses positional arguments (like printf("%2$s, %1$s!\n", "World", "Hello") that isn't supported by the normal MS printf implementation, you have to call _vsprintf_p.

Ivan is going to push a patch to fix this tomorrow (it's essentially the same idea, but #define vsnprintf _vsnprintf_p. Without that patch, messages come out with $s and $d in the error message, rather than the intended strings and numbers intended.

Me and Ivan spent some time figuring this out today. Why do compilers/libraries have to do these sort of things their own way? ;)

ijan1 retitled this revision from Ported test_errors.sh to Python to [Flang] Ported test_errors.sh to Python.Aug 5 2021, 11:53 AM
ijan1 edited the summary of this revision. (Show Details)
ijan1 added a comment.Aug 5 2021, 11:55 AM

There seems to be a problem in how import common not finding common.py in a sibling directory.

common.py is just included in D107041 but not on here. I should've operated on the safe side and also added it here.

Me and Ivan spent some time figuring this out today. Why do compilers/libraries have to do these sort of things their own way? ;)

Usually the underscore exists because these function were introduced by POSIX. Since Windows is not POSIX, those were originally not defined. Microsoft implemented most of them later anyway, but since code may have used their names already (e.g. to implement them because they were missing), they added the underscore to not clash with existing code. See e.g. https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-160#posix-function-names

vsnprintf was standardized only "recently" in C++11 and C99(*). That is, it should work on version of msvc that support C++11 (it does for me), but the dollar-sign again is a POSIX extension. I suggest not to rely on it.

(*) Microsoft only started implementing strategic C99 features in VS2017, generally considers it C++-only compiler.

The missing common.py can be resolved by rebasing this patch to rGeabae4cf57b9e7429db27fdfd3016d31901fa2ea.

Together with D107654, the three tests fail as described in the summary. Those two also on UNIX:

Failed Tests (3):
  Flang :: Semantics/resolve12.f90
  Flang :: Semantics/resolve26.f90
awarzynski added inline comments.Aug 11 2021, 3:24 AM
flang/test/Semantics/test_errors.py
2

#! /usr/bin/env python3

5

This script does not use FLANG_FC1 at all, so I would delete it. Could you instead document what arguments this script takes?

ijan1 updated this revision to Diff 365971.Aug 12 2021, 5:01 AM
ijan1 marked 2 inline comments as done.

"resolve12.f90" and "resolve26.f90" have been fixed. Addressed feedback from Andrzej.

Only resolve67.f90 still failing. What is the nondeterministic function?

Only resolve67.f90 still failing. What is the nondeterministic function?

I think it is the following condition from CheckHelper::CheckDefinedOperator in lib/Semantics/check-declarations.cpp. I suspect this happens due to the fact that operands of a bitwise operator can be evaluated in any order. Since there are errors for both the operands (invocations of CheckDefinedOperatorArg on arguments 0 and 1), the order in which the error is printed depends on the evaluation order.

} else if (!CheckDefinedOperatorArg(opName, specific, proc, 0) |
    !CheckDefinedOperatorArg(opName, specific, proc, 1)) {
  return false; // error was reported
}

The evaluation order is indeed undefined for operators except || and &&. I suggest to store the results in a variables. Them being part of a if-else will require some restructuring.

ijan1 added a comment.Aug 17 2021, 1:57 AM

The evaluation order is indeed undefined for operators except || and &&. I suggest to store the results in a variables. Them being part of a if-else will require some restructuring.

I will have a patch fixing this issue soon.

ijan1 updated this revision to Diff 367219.Aug 18 2021, 8:19 AM

Rebased and added support for 3 new tests.

kiranchandramohan edited the summary of this revision. (Show Details)Aug 20 2021, 6:28 AM
ijan1 updated this revision to Diff 370512.Sep 3 2021, 2:22 AM

Rebased to add support for new tests and also see if "resolve67.f90" passes the test.

ijan1 updated this revision to Diff 370567.Sep 3 2021, 6:19 AM

Rebased to stop a Clang test failure from some other change in main.

ijan1 edited the summary of this revision. (Show Details)Sep 3 2021, 6:25 AM
Meinersbur accepted this revision.Sep 4 2021, 4:41 PM

LGTM

I am pretty sure the the Clang SVE tests are unrelated.

This revision is now accepted and ready to land.Sep 4 2021, 4:41 PM
This revision was landed with ongoing or failed builds.Sep 6 2021, 1:20 AM
This revision was automatically updated to reflect the committed changes.