This is an archive of the discontinued LLVM Phabricator instance.

[libc] Fix compilation with gcc
AbandonedPublic

Authored by gchatelet on Feb 4 2022, 8:00 AM.

Details

Summary

We may want to break this patch into smaller independent pieces but it's easier to have the full perspective first.

  1. Type punning is used throughout llvmlibc but this is undefined behavior in C++ (e.g. FPUtils using FP/Integral union)
  2. Clang is happy with vector type conversions in elements_x86.h but vector types are slightly different in GCC and so bit casting is necessary.
  3. Some compilation options were only available in Clang, preventing compilation with GCC.

I still have compilation issues with an asm statement and link errors so I'll iterate a bit more on the patch.

Diff Detail

Event Timeline

gchatelet created this revision.Feb 4 2022, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 8:00 AM
gchatelet requested review of this revision.Feb 4 2022, 8:00 AM
gchatelet added inline comments.Feb 4 2022, 8:07 AM
libc/src/math/generic/log10f.cpp
167

This is just a copy, right? This could be FPBits f = xbits;

lntue added inline comments.Feb 4 2022, 8:47 AM
libc/src/math/generic/log10f.cpp
167

Yes, it's just a copy.

Thanks for doing this. As you have mentioned in the description, please split this into independent patches.

libc/src/__support/FPUtil/FPBits.h
108–112

To be consistent with other getters, we might want to call this get_val.

libc/src/__support/FPUtil/x86_64/LongDoubleBits.h
99

While here, can we make these constructors constexpr?

libc/src/__support/builtin_bit_cast.h
22 ↗(On Diff #405972)

Few comments, some of them might just be bikeshed kind:

  1. Why should the name have builtin in it? For that matter, remove the llvmlibc_ prefix and put it in a namespace.
  2. constexpr instead of static?
  3. This should ideally be in __support/CPP/bit.h?
libc/src/__support/builtin_memcpy.h
21 ↗(On Diff #405972)

Seems like we can just inline this into llvmlibc_bit_cast?

libc/src/fenv/fesetexceptflag.cpp
30

Why is this required?

libc/utils/MPFRWrapper/MPFRUtils.cpp
486

Why was this block moved?

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.
gchatelet updated this revision to Diff 406350.Feb 7 2022, 1:36 AM
gchatelet marked 7 inline comments as done.
  • Address comments

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'
libc/src/__support/FPUtil/x86_64/sqrt.h
36–37

@lntue I had to use +t here for gcc to be happy.
https://stackoverflow.com/a/53680288

libc/src/fenv/fesetexceptflag.cpp
30

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)

libc/utils/MPFRWrapper/MPFRUtils.cpp
486

Explicit template instantiation cannot be declared within the class itself. It has to be defined outside of the class.

gchatelet updated this revision to Diff 406386.Feb 7 2022, 4:03 AM
  • Fix Bazel build

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'

This seems to be related to caching. This is now fixed.

gchatelet updated this revision to Diff 406394.Feb 7 2022, 4:21 AM
  • Fixed typo and documentation
lntue accepted this revision.Feb 7 2022, 6:39 AM
lntue added inline comments.
libc/src/__support/CPP/Bit.h
36–43

Nit: alignment

This revision is now accepted and ready to land.Feb 7 2022, 6:39 AM
gchatelet updated this revision to Diff 406451.Feb 7 2022, 7:07 AM
  • Fix wrong code alignment
gchatelet marked an inline comment as done.Feb 7 2022, 7:10 AM

Since most of the comments here have been fixed, I'll split the patch, one for each issue. We can continue the discussion on the individual patches.

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.

You have kind of provided GCC compilation as the motivation for your changes. My view is that, all of your changes are good even without GCC in picture. So, couple of things about this:

  1. We could add those flags by default when the compiler supports it. But if this has the potential to hinder the development cycle, for example it stops experimenting with type punning, then we should have a CMake flag to enable/disable the compiler flags. Iff we chose the route of providing a CMake flag, that is when my point about developer guidance becomes relevant.
  2. The CI should cover them of course, but it also means that developers should know how to reproduce the errors when they happen.
gchatelet abandoned this revision.Feb 10 2022, 1:34 AM

This patch has been broken down and submitted:

  • D119142: [libc][NFC] moving template specialization outside class declaration
  • D119143: [libc] Don't use Clang flags on other compilers
  • D119145: [libc] Replace type punning with bit_cast.
  • D119242: [libc] undefined reference in LibcTest.cpp.