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

Mon Jan 4 04:33:55 UTC 2021

On Mon, 4 Jan 2021 01:18:47 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 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.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.

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

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