Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258010: Debug build failure with clang-10 due to -Wdeprecated-copy #1874

Closed
wants to merge 5 commits into from

Conversation

@shqking
Copy link
Contributor

@shqking shqking commented Dec 23, 2020

  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-64/AArch64 machines.
Note that '--with-extra-cxxflags=-Wno-implicit-int-float-conversion' should be added when configuration. It's another issue (See JDK-8259288)


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258010: Debug build failure with clang-10 due to -Wdeprecated-copy

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1874/head:pull/1874
$ git checkout pull/1874

…nd -Wimplicit-int-float-conversion

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
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 23, 2020

👋 Welcome back shqking! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 23, 2020

@shqking The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot label Dec 23, 2020
@shqking
Copy link
Contributor Author

@shqking shqking commented Dec 23, 2020

/label add build

@openjdk openjdk bot added the build label Dec 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 23, 2020

@shqking
The build label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 23, 2020

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Dec 24, 2020

I think the two issues described here are distinct and should be dealt
with in separate bugs and PRs. Their only relation is that both arise
with using clang-10. But they are very different problems, in very
different parts of the code, and probably ought to be reviewed by
folks from different teams.

src/hotspot/share/opto/node.hpp Outdated Show resolved Hide resolved
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 24, 2020

Mailing list message from Kim Barrett on build-dev:

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

Using -fno-elide-constructors makes it obvious that the deprecated
implicit copy constructors are indeed being elided, so no deprecation
warning.

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.

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.

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.

@shqking
Copy link
Contributor Author

@shqking shqking commented Dec 25, 2020

I think the two issues described here are distinct and should be dealt
with in separate bugs and PRs. Their only relation is that both arise
with using clang-10. But they are very different problems, in very
different parts of the code, and probably ought to be reviewed by
folks from different teams.

Thanks for your comment.

Warning message of '-Wimplicit-int-float-conversion' was further encountered after we fixed the build failure caused by '-Wdeprecated-copy' first. That's why we put them in one PR initially.

Yes. Your way is much better. But we suppose the issue of '-Wimplicit-int-float-conversion' is trivial and putting them in separate PRs might raise another internal review process (for our side) by which extra time is needed. I was wondering could we continue in one single PR. :)

Instead of adding the copy constructor, remove the assignment operator
of DUIterator_Last since it's never used.

Change-Id: Idf5658e38861eb2b0e724b064d17e9ab4e93905f
CustomizedGitHooks: yes
Copy link
Member

@navyxliu navyxliu left a comment

LGTM. It still needs other's approval.

@shqking
Copy link
Contributor Author

@shqking shqking commented Dec 30, 2020

Mailing list message from Kim Barrett on build-dev:

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 2, 2021

Mailing list message from Kim Barrett on build-dev:

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 3, 2021

Mailing list message from Kim Barrett on build-dev:

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.

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.)

Yes, that?s weird.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 3, 2021

Mailing list message from Kim Barrett on build-dev:

On Jan 2, 2021, at 11:17 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

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):_
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.

@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 4, 2021

Mailing list message from Kim Barrett on build-dev:

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 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 ?

@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 4, 2021

Mailing list message from Kim Barrett on build-dev:

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'.

  1. 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.

@@ -1392,6 +1392,9 @@ class DUIterator_Fast : public DUIterator_Common {
DUIterator_Fast()
{ /*initialize to garbage*/ debug_only(_vdui = false); }

DUIterator_Fast(const DUIterator_Fast& that)
{ _outp = that._outp; debug_only(reset(that)); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@shqking shqking Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Take the following code snippet as an example.

DUIterator_Fast t1, t2;
t2 = t1;    // assignment operator
DUIterator_Fast t3 = t1;      // copy constructor. same as "t3(t1)"

My point is that, the ctor for t2 has already invoked, i.e. initializing _vdui as false. That's why operator= works well.

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)); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true on the first assignment of t2. But what if t2 is reassigned
to some other iterator. That assignment sees _vdui true, and keeps the old
value of _last rather than updating the value from that other iterator. Is
that really correct? It certainly seems strange. I'm trying to find someone
who understands this code to get involved, but holidays.

