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

[Kim Barrett]

> 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