[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
https://github.com/Bigcheese closed https://github.com/llvm/llvm-project/pull/84423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
dwblaikie wrote: Cool cool - thanks for the extra context! https://github.com/llvm/llvm-project/pull/84423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
Bigcheese wrote: 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. https://github.com/llvm/llvm-project/pull/84423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
https://github.com/dwblaikie approved this pull request. 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) https://github.com/llvm/llvm-project/pull/84423 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
llvmbot wrote:
@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
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([]() {
auto = 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>
clang::DirectoryWatcher::creat
const int InotifyFD = inotify_init1(IN_CLOEXEC);
if (InotifyFD == -1)
return llvm::make_error(
-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>
clang::DirectoryWatcher::creat
);
if (InotifyWD == -1)
return llvm::make_error(
-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(
-std::string("SemaphorePipe::create() error: ") + strerror(errno),
-llvm::inconvertibleErrorCode());
+llvm::errnoAsErrorCode(), std::string(": SemaphorePipe::create()"));
return std::make_unique(
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++
[clang] [clang-tools-extra] [llvm] [llvm][Support] Add and use errnoAsErrorCode (PR #84423)
https://github.com/Bigcheese created
https://github.com/llvm/llvm-project/pull/84423
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.
>From 858effb8f8225e9ca7c367037046f07b576a3348 Mon Sep 17 00:00:00 2001
From: Michael Spencer
Date: Thu, 7 Mar 2024 17:36:33 -0800
Subject: [PATCH] [llvm][Support] Add and use errnoAsErrorCode
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.
---
clang-tools-extra/clangd/JSONTransport.cpp| 3 +-
.../linux/DirectoryWatcher-linux.cpp | 9 +--
llvm/include/llvm/Support/Error.h | 14
llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp | 9 +--
.../ExecutorSharedMemoryMapperService.cpp | 11 ++-
llvm/lib/Object/ArchiveWriter.cpp | 2 +-
llvm/lib/Support/AutoConvert.cpp | 8 +--
llvm/lib/Support/LockFileManager.cpp | 2 +-
llvm/lib/Support/Path.cpp | 19 ++
llvm/lib/Support/RandomNumberGenerator.cpp| 7 +-
llvm/lib/Support/Unix/Memory.inc | 10 +--
llvm/lib/Support/Unix/Path.inc| 67 +--
llvm/lib/Support/Unix/Process.inc | 12 ++--
llvm/lib/Support/Windows/Process.inc | 2 +-
llvm/lib/Support/Windows/Program.inc | 4 +-
llvm/lib/Support/raw_ostream.cpp | 6 +-
llvm/lib/Support/raw_socket_stream.cpp| 2 +-
17 files changed, 94 insertions(+), 93 deletions(-)
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([]() {
auto = 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>
clang::DirectoryWatcher::creat
const int InotifyFD = inotify_init1(IN_CLOEXEC);
if (InotifyFD == -1)
return llvm::make_error(
-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>
clang::DirectoryWatcher::creat
);
if (InotifyWD == -1)
return llvm::make_error(
-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(
-std::string("SemaphorePipe::create() error: ") + strerror(errno),
-llvm::inconvertibleErrorCode());
+llvm::errnoAsErrorCode(), std::string(": SemaphorePipe::create()"));
return std::make_unique(
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
---
