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

[Kim Barrett]

On Mon, 28 Dec 2020 10:28:09 GMT, Hao Sun <github.com+16932759+shqking at openjdk.org> 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.
>> 
>> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
>> warnings for deprecated defintions of copy constructors. However,
>> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
>> and clang-9 are not affected.
>> 
>> 2. '-Wimplicit-int-float-conversion'
>> Making the conversion explicit would fix it.
>> 
>> Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.
>> Therefore clang-8 and clang-9 are not affected. The flag with similar
>> functionality in gcc is '-Wfloat-conversion', but it is not enabled by
>> '-Wall' or '-Wextra'. That's why this warning does not apprear when
>> building with gcc.
>> 
>> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
>> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
>> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
>> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
>> [5] https://godbolt.org/z/err4jM
>> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
>> 
>> 
>> Note that we have tested with this patch, debug build succeeded with clang-10 on Linux X86/AArch64 machines.
>
> 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

src/hotspot/share/opto/node.hpp line 1396:

> 1394: 
> 1395:   DUIterator_Fast(const DUIterator_Fast& that)
> 1396:     { _outp = that._outp;               debug_only(reset(that)); }

`reset` tests `_vdui`, but nothing here has set it, so it's uninitialized and that test wil be UB.

I'm also not sure why it's okay for `operator=` to use whatever the current value of `_vdui` might be; that could leave `_last` as the old value rather than refreshing from `that`, which seems wrong.  This is aabout pre-existing code that looks questionable to me.

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

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