[libc-commits] [libc] [libc] Refactor getopt to use standard reset and internal test hook (PR #197644)
via libc-commits
libc-commits at lists.llvm.org
Thu May 14 08:11:08 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-risc-v
Author: Jeff Bailey (kaladron)
<details>
<summary>Changes</summary>
Implemented standard POSIX optind = 0 reset logic in getopt and getopt_r.
To support hermetic testing of stderr error messages, provided an internal __llvm_libc_getopt_set_errorstream function. Added this as a library entrypoint for linkage but omitted it from public headers to keep the namespace clean.
Refactored getopt_r to use a reference-based context to avoid redundant dereferencing of global variables while maintaining standard compliance.
Updated getopt_test.cpp to use the new internal hook and standard optind reset.
Simplified the internal implementation by removing the RefWrapper abstraction and grouping all state into a pointer-based GetoptContext.
Assisted-by: Automated tooling, human reviewed.
---
Full diff: https://github.com/llvm/llvm-project/pull/197644.diff
7 Files Affected:
- (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
- (modified) libc/config/linux/riscv/entrypoints.txt (+1)
- (modified) libc/config/linux/x86_64/entrypoints.txt (+1)
- (modified) libc/src/unistd/CMakeLists.txt (+15)
- (modified) libc/src/unistd/getopt.cpp (+45-46)
- (modified) libc/src/unistd/getopt.h (+8-1)
- (modified) libc/test/src/unistd/getopt_test.cpp (+25-36)
``````````diff
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index e61b127e42102..a0f18558922ba 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -1238,6 +1238,7 @@ if(LLVM_LIBC_FULL_BUILD)
libc.src.unistd.execv
libc.src.unistd.fork
libc.src.unistd.getopt
+ libc.src.unistd.__llvm_libc_getopt_set_errorstream
libc.src.unistd.optarg
libc.src.unistd.opterr
libc.src.unistd.optind
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 7a34cc5fba201..756f5ffcd88c9 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -1372,6 +1372,7 @@ if(LLVM_LIBC_FULL_BUILD)
libc.src.unistd.execv
libc.src.unistd.fork
libc.src.unistd.getopt
+ libc.src.unistd.__llvm_libc_getopt_set_errorstream
libc.src.unistd.optarg
libc.src.unistd.opterr
libc.src.unistd.optind
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 00c94e1e9b5a0..80a0c8a9507a0 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -1448,6 +1448,7 @@ if(LLVM_LIBC_FULL_BUILD)
libc.src.unistd.execv
libc.src.unistd.fork
libc.src.unistd.getopt
+ libc.src.unistd.__llvm_libc_getopt_set_errorstream
libc.src.unistd.optarg
libc.src.unistd.opterr
libc.src.unistd.optind
diff --git a/libc/src/unistd/CMakeLists.txt b/libc/src/unistd/CMakeLists.txt
index 5e6df74ac92db..eba37a66c114f 100644
--- a/libc/src/unistd/CMakeLists.txt
+++ b/libc/src/unistd/CMakeLists.txt
@@ -372,6 +372,21 @@ add_entrypoint_object(
libc.src.stdio.stderr
)
+add_entrypoint_object(
+ __llvm_libc_getopt_set_errorstream
+ SRCS
+ getopt.cpp
+ HDRS
+ getopt.h
+ DEPENDS
+ libc.include.unistd
+ libc.src.__support.CPP.optional
+ libc.src.__support.CPP.string_view
+ libc.src.__support.File.file
+ libc.src.stdio.fprintf
+ libc.src.stdio.stderr
+)
+
add_entrypoint_object(
swab
SRCS
diff --git a/libc/src/unistd/getopt.cpp b/libc/src/unistd/getopt.cpp
index 84b962d0cdda8..2de079ac1f401 100644
--- a/libc/src/unistd/getopt.cpp
+++ b/libc/src/unistd/getopt.cpp
@@ -23,30 +23,22 @@
namespace LIBC_NAMESPACE_DECL {
-template <typename T> struct RefWrapper {
- RefWrapper() = delete;
- constexpr RefWrapper(T *p) : ptr{p} {}
- constexpr RefWrapper(const RefWrapper &) = default;
- RefWrapper &operator=(const RefWrapper &) = default;
- operator T &() { return *ptr; }
- T &get() { return *ptr; }
- T *ptr;
-};
-
+// GetoptContext allows the getopt_r engine to update the required POSIX
+// global state (optind, optarg, etc.) directly in place when called by the
+// standard getopt() function. Using pointers avoids the overhead and
+// complexity of copying values to and from a state object, while still
+// allowing the engine to be used reentrantly by pointing it to caller-provided
+// local variables.
struct GetoptContext {
- RefWrapper<char *> optarg;
- RefWrapper<int> optind;
- RefWrapper<int> optopt;
- RefWrapper<unsigned> optpos;
-
- RefWrapper<int> opterr;
-
+ char **optarg;
+ int *optind;
+ int *optopt;
+ unsigned *optpos;
+ int *opterr;
FILE *errstream;
- GetoptContext &operator=(const GetoptContext &) = default;
-
template <typename... Ts> void report_error(const char *fmt, Ts... ts) {
- if (opterr)
+ if (opterr && *opterr)
LIBC_NAMESPACE::fprintf(
errstream ? errstream
: reinterpret_cast<FILE *>(LIBC_NAMESPACE::stderr),
@@ -93,30 +85,40 @@ struct OptstringParser {
int getopt_r(int argc, char *const argv[], const char *optstring,
GetoptContext &ctx) {
- auto failure = [&ctx](int ret = -1) {
- ctx.optpos.get() = 0;
+ // Alias the context members to local references for clean code
+ int &optind = *ctx.optind;
+ int &optopt = *ctx.optopt;
+ char *&optarg = *ctx.optarg;
+ unsigned &optpos = *ctx.optpos;
+
+ auto failure = [&optpos](int ret = -1) {
+ optpos = 0;
return ret;
};
- if (ctx.optind >= argc || !argv[ctx.optind])
+ if (optind == 0) {
+ optind = 1;
+ optpos = 0;
+ }
+
+ if (optind >= argc || !argv[optind])
return failure();
- cpp::string_view current =
- cpp::string_view{argv[ctx.optind]}.substr(ctx.optpos);
+ cpp::string_view current = cpp::string_view{argv[optind]}.substr(optpos);
- auto move_forward = [¤t, &ctx] {
+ auto move_forward = [¤t, &optpos] {
current = current.substr(1);
- ctx.optpos.get()++;
+ optpos++;
};
// If optpos is nonzero, then we are already parsing a valid flag and these
// need not be checked.
- if (ctx.optpos == 0) {
+ if (optpos == 0) {
if (current[0] != '-')
return failure();
if (current == "--") {
- ctx.optind.get()++;
+ optind++;
return failure();
}
@@ -137,7 +139,7 @@ int getopt_r(int argc, char *const argv[], const char *optstring,
auto match = find_match();
if (!match) {
ctx.report_error("%s: illegal option -- %c\n", argv[0], current[0]);
- ctx.optopt.get() = current[0];
+ optopt = current[0];
return failure('?');
}
@@ -152,24 +154,24 @@ int getopt_r(int argc, char *const argv[], const char *optstring,
// This const cast is fine because current was already holding a mutable
// string, it just doesn't have the semantics to note that, we could use
// span but it doesn't have string_view string niceties.
- ctx.optarg.get() = const_cast<char *>(current.data());
+ optarg = const_cast<char *>(current.data());
} else {
// One char lookahead to see if we ran out of arguments. If so, return ':'
// if the first character of optstring is ':'. optind must stay at the
// current value so only increase it after we known there is another arg.
- if (ctx.optind + 1 >= argc || !argv[ctx.optind + 1]) {
+ if (optind + 1 >= argc || !argv[optind + 1]) {
ctx.report_error("%s: option requires an argument -- %c\n", argv[0],
match->c);
return failure(optstring[0] == ':' ? ':' : '?');
}
- ctx.optarg.get() = argv[++ctx.optind];
+ optarg = argv[++optind];
}
- ctx.optind++;
- ctx.optpos.get() = 0;
+ optind++;
+ optpos = 0;
} else if (current.empty()) {
// If this argument is now empty we are safe to move onto the next one.
- ctx.optind++;
- ctx.optpos.get() = 0;
+ optind++;
+ optpos = 0;
}
return match->c;
@@ -184,21 +186,18 @@ int optopt = 0;
int opterr = 0;
}
-static unsigned optpos;
+static unsigned optpos = 0;
static GetoptContext ctx{&impl::optarg, &impl::optind, &impl::optopt,
&optpos, &impl::opterr, /*errstream=*/nullptr};
-#ifndef LIBC_COPT_PUBLIC_PACKAGING
-// This is used exclusively in tests.
-void set_getopt_state(char **optarg_in, int *optind_in, int *optopt_in,
- unsigned *optpos_in, int *opterr_in, FILE *errstream) {
- ctx = {optarg_in, optind_in, optopt_in, optpos_in, opterr_in, errstream};
-}
-#endif
-
} // namespace impl
+LLVM_LIBC_FUNCTION(void, __llvm_libc_getopt_set_errorstream,
+ (FILE * errstream)) {
+ impl::ctx.errstream = errstream;
+}
+
LLVM_LIBC_FUNCTION(int, getopt,
(int argc, char *const argv[], const char *optstring)) {
return getopt_r(argc, argv, optstring, impl::ctx);
diff --git a/libc/src/unistd/getopt.h b/libc/src/unistd/getopt.h
index 0be639d871196..297b01bea1dbd 100644
--- a/libc/src/unistd/getopt.h
+++ b/libc/src/unistd/getopt.h
@@ -16,8 +16,15 @@
namespace LIBC_NAMESPACE_DECL {
namespace impl {
-void set_getopt_state(char **, int *, int *, unsigned *, int *, FILE *);
+extern "C" {
+extern char *optarg;
+extern int optind;
+extern int optopt;
+extern int opterr;
}
+} // namespace impl
+
+void __llvm_libc_getopt_set_errorstream(FILE *);
int getopt(int argc, char *const argv[], const char *optstring);
diff --git a/libc/test/src/unistd/getopt_test.cpp b/libc/test/src/unistd/getopt_test.cpp
index 1a31094e98fc8..edbb6d634c350 100644
--- a/libc/test/src/unistd/getopt_test.cpp
+++ b/libc/test/src/unistd/getopt_test.cpp
@@ -15,21 +15,8 @@
using LIBC_NAMESPACE::cpp::array;
-namespace test_globals {
-char *optarg;
-int optind = 1;
-int optopt;
-int opterr = 1;
-
-unsigned optpos;
-} // namespace test_globals
-
-// This can't be a constructor because it will get run before the constructor
-// which sets the default state in getopt.
void set_state(FILE *errstream) {
- LIBC_NAMESPACE::impl::set_getopt_state(
- &test_globals::optarg, &test_globals::optind, &test_globals::optopt,
- &test_globals::optpos, &test_globals::opterr, errstream);
+ LIBC_NAMESPACE::__llvm_libc_getopt_set_errorstream(errstream);
}
static void my_memcpy(char *dest, const char *src, size_t size) {
@@ -68,12 +55,14 @@ struct LlvmLibcGetoptTest : public LIBC_NAMESPACE::testing::Test {
void SetUp() override {
ASSERT_TRUE(!!(errstream = memopen(&pos)));
set_state(errstream);
- ASSERT_EQ(test_globals::optind, 1);
+ LIBC_NAMESPACE::impl::optind = 0;
+ LIBC_NAMESPACE::getopt(0, nullptr, "");
}
void TearDown() override {
- test_globals::optind = 1;
- test_globals::opterr = 1;
+ LIBC_NAMESPACE::impl::optind = 0;
+ LIBC_NAMESPACE::getopt(0, nullptr, "");
+ LIBC_NAMESPACE::impl::opterr = 1;
}
};
@@ -88,42 +77,42 @@ TEST_F(LlvmLibcGetoptTest, NoMatch) {
EXPECT_EQ(LIBC_NAMESPACE::getopt(1, argv.data(), "..."), -1);
// argv[optind] == nullptr
- test_globals::optind = 2;
+ LIBC_NAMESPACE::impl::optind = 2;
EXPECT_EQ(LIBC_NAMESPACE::getopt(100, argv.data(), "..."), -1);
// argv[optind][0] != '-'
- test_globals::optind = 1;
+ LIBC_NAMESPACE::impl::optind = 1;
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "a"), -1);
- ASSERT_EQ(test_globals::optind, 1);
+ ASSERT_EQ(LIBC_NAMESPACE::impl::optind, 1);
// argv[optind] == "-"
argv[1] = "-"_c;
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "a"), -1);
- ASSERT_EQ(test_globals::optind, 1);
+ ASSERT_EQ(LIBC_NAMESPACE::impl::optind, 1);
// argv[optind] == "--", then return -1 and incremement optind
argv[1] = "--"_c;
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "a"), -1);
- EXPECT_EQ(test_globals::optind, 2);
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 2);
}
TEST_F(LlvmLibcGetoptTest, WrongMatch) {
array<char *, 3> argv{"prog"_c, "-b"_c, nullptr};
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "a"), int('?'));
- EXPECT_EQ(test_globals::optopt, (int)'b');
- EXPECT_EQ(test_globals::optind, 1);
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optopt, (int)'b');
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_STREQ(get_error_msg(), "prog: illegal option -- b\n");
}
TEST_F(LlvmLibcGetoptTest, OpterrFalse) {
array<char *, 3> argv{"prog"_c, "-b"_c, nullptr};
- test_globals::opterr = 0;
+ LIBC_NAMESPACE::impl::opterr = 0;
set_state(errstream);
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "a"), int('?'));
- EXPECT_EQ(test_globals::optopt, (int)'b');
- EXPECT_EQ(test_globals::optind, 1);
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optopt, (int)'b');
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_STREQ(get_error_msg(), "");
}
@@ -131,11 +120,11 @@ TEST_F(LlvmLibcGetoptTest, MissingArg) {
array<char *, 3> argv{"prog"_c, "-b"_c, nullptr};
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), ":b:"), (int)':');
- ASSERT_EQ(test_globals::optind, 1);
+ ASSERT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_STREQ(get_error_msg(), "prog: option requires an argument -- b\n");
reset_errstream();
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "b:"), int('?'));
- EXPECT_EQ(test_globals::optind, 1);
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_STREQ(get_error_msg(), "prog: option requires an argument -- b\n");
}
@@ -143,25 +132,25 @@ TEST_F(LlvmLibcGetoptTest, ParseArgInCurrent) {
array<char *, 3> argv{"prog"_c, "-barg"_c, nullptr};
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "b:"), (int)'b');
- EXPECT_STREQ(test_globals::optarg, "arg");
- EXPECT_EQ(test_globals::optind, 2);
+ EXPECT_STREQ(LIBC_NAMESPACE::impl::optarg, "arg");
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 2);
}
TEST_F(LlvmLibcGetoptTest, ParseArgInNext) {
array<char *, 4> argv{"prog"_c, "-b"_c, "arg"_c, nullptr};
EXPECT_EQ(LIBC_NAMESPACE::getopt(3, argv.data(), "b:"), (int)'b');
- EXPECT_STREQ(test_globals::optarg, "arg");
- EXPECT_EQ(test_globals::optind, 3);
+ EXPECT_STREQ(LIBC_NAMESPACE::impl::optarg, "arg");
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 3);
}
TEST_F(LlvmLibcGetoptTest, ParseMultiInOne) {
array<char *, 3> argv{"prog"_c, "-abc"_c, nullptr};
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "abc"), (int)'a');
- ASSERT_EQ(test_globals::optind, 1);
+ ASSERT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "abc"), (int)'b');
- ASSERT_EQ(test_globals::optind, 1);
+ ASSERT_EQ(LIBC_NAMESPACE::impl::optind, 1);
EXPECT_EQ(LIBC_NAMESPACE::getopt(2, argv.data(), "abc"), (int)'c');
- EXPECT_EQ(test_globals::optind, 2);
+ EXPECT_EQ(LIBC_NAMESPACE::impl::optind, 2);
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/197644
More information about the libc-commits
mailing list