Bug 129786 – [GTK] Only build the Udis86 disassembler with the CMake build and when explicitly enabled

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Zan Dobersek]

[GTK] Only build the Udis86 disassembler with the CMake build and when explicitly enabled

[Zan Dobersek]

Created attachment 225961 [details]
Patch

[Martin Robinson]

Comment on attachment 225961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225961&action=review

So looking at this it seems like it isn't even a configurable feature for the Mac port. Instead it appears that Platform.h just enables it unconditionally if the platform is one that supports the diassembler. From what you've said about the disassembler, that might be enough for us.

> Source/WTF/wtf/Platform.h:671
> +#if !defined(WTF_USE_UDIS86) && ENABLE(JIT) && ((OS(DARWIN) && !PLATFORM(EFL) && !PLATFORM(GTK)) || (PLATFORM(EFL) && OS(LINUX))) \

It looks like this

> Source/cmake/OptionsGTK.cmake:255
> +if (WTF_USE_UDIS86)
> +    add_definitions(-DWTF_USE_UDIS86)
> +endif ()
> +

It's probably better to use the cmakeconfig.h instead of adding a new command-line argument to the linker.

> Tools/Scripts/webkitdirs.pm:1999
> +    push @args, $ENV{"WebKitCMakeArguments"} if $ENV{"WebKitCMakeArguments"};

Additionally arguments can be passed to build-webkit directly, I believe. Failing that the right thing to do is to add an advanced CMake configuration variable, I believe.

[Zan Dobersek]

Comment on attachment 225961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225961&action=review

>> Source/cmake/OptionsGTK.cmake:255
>> +
> 
> It's probably better to use the cmakeconfig.h instead of adding a new command-line argument to the linker.

Agreed.

>> Tools/Scripts/webkitdirs.pm:1999
>> +    push @args, $ENV{"WebKitCMakeArguments"} if $ENV{"WebKitCMakeArguments"};
> 
> Additionally arguments can be passed to build-webkit directly, I believe. Failing that the right thing to do is to add an advanced CMake configuration variable, I believe.

There's indeed a --cmakeargs flag available in build-webkit. There isn't one in build-jsc, but I'll add that in another patch and remove this.

[Zan Dobersek]

(In reply to comment #2)
> (From update of attachment 225961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225961&action=review
> 
> So looking at this it seems like it isn't even a configurable feature for the Mac port. Instead it appears that Platform.h just enables it unconditionally if the platform is one that supports the diassembler. From what you've said about the disassembler, that might be enough for us.
> 

Thinking about it, the Udis86 disassembler only supports x86 and x86-64 architectures, and it is a very specific debugging tool that isn't really useful to a great majority of people using any build, release or development.

Because of that I'd recommend keeping it disabled by default, but make it easy to enable by passing -DWTF_USE_UDIS86 to cmake (through --cmakeargs or directly).

[Michael Catanzaro]

Comment on attachment 225961 [details]
Patch

Removing from request queue; this patch is not actionable in its current form....

[Zan Dobersek]

The Udis86 disassembler is now enabled by default in Platform.h, so the USE_UDIS86 guards in CMakeFiles should be removed.

[Zan Dobersek]

(In reply to comment #6)
> The Udis86 disassembler is now enabled by default in Platform.h, so the
> USE_UDIS86 guards in CMakeFiles should be removed.

https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Platform.h#L735

[Zan Dobersek]

Continued in bug #152731.