RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Hao Sun]

On Mon, 4 Jan 2021 19:33:45 GMT, Xin Liu <xliu at openjdk.org> wrote:

>>> _Mailing list message from [Kim Barrett](mailto:kim.barrett at oracle.com) on [build-dev](mailto:build-dev at 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.
>
>> > _Mailing list message from [Kim Barrett](mailto:kim.barrett at oracle.com) on [build-dev](mailto:build-dev at openjdk.java.net):_
>> > > On Dec 24, 2020, at 3:44 PM, Xin Liu  wrote:
>> > > On Thu, 24 Dec 2020 17:27:39 GMT, Kim Barrett  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 ?
> Sorry, I overlooked that copy assignment case.
> Making it explicitly default is better to me too. no objection!

Thanks for your comments. @kimbarrett and @navyxliu 
I updated the patch based on my understanding. Please check the latest commit.

As @kimbarrett mentioned, I suppose there still exist the following problems to be address.
1) why clang-10 with '-Wdeprecated-copy' does NOT raise warning for class DUIterator. It's weird.
2) the assert failure when building with gcc '-fno-elide-constructors'. Might not be related to our patch.
3) the implementation of 'operator=' for class DUIterator_Fast might be problematic.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1874