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

[llvm][Support] Add and use errnoAsErrorCode #84423

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

Bigcheese
Copy link
Contributor

LLVM is inconsistent about how it converts errno to std::error_code. This can cause problems because values outside of std::errc compare differently if one is system and one is generic on POSIX systems.

This is even more of a problem on Windows where use of the system category is just wrong, as that is for Windows errors, which have a completely different mapping than POSIX/generic errors. This patch fixes one instance of this mistake in JSONTransport.cpp.

This patch adds errnoAsErrorCode() which makes it so people do not need to think about this issue in the future. It also cleans up a lot of usage of errno in LLVM and Clang.

LLVM is inconsistent about how it converts `errno` to
`std::error_code`. This can cause problems because values outside of
`std::errc` compare differently if one is system and one is generic on
POSIX systems.

This is even more of a problem on Windows where use of the system
category is just wrong, as that is for Windows errors, which have a
completely different mapping than POSIX/generic errors. This patch
fixes one instance of this mistake in `JSONTransport.cpp`.

This patch adds `errnoAsErrorCode()` which makes it so people do not
need to think about this issue in the future. It also cleans up a lot
of usage of errno in LLVM and Clang.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-llvm-binary-utilities

Author: Michael Spencer (Bigcheese)

Changes

LLVM is inconsistent about how it converts errno to std::error_code. This can cause problems because values outside of std::errc compare differently if one is system and one is generic on POSIX systems.

This is even more of a problem on Windows where use of the system category is just wrong, as that is for Windows errors, which have a completely different mapping than POSIX/generic errors. This patch fixes one instance of this mistake in JSONTransport.cpp.

This patch adds errnoAsErrorCode() which makes it so people do not need to think about this issue in the future. It also cleans up a lot of usage of errno in LLVM and Clang.


Patch is 30.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84423.diff

17 Files Affected:

  • (modified) clang-tools-extra/clangd/JSONTransport.cpp (+1-2)
  • (modified) clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp (+3-6)
  • (modified) llvm/include/llvm/Support/Error.h (+14)
  • (modified) llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp (+3-6)
  • (modified) llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp (+5-6)
  • (modified) llvm/lib/Object/ArchiveWriter.cpp (+1-1)
  • (modified) llvm/lib/Support/AutoConvert.cpp (+4-4)
  • (modified) llvm/lib/Support/LockFileManager.cpp (+1-1)
  • (modified) llvm/lib/Support/Path.cpp (+7-12)
  • (modified) llvm/lib/Support/RandomNumberGenerator.cpp (+4-3)
  • (modified) llvm/lib/Support/Unix/Memory.inc (+5-5)
  • (modified) llvm/lib/Support/Unix/Path.inc (+33-34)
  • (modified) llvm/lib/Support/Unix/Process.inc (+6-6)
  • (modified) llvm/lib/Support/Windows/Process.inc (+1-1)
  • (modified) llvm/lib/Support/Windows/Program.inc (+2-2)
  • (modified) llvm/lib/Support/raw_ostream.cpp (+3-3)
  • (modified) llvm/lib/Support/raw_socket_stream.cpp (+1-1)
