Skip to content

[Support] Report EISDIR when opening a directory #79880

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Apr 17, 2024
Merged

[Support] Report EISDIR when opening a directory #79880

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
066a8ca
Report eisdir on aix
alisonzhang4 Jan 29, 2024
267039f
Fix formatting error
alisonzhang4 Jan 29, 2024
76670d2
Add ifdef for AIX/MVS
alisonzhang4 Feb 14, 2024
fe7209d
Fix formatting
alisonzhang4 Feb 14, 2024
cdb8623
Fix failing lit test on linux
alisonzhang4 Apr 9, 2024
37a2b46
fix formatting
alisonzhang4 Apr 10, 2024
6b36693
Disable test on Windows
alisonzhang4 Apr 11, 2024
b667596
Fix test on Windows
alisonzhang4 Apr 15, 2024
7b3bc49
Do not create new directory for each test
alisonzhang4 Apr 15, 2024
166a504
Update llvm/unittests/Support/Path.cpp
azhan92 Apr 16, 2024
07bcca1
Suggestions from reviewer
alisonzhang4 Apr 16, 2024
f77a474
Move tests in file
alisonzhang4 Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions llvm/lib/Support/Unix/Path.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,15 @@ Expected<size_t> readNativeFile(file_t FD, MutableArrayRef<char> Buf) {
ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Size);
if (ssize_t(NumRead) == -1)
return errorCodeToError(errnoAsErrorCode());
// The underlying operation on these platforms allow opening directories
// for reading in more cases than other platforms.
#if defined(__MVS__) || defined(_AIX)
struct stat Status;
if (fstat(FD, &Status) == -1)
return errorCodeToError(errnoAsErrorCode());
if (S_ISDIR(Status.st_mode))
return errorCodeToError(make_error_code(errc::is_a_directory));
#endif
return NumRead;
}

Expand Down
2 changes: 0 additions & 2 deletions llvm/test/tools/llvm-symbolizer/input-file-err.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
Failing on AIX due to D153595. The test expects a different error message from the one given on AIX.
XFAIL: target={{.*}}-aix{{.*}}
RUN: not llvm-addr2line -e %p/Inputs/nonexistent 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
RUN: not llvm-addr2line -e %p/Inputs/nonexistent 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
CHECK-NONEXISTENT-A2L: llvm-addr2line{{.*}}: error: '{{.*}}Inputs/nonexistent': [[MSG]]
Expand Down
2 changes: 0 additions & 2 deletions llvm/unittests/Support/CommandLineTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,15 +1117,13 @@ TEST(CommandLineTest, BadResponseFile) {
ASSERT_STREQ(Argv[0], "clang");
ASSERT_STREQ(Argv[1], AFileExp.c_str());

#if !defined(_AIX) && !defined(__MVS__)
std::string ADirExp = std::string("@") + std::string(ADir.path());
Argv = {"clang", ADirExp.c_str()};
Res = cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
ASSERT_FALSE(Res);
ASSERT_EQ(2U, Argv.size());
ASSERT_STREQ(Argv[0], "clang");
ASSERT_STREQ(Argv[1], ADirExp.c_str());
#endif
}

TEST(CommandLineTest, SetDefaultValue) {
Expand Down
30 changes: 30 additions & 0 deletions llvm/unittests/Support/Path.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,36 @@ TEST_F(FileSystemTest, UTF8ToUTF16DirectoryIteration) {
}
#endif

#ifndef _WIN32
TEST_F(FileSystemTest, OpenDirectoryAsFileForRead) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: looking at other tests in this file, I believe these two would be better grouped in the file with other tests that attempt to open files for read/write. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, which other file would you suggest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, which other file would you suggest?

@azhan92, I think the idea is to leave the tests in this file but to move them within the file to be adjacent with tests that open files for read/write.

ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code here does not include code to remove this directory. Perhaps it should?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The use of TestDirectory here does not seem consistent with the other uses of the TestDirectory that is shared between tests.

llvm-project/llvm/unittests/Support/Path.cpp

Lines 665 to 667 in b667596

/// Unique temporary directory in which all created filesystem entities must
/// be placed. It is removed at the end of each test (must be empty).
SmallString<128> TestDirectory;

It seems that TestDirectory is created (and removed) by the test harness.

It seems we don't need to create/remove a directory here?

ASSERT_EQ(fs::create_directory(Twine(TestDirectory), false),
errc::file_exists);

std::string Buf(5, '?');
Expected<fs::file_t> FD = fs::openNativeFileForRead(TestDirectory);
ASSERT_NO_ERROR(errorToErrorCode(FD.takeError()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test fails on Windows here, because on Windows even attempting to opene the directory for read fails, so the ASSERT_NO_ERROR fires.

Side note: ASSERT_NO_ERROR(errorToErrorCode(FD.takeError())); can be simplified to ASSERT_THAT_EXPECTED(FD, Succeeded());. There are similar ones if you just want to check whether it failed with a particular message, for example (but in that case you wouldn't be able to check the actual value).

To fix this on Windows simply replace the ASSERT_NO_ERROR check with a check to make sure the returned error is in a failed state with the is_directory value (then skip the rest of the test).

auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
Expected<size_t> BytesRead =
fs::readNativeFile(*FD, MutableArrayRef(&*Buf.begin(), Buf.size()));
ASSERT_EQ(errorToErrorCode(BytesRead.takeError()), errc::is_a_directory);
}
#endif

TEST_F(FileSystemTest, OpenDirectoryAsFileForWrite) {
ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory)));
ASSERT_EQ(fs::create_directory(Twine(TestDirectory), false),
errc::file_exists);

int FD;
std::error_code EC;
EC = fs::openFileForWrite(Twine(TestDirectory), FD);
ASSERT_EQ(EC, errc::is_a_directory);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ASSERT_* should be used if the condition failing prevents the rest of the test from running. Otherwise, you should prefer the EXPECT_* macros (so EXPECT_EQ here).

Same goes for your other test too.


ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to use make_scope_exit right after the directory is created.

::close(FD);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test crashes on Windows, because you're attempting to close a file handle that hasn't been opened (more precisely, FD is unitialized). I'm surprised this doesn't provoke an assertion or similar on Linux (maybe it does and the pre-merge checks don't have assertions enabled). Either way, this close here is incorrect. You probably should have something to make sure FD is closed if openFileForWrite succeeds (i.e. the test is failing), to prevent a race condition, but the "normal" flow of the test shouldn't attempt to call close at all.

}

TEST_F(FileSystemTest, Remove) {
SmallString<64> BaseDir;
SmallString<64> Paths[4];
Expand Down