Bug 223916 – [JSC] Remove warnings about unnecessary operator= for ARMv7Assembler LinkRecord

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Xan Lopez]

GCC (10.2.1) complains very often and very verbosely like this:

In file included from WTF/Headers/wtf/RedBlackTree.h:33,
                 from WTF/Headers/wtf/MetaAllocatorHandle.h:33,
                 from ../../Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:30,
                 from ../../Source/JavaScriptCore/interpreter/AbstractPC.h:28,
                 from ../../Source/JavaScriptCore/interpreter/CallFrame.h:25,
                 from ../../Source/JavaScriptCore/API/JSAPIValueWrapper.h:25,
                 from ../../Source/JavaScriptCore/API/APICast.h:29,
                 from ../../Source/JavaScriptCore/API/tests/testapi.cpp:28:
WTF/Headers/wtf/Vector.h: In instantiation of 'bool WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::appendSlowCase(U&&) [with WTF::FailureAction <anonymous> = WTF::FailureAction::Crash; U = JSC::ARMv7Assembler::LinkRecord; T = JSC::ARMv7Assembler::LinkRecord; unsigned int inlineCapacity = 0; OverflowHandler = WTF::UnsafeVectorOverflow; unsigned int minCapacity = 16; Malloc = WTF::FastMalloc]':
WTF/Headers/wtf/Vector.h:1295:37:   required from 'bool WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::append(U&&) [with WTF::FailureAction <anonymous> = WTF::FailureAction::Crash; U = JSC::ARMv7Assembler::LinkRecord; T = JSC::ARMv7Assembler::LinkRecord; unsigned int inlineCapacity = 0; OverflowHandler = WTF::UnsafeVectorOverflow; unsigned int minCapacity = 16; Malloc = WTF::FastMalloc]'
WTF/Headers/wtf/Vector.h:776:92:   required from 'void WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::append(U&&) [with U = JSC::ARMv7Assembler::LinkRecord; T = JSC::ARMv7Assembler::LinkRecord; unsigned int inlineCapacity = 0; OverflowHandler = WTF::UnsafeVectorOverflow; unsigned int minCapacity = 16; Malloc = WTF::FastMalloc]'
WTF/Headers/wtf/Vector.h:775:69:   required from 'void WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::append(WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::ValueType&&) [with T = JSC::ARMv7Assembler::LinkRecord; unsigned int inlineCapacity = 0; OverflowHandler = WTF::UnsafeVectorOverflow; unsigned int minCapacity = 16; Malloc = WTF::FastMalloc; WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous>, Malloc>::ValueType = JSC::ARMv7Assembler::LinkRecord]'
../../Source/JavaScriptCore/assembler/ARMv7Assembler.h:2198:85:   required from here
WTF/Headers/wtf/Vector.h:1328:5: warning: implicitly-declared 'constexpr JSC::ARMv7Assembler::LinkRecord::LinkRecord(const JSC::ARMv7Assembler::LinkRecord&)' is deprecated [-Wdeprecated-copy]
 1328 |     new (NotNull, end()) T(std::forward<U>(*ptr));
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:31,
                 from ../../Source/JavaScriptCore/assembler/MacroAssembler.h:35,
                 from ../../Source/JavaScriptCore/jit/GPRInfo.h:28,
                 from ../../Source/JavaScriptCore/jit/RegisterSet.h:30,
                 from ../../Source/JavaScriptCore/jit/JITCode.h:33,
                 from ../../Source/JavaScriptCore/runtime/ExecutableBase.h:32,
                 from ../../Source/JavaScriptCore/runtime/ExecutableBaseInlines.h:28,
                 from ../../Source/JavaScriptCore/bytecode/CallVariant.h:28,
                 from ../../Source/JavaScriptCore/bytecode/CallEdge.h:28,
                 from ../../Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.h:30,
                 from ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:31,
                 from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34,
                 from ../../Source/JavaScriptCore/interpreter/RegisterInlines.h:28,
                 from ../../Source/JavaScriptCore/interpreter/CallFrameInlines.h:31,
                 from ../../Source/JavaScriptCore/runtime/JSCellInlines.h:32,
                 from ../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h:35,
                 from ../../Source/JavaScriptCore/API/APICast.h:31,
                 from ../../Source/JavaScriptCore/API/tests/testapi.cpp:28:
