Bug 29034 - ARM compiler does not understand reinterpret_cast<void*>
Summary: ARM compiler does not understand reinterpret_cast<void*>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-09-08 08:18 PDT by Laszlo Gombos
Modified: 2009-10-13 14:34 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch. (5.09 KB, patch)
2009-09-08 08:41 PDT, Laszlo Gombos
eric: review-
Details | Formatted Diff | Diff
2nd try (6.51 KB, patch)
2009-10-12 22:21 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-09-08 08:18:42 PDT
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);
                  ^
Comment 1 Laszlo Gombos 2009-09-08 08:41:53 PDT
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.
Comment 2 Eric Seidel (no email) 2009-09-08 09:24:27 PDT
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?
Comment 3 Laszlo Gombos 2009-09-08 10:04:47 PDT
(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
Comment 4 Eric Seidel (no email) 2009-09-08 10:20:00 PDT
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.
Comment 5 Laszlo Gombos 2009-09-08 10:30:58 PDT
(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
Comment 6 Eric Seidel (no email) 2009-09-08 10:33:43 PDT
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.
Comment 7 Eric Seidel (no email) 2009-09-08 10:34:24 PDT
Sorry, I flipped those:
const_cast<void*>(reinterpret_cast <const void*>(value))
Comment 8 Laszlo Gombos 2009-09-08 11:11:47 PDT
Adding Darin to the CC list. Darin can you help to review this patch ?
Comment 9 Darin Adler 2009-09-08 13:03:26 PDT
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 10 Eric Seidel (no email) 2009-09-08 14:47:34 PDT
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.
Comment 11 Laszlo Gombos 2009-10-12 22:21:28 PDT
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 12 Darin Adler 2009-10-13 08:54:19 PDT
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.
Comment 13 Laszlo Gombos 2009-10-13 14:34:01 PDT
Committed as http://trac.webkit.org/changeset/49509 with the changes suggested by Darin. Thanks for the review again.