On Mon, 4 Jan 2021 01:18:47 GMT, Hao Sun <github.com+16932759+shqk...@openjdk.org> wrote:
>>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >>> [build-dev](mailto:build-dev@openjdk.java.net):_ >>> >>> > On Dec 22, 2020, at 8:52 PM, Hao Sun <github.com+16932759+shqking at >>> > openjdk.java.net> wrote: >>> > 1. '-Wdeprecated-copy' >>> > As specified in C++11 [1], "the generation of the implicitly-defined >>> > copy constructor is deprecated if T has a user-defined destructor or >>> > user-defined copy assignment operator". The rationale behind is the >>> > well-known Rule of Three [2]. >>> > Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy' >>> > warns about the C++11 deprecation of implicitly declared copy >>> > constructor and assignment operator if one of them is user-provided. >>> > Defining an explicit copy constructor would suppress this warning. >>> > The main reason why debug build with gcc-9 or higher succeeds lies in >>> > the inconsistent warning behaviors between gcc and clang. See the >>> > reduced code example [5]. We suspect it might be return value >>> > optimization/copy elision [6] that drives gcc not to declare implicit >>> > copy constructor for this case. >>> >>> C++17 "guaranteed copy elision" is implemented in gcc7. >>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0135r1.html >>> >> >> Thanks for your comment. >> Initially we only suspected it might be copy elision that made gcc and clang >> to behave differently on this warning, and we were not aware of this flag >> '-fno-elide-constructors'. >> Thank you for pointing it out. >> >>> Using -fno-elide-constructors makes it obvious that the deprecated >>> implicit copy constructors are indeed being elided, so no deprecation >>> warning. >>> >> >> I suppose you want to express 'Using -**felide-constructors**' here. >> gcc with '-fno-elide-constructos' would produce derepcated warnings for this >> issue as clang-10 does. >> >>> Why doesn't this patch similarly define DUIterator copy constructor? >>> It seems to have the same issue, and I'm surprised clang-10 didn't >>> complain about it too. Certainly gcc with -fno-elide-constructors >>> complains about the defaulted implicit constructor. >>> >> >> I'm afraid we have noticed the same issue for 'DUIterator' before. >> Yes. It's weird that clang-10 didn't raise warning here. ( verified it on my >> local machine.) >> >>> But I discovered something alarming while experimenting. Building >>> with gcc10.2 with -fno-elide-constructors doesn't seem to be possible. >>> I get different kinds of failures depending on how DUIterator is >>> defined: >>> >>> - implict: deprecation warning (as expected) >>> - =delete: error, deleted function used >>> - =default: assert in os::free >>> - _idx and reset from that: assert in reset >>> >>> Without -fno-elide-constructors, all of the variants seem to work >>> except =delete, which still fails because the deleted function is >>> used. (I didn't test the "working" cases extensively though.) >>> >>> So there's something problematic, though I don't understand the code >>> well enough to understand what. >> >> Thanks for your tests. >> But I have no idea how to fix it right now either. >> Do you know anyone who is familiar with these code and maybe we can invite >> him/her to help take a look at this issue? >> Thanks. >> >>> >>> Interestingly, some of the complexity and weirdness around copy/assign >>> for these classes may be an attempt at providing something like move >>> semantics in a C++98 code base. I've noticed a number of places in >>> HotSpot that are doing that. >>> >>> Defining DUIterator_Fast and DUIterator_Last as movable but not >>> copyable (explicitly delete the copy constructor and copy assign >>> operator, and define the move constructor and move assign operator >>> with the reset) works, even with -fno-elide-constructors. > >> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >> [build-dev](mailto:build-dev@openjdk.java.net):_ >> >> > On Dec 24, 2020, at 3:44 PM, Xin Liu <xliu at openjdk.java.net> wrote: >> > On Thu, 24 Dec 2020 17:27:39 GMT, Kim Barrett <kbarrett at openjdk.org> >> > wrote: >> > > [?] >> > > Since DUIterator_Last is just delegating both the copy constructor and >> > > assignment operator to the base class, their copy constructor and >> > > assignment operator would be better as the default (either implicit or >> > > explicit using `=default`) rather than explicit code. >> > >> > >> > Agree. c2 actually never uses the assignment operator of DUIterator_Last. >> > It needs the copy constructor in this line. >> > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1499 >> > you can delete the following code or leave it =default. >> > - void operator=(const DUIterator_Last& that) >> > - { DUIterator_Fast::operator=(that); } >> >> DUIterator_Last::operator= *is* used, for example: >> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1473 >> >> That doesn't change whether defaulting DUIIterator_Last's copy constructor >> and copy assign works though (whether implicit or explicit). So making it >> implicit does work. >> >> I think making it explicitly defaulted might be better though, to make it >> clear the default behavior is intentional and it wasn't accidentally left as >> the implicit default. This is because the default isn't right for the other >> related classes. > > Yes. You're right. It's much better to make it explicitly defaulted. > May I have your opinion @navyxliu ? > _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on > [build-dev](mailto:build-dev@openjdk.java.net):_ > > > On Dec 29, 2020, at 10:33 PM, Hao Sun <github.com+16932759+shqking at > > openjdk.java.net> wrote: > > > _Mailing list message from [Kim Barrett](mailto:kim.barrett at > > > oracle.com) on [build-dev](mailto:build-dev at openjdk.java.net):_ > > > C++17 "guaranteed copy elision" is implemented in gcc7. > > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0135r1.html > > > > > > Thanks for your comment. > > Initially we only suspected it might be copy elision that made gcc and > > clang to behave differently on this warning, and we were not aware of this > > flag '-fno-elide-constructors'. > > Thank you for pointing it out. > > > Using -fno-elide-constructors makes it obvious that the deprecated > > > implicit copy constructors are indeed being elided, so no deprecation > > > warning. > > > > > > I suppose you want to express 'Using -**felide-constructors**' here. > > gcc with '-fno-elide-constructos' would produce derepcated warnings for > > this issue as clang-10 does. > > I really did mean "-fno-elide-constructors". The idea is that having the > normally working build fail with "-fno-elide-constructors" provides evidence > for the "working because of copy elision" conjecture. > > clang has supported -f[no-]elide-constructors for a while. > > It appears that either clang is different from gcc for -felide-constructors > (which seems unlikely), or clang (clang-10) is doing the deprecation warning > at a different point from gcc (quite plausible). That is, clang could be > checking for the deprecated case before eliding the call, while gcc is > checking for the deprecated case after copy elision has been applied. Thanks for your reply. I checked the source code of clang-10 and gcc-9 and found that: 1) for clang-10, 'Wdeprecated-copy' is implemented at the 'Sema' module of clang. See https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Sema/SemaDeclCXX.cpp#L13585 Flag 'felide-constructors' would enable 'felide_constructors' and flag 'fno-elide-constructors' would enables 'fno_elide_constructors'. (See https://github.com/llvm/llvm-project/blob/release/10.x/clang/include/clang/Driver/Options.td). Then 'ElideConstructors' will be set (See https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Frontend/CompilerInvocation.cpp#L2863) Finally, constructors might be elided in several spots in 'CodeGen' module. See: https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGStmt.cpp#L1094 https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGExprCXX.cpp#L611 https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGDecl.cpp#L1405 As far as I know, 'Sema' module is conducted to check semantics errors before 'CodeGen' module. That verified your conjecture, i.e. 'clang could be checking for the derepcated case before eliding the call'. 2) for gcc-9, 'felide-constructors' and 'Wdeprecated-copy' are implemented in a number of spots in gcc. I currently didn't figure out their execution order clearly. But in one of the use points at function build_over_call(), 'flag_elide_constructors' (See https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8538) is handled **before** 'warn_deprecated_copy' (See https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8608 and https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8679) Hope that my finding is helpful. Thanks. ------------- PR: https://git.openjdk.java.net/jdk/pull/1874
