[libc-commits] [PATCH] D119002: [libc] Fix compilation with gcc

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a comment.

In D119002#3297298 <https://reviews.llvm.org/D119002#3297298>, @sivachandra wrote:

> While doing this cleanup is great, I think protecting against slipping is also important. So, a few things that need to be done in parallel or as immediate follow ups:
>
> 1. Update developer guidance on how and what to build and test as part of their development workflows.
> 2. Update or add CI builders to protect against slipping.

For type punning not all compilers warn about it, do you mean we should ask developers to test their code against both Clang and GCC? I think we should have this covered by the CI.

Also I'm seeing build error for Bazel that weren't there before, any idea why? Which headers should be picked for <stddef.h> and similar headers?

   % bazelisk-linux-amd64 test --sandbox_base=/dev/shm --config=generic_clang @llvm-project//libc:all
  INFO: Analyzed 254 targets (15 packages loaded, 591 targets configured).
  INFO: Found 254 targets and 0 test targets...
  ERROR: /redacted/.cache/bazel/_bazel_gchatelet/903566bd2da61ef16af7f3375ea676d6/external/llvm-project/libc/BUILD.bazel:572:14: Compiling libc/src/stdlib/bsearch.cpp failed: undeclared inclusion(s) in rule '@llvm-project//libc:bsearch':
  this rule is missing dependency declarations for the following files included by 'libc/src/stdlib/bsearch.cpp':
    '/usr/lib/clang/13.0.1/include/stddef.h'
    '/usr/lib/clang/13.0.1/include/stdint.h'



================
Comment at: libc/src/__support/FPUtil/x86_64/sqrt.h:36
 template <> inline long double sqrt<long double>(long double x) {
-  long double result;
-  __asm__ __volatile__("fsqrt" : "=t"(result) : "t"(x));
-  return result;
+  __asm__ __volatile__("fsqrt" : "+t"(x));
+  return x;
----------------
@lntue I had to use `+t` here for gcc to be happy.
https://stackoverflow.com/a/53680288


================
Comment at: libc/src/fenv/fesetexceptflag.cpp:30
+  const int excepts_as_int = llvmlibc_builtin_bit_cast<IntType>(*flagp);
+  const int excepts_to_set = excepts_as_int & excepts;
   fputil::clear_except(FE_ALL_EXCEPT);
----------------
sivachandra wrote:
> Why is this required?
`sizeof(fexcept_t)` is unknown as it depends on the opaque `fexcept_t` data type.
When I tried to bitcast `*flagp` into an `int` I had a static assert failure from bit_cast implementation that `sizeof(fexcept_t) != sizeof(int)`, it turns out that on my system `sizeof(fexcept_t) != sizeof(int16_t)`.
So for the bitcast to be correct we need to pick the right integral type and then convert it to int.
I've selected `int16_t` and `int32_t` but we may need to provide other integral types depending on `sizeof(fexcept_t)`


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:486
+}
+
 namespace internal {
----------------
sivachandra wrote:
> Why was this block moved?
Explicit template instantiation cannot be declared within the class itself. It has to be defined [[ https://en.cppreference.com/w/cpp/language/class_template#:~:text=An%20explicit%20instantiation%20definition%20forces,entire%20program%2C%20no%20diagnostic%20required. | outside of the class ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119002