Currently gcc does not warn if I pass a non-POD to a varargs function. It would have been useful recently if there was an option to make gcc warn about this.
From https://gcc.gnu.org/gcc-4.5/changes.html: Diagnostics that used to complain about passing non-POD types to ... or jumping past the declaration of a non-POD variable now check for triviality rather than PODness, as per C++0x. See PR 37907. And having an example would be useful too. Because of the change between C++98 and C++11.
Jason informs me it's now a warning enabled by -Wconditionally-supported
Passing a non-POD to a varargs function is conditionally-supported, with implementation-defined semantics. In GCC 5 it's supported and treated like normal pass-by-value. You can get a diagnostic about it with -Wconditionally-supported.
Thanks, I'll give it a try. Here's my test case FWIW and a short demo showing what clang does: pokyo. cat q.cc #include <stdarg.h> class ConstUTF8CharsZ { const char *mData; public: ConstUTF8CharsZ() : mData(0) { } explicit ConstUTF8CharsZ(const char *aBytes) : mData(aBytes) { } const void *get() const { return mData; } const char *c_str() const { return mData; } operator bool() const { return mData != 0; } }; void zzz(const char *x, ...) { } void m(const char *m) { ConstUTF8CharsZ cu(m); zzz(m, cu); } pokyo. clang++ q.cc q.cc:30:10: error: cannot pass object of non-POD type 'ConstUTF8CharsZ' through variadic function; call will abort at runtime [-Wnon-pod-varargs] zzz(m, cu); ^ 1 error generated.
(In reply to Jason Merrill from comment #3) > Passing a non-POD to a varargs function is conditionally-supported, with > implementation-defined semantics. In GCC 5 it's supported and treated like > normal pass-by-value. You can get a diagnostic about it with > -Wconditionally-supported. I tried this today and it does not work with the test case in comment #4. pokyo. gcc --version gcc (GCC) 5.0.0 20150129 (experimental) Copyright (C) 2015 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. pokyo. g++ --std=c++11 --syntax-only -Wall -Wconditionally-supported q.cc pokyo.
Also, -Wconditionally-supported triggers other warning as well. It would be more convenient if different warnings were individually controllable; and then something like -Wconditionally-supported would act as if I'd specified all the sub-options.
(In reply to Tom Tromey from comment #5) > (In reply to Jason Merrill from comment #3) > > Passing a non-POD to a varargs function is conditionally-supported, with > > implementation-defined semantics. In GCC 5 it's supported and treated like > > normal pass-by-value. You can get a diagnostic about it with > > -Wconditionally-supported. > > I tried this today and it does not work with the test case in comment #4. > > pokyo. gcc --version > gcc (GCC) 5.0.0 20150129 (experimental) > Copyright (C) 2015 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > pokyo. g++ --std=c++11 --syntax-only -Wall -Wconditionally-supported q.cc > pokyo. Are you sure that class is not trivial which is why gcc is not warning about it? C++11 does not really have pod and non-pod any more but rather trivial and non-trivial and the rules has chnaged with respect of constructures.
See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2342.htm . It does look like gcc is implementing the correct new pod rules and clang is not.
> Are you sure that class is not trivial which is why gcc is not warning about > it? C++11 does not really have pod and non-pod any more but rather trivial > and non-trivial and the rules has chnaged with respect of constructures. I hesitate to say I'm sure :) However my belief is that because this class has a user-provided default constructor, it is not trivial. I tested this by adding "#include <type_traits>" and then static_assert(!std::is_trivial<ConstUTF8CharsZ>::value, "whoops"); Trivial or not, I'd like to get a warning here. In firefox varargs is really only used for printf-like things; but for various reasons there are some custom implementations which can't be marked with the "printf" attribute. So while doing a refactoring it turned out I inadvertently changed code to pass objects rather than char* to one of these ... but finding the problem locations is proving to be rather difficult.
FYI, g++ stopped warning on #c4 with r149721.
(In reply to Tom Tromey from comment #9) > However my belief is that because this class has a user-provided > default constructor, it is not trivial. True, but ... > I tested this by adding "#include <type_traits>" and then > > static_assert(!std::is_trivial<ConstUTF8CharsZ>::value, "whoops"); That's the wrong thing to assert: Passing a potentially-evaluated argument of class type (Clause 9) having a non-trivial copy constructor, a non-trivial move constructor, or a non-trivial destructor, with no corresponding parameter, is conditionally-supported with implementation-defined semantics. Your type has a trivial copy constructor and trivial move constructor and trivial destructor, so passing it to a varargs function is well-defined. Give it a non-trivial destructor and you get a warning: cs.cc: In function ‘void m(const char*)’: cs.cc:33:12: warning: passing objects of non-trivially-copyable type ‘class ConstUTF8CharsZ’ through ‘...’ is conditionally supported [-Wconditionally-supported] zzz(m, cu); ^
N.B. trivially-copyable is not the same as trivial (and there are separate type traits for testing those two properties).
(In reply to Jonathan Wakely from comment #11) > That's the wrong thing to assert: Aha, thank you very much. I obviously did not realize the difference :) Unfortunately I think even if I made this change I probably wouldn't want to enable -Wconditionally-supported here, as it enables some other warning that IIRC isn't desirable here. I ended up writing a gcc plugin to find places passing an aggregate to a varargs function. But I still think some simpler way to get a warning here would be nice.
FWIW Clang has this warning via -Wnon-pod-varargs.
*** Bug 67559 has been marked as a duplicate of this bug. ***
(In reply to John Steele Scott from comment #14) > FWIW Clang has this warning via -Wnon-pod-varargs. Confirmed, gcc having this warning would help avoid: http://gcc.gnu.org/ml/gcc-patches/2017-09/msg00126.html
This adds -Wnon-pod-varargs, enabled by -Wconditionally-supported, allowing e.g. -Wconditionally-supported -Werror=non-pod-varargs diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 3435fe90cca..b6ac29cda64 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -714,6 +714,10 @@ Wnamespaces C++ ObjC++ Var(warn_namespaces) Warning Warn on namespace definition. +Wnon-pod-varargs +C++ ObjC++ Var(warn_non_pod_varargs) Warning LangEnabledBy(C++ ObjC++,Wconditionally-supported) +Warn if passing a non-POD object through the \"...\" of a varargs function. + Wpacked-not-aligned C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn when fields in a struct with the packed attribute are misaligned. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 9e4a5c1b9ae..fc9f74f5968 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -7164,10 +7164,12 @@ convert_arg_to_ellipsis (tree arg, tsubst_flags_t complain) || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (arg_type))) { if (complain & tf_warning) - warning (OPT_Wconditionally_supported, - "passing objects of non-trivially-copyable " - "type %q#T through %<...%> is conditionally supported", - arg_type); + { + warning (OPT_Wnon_pod_varargs, + "passing objects of non-trivially-copyable " + "type %q#T through %<...%> is conditionally supported", + arg_type); + } return cp_build_addr_expr (arg, complain); } }
(In reply to Eric Gallager from comment #16) > (In reply to John Steele Scott from comment #14) > > FWIW Clang has this warning via -Wnon-pod-varargs. > > Confirmed, gcc having this warning would help avoid: > http://gcc.gnu.org/ml/gcc-patches/2017-09/msg00126.html Yeah. The situation is similar to the code in comment #4, which as Jakub says in comment #10 no longer produces a warning with g++ -Wconditionally-supported. I had to add a dummy destructor to get one.
Whether Clang warns or not depends on the -std mode active, as that decides the definition of a POD it uses. GCC always uses the C++11 rule, which I quoted in comment 9.
(In reply to Jonathan Wakely from comment #17) > This adds -Wnon-pod-varargs, enabled by -Wconditionally-supported, allowing > e.g. > -Wconditionally-supported -Werror=non-pod-varargs > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index 3435fe90cca..b6ac29cda64 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -714,6 +714,10 @@ Wnamespaces > C++ ObjC++ Var(warn_namespaces) Warning > Warn on namespace definition. > > +Wnon-pod-varargs > +C++ ObjC++ Var(warn_non_pod_varargs) Warning LangEnabledBy(C++ > ObjC++,Wconditionally-supported) > +Warn if passing a non-POD object through the \"...\" of a varargs function. > + > Wpacked-not-aligned > C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC > C++ ObjC++,Wall) > Warn when fields in a struct with the packed attribute are misaligned. > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > index 9e4a5c1b9ae..fc9f74f5968 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -7164,10 +7164,12 @@ convert_arg_to_ellipsis (tree arg, tsubst_flags_t > complain) > || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (arg_type))) > { > if (complain & tf_warning) > - warning (OPT_Wconditionally_supported, > - "passing objects of non-trivially-copyable " > - "type %q#T through %<...%> is conditionally supported", > - arg_type); > + { > + warning (OPT_Wnon_pod_varargs, > + "passing objects of non-trivially-copyable " > + "type %q#T through %<...%> is conditionally > supported", > + arg_type); > + } > return cp_build_addr_expr (arg, complain); > } > } Has this patch been posted to the gcc-patches mailing list yet?
No, because it doesn't have any tests. It should probably adjust: ../gcc/testsuite/g++.dg/overload/ellipsis1.C ../gcc/testsuite/g++.dg/overload/ellipsis2.C ../gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C to use the new flag, and add a test that the new flag is enabled by -Wconditionally-supported
(In reply to Jonathan Wakely from comment #21) > No, because it doesn't have any tests. > > It should probably adjust: > > ../gcc/testsuite/g++.dg/overload/ellipsis1.C > ../gcc/testsuite/g++.dg/overload/ellipsis2.C > ../gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C > > to use the new flag, and add a test that the new flag is enabled by > -Wconditionally-supported OK, I think I can do this.
(In reply to Eric Gallager from comment #22) > (In reply to Jonathan Wakely from comment #21) > > No, because it doesn't have any tests. > > > > It should probably adjust: > > > > ../gcc/testsuite/g++.dg/overload/ellipsis1.C > > ../gcc/testsuite/g++.dg/overload/ellipsis2.C > > ../gcc/testsuite/g++.old-deja/g++.pt/vaarg3.C > > > > to use the new flag, and add a test that the new flag is enabled by > > -Wconditionally-supported > > OK, I think I can do this. Actually never mind; I can't test because I'm blocked from bootstrapping by bug 83284; unassigning...
*** Bug 86562 has been marked as a duplicate of this bug. ***
retitling
Martin Sebor has been doing stuff related to warnings about POD-ness lately; cc-ing him
This should really be enabled by -Wall or -Wextra as it generates code that may easily crash or corrupt something. Clang even turns it into an error by default and inserts a trap to abort at runtime: https://godbolt.org/z/8DAUAr
(In reply to Trass3r from comment #27) > This should really be enabled by -Wall or -Wextra as it generates code that > may easily crash or corrupt something. But GCC supports this fully. That is why it is/was part of -Wconditionally-supported . The code if used correctly does not cause crashes or corrupts anything. In fact GCC documents this behavior: https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Conditionally-supported-behavior.html#Conditionally-supported-behavior :) > Clang even turns it into an error by default and inserts a trap to abort at > runtime: https://godbolt.org/z/8DAUAr But clang does not support this. Both compilers are correct; in that this feature is considered as conditionally supported with implementation defined behavior. So both compilers are prefectly ok in having their own choices.
Ah I see, ok.
(In reply to Trass3r from comment #27) > This should really be enabled by -Wall or -Wextra as it generates code that > may easily crash or corrupt something. +1. Clang errors, intel warns and for gcc you have to enable the somewhat obscure flag -Wconditionally-supported to see anything at all. gcc looks to be a trailer on this issue. It's not about standards conformance, its about making it easy for users to find bugs. Source code I used is: struct S { S(); S( const S &); ~S(); char * p; }; void f( int, ...); void g() { S s1; f( 3, s1); }; $ /home/dcb/llvm/results/bin/clang++ sep3f.cc sep3f.cc:19:8: error: cannot pass object of non-trivial type 'S' through variadic function; call will abort at runtime [-Wnon-pod-varargs] f( 3, s1); ^ 1 error generated. $ /home/dcb34/intel/oneapi/compiler/2021.3.0/linux/bin/intel64/icpc sep3f.cc sep3f.cc(19): warning #1595: a class type that is not trivially copyable passed through ellipsis f( 3, s1); ^ $ /home/dcb/gcc/results/bin/g++ -c -g -O2 -Wall -Wextra sep3f.cc $ /home/dcb/gcc/results/bin/g++ -c -g -O2 -Wall -Wextra -Wconditionally-supported sep3f.cc sep3f.cc: In function ‘void g()’: sep3f.cc:19:10: warning: passing objects of non-trivially-copyable type ‘struct S’ through ‘...’ is conditionally supported [-Wconditionally-supported] 19 | f( 3, s1); | ~^~~~~~~~ I'll add flag -Wconditionally-supported to a build of Fedora and see what happens.
(In reply to David Binderman from comment #30) > gcc looks to be a trailer on this issue. It's not about standards conformance, > its about making it easy for users to find bugs. But under GCC it's not a bug, it just works. Clang is welcome to adopt the same semantics. But I'm also happy to accept the patch in comment #17 once it includes testing. And GCC should build with -Wconditionally-supported so we support bootstrapping with compilers that make different choices.
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>: https://gcc.gnu.org/g:263c22a95bc9a0d80c4873c0291b0f938cea7310 commit r13-4795-g263c22a95bc9a0d80c4873c0291b0f938cea7310 Author: Jason Merrill <jason@redhat.com> Date: Mon Dec 5 10:00:31 2022 -0500 build: add -Wconditionally-supported to strict_warn [PR64867] The PR (which isn't resolved by this commit) pointed out to me that GCC should build with -Wconditionally-supported to support bootstrapping with a C++11 compiler that makes different choices. PR c++/64867 gcc/ChangeLog: * configure.ac (strict_warn): Add -Wconditionally-supported. * configure: Regenerate.
*** Bug 118435 has been marked as a duplicate of this bug. ***