Copy link
Contributor Author

@shqking shqking Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation. Yes, you're right. I didn't realize the re-assignment scenario.

Copy link
Contributor Author

@shqking shqking Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vnkozlov I was wondering if you could take a look at this? We're not sure whether 'operator=' is problematic or not. Thanks.

Copy link
Contributor Author

@shqking shqking Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually checked the usages of assignment operators for class DUIterator, DUIterator_Fast and DUIterator_Last. (Simply grep the class names in the source code and check the context).

I found there exist only a couple of re-assignment usages. However, I guess kind of reset operations are conducted in these sites, and I suspect the implementation of operator= might be good.

As I examined, only the following 3 sites are found where re-assignment happens
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L565
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/phaseX.cpp#L1878
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/split_if.cpp#L452

Please take cfgnode.cpp as an example. j is first assigned at line 568 and it would be assigned again if flag progress is true. However, I suppose j gets reset at line 578 in such case. Note that j might be re-assigned at line 578. However, it's self assignment and nothing is conducted.

It might be incorrect if I missed something.
Hope that this finding would be helpful to analyze this problem.

Copy link
Contributor

@vnkozlov vnkozlov Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is correct. _last should not be updated with additional assignments which updates only node's information.

Copy link
Contributor Author

@shqking shqking Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your explanation @vnkozlov .

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 4, 2021

Mailing list message from Kim Barrett on build-dev:

On Jan 3, 2021, at 11:33 PM, Hao Sun <github.com+16932759+shqking at openjdk.java.net> wrote:

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):_

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.

@navyxliu
Copy link
Member

@navyxliu navyxliu commented Jan 4, 2021

Mailing list message from Kim Barrett on build-dev:

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 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 ?
Sorry, I overlooked that copy assignment case.
Making it explicitly default is better to me too. no objection!

…d DUIterator_Last

1. Update copyright year to 2021.
2. Add the definition of copy constructor for class DUIterator.
Otherwise, gcc with '-fno-elide-constructors' would raise a warning.
3. For the copy constructor of class DUIterator_Fast, we initialize
'_vdui' as false, otherwise UB is introduced.
4. It's better to define the copy constructor of class DUIterator_Last
as explicitly-defaulted, instead of leaving it for compilers to
implicitly define.

Change-Id: I3d2f5b396aa116d1832f52da361ff3172459a87e
CustomizedGitHooks: yes
@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 5, 2021

Thanks for your comments. @kimbarrett and @navyxliu
I updated the patch based on my understanding. Please check the latest commit.

As @kimbarrett mentioned, I suppose there still exist the following problems to be addressed.

  1. why clang-10 with '-Wdeprecated-copy' does NOT raise warning for class DUIterator. It's weird.
  2. the assert failure when building with gcc '-fno-elide-constructors'. Might not be related to our patch. (JDK-8259036)
  3. the implementation of 'operator=' for class DUIterator_Fast might be problematic.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

node.hpp changes seems fine.
Passed tier1 builds and testing.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 5, 2021

@shqking This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8258010: Debug build failure with clang-10 due to -Wdeprecated-copy

Reviewed-by: xliu, kvn, kbarrett

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 177 new commits pushed to the master branch:

  • e4df209: 7018932: Drawing very large coordinates with a dashed Stroke can cause Java to hang
  • 5f7ccce: 8226810: Failed to launch JVM because of NullPointerException occured on System.props
  • d6a2105: 8259343: [macOS] Update JNI error handling in Cocoa code.
  • c338f11: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right
  • ccac7aa: 8258932: AArch64: Enhance floating-point Min/MaxReductionV with fminp/fmaxp
  • 4c75d14: 8259374: Make ThreadInVMfromNative have ResetNoHandleMark
  • 563b268: 8257709: C1: Double assignment in InstructionPrinter::print_stack
  • 400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • ... and 167 more: https://git.openjdk.java.net/jdk/compare/4ea88512ddb89470ff5a043bc1865b9e4af661d6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @kimbarrett) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 5, 2021
@shqking shqking changed the title 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy Jan 6, 2021
@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 6, 2021

