Bug 29034 – ARM compiler does not understand reinterpret_cast<void*>

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

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

[Laszlo Gombos]

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.

[Eric Seidel (no email)]

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?

[Laszlo Gombos]

(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

[Eric Seidel (no email)]

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.

[Laszlo Gombos]

(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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Sorry, I flipped those:
const_cast<void*>(reinterpret_cast <const void*>(value))

[Laszlo Gombos]

Adding Darin to the CC list. Darin can you help to review this patch ?

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Laszlo Gombos]

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.

[Darin Adler]

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.

[Laszlo Gombos]

Committed as http://trac.webkit.org/changeset/49509 with the changes suggested by Darin. Thanks for the review again.