Bug 154323 – Remove LLVM dependencies from WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

We don't need LLVM anymore, since we have B3.

[Csaba Osztrogonác]

GTK and EFL already switched to B3 and removed LLVM dependency
from the cmake build system: http://trac.webkit.org/changeset/196077

Feel free to remove all LLVM-FTL related code path. ;)

But it would be great if we can leave LLVM disassebmler in the trunk.

[Filip Pizlo]

(In reply to comment #1)
> GTK and EFL already switched to B3 and removed LLVM dependency
> from the cmake build system: http://trac.webkit.org/changeset/196077
> 
> Feel free to remove all LLVM-FTL related code path. ;)
> 
> But it would be great if we can leave LLVM disassebmler in the trunk.

I plan to remove it as well because of how hard it is to link to LLVM and how often they break API.

[Csaba Osztrogonác]

(In reply to comment #2)

> > But it would be great if we can leave LLVM disassebmler in the trunk.
> 
> I plan to remove it as well because of how hard it is to link to LLVM and
> how often they break API.

Is the existance of the LLVM disassembler blocker for anything?

I haven't found any issue related to it in the past years ever.
But removing it would make impossible to debug ARM traditional bugs.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> 
> > > But it would be great if we can leave LLVM disassebmler in the trunk.
> > 
> > I plan to remove it as well because of how hard it is to link to LLVM and
> > how often they break API.
> 
> Is the existance of the LLVM disassembler blocker for anything?
> 
> I haven't found any issue related to it in the past years ever.
> But removing it would make impossible to debug ARM traditional bugs.

It would be a lot better to just write our own arm disassembler.

The issue with the llvm one is that it goes through the same soft linking nonsense that we do for the rest of llvm. So, I'm not sure how you're proposing to keep it of gtk/efl removed the dependency from the build system.

[Michael Catanzaro]

(In reply to comment #4)
> The issue with the llvm one is that it goes through the same soft linking
> nonsense that we do for the rest of llvm. So, I'm not sure how you're
> proposing to keep it of gtk/efl removed the dependency from the build system.

We depend on LLVM only if USE_LLVM_DISASSEMBLER is set, which is off by default and which we do not expose as a public build option.

[Csaba Osztrogonác]

(In reply to comment #4)
> It would be a lot better to just write our own arm disassembler.

It can't be serious that writing a brand new disassembler
is better choice than using a working one. 
 
> The issue with the llvm one is that it goes through the same soft linking
> nonsense that we do for the rest of llvm. 

It's not the most beautiful think I've ever seen, but it works.
But I still don't understand why its existence is an issue?

Do you have a political reason that everything related
to LLVM should be removed from the trunk immediately?
I can understand that Apple don't want to use it,
in this case just remove it from your build system
and set USE_LLVM_DISASSEMBLER to 0 on your platform.

But I don't understand why do you want to force Linux 
users to drop a working debugging possibility?

> So, I'm not sure how you're proposing to keep it of gtk/efl 
> removed the dependency from the build system.
We only removed LLVM based FTL JIT support from the build system.

LLVM disassembler is optional and still can be enabled explicitly:
build-jsc --cmakeargs="-DUSE_LLVM_DISASSEMBLER=1"

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #4)
> > It would be a lot better to just write our own arm disassembler.
> 
> It can't be serious that writing a brand new disassembler
> is better choice than using a working one. 
>  
> > The issue with the llvm one is that it goes through the same soft linking
> > nonsense that we do for the rest of llvm. 
> 
> It's not the most beautiful think I've ever seen, but it works.
> But I still don't understand why its existence is an issue?
> 
> Do you have a political reason that everything related
> to LLVM should be removed from the trunk immediately?
> I can understand that Apple don't want to use it,
> in this case just remove it from your build system
> and set USE_LLVM_DISASSEMBLER to 0 on your platform.
> 
> But I don't understand why do you want to force Linux 
> users to drop a working debugging possibility?
> 
> > So, I'm not sure how you're proposing to keep it of gtk/efl 
> > removed the dependency from the build system.
> We only removed LLVM based FTL JIT support from the build system.
> 
> LLVM disassembler is optional and still can be enabled explicitly:
> build-jsc --cmakeargs="-DUSE_LLVM_DISASSEMBLER=1"

Wow first of all, politics has nothing to do with this. I'm trying to determine if we will need to keep the llvm soft linking madness or not. That is how we currently use llvm API. You would have to change the llvm disassembler if you wanted it to no longer depend on the soft linking. 

Second, I have no idea how you think the llvm disassembler helps you on arm traditional. If you look in LLVMDisassembler.cpp you will see that it doesn't have support for arm traditional - only arm64 and x86. 

Also, any support for using the llvm disassembler on x86 should be deleted, regardless of the arm traditonal situation. 

Can you provide some technical assistance instead of acting up?  I'm arguing for the deletion of llvm dependencies on technical grounds based on considering the details of how it's used, and you're coming at me with imaginary hypotheticals like arm traditional.

[Filip Pizlo]

I've confirmed that the LLVM disassembler is only used when the FTL is used. For example:

#if !defined(USE_LLVM_DISASSEMBLER) && HAVE(LLVM) && (CPU(X86_64) || CPU(X86) || CPU(ARM64))
#define USE_LLVM_DISASSEMBLER 1
#endif

Notice that it's only enabled on platforms where we also have our own disassembler, and we will use our own disassembler on those platforms except for code generated by LLVM.

Also this:

    const char* triple;
#if CPU(X86_64)
    triple = "x86_64-apple-darwin";
#elif CPU(X86)
    triple = "x86-apple-darwin";
#elif CPU(ARM64)
    triple = "arm64-apple-darwin";
#else
#error "LLVM disassembler currently not supported on this CPU."
#endif

Clearly, if you tried to use the LLVM disassembler on any platform other than the ones where we also have our own disassemblers, you'd get a compile-time error.

Because of this, it's clear that the LLVM disassembler is dead code on every platform, including ARM traditional.

[Filip Pizlo]

Created attachment 271581 [details]
so far

Still more things to delete!

[Csaba Osztrogonác]

(In reply to comment #7)
> Wow first of all, politics has nothing to do with this. I'm trying to
> determine if we will need to keep the llvm soft linking madness or not. That
> is how we currently use llvm API. You would have to change the llvm
> disassembler if you wanted it to no longer depend on the soft linking. 

I don't need soft linking, but I don't have time 
and don't want to fix something works.
 
> Second, I have no idea how you think the llvm disassembler helps you on arm
> traditional. If you look in LLVMDisassembler.cpp you will see that it
> doesn't have support for arm traditional - only arm64 and x86. 

I used it not so long ago (~months ago) to debug an ARM traditional bug.
It isn't supported in trunk, but managed to make it work super easily.
(But forgot to upstream the ARM traditional LLVM disassembler support.)
 
> Also, any support for using the llvm disassembler on x86 should be deleted,
> regardless of the arm traditonal situation. 

I don't care about x86, feel free to do it whatever you want.

> Can you provide some technical assistance instead of acting up?  I'm arguing
> for the deletion of llvm dependencies on technical grounds based on
> considering the details of how it's used, and you're coming at me with
> imaginary hypotheticals like arm traditional.

No it isn't hypotetical, ARM traditional backend is still 
supported and a disassembler is really helpful to debug bugs.

[Csaba Osztrogonác]

To make it clear, I have strongly against 
removing LLVM disassembler. Please don't do it.

[Filip Pizlo]

(In reply to comment #10)
> (In reply to comment #7)
> > Wow first of all, politics has nothing to do with this. I'm trying to
> > determine if we will need to keep the llvm soft linking madness or not. That
> > is how we currently use llvm API. You would have to change the llvm
> > disassembler if you wanted it to no longer depend on the soft linking. 
> 
> I don't need soft linking, but I don't have time 
> and don't want to fix something works.
>  
> > Second, I have no idea how you think the llvm disassembler helps you on arm
> > traditional. If you look in LLVMDisassembler.cpp you will see that it
> > doesn't have support for arm traditional - only arm64 and x86. 
> 
> I used it not so long ago (~months ago) to debug an ARM traditional bug.
> It isn't supported in trunk, but managed to make it work super easily.
> (But forgot to upstream the ARM traditional LLVM disassembler support.)
>  
> > Also, any support for using the llvm disassembler on x86 should be deleted,
> > regardless of the arm traditonal situation. 
> 
> I don't care about x86, feel free to do it whatever you want.
> 
> > Can you provide some technical assistance instead of acting up?  I'm arguing
> > for the deletion of llvm dependencies on technical grounds based on
> > considering the details of how it's used, and you're coming at me with
> > imaginary hypotheticals like arm traditional.
> 
> No it isn't hypotetical, ARM traditional backend is still 
> supported and a disassembler is really helpful to debug bugs.

Using the LLVM disassembler for ARM traditional is purely hypothetical as far as I can tell.  We're not going to maintain this dependency because of something that isn't even in the tree.

If you have an out-of-tree patch to make the LLVM disassembler work for ARM traditional, then it should be easy for you to make this out-of-tree patch include the current contents of LLVMDisassembler.cpp.

Let me put it this way: if at some point (either before this patch lands or after), you provided a patch that made the LLVM disassembler work for ARM traditional without the use of soft linking or any of the garbage in JavaScriptCore/llvm (like using the LLVM API via llvm->Foo() instead of LLVMFoo()), then I'd be happy to review this patch and I'd have no objection to it.

What I'm objecting to is maintaining the current very sophisticated LLVM soft linking code, which the current LLVM disassembler depends on.  I'm also objecting to maintaining it for something that isn't even in the tree.

[Filip Pizlo]

(In reply to comment #11)
> To make it clear, I have strongly against 
> removing LLVM disassembler. Please don't do it.

It's 100% dead code.  There is strong precedent in the WebKit project to remove dead code.

[Csaba Osztrogonác]

(In reply to comment #12)
> Using the LLVM disassembler for ARM traditional is purely hypothetical as
> far as I can tell.  We're not going to maintain this dependency because of
> something that isn't even in the tree.
There are many unused code in WebKit tree, which is used by Apple
only for internal purposes. Should we remove these code too? ( bug142043)

As I mentioned, I used it, but forgot to upstream. But will do it soon.

> If you have an out-of-tree patch to make the LLVM disassembler work for ARM
> traditional, then it should be easy for you to make this out-of-tree patch
> include the current contents of LLVMDisassembler.cpp.
> 
> Let me put it this way: if at some point (either before this patch lands or
> after), you provided a patch that made the LLVM disassembler work for ARM
> traditional without the use of soft linking or any of the garbage in
> JavaScriptCore/llvm (like using the LLVM API via llvm->Foo() instead of
> LLVMFoo()), then I'd be happy to review this patch and I'd have no objection
> to it.

No, I don't have time for any refactoring and won't do it 
just because you don't like the code you added previously.
 
> What I'm objecting to is maintaining the current very sophisticated LLVM
> soft linking code, which the current LLVM disassembler depends on.  I'm also
> objecting to maintaining it for something that isn't even in the tree.

You won't use it, you don't need to maintain it. I don't understand
why are you worrying about it. I'll upstream the ARM traditional
disassembler soon and will maintain this code path if it is necessary.
Maybe I will do the refactoring you want in the future once I have
more time.

(In reply to comment #13)
> (In reply to comment #11)
> > To make it clear, I have strongly against 
> > removing LLVM disassembler. Please don't do it.
> 
> It's 100% dead code.  There is strong precedent in the WebKit project to
> remove dead code.

I haven't heard about any rule that a theoretically dead code should
be removed immediately. After I said I need it and will used soon
(this week) it isn't dead code.

My strong opinion means r- for removing it.

[Filip Pizlo]

(In reply to comment #14)
> (In reply to comment #12)
> > Using the LLVM disassembler for ARM traditional is purely hypothetical as
> > far as I can tell.  We're not going to maintain this dependency because of
> > something that isn't even in the tree.
> There are many unused code in WebKit tree, which is used by Apple
> only for internal purposes. Should we remove these code too? ( bug142043)
> 
> As I mentioned, I used it, but forgot to upstream. But will do it soon.

When you do that, you can take the contents of LLVMDisassembler.cpp and resubmit it but this time without dependence on soft linking.

> 
> > If you have an out-of-tree patch to make the LLVM disassembler work for ARM
> > traditional, then it should be easy for you to make this out-of-tree patch
> > include the current contents of LLVMDisassembler.cpp.
> > 
> > Let me put it this way: if at some point (either before this patch lands or
> > after), you provided a patch that made the LLVM disassembler work for ARM
> > traditional without the use of soft linking or any of the garbage in
> > JavaScriptCore/llvm (like using the LLVM API via llvm->Foo() instead of
> > LLVMFoo()), then I'd be happy to review this patch and I'd have no objection
> > to it.
> 
> No, I don't have time for any refactoring and won't do it 
> just because you don't like the code you added previously.

I don't have time to maintain your code.  WebKit isn't going to keep code around that is only meant to support your out-of-tree use case.

>  
> > What I'm objecting to is maintaining the current very sophisticated LLVM
> > soft linking code, which the current LLVM disassembler depends on.  I'm also
> > objecting to maintaining it for something that isn't even in the tree.
> 
> You won't use it, you don't need to maintain it. I don't understand
> why are you worrying about it. I'll upstream the ARM traditional
> disassembler soon and will maintain this code path if it is necessary.
> Maybe I will do the refactoring you want in the future once I have
> more time.

This is about removing dead code.

> 
> (In reply to comment #13)
> > (In reply to comment #11)
> > > To make it clear, I have strongly against 
> > > removing LLVM disassembler. Please don't do it.
> > 
> > It's 100% dead code.  There is strong precedent in the WebKit project to
> > remove dead code.
> 
> I haven't heard about any rule that a theoretically dead code should
> be removed immediately. After I said I need it and will used soon
> (this week) it isn't dead code.
> 
> My strong opinion means r- for removing it.

You're being unreasonable.  Do you propose introducing a WebKit policy that we should have dead code to support any reviewer's out-of-tree use case?

[Filip Pizlo]

Let me be clear on my view here: an R- is not a valid reason to keep around dead code.  I will ignore your R- if it is only about keeping the LLVM disassembler for ARM traditional since in trunk it is not possible to use the LLVM disassembler for ARM traditional.

[Csaba Osztrogonác]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Using the LLVM disassembler for ARM traditional is purely hypothetical as
> > > far as I can tell.  We're not going to maintain this dependency because of
> > > something that isn't even in the tree.
> > There are many unused code in WebKit tree, which is used by Apple
> > only for internal purposes. Should we remove these code too? ( bug142043)
> > 
> > As I mentioned, I used it, but forgot to upstream. But will do it soon.
> 
> When you do that, you can take the contents of LLVMDisassembler.cpp and
> resubmit it but this time without dependence on soft linking.

No, I won't play this game. You want to remove your own code you don't like
and then only let it back only if I refactor it to be more comfortable for you.

> > > If you have an out-of-tree patch to make the LLVM disassembler work for ARM
> > > traditional, then it should be easy for you to make this out-of-tree patch
> > > include the current contents of LLVMDisassembler.cpp.
> > > 
> > > Let me put it this way: if at some point (either before this patch lands or
> > > after), you provided a patch that made the LLVM disassembler work for ARM
> > > traditional without the use of soft linking or any of the garbage in
> > > JavaScriptCore/llvm (like using the LLVM API via llvm->Foo() instead of
> > > LLVMFoo()), then I'd be happy to review this patch and I'd have no objection
> > > to it.
> > 
> > No, I don't have time for any refactoring and won't do it 
> > just because you don't like the code you added previously.
> 
> I don't have time to maintain your code.  WebKit isn't going to keep code
> around that is only meant to support your out-of-tree use case.

Don't have time to maintain what? What are you talking about? You wasting 
your time to try to convince me that you have to remove the code I need. 
If you don't need, just remove it from xcode build system and don't define
LLVM_DISASSEMBLER. That's all.

To be more practical, LLVM disassembler is still in cmake build system,
it builds and works fine. (now on x86 an ARM64) This isn't dead code at all.

[snip]
 
> > My strong opinion means r- for removing it.
> 
> You're being unreasonable.  Do you propose introducing a WebKit policy that
> we should have dead code to support any reviewer's out-of-tree use case?

I don't propose any policy change. I _said_ I _will_ submit a patch _soon_
to support ARM traditional disassembler with the existing code base. But
you decided that you want to remove the basic of this patch and make my
work harder with stating that you will let it in only if I do the refactoring
what you want. That's not fair and I still have strong objection.

[Filip Pizlo]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #12)
> > > > Using the LLVM disassembler for ARM traditional is purely hypothetical as
> > > > far as I can tell.  We're not going to maintain this dependency because of
> > > > something that isn't even in the tree.
> > > There are many unused code in WebKit tree, which is used by Apple
> > > only for internal purposes. Should we remove these code too? ( bug142043)
> > > 
> > > As I mentioned, I used it, but forgot to upstream. But will do it soon.
> > 
> > When you do that, you can take the contents of LLVMDisassembler.cpp and
> > resubmit it but this time without dependence on soft linking.
> 
> No, I won't play this game. You want to remove your own code you don't like
> and then only let it back only if I refactor it to be more comfortable for
> you.
> 
> > > > If you have an out-of-tree patch to make the LLVM disassembler work for ARM
> > > > traditional, then it should be easy for you to make this out-of-tree patch
> > > > include the current contents of LLVMDisassembler.cpp.
> > > > 
> > > > Let me put it this way: if at some point (either before this patch lands or
> > > > after), you provided a patch that made the LLVM disassembler work for ARM
> > > > traditional without the use of soft linking or any of the garbage in
> > > > JavaScriptCore/llvm (like using the LLVM API via llvm->Foo() instead of
> > > > LLVMFoo()), then I'd be happy to review this patch and I'd have no objection
> > > > to it.
> > > 
> > > No, I don't have time for any refactoring and won't do it 
> > > just because you don't like the code you added previously.
> > 
> > I don't have time to maintain your code.  WebKit isn't going to keep code
> > around that is only meant to support your out-of-tree use case.
> 
> Don't have time to maintain what? What are you talking about? You wasting 
> your time to try to convince me that you have to remove the code I need. 
> If you don't need, just remove it from xcode build system and don't define
> LLVM_DISASSEMBLER. That's all.
> 
> To be more practical, LLVM disassembler is still in cmake build system,
> it builds and works fine. (now on x86 an ARM64) This isn't dead code at all.
> 
> [snip]
>  
> > > My strong opinion means r- for removing it.
> > 
> > You're being unreasonable.  Do you propose introducing a WebKit policy that
> > we should have dead code to support any reviewer's out-of-tree use case?
> 
> I don't propose any policy change. I _said_ I _will_ submit a patch _soon_
> to support ARM traditional disassembler with the existing code base. But
> you decided that you want to remove the basic of this patch and make my
> work harder with stating that you will let it in only if I do the refactoring
> what you want. That's not fair and I still have strong objection.

When you submit that patch, you are more than welcome to include any amount of removed code in that patch.

Your future plans are just not a valid reason to block removal of dead code.

[Csaba Osztrogonác]

(In reply to comment #16)
> Let me be clear on my view here: an R- is not a valid reason to keep around
> dead code.  I will ignore your R- if it is only about keeping the LLVM
> disassembler for ARM traditional since in trunk it is not possible to use
> the LLVM disassembler for ARM traditional.

no comment .... I stopped arguing with you at this point ...

[Filip Pizlo]

(In reply to comment #19)
> (In reply to comment #16)
> > Let me be clear on my view here: an R- is not a valid reason to keep around
> > dead code.  I will ignore your R- if it is only about keeping the LLVM
> > disassembler for ARM traditional since in trunk it is not possible to use
> > the LLVM disassembler for ARM traditional.
> 
> no comment .... I stopped arguing with you at this point ...

Like I said, you are welcome to take any amount of the removal patch and include it in your patch to enable the LLVM disassembler for ARM traditional.  Nobody will argue against that.

The issue here is dead code.  Plain and simple.

[Filip Pizlo]

Created attachment 271587 [details]
the patch

Note: the actual patch I will land also includes removal of the LLVM drops, but I didn't include it in this patch file because that would bloat it to >20MB.  That part is just deleting binary files.

[WebKit Commit Bot]

Attachment 271587 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/disassembler/UDis86Disassembler.h:35:  The parameter name "codePtr" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/disassembler/UDis86Disassembler.h:35:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 271589 [details]
the patch

Fix CMake build.

[Benjamin Poulain]

Comment on attachment 271589 [details]
the patch

TL;DR

[WebKit Commit Bot]

Attachment 271589 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/disassembler/UDis86Disassembler.h:35:  The parameter name "codePtr" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/disassembler/UDis86Disassembler.h:35:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

> I'll upstream the ARM traditional
> disassembler soon and will maintain this code path if it is necessary.

Caaba, thank you for your offer to be the maintainer for the ARM traditional disassembler.

We're moving forward with a design change not to soft-link LLVM anymore. How soon do you think you can get the ARM traditional disassembler up to speed with this change?

You said:

> No, I don't have time for any refactoring and won't do it 

Strange. Usually, being a maintainer means putting in the time to, you know, maintain the code.

Maintaining a CPU port of a first-rate JIT compiler is indeed a time-consuming job, and if you don't have the time for it, I understand. But I'll be happy to review your patches if you do find the time.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/196729