I think the two issues described here are distinct and should be dealt
with in separate bugs and PRs. Their only relation is that both arise
with using clang-10. But they are very different problems, in very
different parts of the code, and probably ought to be reviewed by
folks from different teams.

Thanks for your comment.

Warning message of '-Wimplicit-int-float-conversion' was further encountered after we fixed the build failure caused by '-Wdeprecated-copy' first. That's why we put them in one PR initially.

Yes. Your way is much better. But we suppose the issue of '-Wimplicit-int-float-conversion' is trivial and putting them in separate PRs might raise another internal review process (for our side) by which extra time is needed. I was wondering could we continue in one single PR. :)

Will split this PR.
In this PR, we focus on the warnings caused by -Wdeprecated-copy.
Will update the code soon. Will create a new PR to address JDK-8259288.

As suggested by kimbarrett, we should focus on warnings produced by
'-Wdeprecated-copy' in this PR. Because JDK-8259288 is a very different
problem and might be reviewed by folks from different teams.

Will create a new PR to address JDK-8259288.

Change-Id: I1b9f434ab6fcdf2763a46870eaed91641984fd76
CustomizedGitHooks: yes
@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 6, 2021

Thanks for your comments. @kimbarrett and @navyxliu
I updated the patch based on my understanding. Please check the latest commit.

As @kimbarrett mentioned, I suppose there still exist the following problems to be addressed.

  1. why clang-10 with '-Wdeprecated-copy' does NOT raise warning for class DUIterator. It's weird.
  2. the assert failure when building with gcc '-fno-elide-constructors'. Might not be related to our patch. (JDK-8259036)
  3. the implementation of 'operator=' for class DUIterator_Fast might be problematic.

@kimbarrett
Regarding problem 1, I believe this is because there exists use-defined dtor for class DUIterator and the deprecated copy ctor warning message ought to be emitted by '-Wdeprecated-copy-dtor' (which is not enabled by '-Wall' or '-Wextra').

Please refer to https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Sema/SemaDeclCXX.cpp#L13585
The main logic is user-defined dtor/copy constructor/assignment operator is checked in order when diagnosing implicit-defined copy constructor, and warning message will be emitted (lines 13619 to 13625).

In this case of class DUIterator, user-defined dtor is provided but '-Wdeprecated-copy-dtor' is not enabled, clang-10 does not further check whether '-Wdeprecated-copy' is on or not.

I further checked

  1. I built openjdk with "--with-extra-cxxflags='-Wdeprecated-copy-dtor'", but the building failed earlier before class DUIterator.
  2. I removed the dtor of class DUIterator manually and built openjdk with clang-10. Then clang-10 would produce the warning as we expected, which I think verified my conjecture.
=== Output from failing command(s) repeated here ===
* For target hotspot_variant-server_libjvm_gtest_objs_test_mathexact.o:
In file included from /home/haosun01/jdk/jdk_src/test/hotspot/gtest/opto/test_mathexact.cpp:26:
In file included from /home/haosun01/jdk/jdk_src/src/hotspot/share/opto/mulnode.hpp:28:
/home/haosun01/jdk/jdk_src/src/hotspot/share/opto/node.hpp:1342:8: error: definition of implicit copy constructor for 'DUIterator' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const DUIterator& that)
       ^
/home/haosun01/jdk/jdk_src/src/hotspot/share/opto/node.hpp:1347:12: note: in implicit copy constructor for 'DUIterator' first required here
  { return DUIterator(this, 0); }
           ^
/home/haosun01/jdk/jdk_src/src/hotspot/share/opto/node.hpp:1410:8: error: definition of implicit copy constructor for 'DUIterator_Fast' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const DUIterator_Fast& that)
       ^
/home/haosun01/jdk/jdk_src/src/hotspot/share/opto/node.hpp:1418:10: note: in implicit copy constructor for 'DUIterator_Fast' first required here
  return DUIterator_Fast(this, 0);
         ^
/home/haosun01/jdk/jdk_src/src/hotspot/share/opto/node.hpp:1467:8: error: definition of implicit copy constructor for 'DUIterator_Last' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
   ... (rest of output omitted)
