[flang][runtime] Add ACCESS library procedure #88517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a GNU extension: https://gcc.gnu.org/onlinedocs/gfortran/ACCESS.html Used in SALMON: https://salmon-tddft.jp/download.html Unfortunately the intrinsic takes a file path to operate on so there isn't an easy way to make the test robust. The unit test expects to be able to create, set read write and execute permissions, and delete files called std::filesystem::temp_directory_path() / <test_name>.<pid> The test will fail if a file already exists with that name. I have not implemented the intrinsic on Windows because this is wrapping a POSIX system call and Windows doesn't support all of the permission bits tested by the intrinsic. I don't have a Windows machine easily available to check if Gfortran implements this intrinsic on Windows.
I didn't apply braced initialization to very common C programming
patterns in the unittest because when something is extremely common, I
think it increases cognitive load when reading. For example
int fd = open(...);
vs
int fd{open(...)};
I did change all uses in the runtime implementation as requested in code
review.
(Hopefully) this will fix two buildbot failures: - https://lab.llvm.org/buildbot/#/builders/285/builds/245 - https://lab.llvm.org/buildbot/#/builders/21/builds/96988
tblah
requested review from
DavidTruby,
kiranchandramohan,
pawosm-arm,
klausler,
yi-wu-arm and
vzakhari
llvmbot
added
flang:runtime
flang
|
@llvm/pr-subscribers-flang-runtime Author: Tom Eccles (tblah) ChangesRe-land #88395 Two build-bots were broken by the old version:
The problem in both cases was that the compiler did not support I believe that flang already requires a compiler capable of c++ 17 so this should be okay. I have adjusted CMake in 66f32b5 so that hopefully Another alternative would be to rewrite the unit test not to use [1] https://gitlab.kitware.com/cmake/cmake/-/issues/17834 This is a GNU extension: Used in SALMON: Unfortunately the intrinsic takes a file path to operate on so there isn't an easy way to make the test robust. The unit test expects to be able to create, set read write and execute permissions, and delete files called The test will fail if a file already exists with that name. I have not implemented the intrinsic on Windows because this is wrapping a POSIX system call and Windows doesn't support all of the permission bits tested by the intrinsic. I don't have a Windows machine easily available to check if Gfortran implements this intrinsic on Windows. Full diff: https://github.com/llvm/llvm-project/pull/88517.diff 5 Files Affected:
diff --git a/flang/docs/Intrinsics.md b/flang/docs/Intrinsics.md
index ccb93e104dab65..848619cb65d909 100644
--- a/flang/docs/Intrinsics.md
+++ b/flang/docs/Intrinsics.md
@@ -657,6 +657,14 @@ CALL CO_REDUCE
CALL CO_SUM
```
+### Inquiry Functions
+ACCESS (GNU extension) is not supported on Windows. Otherwise:
+```
+CHARACTER(LEN=*) :: path = 'path/to/file'
+IF (ACCESS(path, 'rwx')) &
+ ...
+```
+
## Non-standard intrinsics
### PGI
```
diff --git a/flang/include/flang/Runtime/extensions.h b/flang/include/flang/Runtime/extensions.h
index 7d0952206fc195..fef651f3b2eedb 100644
--- a/flang/include/flang/Runtime/extensions.h
+++ b/flang/include/flang/Runtime/extensions.h
@@ -44,5 +44,12 @@ std::int64_t RTNAME(Signal)(std::int64_t number, void (*handler)(int));
// GNU extension subroutine SLEEP(SECONDS)
void RTNAME(Sleep)(std::int64_t seconds);
+// GNU extension function ACCESS(NAME, MODE)
+// TODO: not supported on Windows
+#ifndef _WIN32
+std::int64_t FORTRAN_PROCEDURE_NAME(access)(const char *name,
+ std::int64_t nameLength, const char *mode, std::int64_t modeLength);
+#endif
+
} // extern "C"
#endif // FORTRAN_RUNTIME_EXTENSIONS_H_
diff --git a/flang/runtime/extensions.cpp b/flang/runtime/extensions.cpp
index 3ac98000335d7d..12498b502ae1cf 100644
--- a/flang/runtime/extensions.cpp
+++ b/flang/runtime/extensions.cpp
@@ -17,6 +17,7 @@
#include "flang/Runtime/entry-names.h"
#include "flang/Runtime/io-api.h"
#include <chrono>
+#include <cstring>
#include <ctime>
#include <signal.h>
#include <thread>
@@ -138,5 +139,77 @@ void RTNAME(Sleep)(std::int64_t seconds) {
std::this_thread::sleep_for(std::chrono::seconds(seconds));
}
+// TODO: not supported on Windows
+#ifndef _WIN32
+std::int64_t FORTRAN_PROCEDURE_NAME(access)(const char *name,
+ std::int64_t nameLength, const char *mode, std::int64_t modeLength) {
+ std::int64_t ret{-1};
+ if (nameLength <= 0 || modeLength <= 0 || !name || !mode) {
+ return ret;
+ }
+
+ // ensure name is null terminated
+ char *newName{nullptr};
+ if (name[nameLength - 1] != '\0') {
+ newName = static_cast<char *>(std::malloc(nameLength + 1));
+ std::memcpy(newName, name, nameLength);
+ newName[nameLength] = '\0';
+ name = newName;
+ }
+
+ // calculate mode
+ bool read{false};
+ bool write{false};
+ bool execute{false};
+ bool exists{false};
+ int imode{0};
+
+ for (std::int64_t i = 0; i < modeLength; ++i) {
+ switch (mode[i]) {
+ case 'r':
+ read = true;
+ break;
+ case 'w':
+ write = true;
+ break;
+ case 'x':
+ execute = true;
+ break;
+ case ' ':
+ exists = true;
+ break;
+ default:
+ // invalid mode
+ goto cleanup;
+ }
+ }
+ if (!read && !write && !execute && !exists) {
+ // invalid mode
+ goto cleanup;
+ }
+
+ if (!read && !write && !execute) {
+ imode = F_OK;
+ } else {
+ if (read) {
+ imode |= R_OK;
+ }
+ if (write) {
+ imode |= W_OK;
+ }
+ if (execute) {
+ imode |= X_OK;
+ }
+ }
+ ret = access(name, imode);
+
+cleanup:
+ if (newName) {
+ free(newName);
+ }
+ return ret;
+}
+#endif
+
} // namespace Fortran::runtime
} // extern "C"
diff --git a/flang/unittests/Runtime/AccessTest.cpp b/flang/unittests/Runtime/AccessTest.cpp
new file mode 100644
index 00000000000000..5c8a634192d92b
--- /dev/null
+++ b/flang/unittests/Runtime/AccessTest.cpp
@@ -0,0 +1,390 @@
+//===-- flang/unittests/Runtime/AccessTest.cpp ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// TODO: ACCESS is not yet implemented on Windows
+#ifndef _WIN32
+
+#include "CrashHandlerFixture.h"
+#include "gtest/gtest.h"
+#include "flang/Runtime/extensions.h"
+
+#include <fcntl.h>
+#include <filesystem>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+namespace {
+
+struct AccessTests : public CrashHandlerFixture {};
+
+struct AccessType {
+ bool read{false};
+ bool write{false};
+ bool execute{false};
+ bool exists{false};
+};
+
+} // namespace
+
+static std::string addPIDSuffix(const char *name) {
+ std::stringstream ss;
+ ss << name;
+ ss << '.';
+
+ ss << getpid();
+
+ return ss.str();
+}
+
+static std::filesystem::path createTemporaryFile(
+ const char *name, const AccessType &accessType) {
+ std::filesystem::path path{
+ std::filesystem::temp_directory_path() / addPIDSuffix(name)};
+
+ // O_CREAT | O_EXCL enforces that this file is newly created by this call.
+ // This feels risky. If we don't have permission to create files in the
+ // temporary directory or if the files already exist, the test will fail.
+ // But we can't use std::tmpfile() because we need a path to the file and
+ // to control the filesystem permissions
+ mode_t mode{0};
+ if (accessType.read) {
+ mode |= S_IRUSR;
+ }
+ if (accessType.write) {
+ mode |= S_IWUSR;
+ }
+ if (accessType.execute) {
+ mode |= S_IXUSR;
+ }
+
+ int file = open(path.c_str(), O_CREAT | O_EXCL, mode);
+ if (file == -1) {
+ return {};
+ }
+
+ close(file);
+
+ return path;
+}
+
+static std::int64_t callAccess(
+ const std::filesystem::path &path, const AccessType &accessType) {
+ const char *cpath{path.c_str()};
+ std::int64_t pathlen = std::strlen(cpath);
+
+ std::string mode;
+ if (accessType.read) {
+ mode += 'r';
+ }
+ if (accessType.write) {
+ mode += 'w';
+ }
+ if (accessType.execute) {
+ mode += 'x';
+ }
+ if (accessType.exists) {
+ mode += ' ';
+ }
+
+ const char *cmode = mode.c_str();
+ std::int64_t modelen = std::strlen(cmode);
+
+ return FORTRAN_PROCEDURE_NAME(access)(cpath, pathlen, cmode, modelen);
+}
+
+TEST(AccessTests, TestExists) {
+ AccessType accessType;
+ accessType.exists = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotExists) {
+ std::filesystem::path nonExistant{addPIDSuffix(__func__)};
+ ASSERT_FALSE(std::filesystem::exists(nonExistant));
+
+ AccessType accessType;
+ accessType.exists = true;
+ std::int64_t res = callAccess(nonExistant, accessType);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestRead) {
+ AccessType accessType;
+ accessType.read = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotRead) {
+ AccessType accessType;
+ accessType.read = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestWrite) {
+ AccessType accessType;
+ accessType.write = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotWrite) {
+ AccessType accessType;
+ accessType.write = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.write = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestReadWrite) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotReadWrite0) {
+ AccessType accessType;
+ accessType.read = false;
+ accessType.write = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotReadWrite1) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotReadWrite2) {
+ AccessType accessType;
+ accessType.read = false;
+ accessType.write = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestExecute) {
+ AccessType accessType;
+ accessType.execute = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotExecute) {
+ AccessType accessType;
+ accessType.execute = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestRWX) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_EQ(res, 0);
+}
+
+TEST(AccessTests, TestNotRWX0) {
+ AccessType accessType;
+ accessType.read = false;
+ accessType.write = false;
+ accessType.execute = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotRWX1) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = false;
+ accessType.execute = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotRWX2) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = false;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotRWX3) {
+ AccessType accessType;
+ accessType.read = true;
+ accessType.write = false;
+ accessType.execute = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+TEST(AccessTests, TestNotRWX4) {
+ AccessType accessType;
+ accessType.read = false;
+ accessType.write = true;
+ accessType.execute = true;
+
+ std::filesystem::path path = createTemporaryFile(__func__, accessType);
+ ASSERT_FALSE(path.empty());
+
+ accessType.read = true;
+ accessType.write = true;
+ accessType.execute = true;
+ std::int64_t res = callAccess(path, accessType);
+
+ std::filesystem::remove(path);
+
+ ASSERT_NE(res, 0);
+}
+
+#endif // !_WIN32
diff --git a/flang/unittests/Runtime/CMakeLists.txt b/flang/unittests/Runtime/CMakeLists.txt
index 23f02aa751246b..10d48b2bf26168 100644
--- a/flang/unittests/Runtime/CMakeLists.txt
+++ b/flang/unittests/Runtime/CMakeLists.txt
@@ -1,4 +1,5 @@
add_flang_unittest(FlangRuntimeTests
+ AccessTest.cpp
Allocatable.cpp
ArrayConstructor.cpp
BufferTest.cpp
@@ -32,4 +33,7 @@ add_flang_unittest(FlangRuntimeTests
target_link_libraries(FlangRuntimeTests
PRIVATE
FortranRuntime
+ stdc++fs
)
+
+set_property(TARGET FlangRuntimeTests PROPERTY CXX_STANDARD 17)
|
|
You could alternatively submit the code and add a test in the llvm-testsuite. Maybe add a new directory under Fortran for non-standard intrinsics and add a test there. At least 2 buildbot CI run the testsuite so that should be reasonable testing in my opinion. |
This reverts commit 66f32b5.
|
I have changed the test not to use std::filesystem. The benefit of doing so was that I could avoid special case code for different target environments. If I need to conditionally add linker options in cmake depending upon the target compiler, this ceases to be useful. |
dankm
reviewed
Apr 17, 2024
| } | ||
|
|
||
| if (!read && !write && !execute) { | ||
| imode = F_OK; |
There was a problem hiding this comment.
Using F_OK and related constants means that unistd.h needs to be included unconditionally, otherwise at least FreeBSD is broken.
There was a problem hiding this comment.
Thank you for reporting the issue. Does this fix the problem for you? 2583b2e
There was a problem hiding this comment.
The latest in main does, thanks!
There was a problem hiding this comment.
Looks like POSIX says access and F_OK related constants are in unistd.h, but faccessat is in fcntl.h. I did a small amount of digging, and glibc defines F_OK in both unistd.h and fcntl.h, as does musl. FreeBSD and NetBSD only define them in unistd.h. Didn't check any other systems.
tblah
added a commit
to tblah/llvm-project
that referenced
this pull request
Apr 17, 2024
Suggested by dankm here: llvm#88517
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-land #88395
Two build-bots were broken by the old version:
The problem in both cases was that the compiler did not support
std::filesystem(which I use in the unit test).I have removed the dependency upon std::filesystem because there isn't an easy way to add the right linker options so that this is supported correctly in all build environments [1]
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/17834
This is a GNU extension:
https://gcc.gnu.org/onlinedocs/gfortran/ACCESS.html
Used in SALMON:
https://salmon-tddft.jp/download.html
Unfortunately the intrinsic takes a file path to operate on so there isn't an easy way to make the test robust. The unit test expects to be able to create, set read write and execute permissions, and delete files called
std::filesystem::temp_directory_path() / <test_name>.
The test will fail if a file already exists with that name.
I have not implemented the intrinsic on Windows because this is wrapping a POSIX system call and Windows doesn't support all of the permission bits tested by the intrinsic. I don't have a Windows machine easily available to check if Gfortran implements this intrinsic on Windows.