The error message is the following: armcc [...] jitopcodes.cpp "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #694: reinterpret_cast cannot cast away const or other type qualifiers move(ImmPtr(reinterpret_cast<void*>(ctiVMThrowTrampoline)), regT2); ^ Changing reinterpret_cast to static_cast does not help either... armcc [...] jitopcodes.cpp "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #171: invalid type conversion move(ImmPtr(static_cast<void*>(ctiVMThrowTrampoline)), regT2); ^
Created attachment 39190 [details] proposed patch. Change reinterpret_cast<void*> to regular C style (void*) cast. I share the view that reinterpret_cast is more descriptive, however when it comes to casting to void* I'm less certain about the difference.
reinterpret_cast cannot cast away const or other type qualifiers Did you try casting to <const void*> to see if that made the compiler happy?
(In reply to comment #2) > reinterpret_cast cannot cast away const or other type qualifiers > > Did you try casting to <const void*> to see if that made the compiler happy? Eric casting it to const void* does not help in fact it just makes it worse. The armcc error message is confusing. Here are the build logs from casting to const void*: armcc [...] jitopcodes.cpp "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #694: reinterpret_cast cannot cast away const or other type qualifiers move(ImmPtr(reinterpret_cast<const void*>(ctiVMThrowTrampoline)), regT2); ^ "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #289: no instance of constructor "JSC::AbstractMacroAssembler<AssemblerType>::ImmPtr::ImmPtr [with AssemblerType=JSC::ARMAssembler]" matches the argument list argument types are: (const void *) move(ImmPtr(reinterpret_cast<const void*>(ctiVMThrowTrampoline)), regT2); ^ \webkit\JavaScriptCore\jit\jitopcodes.cpp: 0 warnings, 2 errors armcc [...] jitopcodes.cpp "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #171: invalid ty pe conversion move(ImmPtr(static_cast<const void*>(ctiVMThrowTrampoline)), regT2); ^ \webkit\JavaScriptCore\jit\jitopcodes.cpp: 0 warnings, 1 error
Sure, so maybe you'd have to use const_cast in addition to reinterpret_cast. You'd have to ask someone with more c++ knowledge than I. You could certainly try using both and seeing if that makes the compiler happy.
(In reply to comment #4) > Sure, so maybe you'd have to use const_cast in addition to reinterpret_cast. > You'd have to ask someone with more c++ knowledge than I. You could certainly > try using both and seeing if that makes the compiler happy. Eric, thanks for the reviews ! Given that the regular C style (const void*) cast fails - but cast to (void*) succeeds - , I belie we want to cast it to (void*) - and not to const - regardless of what armcc complains about. armcc [...] jitopcodes.cpp "\webkit\JavaScriptCore\jit\jitopcodes.cpp", line 1751: Error: #289: no instance of constructor "JSC::AbstractMacroAssembler<AssemblerType>::ImmPtr::ImmPtr [with AssemblerType=JSC::ARMAssembler]" matches the argument list argument types are: (const void *) move(ImmPtr((const void*)(ctiVMThrowTrampoline)), regT2); ^ \webkit\JavaScriptCore\jit\jitopcodes.cpp: 0 warnings, 1 error
Example of what I mean: reinterpret_cast<void*>(const_cast<const void*>(value)) Again, you'd have to ask a c++ expert if that really should be necessary.
Sorry, I flipped those: const_cast<void*>(reinterpret_cast <const void*>(value))
Adding Darin to the CC list. Darin can you help to review this patch ?
Comment on attachment 39190 [details] proposed patch. These are all casts that convert a function pointer into a void*. It's not really a reinterpret_cast issue, but rather the need for a way to convert function pointers to a single type (specifically, void*, I guess, but it might work to use some generic function pointer type instead). I'd prefer that we come up with a way of doing this other than sprinkling the C-style casts in -- something easier to search for later. Maybe a template function like this: template <typename T> void* function_pointer_cast(T functionPointer) { return (void*)functionPointer; } And then later we could change it to work only with function pointers if we can get that to happen. Not sure about the name, but I'd like to avoid having to modify so many different sites to deal with this.
Comment on attachment 39190 [details] proposed patch. r- based on Darin's comment above. Introduction of a function to do this would be a better solution.
Created attachment 41086 [details] 2nd try Darin, Eric thanks for the feedback. It seems to me that existing FunctionPtr class in assembler/MacroAssemblerCodeRef.h could be the single place where we could special-case for RVCT, instead of sprinkling the C-style casts all over - as Darin suggested. SunSpider reports no change in JIT mode.
Comment on attachment 41086 [details] 2nd try Looks much better! > + Change reinterpret_cast<void*> to regular C style (void*) cast > + for the ARM RVCT compiler. > + > + * assembler/MacroAssemblerCodeRef.h: > + (JSC::FunctionPtr::FunctionPtr): > + * jit/JITOpcodes.cpp: > + * jit/JITStubCall.h: > + (JSC::JITStubCall::JITStubCall): > + * jit/JITStubs.cpp: > + (JSC::DEFINE_STUB_FUNCTION): I believe this patch does not change the DEFINE_STUB_FUNCTION macro, nor is a macro something that's inside a namespace, so this is wrong. Please keep in mind that prepare-ChangeLog helps you write the change log, but you need to read it over and correct it if it's wrong. I think that per-file and per-function comments help make things clearer and I wish you would write those. > +#if COMPILER(RVCT) > + : m_value((void*)(value)) > +#else > : m_value(reinterpret_cast<void*>(value)) > +#endif This needs a comment. It's not obvious why the compiler is relevant here. > - move(ImmPtr(reinterpret_cast<void*>(ctiVMThrowTrampoline)), regT2); > + move(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value()), regT2); I'm not sure I understand why you used value() here and executableAddress() elsewhere. > #include <wtf/Platform.h> > > +#include "MacroAssemblerCodeRef.h" Since MacroAssemblerCodeRef.h already includes Platform.h you should replace the existing include with it rather than keeping the old include around. > - m_jit->m_calls.append(CallRecord(call, m_jit->m_bytecodeIndex, m_stub)); > + m_jit->m_calls.append(CallRecord(call, m_jit->m_bytecodeIndex, m_stub.value())); Same comment here. I'm going to say review+ with this, but it might be better to address the issues I mentioned first instead of landing it as-is.
Committed as http://trac.webkit.org/changeset/49509 with the changes suggested by Darin. Thanks for the review again.