[flang] Version information in flang/f18

[coti]

Summary

Fixed some version information in flang/f18:

• • fixed the behavior of the -v switch: this flag enables verbosity with used with arguments, but just displays the version when used alone (related to this bug: https://bugs.llvm.org/show_bug.cgi?id=46017)
• added FLANG, FLANG_MAJOR, FLANG_MINOR__ and FLANG_PATCHLEVEL (similar to their __F18* counterparts) for compatibility purpose

[richard.barton.arm]

Looking good. Please can you add some tests?

• For --version, there already is one for the version screen in flang/test/Driver that could be updated to capture the version number also and run multiple times to test the synonyms.
  • Testing {{\-v}} when it means verbose is hard so suggest not doing that. We need {{-###}} really to test the driver more, which is outside the scope of your change.
• For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

[tskeith]

In D84334#2192984, @richard.barton.arm wrote:

• For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

The preprocessing tests are being executed now so that would be a good place to add a test.

[klausler]

In D84334#2192984, @richard.barton.arm wrote:

Looking good. Please can you add some tests?

• For --version, there already is one for the version screen in flang/test/Driver that could be updated to capture the version number also and run multiple times to test the synonyms.
  • Testing {{\-v}} when it means verbose is hard so suggest not doing that. We need {{-###}} really to test the driver more, which is outside the scope of your change.
• For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

Please don't delete the tests in flang/test/Preprocessing.

[richard.barton.arm]

The preprocessing tests are being executed now so that would be a good place to add a test.

Ha - if only I had written that comment yesterday it would have been right :-) Thanks for enabling them @klausler and @tskeith for the correction.

[coti]

I am writing the tests.

I have a little problem with` flang_version. It looks like f18 considers it as a numeric constants that it cannot interpret (probably because it contains too many dots). For example:

print *, "VERSION", __flang_version__

Gives me:

/home/users/coti/toto.f90:10:4: error: expected execution part construct
    print *, "VERSION", __flang_version__
     ^
/home/users/coti/toto.f90:10:3: in the context: execution part
    print *, "VERSION", __flang_version__
    ^
/home/users/coti/toto.f90:1:1: in the context: main program
  program version
  ^
f18: could not parse /home/users/coti/toto.f90

Then

#if __flang_version__ == "12.0.0"
  print *, "Correct version number"
#endif

gives me:

/home/users/coti/toto.f90:15:5: error: Uninterpretable numeric constant '12.0.0'
  #if __flang_version__ == "12.0.0"
      ^^^^^^^^^^^^^^^^^
in a macro defined here
that expanded to:
  12.0.0
  ^

[coti]

Some tests for the -v flag and associated macros.

Some comments:

• I could not find a way to exploit __flang_version__, since it is interpreted as a numeric constant instead of a string (see above)
• The version number is simply matched against a regex. I can retrieve it from cmake, similarly to what is done in f18.cpp, but I chose to keep things simple and not add anything into flang/test/CMakeLists.txt. What do you think?

[richard.barton.arm]

Thanks for adding the tests. IMO, the regex approach in the test is just fine.

I have made a couple of small suggestions and am happy to approve on the basis that those are addressed.

I think one of @AlexisPerry or @sscalpone should also approve before you push.

Thanks for the work on this.

flang/test/Preprocessing/macros.F90

1 ↗

(On Diff #283086)

Suggest this be renamed "compiler_defined_macros" or "predefined_macros" to separate it from a general test on user-defined macros.

flang/tools/f18/f18.cpp

418

Given the bug that you have found with flang_version, I suggest we don't define the external macro in this patch, given that it won't be usable and it's essentially exposing the bug that wasn't there before.

[tskeith]

flang/test/Preprocessing/macros.F90

7 ↗

(On Diff #283086)

Does this depend on having gfortran available? It should use %f18 -E like the others.

[sscalpone]

Here are the clang definitions:

% clang -E -dM -x c /dev/null | grep clang
#define clang 1
#define clang_major 12
#define clang_minor 0
#define clang_patchlevel 0
#define clang_version "12.0.0 (ssh://[e-mail]:12051/fortran/fir-dev-mirror.git 36a44dbf0b4e55917b70fbf209d8c992249e651b)"

flang/tools/f18/f18.cpp

392

Perhaps the message could be "flang front end" ?

[coti]

All the comments should be addressed:

• renamed the test for pre-processor macros compiler_defined_macros.F90
• removed __flang_version__ (in f18.cpp and in the tests)
• use %f18 -E in the pre-processor test in order to avoid a full compilation

[tskeith]

flang/test/Preprocessing/compiler_defined_macros.F90

4

I don't know if you can count on grep being available everywhere. I think FileCheck is better anyway. E.g. something like this:

!CHECK: flang_major = {{[0-9]+$}}
integer, parameter :: flang_major = __FLANG_MAJOR__

[coti]

Thank you @tskeith for the tip on the tests!

[tskeith]

Thanks, looks good to me.

[richard.barton.arm]

Do you have commit access @coti ?

[coti]

No, I don't ;)

[richard.barton.arm]

It needs rebasing - please can you do this then I will commit it on your behalf?

[coti]

Updated my local clone, fixed the conflicts and re-generated the diff.

[richard.barton.arm]

I'm afraid that still doesn't work. My master is on 7d0f691.

[coti]

I generated the diff again on a clean clone. I don't know why but my CMakeList.txt was wrong even after a git stash. Sorry about that, I hope it should be fine now.

[richard.barton.arm]

I'm afraid arcanist does not apply the patch well. It does not like the new file creation. I'm not sure how you are creating this diff, but are you able to re-upload a diff created with diff -Naur to handle the new files correctly?

$ arc patch D84334 --nobranch
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch flang/tools/f18/f18_version.h.in...
error: flang/tools/f18/f18_version.h.in: does not exist in index
Checking patch flang/tools/f18/f18.cpp...
Checking patch flang/tools/f18/CMakeLists.txt...
Checking patch flang/test/Preprocessing/compiler_defined_macros.F90...
error: flang/test/Preprocessing/compiler_defined_macros.F90: does not exist in index
Checking patch flang/test/Driver/version_test.f90...
Applied patch flang/tools/f18/f18.cpp cleanly.
Applied patch flang/tools/f18/CMakeLists.txt cleanly.
Applied patch flang/test/Driver/version_test.f90 cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!

[coti]

Outch, sorry :(
I create my patches with git diff....

Thank you for showing me the arc command, this time I checked:

$ arc patch --patch ~/f18_version.patch 
 INFO  Base commit is not in local repository; trying to fetch.
Branch name arcpatch already exists; trying a new name.
Branch name arcpatch_1 already exists; trying a new name.
Branch name arcpatch_2 already exists; trying a new name.
Created and checked out branch arcpatch_3.
Checking patch flang/test/Driver/version_test.f90...
Checking patch flang/test/Preprocessing/compiler_defined_macros.F90...
Checking patch flang/tools/f18/CMakeLists.txt...
Checking patch flang/tools/f18/f18.cpp...
Checking patch flang/tools/f18/f18_version.h.in...
Applied patch flang/test/Driver/version_test.f90 cleanly.
Applied patch flang/test/Preprocessing/compiler_defined_macros.F90 cleanly.
Applied patch flang/tools/f18/CMakeLists.txt cleanly.
Applied patch flang/tools/f18/f18.cpp cleanly.
Applied patch flang/tools/f18/f18_version.h.in cleanly.
Usage Exception: User aborted the workflow.

[richard.barton.arm]

Please can you send me your email address offline for the commit log?

[coti]

yes ;)

[sscalpone]

This commit has broken the out of tree builds.

llvm-project/flang/tools/f18/f18.cpp:41:10: fatal error: f18_version.h: No such file or directory
   41 | #include "f18_version.h"
      |          ^~~~~~~~~~~~~~~

[isuruf]

In D84334#2208010, @sscalpone wrote:

This commit has broken the out of tree builds.

This probably needs to include CMAKE_CURRENT_BINARY_DIR as a include directory.

[echristo]

I'm also not sure this is passing?

---

FAIL: Flang :: Preprocessing/compiler_defined_macros.F90 (31682 of 71064)

• TEST 'Flang ::

Preprocessing/compiler_defined_macros.F90' FAILED ****

Script:

...

Exit Code: 1

Command Output (stderr):

/usr/local/google/home/echristo/sources/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90:3:9:
error: CHECK: expected string not found in input
!CHECK: flang_major = {{[1-9][0-9]*$}}

^

<stdin>:1:1: note: scanning from here
integer, parameter :: flang_major = flang_major
^
<stdin>:1:23: note: possible intended match here
integer, parameter :: flang_major = flang_major

^

Input file: <stdin>
Check file:
/usr/local/google/home/echristo/sources/llvm-project/flang/test/Preprocessing/compiler_defined_macros.F90

-dump-input=help explains the following input dump.

Input was:
<<<<<<

1: integer, parameter :: flang_major = __flang_major__

check:3'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no
match found
check:3'1 ? possible
intended match

2: integer, parameter :: flang_minor = __flang_minor__

check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

3: integer, parameter :: flang_patchlevel = __flang_patchlevel__

check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

--

[richard.barton.arm]

I have reverted the patch

[richard.barton.arm]

@coti - I've had another look and I think your new test is stumbling across an existing bug. If you play around with the existing __F18* macros, you can see that they are not being applied correctly in the code:

ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ cat test.f90
PROGRAM FOO
  integer i = __F18__
end program foo
ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ ../../build/bin/flang -DFOO=bar test.f90 -E
program foo
integer i = __f18__
end program foo

So I don't think there is anything wrong with the code changes or test, but perhaps another bug to fix in the preprocessor itself. Its the end of the day for me now, but I'll look again tomorrow.

I have not looked at the out of tree build issues. Have you tried isuru's suggestion?

[klausler]

In D84334#2213327, @richard.barton.arm wrote:

@coti - I've had another look and I think your new test is stumbling across an existing bug. If you play around with the existing __F18* macros, you can see that they are not being applied correctly in the code:

ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ cat test.f90
PROGRAM FOO
  integer i = __F18__
end program foo
ricbar01@hw-a21-17:~/ossf18/llvm-project/flang$ ../../build/bin/flang -DFOO=bar test.f90 -E
program foo
integer i = __f18__
end program foo

So I don't think there is anything wrong with the code changes or test, but perhaps another bug to fix in the preprocessor itself. Its the end of the day for me now, but I'll look again tomorrow.

I have not looked at the out of tree build issues. Have you tried isuru's suggestion?

I think I remember fixing something in that area with predefined (command-line -D flag) options; I'll see whether it still needs to be pushed.

[coti]

Added include ${CMAKE_CURRENT_BINARY_DIR}, as suggested by @isuruf. I tested it on a clean tree, it seems to work.

[coti]

Typo in a comment.

[DavidTruby]

flang/tools/f18/CMakeLists.txt

33

as a small nit: I think target_include_directories(f18 ${CMAKE_CURRENT_BINARY_DIR}) is slightly cleaner. I'm not that fussed about it though

[coti]

@DavidTruby so, how about replacing:

set(include ${FLANG_BINARY_DIR}/include/flang)

set(include ${FLANG_BINARY_DIR}/include/flang)
set(include ${CMAKE_CURRENT_BINARY_DIR})

with:

target_include_directories(f18
  ${CMAKE_CURRENT_BINARY_DIR}
  ${FLANG_BINARY_DIR}/include/flang 
)

[DavidTruby]

In D84334#2214441, @coti wrote:

@DavidTruby so, how about replacing:

set(include ${FLANG_BINARY_DIR}/include/flang)

set(include ${FLANG_BINARY_DIR}/include/flang)
set(include ${CMAKE_CURRENT_BINARY_DIR})

with:

target_include_directories(f18
  ${CMAKE_CURRENT_BINARY_DIR}
  ${FLANG_BINARY_DIR}/include/flang 
)

looks good to me!

[sscalpone]

This worked for me:

target_include_directories(f18
  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)

And I got rid of the duplicate:

set(include ${FLANG_BINARY_DIR}/include/flang)

flang/tools/f18/CMakeLists.txt

84–86

Perhaps using FLANG_BINARY_DIR will fix the build?

[coti]

This comment was removed by coti.

[coti]

Followed @DavidTruby's recommendation on how include directories are set by CMake.

[sscalpone]

flang/tools/f18/CMakeLists.txt

30

Did this work for you?

CMake requires a PUBLIC or PRIVATE qualifier before the first directory argument.

I'm getting:

CMake Error at /local/home/sjs/llvm/llvm-project/flang/tools/f18/CMakeLists.txt:34 (target_include_directories):
  target_include_directories called with invalid arguments

[coti]

Actually, I cannot find a way to make it work with target_include_directories on a clean build tree.

With PRIVATE I get an error at the next step, what is being compiled is expecting an input on stdin:

[100%] Built target f18
[100%] Generating /__fortran_builtins.mod
Enter Fortran source
Use EOF character (^D) to end file

With PUBLIC

CMake Error in /home/users/coti/tmp/llvm-project/flang/tools/f18/CMakeLists.txt:
  Target "f18" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/home/users/coti/tmp/tmpbuild/tools/flang/include/flang"

  which is prefixed in the build directory.


CMake Error in /home/users/coti/tmp/llvm-project/flang/tools/f18/CMakeLists.txt:
  Target "f18" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/home/users/coti/tmp/tmpbuild/tools/flang/tools/f18"

  which is prefixed in the build directory.

But, why not put f18_version.h in ${FLANG_BINARY_DIR}/include/flang? This way we only need to include this directory, and it is not totally irrelevant to have the version numbers in this directory, is it? However, I still cannot make target_include_directories work (same as the first error).

[sscalpone]

Did this work for you?

target_include_directories(f18
  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)

[coti]

On a clean build, with:

target_include_directories(f18
    PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
)

I get:

[ 91%] Building CXX object tools/flang/tools/f18/CMakeFiles/f18.dir/f18.cpp.o
/home/users/coti/tmp/llvm-project/flang/tools/f18/f18.cpp:40:10: fatal error: f18_version.h: No such file or directory
 #include "f18_version.h"
          ^~~~~~~~~~~~~~~
compilation terminated.

[sscalpone]

Sorry, I'm not sure what to do next. I'm using cmake vesion 3.18.0 and gcc 9.3.0.

Here's the full diff of my CMakefile:

diff --git a/flang/tools/f18/CMakeLists.txt b/flang/tools/f18/CMakeLists.txt
index 6103117123e..d7987e6341e 100644
--- a/flang/tools/f18/CMakeLists.txt
+++ b/flang/tools/f18/CMakeLists.txt
@@ -9,0 +10,3 @@ add_flang_tool(f18
+target_include_directories(f18
+  PRIVATE ${CMAKE_CURRENT_BINARY_DIR}
+)
@@ -32,2 +34,0 @@ set(include ${FLANG_BINARY_DIR}/include/flang)
-set(include ${FLANG_BINARY_DIR}/include/flang)
-
@@ -82,0 +84 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/flang.sh.in ${FLANG_BINARY_DIR}/bin/f
+configure_file(${CMAKE_CURRENT_SOURCE_DIR}/f18_version.h.in ${CMAKE_CURRENT_BINARY_DIR}/f18_version.h @ONLY)

[richard.barton.arm]

Steve's patch works for me (cmake 3.17). @coti , your error message looks like what would happen if you replaced both definitions of {{include}} with the {{target_include_directories}} call. {{include}} is set up to the location of pre-built modules and is used in the compilation of said modules and then pre-processed into the flang.sh script. You still need that as well as setting {{target_include_directories}} to tell CMake where f18_version.h is when building f18.cpp.

Note for future (not part of this patch) those modules ought to end up in the lib directory of the install not the include directory. Clang puts similar headers in lib/clang/<version>/include and we should ultimately do the same for Flang. I think the include dir in the installation is for LLVM header files, e.g. if you wanted to use the LLVM libraries in some project or other.

[coti]

On a clean tree, with the attached patch, I get:

[ 90%] Built target f18
[ 90%] Generating /__fortran_builtins.mod
Enter Fortran source
Use EOF character (^D) to end file

And then it does not progress anymore...

[richard.barton.arm]

@coti the reason for the build failure is because you have removed both of the duplicate definitions of include in flang/tools/f18/CMakeLists.txt. This causes CMake not to be able to find the right files when it creates the builtin module files. One of these needs to be restored in your patch for the build to work as before.

I have marked the line in the original source that needs to be reinstated to your patch to make it work correctly for my in-tree build. If you re-submit a patch with that change then it can be tested out of tree.

flang/tools/f18/CMakeLists.txt

32

This is the line to reinstate.

[coti]

Fixed the include paths. Thank you @richard.barton.arm!

[richard.barton.arm]

This is working well for me in an in-tree build.

[richard.barton.arm]

I have tested this works well on an out of tree build (as it broke it before) so have no reservations about pushing the change again on @coti 's behalf.

[AlexisPerry]

This comment was removed by AlexisPerry.

[naromero77]

This comment was removed by naromero77.