../../Source/JavaScriptCore/assembler/ARMv7Assembler.h:449:14: note: because 'JSC::ARMv7Assembler::LinkRecord' has user-provided 'void JSC::ARMv7Assembler::LinkRecord::operator=(const JSC::ARMv7Assembler::LinkRecord&)'
  449 |         void operator=(const LinkRecord& other)
      |              ^~~~~~~~


The implicit copy constructor will just copy the memory representation for union types (like LinkRecord), which seems adequate in this case. So just remove the user provided copy constructor.

[Xan Lopez]

Created attachment 424622 [details]
Remove user-provided copy-ctor, v1

v1

[Darin Adler]

Comment on attachment 424622 [details]
Remove user-provided copy-ctor, v1

If you are going to remove this then I think you need to do more:

1) Research why it was added, since you are claiming it’s not needed. Let’s make sure we don’t lose the context of why someone thought they needed it. Verify they were mistaken or that something has changed.

2) Remove the unused CopyTypes type and copyTypes data member and remove the use of the union entirely since this is the only place it was used.

If you want to take the GCC warning literally, you could add a copy constructor that uses copyTypes instead of removing the assignment operator.

[Xan Lopez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 424622 [details]
> Remove user-provided copy-ctor, v1
> 
> If you are going to remove this then I think you need to do more:
> 
> 1) Research why it was added, since you are claiming it’s not needed. Let’s
> make sure we don’t lose the context of why someone thought they needed it.
> Verify they were mistaken or that something has changed.

Thank you, this was actually the right thing to do first. This was introduced way back in 2012 as an optimization, the claim being it's a significant speed-up over the compiler-provided default. See https://bugs.webkit.org/show_bug.cgi?id=90930

I think this may very well be still the case, perhaps I should try to measure it again, but then I'd say that we at least:

- Should add a comment about it, with a link to the bug.
- Find a way to shut up GCC about it, maybe by doing what you suggest in the last comment.
- If we leave the operator in place, maybe we might want to fix the operator (at least) in ARMv7, since it's using a 'void' return type instead of the more common/correct reference type? The version in the ARM64 assembler file is fine.

> 
> 2) Remove the unused CopyTypes type and copyTypes data member and remove the
> use of the union entirely since this is the only place it was used.
> 
> If you want to take the GCC warning literally, you could add a copy
> constructor that uses copyTypes instead of removing the assignment operator.

[Darin Adler]

I think we should interpret GCC's warning as "if you had a reason to override operator= then you also need to override copy constructor for the same reason", which seems correct!

[Xan Lopez]

(In reply to Darin Adler from comment #4)
> I think we should interpret GCC's warning as "if you had a reason to
> override operator= then you also need to override copy constructor for the
> same reason", which seems correct!

That makes sense to me, I was thrown off by the doc:

       -Wdeprecated-copy (C++ and Objective-C++ only)
           Warn that the implicit declaration of a copy constructor or copy assignment operator is deprecated if the class has a user-provided copy constructor or copy
           assignment operator, in C++11 and up.  This warning is enabled by -Wextra.  With -Wdeprecated-copy-dtor, also deprecate if the class has a user-provided
           destructor.

I can read that as saying we'd still be warned if any, *or both*, are present? I get now it can be read as you suggest (if you define one, you almost surely want to define the other), but the language is a bit confusing.

[Darin Adler]

Yes, I agree their wording is confusing, confusingly repetitive, but I’m pretty sure my reading of it is correct: "user-provided for both or neither".

[Xan Lopez]

Created attachment 424766 [details]
Remove warnings for LinkRecord, v2

v2

[Darin Adler]

Comment on attachment 424766 [details]
Remove warnings for LinkRecord, v2

I would say, "not as fast" rather than "not optimal". I’m not sure this implementation is "optimal".

[EWS]

Committed r275285: <https://commits.webkit.org/r275285>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424766 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76055800>