[libc-commits] [PATCH] D155766: [libc] Remove global constructor in `getopt` implementation

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added a comment.

In D155766#4517993 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://github.com/llvm/llvm-project/blob/2c38740ca661a10866a796db105752e15372ddce/libc/src/stdlib/atexit.cpp#L45> and the cmake <https://github.com/llvm/llvm-project/blob/2c38740ca661a10866a796db105752e15372ddce/libc/src/stdlib/CMakeLists.txt#L363-L364> 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 <https://github.com/llvm/llvm-project/blob/2c38740ca661a10866a796db105752e15372ddce/libc/src/__support/File/linux/file.cpp#L173>.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155766/new/

https://reviews.llvm.org/D155766