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

[Xin Liu]

On Mon, 4 Jan 2021 04:31:02 GMT, Hao Sun <github.com+16932759+shqking 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 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.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!

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

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