* For target support_gensrc_jdk.localedata__cldr-gensrc.marker:

* All command lines available in /home/haosun01/tmp/deprecated-copy/clang10-fast-build/make-support/failure-logs.
=== End of repeated output ===

@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 7, 2021

Thanks for your comments. @kimbarrett and @navyxliu
I updated the patch based on my understanding. Please check the latest commit.

As @kimbarrett mentioned, I suppose there still exist the following problems to be addressed.

  1. why clang-10 with '-Wdeprecated-copy' does NOT raise warning for class DUIterator. It's weird.
  2. the assert failure when building with gcc '-fno-elide-constructors'. Might not be related to our patch. (JDK-8259036)
  3. the implementation of 'operator=' for class DUIterator_Fast might be problematic.

For problem 1, it's due to '-Wdeprecated-copy-dtor'. Please refer to my previous comment for details.
For problem 2, I suppose it's not related to our patch and it would be issued in JDK-8259036.
For problem 3, as clarified by @vnkozlov , the code is correct.

Besides, the pre-submit tests failed due to one GC problem, which seems not related to this patch. See JDK-8258481.

Hence, I wonder if this patch is ready to merge.
Could you please take another round of review as new commits have been made? @kimbarrett @navyxliu @vnkozlov
Thanks.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Still good.

Copy link

@kimbarrett kimbarrett left a comment

[Can't comment on this inline.] I'd prefer DUIterator_Last::operator= be changed to =default, for consistency with the copy constructor. That would require fixing the return type too.

@@ -1452,6 +1458,8 @@ class DUIterator_Last : private DUIterator_Fast {
DUIterator_Last() { }
// initialize to garbage

DUIterator_Last (const DUIterator_Last& that) = default;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - remove space before parameter list.

Copy link
Contributor Author

@shqking shqking Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for point it out. Will remove.

@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 9, 2021

[Can't comment on this inline.] I'd prefer DUIterator_Last::operator= be changed to =default, for consistency with the copy constructor. That would require fixing the return type too.

Thanks for your comment. Agree. Will update the code.

The copy assignment operator of class DUIterator_Last should also be
defined as defaulted, i.e. =default, keeping consistent with the copy
constructor.

Besides, fix the NIT for the copy ctor definition of class
DUIterator_Last.

Change-Id: I2f9502f023443163910eea9469b72df5bf1e25e0
CustomizedGitHooks: yes
@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 12, 2021

@vnkozlov would you mind helping to review the latest commit (again)? Really appreciate it.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good.

@shqking
Copy link
Contributor Author

@shqking shqking commented Jan 12, 2021

Thanks a lot for your review @kimbarrett @vnkozlov .

/integrate

@openjdk openjdk bot added the sponsor label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@shqking
Your change (at version 5c8195b) is now ready to be sponsored by a Committer.

@nsjian
Copy link

@nsjian nsjian commented Jan 14, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jan 14, 2021

@nsjian @shqking Since your change was applied there have been 214 commits pushed to the master branch:

  • 51e14f2: Merge
  • 5926d75: 8259719: ProblemList runtime/cds/appcds/jigsaw/modulepath/ModulePathAndCP_JFR.java on Windows
  • 8abefde: 8259720: ProblemList java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest1.java on Windows
  • fb8ac24: 8259722: ProblemList two jdk/jfr/startupargs tests on Windows
  • ac4cd2e: 8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference
  • 42d2d6d: 8259063: Possible deadlock with vtable/itable creation vs concurrent class unloading
  • 6bb6093: 8259657: typo in generated HELP page prevents localization
  • 5567530: 8258272: LoadVectorMaskedNode can't be replaced by zero con
  • a99df45: 8259560: Zero m68k: "static assertion failed: align" after JDK-8252049
  • efc36be: 8258985: Parallel WeakProcessor may use too few threads
  • ... and 204 more: https://git.openjdk.java.net/jdk/compare/4ea88512ddb89470ff5a043bc1865b9e4af661d6...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5513f98.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@shqking shqking deleted the deprecated-copy branch Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants