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

[Hao Sun]

On Wed, 30 Dec 2020 03:31:38 GMT, Hao Sun <github.com+16932759+shqking at openjdk.org> wrote:

>> LGTM. It still needs other's approval.
>
>> _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 ?

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

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