Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
On Mon, 4 Jan 2021 06:22:46 GMT, Kim Barrett 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
>
> 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.
I suppose the constructor would be invoked before the copy assignment operator.
That is `_vdui` gets initialized already in the ctor `DUIterator_Fast()` for
`operator=` case. Right?
Yes. For our newly-added copy constructor for `DUIterator_Fast`, we should
initialize `_vdui` as `false`. It may be defined as below.
DUIterator_Fast(const DUIterator_Fast& that)
{ _outp = that._outp; debug_only(_vdui = false; reset(that));
}
-
PR: https://git.openjdk.java.net/jdk/pull/1874
Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
> On Jan 3, 2021, at 11:33 PM, Hao Sun > wrote: > > On Mon, 4 Jan 2021 01:18:47 GMT, Hao Sun > wrote: > _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on [build-dev](mailto:build-dev@openjdk.java.net):_ >> >> 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. Yes, that would explain the different warning behaviors. Thanks for digging through that.
Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
On Mon, 28 Dec 2020 10:28:09 GMT, Hao Sun
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
Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
On Mon, 4 Jan 2021 01:18:47 GMT, Hao Sun
wrote:
>>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on
>>> [build-dev](mailto:build-dev@openjdk.java.net):_
>>>
>>> > On Dec 22, 2020, at 8:52 PM, Hao Sun >> > 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.barr...@oracle.com) on
>> [build-dev](mailto:build-dev@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 defaultin
Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
On Wed, 30 Dec 2020 03:31:38 GMT, Hao Sun
wrote:
>> LGTM. It still needs other's approval.
>
>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on
>> [build-dev](mailto:build-dev@openjdk.java.net):_
>>
>> > On Dec 22, 2020, at 8:52 PM, Hao Sun > > 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.barr...@oracle.com) on
> [build-dev](mailto:build-dev@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
Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]
> On Jan 2, 2021, at 11:17 PM, Kim Barrett wrote: > >> On Dec 29, 2020, at 10:33 PM, Hao Sun >> wrote: >> >>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >>> [build-dev](mailto:build-dev@openjdk.java.net):_ >>> 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. > > I have a suspicion that the assert failure when building with > -fno-elide-constructors has nothing to do with DUIterator stuff, and is > instead a problem elsewhere. But it certainly makes it hard to feel > confident that the additional constructors being added are correct. > > I'm going to try to do some investigating of that assert failure, and see if > I can figure out what's going on. Anyone else should feel free to join in. > The failure is > > # Internal Error (../../src/hotspot/share/runtime/thread.hpp:850), > pid=29939, tid=29939 > # assert(current != __null) failed: Thread::current() called on detached > thread The assert failure when building with -fno-elide-constructors is a known issue: https://bugs.openjdk.java.net/browse/JDK-8234773 I thought it looked vaguely familiar, but it took a while to track down. I think I'm going to deal with it so nobody runs into it again.
