Page MenuHomePhabricator

[libc++] use constexpr for sizeof comparisons
AbandonedPublic

Authored by ldionne on Jul 19 2019, 7:07 PM.

Details

Reviewers
EricWF
asomers
Summary

This fixes a spurious placement-new error from GCC.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70834

Sponsored by: The FreeBSD Foundation

Event Timeline

asomers created this revision.Jul 19 2019, 7:07 PM
asomers added a subscriber: dim.Jul 19 2019, 7:08 PM
ldionne requested changes to this revision.Jul 22 2019, 9:45 AM
ldionne added inline comments.
include/functional
1769–1773

The purpose of _LIBCPP_CONSTEXPR_AFTER_CXX14 isn't to be used that way, it's meant for Standard Library facilities that were marked constexpr in C++17. I think we should have a macro specifically for constructing an if constexpr when supported.

This revision now requires changes to proceed.Jul 22 2019, 9:45 AM
asomers marked an inline comment as done.Jul 22 2019, 11:01 AM
asomers added inline comments.
include/functional
1769–1773

How about something like this:

#ifdef __cpp_if_constexpr
#define __IF_CONSTEXPR constexpr
#else
#define __IF_CONSTEXPR
#endif
ldionne added inline comments.Jul 22 2019, 2:04 PM
include/functional
1769–1773

Yes, except it should be #define _LIBCPP_IF_CONSTEXPR. I'm also unsure about this whole thing: for anything that requires if constexpr for correctness, we can't use _LIBCPP_IF_CONSTEXPR (because it's required for correctness). So this will only be useful for cases where we want to use if constexpr instead of just if as a QOI matter, and it's not clear to me this is worth introducing a macro. So I'm kind of ambivalent between _LIBCPP_IF_CONSTEXPR and just having the #if __cpp_if_constexpr check right there in the code. Thoughts?

asomers updated this revision to Diff 211204.Jul 22 2019, 2:59 PM
asomers marked an inline comment as done.
  • Use __cpp_if_constexpr.
ldionne requested changes to this revision.Mar 25 2020, 8:43 AM

Looking at this again and the corresponding GCC bug report, I think we should either:

  • not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
  • disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.
This revision now requires changes to proceed.Mar 25 2020, 8:43 AM

Looking at this again and the corresponding GCC bug report, I think we should either:

  • not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
  • disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64. The only way I could get it to build was by adding -Wno-placement-new -Wno-attributes. See https://svnweb.freebsd.org/base/head/tests/sys/fs/fusefs/Makefile?r1=350162&r2=350163& . And while we could just disable that warning, that doesn't feel right, because the GCC people don't consider this a bug at all. They say "if constexpr is of course the right solution here, for certain".

include/functional
1769–1773

Whether or not to introduce a macro is strictly a stylistic matter, and you're the one whose opinion on style matters. I'll do it as you suggest.

Looking at this again and the corresponding GCC bug report, I think we should either:

  • not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
  • disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64.

Can you show me how you're building the file that produces a warning? Normally, our headers shouldn't bubble up warnings cause we use the #pragma system header on GCC. If that doesn't work, then either their warning doesn't listen to the pragma, or something else is happening. Either way, I'd like to understand before we change any code.

And while we could just disable that warning, that doesn't feel right, because the GCC people don't consider this a bug at all.

I agree that turning the warning off completely isn't the solution.

They say "if constexpr is of course the right solution here, for certain".

On the Bug report, the GCC folks don't say that, Matt Godbolt does. The GCC developer (Martin Sebor IIUC) says that the warning isn't smart enough at the moment, which is indeed what it looks to me.

Looking at this again and the corresponding GCC bug report, I think we should either:

  • not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
  • disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64.

Can you show me how you're building the file that produces a warning? Normally, our headers shouldn't bubble up warnings cause we use the #pragma system header on GCC. If that doesn't work, then either their warning doesn't listen to the pragma, or something else is happening. Either way, I'd like to understand before we change any code.

Here's the command line that built the offending file:

/usr/local/bin/riscv64-unknown-freebsd13.0-g++  --sysroot=/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp -B/usr/local/riscv64-unknown-freebsd13.0/bin/  -O2 -pipe -march=rv64imafdc -mabi=lp64d -g -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wredundant-decls -Wno-error=address -Wno-error=array-bounds -Wno-error=attributes -Wno-error=bool-compare -Wno-error=cast-align -Wno-error=clobbered -Wno-error=deprecated-declarations -Wno-error=enum-compare -Wno-error=extra -Wno-error=inline -Wno-error=logical-not-parentheses -Wno-error=strict-aliasing -Wno-error=uninitialized -Wno-error=unused-but-set-variable -Wno-error=unused-function -Wno-error=unused-value -Wno-error=nonnull-compare -Wno-error=shift-negative-value -Wno-error=tautological-compare -Wno-error=unused-const-variable -Wno-error=bool-operation -Wno-error=deprecated -Wno-error=expansion-to-defined -Wno-error=format-overflow -Wno-error=format-truncation -Wno-error=implicit-fallthrough -Wno-error=int-in-bool-context -Wno-error=memset-elt-size -Wno-error=noexcept-type -Wno-error=nonnull -Wno-error=pointer-compare -Wno-error=stringop-overflow -Wno-error=aggressive-loop-optimizations -Wno-error=cast-function-type -Wno-error=catch-value -Wno-error=multistatement-macros -Wno-error=restrict -Wno-error=sizeof-pointer-memaccess -Wno-error=stringop-truncation  -I/home/asomers/freebsd/base/head/tests -I/home/asomers/freebsd/base/head/sys/fs/fuse -I/home/asomers/freebsd/base/head/sbin/mount -I/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp/usr/include/private -std=c++14       -c /home/asomers/freebsd/base/head/tests/sys/fs/fusefs/mockfs.cc -o mockfs.o
... Some other warnings ...

/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp/usr/include/c++/v1/functional:1774:36: error: placement new constructing an object of type '_Fun' {aka 'std::__1::__function::__func<ReturnImmediate(std::__1::function<void(const mockfs_buf_in&, mockfs_buf_out&)>)::<lambda(auto:5&, auto:6&)>, std::__1::allocator<ReturnImmediate(std::__1::function<void(const mockfs_buf_in&, mockfs_buf_out&)>)::<lambda(auto:5&, auto:6&)> >, void(const mockfs_buf_in&, std::__1::vector<std::__1::unique_ptr<mockfs_buf_out> >&)>'} and size '64' in a region of type 'std::__1::aligned_storage<24>::type' and size '32' [-Werror=placement-new=]
                     ::new ((void*)&__buf_) _Fun(_VSTD::move(__f), _Alloc(__af));

Here's the command line that built the offending file:

[...]

You are asking for the warning by using -Wsystem-headers. I think we should either not do anything or push/pop an ignore pragma for the warning around the offending code.

dim added a comment.Mar 26 2020, 7:37 AM

FYI FreeBSD is one of the few OSes that enables -Wsystem-headers by default, *and* uses libc++ as its default C++ library. So we've already submitted a bunch of warning fixes for libc++.

That said, sometimes it is not possible to avoid warnings in libc++ headers, for example when providing deprecated features, but generally pushing and popping the warning state for this works well. Maybe we can use something similar for this.

ldionne commandeered this revision.Fri, May 15, 7:14 AM
ldionne edited reviewers, added: asomers; removed: ldionne.

I don't see us moving forward with this patch because of a warning in system headers that's not clever enough on GCC, so I'm going to abandon this to clean up the review queue.

ldionne abandoned this revision.Fri, May 15, 7:15 AM