diff --git a/clang-tools-extra/clangd/JSONTransport.cpp b/clang-tools-extra/clangd/JSONTransport.cpp
index 346c7dfb66a1db..3c0e198433f360 100644
--- a/clang-tools-extra/clangd/JSONTransport.cpp
+++ b/clang-tools-extra/clangd/JSONTransport.cpp
@@ -107,8 +107,7 @@ class JSONTransport : public Transport {
         return error(std::make_error_code(std::errc::operation_canceled),
                      "Got signal, shutting down");
       if (ferror(In))
-        return llvm::errorCodeToError(
-            std::error_code(errno, std::system_category()));
+        return llvm::errorCodeToError(llvm::errnoAsErrorCode());
       if (readRawMessage(JSON)) {
         ThreadCrashReporter ScopedReporter([&JSON]() {
           auto &OS = llvm::errs();
diff --git a/clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp b/clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
index beca9586988b52..2ffbc1a226958a 100644
--- a/clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
+++ b/clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
@@ -333,8 +333,7 @@ llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::creat
   const int InotifyFD = inotify_init1(IN_CLOEXEC);
   if (InotifyFD == -1)
     return llvm::make_error<llvm::StringError>(
-        std::string("inotify_init1() error: ") + strerror(errno),
-        llvm::inconvertibleErrorCode());
+        llvm::errnoAsErrorCode(), std::string(": inotify_init1()"));
 
   const int InotifyWD = inotify_add_watch(
       InotifyFD, Path.str().c_str(),
@@ -346,15 +345,13 @@ llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::creat
       );
   if (InotifyWD == -1)
     return llvm::make_error<llvm::StringError>(
-        std::string("inotify_add_watch() error: ") + strerror(errno),
-        llvm::inconvertibleErrorCode());
+        llvm::errnoAsErrorCode(), std::string(": inotify_add_watch()"));
 
   auto InotifyPollingStopper = SemaphorePipe::create();
 
   if (!InotifyPollingStopper)
     return llvm::make_error<llvm::StringError>(
-        std::string("SemaphorePipe::create() error: ") + strerror(errno),
-        llvm::inconvertibleErrorCode());
+        llvm::errnoAsErrorCode(), std::string(": SemaphorePipe::create()"));
 
   return std::make_unique<DirectoryWatcherLinux>(
       Path, Receiver, WaitForInitialSync, InotifyFD, InotifyWD,
diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index bb4f38f7ec355e..894b6484336aef 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -1180,6 +1180,20 @@ Error errorCodeToError(std::error_code EC);
 /// will trigger a call to abort().
 std::error_code errorToErrorCode(Error Err);
 
+/// Helper to get errno as an std::error_code.
+///
+/// errno should always be represented using the generic category as that's what
+/// both libc++ and libstdc++ do. On POSIX systems you can also represent them
+/// using the system category, however this makes them compare differently for
+/// values outside of those used by `std::errc` if one is generic and the other
+/// is system.
+///
+/// See the libc++ and libstdc++ implementations of `default_error_condition` on
+/// the system category for more details on what the difference is.
+inline std::error_code errnoAsErrorCode() {
+  return std::error_code(errno, std::generic_category());
+}
+
 /// Convert an ErrorOr<T> to an Expected<T>.
 template <typename T> Expected<T> errorOrToExpected(ErrorOr<T> &&EO) {
   if (auto EC = EO.getError())
diff --git a/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp b/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
index 9cfe547c84c310..2c87b344083edb 100644
--- a/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
@@ -241,8 +241,7 @@ void SharedMemoryMapper::reserve(size_t NumBytes,
 
         int SharedMemoryFile = shm_open(SharedMemoryName.c_str(), O_RDWR, 0700);
         if (SharedMemoryFile < 0) {
-          return OnReserved(errorCodeToError(
-              std::error_code(errno, std::generic_category())));
+          return OnReserved(errorCodeToError(errnoAsErrorCode()));
         }
 
         // this prevents other processes from accessing it by name
@@ -251,8 +250,7 @@ void SharedMemoryMapper::reserve(size_t NumBytes,
         LocalAddr = mmap(nullptr, NumBytes, PROT_READ | PROT_WRITE, MAP_SHARED,
                          SharedMemoryFile, 0);
         if (LocalAddr == MAP_FAILED) {
-          return OnReserved(errorCodeToError(
-              std::error_code(errno, std::generic_category())));
+          return OnReserved(errorCodeToError(errnoAsErrorCode()));
         }
 
         close(SharedMemoryFile);
@@ -376,8 +374,7 @@ void SharedMemoryMapper::release(ArrayRef<ExecutorAddr> Bases,
 #if defined(LLVM_ON_UNIX)
 
       if (munmap(Reservations[Base].LocalAddr, Reservations[Base].Size) != 0)
-        Err = joinErrors(std::move(Err), errorCodeToError(std::error_code(
-                                             errno, std::generic_category())));
+        Err = joinErrors(std::move(Err), errorCodeToError(errnoAsErrorCode()));
 
 #elif defined(_WIN32)
 
diff --git a/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp b/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp
index e8b0e240ac1fc9..6614beec760fb3 100644
--- a/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp
@@ -62,15 +62,15 @@ ExecutorSharedMemoryMapperService::reserve(uint64_t Size) {
   int SharedMemoryFile =
       shm_open(SharedMemoryName.c_str(), O_RDWR | O_CREAT | O_EXCL, 0700);
   if (SharedMemoryFile < 0)
-    return errorCodeToError(std::error_code(errno, std::generic_category()));
+    return errorCodeToError(errnoAsErrorCode());
 
   // by default size is 0
   if (ftruncate(SharedMemoryFile, Size) < 0)
-    return errorCodeToError(std::error_code(errno, std::generic_category()));
+    return errorCodeToError(errnoAsErrorCode());
 
   void *Addr = mmap(nullptr, Size, PROT_NONE, MAP_SHARED, SharedMemoryFile, 0);
   if (Addr == MAP_FAILED)
-    return errorCodeToError(std::error_code(errno, std::generic_category()));
+    return errorCodeToError(errnoAsErrorCode());
 
   close(SharedMemoryFile);
 
@@ -140,7 +140,7 @@ Expected<ExecutorAddr> ExecutorSharedMemoryMapperService::initialize(
       NativeProt |= PROT_EXEC;
 
     if (mprotect(Segment.Addr.toPtr<void *>(), Segment.Size, NativeProt))
-      return errorCodeToError(std::error_code(errno, std::generic_category()));
+      return errorCodeToError(errnoAsErrorCode());
 
 #elif defined(_WIN32)
 
@@ -240,8 +240,7 @@ Error ExecutorSharedMemoryMapperService::release(
 #if defined(LLVM_ON_UNIX)
 
     if (munmap(Base.toPtr<void *>(), Size) != 0)
-      Err = joinErrors(std::move(Err), errorCodeToError(std::error_code(
-                                           errno, std::generic_category())));
+      Err = joinErrors(std::move(Err), errorCodeToError(errnoAsErrorCode()));
 
 #elif defined(_WIN32)
     (void)Size;
diff --git a/llvm/lib/Object/ArchiveWriter.cpp b/llvm/lib/Object/ArchiveWriter.cpp
index 96e4ec1ee0b777..be51093933a87c 100644
--- a/llvm/lib/Object/ArchiveWriter.cpp
+++ b/llvm/lib/Object/ArchiveWriter.cpp
@@ -926,7 +926,7 @@ Expected<std::string> computeArchiveRelativePath(StringRef From, StringRef To) {
   ErrorOr<SmallString<128>> PathToOrErr = canonicalizePath(To);
   ErrorOr<SmallString<128>> DirFromOrErr = canonicalizePath(From);
   if (!PathToOrErr || !DirFromOrErr)
-    return errorCodeToError(std::error_code(errno, std::generic_category()));
+    return errorCodeToError(errnoAsErrorCode());
 
   const SmallString<128> &PathTo = *PathToOrErr;
   const SmallString<128> &DirFrom = sys::path::parent_path(*DirFromOrErr);
diff --git a/llvm/lib/Support/AutoConvert.cpp b/llvm/lib/Support/AutoConvert.cpp
index 8170e553ac6e10..74842e9167bde5 100644
--- a/llvm/lib/Support/AutoConvert.cpp
+++ b/llvm/lib/Support/AutoConvert.cpp
@@ -82,21 +82,21 @@ int enableAutoConversion(int FD) {
 
 std::error_code llvm::disableAutoConversion(int FD) {
   if (::disableAutoConversion(FD) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
 
 std::error_code llvm::enableAutoConversion(int FD) {
   if (::enableAutoConversion(FD) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
 
 std::error_code llvm::restoreStdHandleAutoConversion(int FD) {
   if (::restoreStdHandleAutoConversion(FD) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -111,7 +111,7 @@ std::error_code llvm::setFileTag(int FD, int CCSID, bool Text) {
   Tag.ft_rsvflags = 0;
 
   if (fcntl(FD, F_SETTAG, &Tag) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   return std::error_code();
 }
 
diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index facdc5a0d7d421..083f8d7b37be39 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -87,7 +87,7 @@ static std::error_code getHostID(SmallVectorImpl<char> &HostID) {
   struct timespec wait = {1, 0}; // 1 second.
   uuid_t uuid;
   if (gethostuuid(uuid, &wait) != 0)
-    return std::error_code(errno, std::system_category());
+    return errnoAsErrorCode();
 
   uuid_string_t UUIDStr;
   uuid_unparse(uuid, UUIDStr);
diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp
index acee228a0d0462..4db9bc80b415bf 100644
--- a/llvm/lib/Support/Path.cpp
+++ b/llvm/lib/Support/Path.cpp
@@ -23,7 +23,6 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <cctype>
-#include <cerrno>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
@@ -1010,7 +1009,7 @@ static std::error_code copy_file_internal(int ReadFD, int WriteFD) {
   delete[] Buf;
 
   if (BytesRead < 0 || BytesWritten < 0)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   return std::error_code();
 }
 
@@ -1060,7 +1059,7 @@ ErrorOr<MD5::MD5Result> md5_contents(int FD) {
   }
 
   if (BytesRead < 0)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   MD5::MD5Result Result;
   Hash.final(Result);
   return Result;
@@ -1228,7 +1227,7 @@ TempFile::~TempFile() { assert(Done); }
 Error TempFile::discard() {
   Done = true;
   if (FD != -1 && close(FD) == -1) {
-    std::error_code EC = std::error_code(errno, std::generic_category());
+    std::error_code EC = errnoAsErrorCode();
     return errorCodeToError(EC);
   }
   FD = -1;
@@ -1297,10 +1296,8 @@ Error TempFile::keep(const Twine &Name) {
   if (!RenameEC)
     TmpName = "";
 
-  if (close(FD) == -1) {
-    std::error_code EC(errno, std::generic_category());
-    return errorCodeToError(EC);
-  }
+  if (close(FD) == -1)
+    return errorCodeToError(errnoAsErrorCode());
   FD = -1;
 
   return errorCodeToError(RenameEC);
@@ -1319,10 +1316,8 @@ Error TempFile::keep() {
 
   TmpName = "";
 
-  if (close(FD) == -1) {
-    std::error_code EC(errno, std::generic_category());
-    return errorCodeToError(EC);
-  }
+  if (close(FD) == -1)
+    return errorCodeToError(errnoAsErrorCode());
   FD = -1;
 
   return Error::success();
diff --git a/llvm/lib/Support/RandomNumberGenerator.cpp b/llvm/lib/Support/RandomNumberGenerator.cpp
index aea0132a93fe2e..12fe109dbc2b54 100644
--- a/llvm/lib/Support/RandomNumberGenerator.cpp
+++ b/llvm/lib/Support/RandomNumberGenerator.cpp
@@ -18,6 +18,7 @@
 
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #ifdef _WIN32
 #include "llvm/Support/Windows/WindowsSupport.h"
@@ -81,14 +82,14 @@ std::error_code llvm::getRandomBytes(void *Buffer, size_t Size) {
     std::error_code Ret;
     ssize_t BytesRead = read(Fd, Buffer, Size);
     if (BytesRead == -1)
-      Ret = std::error_code(errno, std::system_category());
+      Ret = errnoAsErrorCode();
     else if (BytesRead != static_cast<ssize_t>(Size))
       Ret = std::error_code(EIO, std::system_category());
     if (close(Fd) == -1)
-      Ret = std::error_code(errno, std::system_category());
+      Ret = errnoAsErrorCode();
 
     return Ret;
   }
-  return std::error_code(errno, std::system_category());
+  return errnoAsErrorCode();
 #endif
 }
diff --git a/llvm/lib/Support/Unix/Memory.inc b/llvm/lib/Support/Unix/Memory.inc
index 69bd1164343da7..bac208a7d543ca 100644
--- a/llvm/lib/Support/Unix/Memory.inc
+++ b/llvm/lib/Support/Unix/Memory.inc
@@ -86,7 +86,7 @@ MemoryBlock Memory::allocateMappedMemory(size_t NumBytes,
 #else
   fd = open("/dev/zero", O_RDWR);
   if (fd == -1) {
-    EC = std::error_code(errno, std::generic_category());
+    EC = errnoAsErrorCode();
     return MemoryBlock();
   }
 #endif
@@ -122,7 +122,7 @@ MemoryBlock Memory::allocateMappedMemory(size_t NumBytes,
       return allocateMappedMemory(NumBytes, nullptr, PFlags, EC);
     }
 
-    EC = std::error_code(errno, std::generic_category());
+    EC = errnoAsErrorCode();
 #if !defined(MAP_ANON)
     close(fd);
 #endif
@@ -153,7 +153,7 @@ std::error_code Memory::releaseMappedMemory(MemoryBlock &M) {
     return std::error_code();
 
   if (0 != ::munmap(M.Address, M.AllocatedSize))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   M.Address = nullptr;
   M.AllocatedSize = 0;
@@ -186,7 +186,7 @@ std::error_code Memory::protectMappedMemory(const MemoryBlock &M,
   if (InvalidateCache && !(Protect & PROT_READ)) {
     int Result = ::mprotect((void *)Start, End - Start, Protect | PROT_READ);
     if (Result != 0)
-      return std::error_code(errno, std::generic_category());
+      return errnoAsErrorCode();
 
     Memory::InvalidateInstructionCache(M.Address, M.AllocatedSize);
     InvalidateCache = false;
@@ -196,7 +196,7 @@ std::error_code Memory::protectMappedMemory(const MemoryBlock &M,
   int Result = ::mprotect((void *)Start, End - Start, Protect);
 
   if (Result != 0)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   if (InvalidateCache)
     Memory::InvalidateInstructionCache(M.Address, M.AllocatedSize);
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 9f89d63bb0fda8..968e2c459f3fb1 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -357,7 +357,7 @@ uint32_t file_status::getLinkCount() const { return fs_st_nlinks; }
 ErrorOr<space_info> disk_space(const Twine &Path) {
   struct STATVFS Vfs;
   if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   auto FrSize = STATVFS_F_FRSIZE(Vfs);
   space_info SpaceInfo;
   SpaceInfo.capacity = static_cast<uint64_t>(Vfs.f_blocks) * FrSize;
@@ -386,7 +386,7 @@ std::error_code current_path(SmallVectorImpl<char> &result) {
       // See if there was a real error.
       if (errno != ENOMEM) {
         result.clear();
-        return std::error_code(errno, std::generic_category());
+        return errnoAsErrorCode();
       }
       // Otherwise there just wasn't enough space.
       result.resize_for_overwrite(result.capacity() * 2);
@@ -403,7 +403,7 @@ std::error_code set_current_path(const Twine &path) {
   StringRef p = path.toNullTerminatedStringRef(path_storage);
 
   if (::chdir(p.begin()) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -415,7 +415,7 @@ std::error_code create_directory(const Twine &path, bool IgnoreExisting,
 
   if (::mkdir(p.begin(), Perms) == -1) {
     if (errno != EEXIST || !IgnoreExisting)
-      return std::error_code(errno, std::generic_category());
+      return errnoAsErrorCode();
   }
 
   return std::error_code();
@@ -431,7 +431,7 @@ std::error_code create_link(const Twine &to, const Twine &from) {
   StringRef t = to.toNullTerminatedStringRef(to_storage);
 
   if (::symlink(t.begin(), f.begin()) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -444,7 +444,7 @@ std::error_code create_hard_link(const Twine &to, const Twine &from) {
   StringRef t = to.toNullTerminatedStringRef(to_storage);
 
   if (::link(t.begin(), f.begin()) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -456,7 +456,7 @@ std::error_code remove(const Twine &path, bool IgnoreNonExisting) {
   struct stat buf;
   if (lstat(p.begin(), &buf) != 0) {
     if (errno != ENOENT || !IgnoreNonExisting)
-      return std::error_code(errno, std::generic_category());
+      return errnoAsErrorCode();
     return std::error_code();
   }
 
@@ -470,7 +470,7 @@ std::error_code remove(const Twine &path, bool IgnoreNonExisting) {
 
   if (::remove(p.begin()) == -1) {
     if (errno != ENOENT || !IgnoreNonExisting)
-      return std::error_code(errno, std::generic_category());
+      return errnoAsErrorCode();
   }
 
   return std::error_code();
@@ -563,7 +563,7 @@ static bool is_local_impl(struct STATVFS &Vfs) {
 std::error_code is_local(const Twine &Path, bool &Result) {
   struct STATVFS Vfs;
   if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   Result = is_local_impl(Vfs);
   return std::error_code();
@@ -572,7 +572,7 @@ std::error_code is_local(const Twine &Path, bool &Result) {
 std::error_code is_local(int FD, bool &Result) {
   struct STATVFS Vfs;
   if (::FSTATVFS(FD, &Vfs))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   Result = is_local_impl(Vfs);
   return std::error_code();
@@ -586,7 +586,7 @@ std::error_code rename(const Twine &from, const Twine &to) {
   StringRef t = to.toNullTerminatedStringRef(to_storage);
 
   if (::rename(f.begin(), t.begin()) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -595,7 +595,7 @@ std::error_code resize_file(int FD, uint64_t Size) {
   // Use ftruncate as a fallback. It may or may not allocate space. At least on
   // OS X with HFS+ it does.
   if (::ftruncate(FD, Size) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   return std::error_code();
 }
@@ -617,7 +617,7 @@ std::error_code access(const Twine &Path, AccessMode Mode) {
   StringRef P = Path.toNullTerminatedStringRef(PathStorage);
 
   if (::access(P.begin(), convertAccessMode(Mode)) == -1)
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
 
   if (Mode == AccessMode::Execute) {
     // Don't say that directories are executable.
@@ -726,7 +726,7 @@ static file_type typeForMode(mode_t Mode) {
 static std::error_code fillStatus(int StatRet, const struct stat &Status,
                                   file_status &Result) {
   if (StatRet != 0) {
-    std::error_code EC(errno, std::generic_category());
+    std::error_code EC = errnoAsErrorCode();
     if (EC == errc::no_such_file_or_directory)
       Result = file_status(file_type::file_not_found);
     else
@@ -782,13 +782,13 @@ std::error_code setPermissions(const Twine &Path, perms Permissions) {
   StringRef P = Path.toNullTerminatedStringRef(PathStorage);
 
   if (::chmod(P.begin(), Permissions))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   return std::error_code();
 }
 
 std::error_code setPermissions(int FD, perms Permissions) {
   if (::fchmod(FD, Permissions))
-    return std::error_code(errno, std::generic_category());
+    return errnoAsErrorCode();
   return std::error_code();
 }
 
@@ -799,7 +799,7 @@ std::error_code setLastAccessAndModificationTime(int FD, TimePoint<> AccessTime,
   Times[0] = sys::toTimeSpec(AccessTime);
   Times[1] = sys::toTimeSpec(ModificationTime);
   if (::futimens(FD, Times))
-    return std::error_...
[truncated]

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Sounds OK to me.

(though several other instances of system_category are touched in this patch - out of curiosity, how are those different from the one fixed case you called out in JSONTransport.cpp? Are the other uses of system_category sort of more benign in some way? Or equally buggy/fixed?

Guess there's no practical test coverage for this? Not worth unit testing JSONTransport to inspect its error code, etc, probably)

@Bigcheese
Copy link
Contributor Author

The other cases of std::system_category were in LLVM_ON_UNIX (or similar) blocks that would only be used on systems where it's mostly fine to use either one, as most of the time you'll get an error that's in std::errc, and then there's no difference (or they just are never compared in general). The initial desire to do this came from spending 30m looking into which one to use on UNIX systems in general and wanting to avoid that in the future. The JSONTransport.cpp case was just more indication to me that the existing way was error prone.

In auditing all uses of errno I did find a few other places where the code isn't quite wrong, but it's not really using llvm::Error correctly. There's quite a few places where people use llvm::inconvertibleErrorCode() where they really shouldn't be. For example llvm-exegesis has a bunch of places where they call strerror(errno) to construct an llvm::Error that implicitly uses llvm::inconvertibleErrorCode() as the std::error_code value. Our existing documentation here is pretty nice for Error and Expected (https://llvm.org/docs/ProgrammersManual.html#error-handling), but it would be nice to better cover how std::error_code is supposed to be propagated, as it currently kind of implies they are going away, but they are always needed for OS errors. I'll try to get to this when I can find time.

As for test coverage, some of these have existing error tests, but for a lot of the rest it's either incredibly difficult or just not possible (without using LD_PRELOAD or something similar) to test. Given that, it's good to make handling them as easy to get correct as practicable.

@dwblaikie
Copy link
Collaborator

Cool cool - thanks for the extra context!

@Bigcheese Bigcheese merged commit ba13fa2 into llvm:main Mar 9, 2024
10 of 11 checks passed
@Bigcheese Bigcheese deleted the dev/errno branch March 9, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants