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

[Hao Sun]

On Mon, 28 Dec 2020 19:23:34 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> Hao Sun has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove the unused assignment operator for DUIterator_Last
>>   
>>   Instead of adding the copy constructor, remove the assignment operator
>>   of DUIterator_Last since it's never used.
>>   
>>   Change-Id: Idf5658e38861eb2b0e724b064d17e9ab4e93905f
>>   CustomizedGitHooks: yes
>
> 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.

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

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