[libc] Remove global constructor in `getopt` implementation

[jhuber6]

Summary

This file required a global constructor due to copying the file stream
and have a non-constexpr constructor for the wrapper type. Also, I
changes the opterr to be a pointer, because it seemed like it wasn't
being set correctly as an externally visibile variable if we just
captured it by value.

[sivachandra]

I think this is OK but please wait for @abrachet .

Would it make sense to add -Wglobal-constructors here: https://github.com/llvm/llvm-project/blob/main/libc/cmake/modules/LLVMLibCObjectRules.cmake#L36

[abrachet]

It's not clear what's different about the impl:: objects and the __llvm_libc::stderr one. [1] If this changes anything it just changes the decision the compiler decided to make, but it is still allowed to use a constructor here. I think to properly remove a global constructor the better route is what we did for atexit and the cmake that goes with it.

The opterr change looks good.

[1] In fact, if there is a difference it would be that __llvm_libc::stderr is (should be if not) a hidden alias to a default visibility stderr, which would make this a relative relocation, while the impl:: objects are symbolic relocations.

libc/src/unistd/getopt.cpp

24–25

This explicit constructor can just be removed instead, I think.

[jhuber6]

In D155766#4517993, @sivachandra wrote:

I think this is OK but please wait for @abrachet .

Would it make sense to add -Wglobal-constructors here: https://github.com/llvm/llvm-project/blob/main/libc/cmake/modules/LLVMLibCObjectRules.cmake#L36

This is the patch I had in https://reviews.llvm.org/D155721, but @michaelrj seemed concerned about adding more warnings so I figured I'd fix it first.

In D155766#4518212, @abrachet wrote:

It's not clear what's different about the impl:: objects and the __llvm_libc::stderr one. [1] If this changes anything it just changes the decision the compiler decided to make, but it is still allowed to use a constructor here. I think to properly remove a global constructor the better route is what we did for atexit and the cmake that goes with it.

The opterr change looks good.

[1] In fact, if there is a difference it would be that __llvm_libc::stderr is (should be if not) a hidden alias to a default visibility stderr, which would make this a relative relocation, while the impl:: objects are symbolic relocations.

The __llvm_libc::stderr depends on the address of a value that cannot be statically determined, see https://godbolt.org/z/WYjeq3bnK, so it's like you say with the relocations. I don't think there's a way to make this a constant expression without the File API exporting the underlying static variable here.

[abrachet]

In D155766#4518486, @jhuber6 wrote:

The __llvm_libc::stderr depends on the address of a value that cannot be statically determined, see https://godbolt.org/z/WYjeq3bnK, so it's like you say with the relocations. I don't think there's a way to make this a constant expression without the File API exporting the underlying static variable here.

Oh right I see now, we aren't taking the address of __llvm_libc::stderr. I guess then we could hold a FILE** instead and do fprintf(*errstream, ...) or I'm fine with the change as is too.

libc/src/unistd/getopt.cpp

46–50

I think this or something similar is cleaner than conditionals, but up to you.

libc/src/unistd/getopt.h

 #define LLVM_LIBC_SRC_UNISTD_GETOPT_H
 
 #include <stdio.h>
 #include <unistd.h>
 
 namespace __llvm_libc {
 
 namespace impl {
-void set_getopt_state(char **, int *, int *, unsigned *, int, FILE *);
+void set_getopt_state(char **, int *, int *, unsigned *, int *, FILE *);
 }
 
 int getopt(int argc, char *const argv[], const char *optstring);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_UNISTD_GETOPT_H

libc/src/unistd/getopt.cpp

 #include <stdio.h>
 
 // This is POSIX compliant and does not support GNU extensions, mainly this is
 // just the re-ordering of argv elements such that unknown arguments can be
 // easily iterated over.
 
 namespace __llvm_libc {
 
 template <typename T> struct RefWrapper {
-  RefWrapper(T *ptr) : ptr(ptr) {}
   RefWrapper &operator=(const RefWrapper &) = default;
   operator T &() { return *ptr; }
   T &get() { return *ptr; }
   T *ptr;
 };
 
 struct GetoptContext {
   RefWrapper<char *> optarg;
   RefWrapper<int> optind;
   RefWrapper<int> optopt;
   RefWrapper<unsigned> optpos;
 
-  int opterr;
+  RefWrapper<int> opterr;
 
   FILE *errstream;
 
   GetoptContext &operator=(const GetoptContext &) = default;
 
   template <typename... Ts> void report_error(const char *fmt, Ts... ts) {
     if (opterr)
-      __llvm_libc::fprintf(errstream, fmt, ts...);
+      __llvm_libc::fprintf(
+          errstream ? errstream : reinterpret_cast<FILE *>(__llvm_libc::stderr),
+          fmt, ts...);
   }
 };
 
 struct OptstringParser {
   using value_type = struct {
     char c;
     bool arg;
   };
 
   cpp::string_view optstring;
 
int getopt_r(int argc, char *const argv[], const char *optstring,
   }
 
   return match->c;
 }
 
 namespace impl {
 
 extern "C" {
-
 char *optarg = nullptr;
 int optind = 1;
 int optopt = 0;
 int opterr = 0;
-
 }
 
 static unsigned optpos;
 
-static GetoptContext ctx{
-    &impl::optarg, &impl::optind,
-    &impl::optopt, &optpos,
-    impl::opterr,  reinterpret_cast<FILE *>(__llvm_libc::stderr)};
+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, int *optind, int *optopt, unsigned *optpos,
-                      int opterr, FILE *errstream) {
+                      int *opterr, FILE *errstream) {
   ctx = {optarg, optind, optopt, optpos, opterr, errstream};
 }
 #endif
 
 } // namespace impl
 
 LLVM_LIBC_FUNCTION(int, getopt,
                    (int argc, char *const argv[], const char *optstring)) {
   return getopt_r(argc, argv, optstring, impl::ctx);
 }
 
 } // namespace __llvm_libc

libc/test/src/unistd/getopt_test.cpp

 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) {
   __llvm_libc::impl::set_getopt_state(
       &test_globals::optarg, &test_globals::optind, &test_globals::optopt,
-      &test_globals::optpos, test_globals::opterr, errstream);
+      &test_globals::optpos, &test_globals::opterr, errstream);
 }
 
 // TODO: <stdio> could be either llvm-libc's or the system libc's. The former
 // doesn't currently support fmemopen but does have fopencookie. In the future
 // just use that instead. This memopen does no error checking for the size
 // of the buffer, etc.
 FILE *memopen(char **pos) {
   static auto memcpy = [](char *dest, const char *src, size_t size) {