llvmlite icon indicating copy to clipboard operation
llvmlite copied to clipboard

Wrap LLD into python

Open ArachnidAbby opened this issue 2 years ago • 65 comments

Based on: Add dependency on LLD, wrap it into python

todo:

  • [x] Bring the old pr up-to-date with the 800 commits made to llvmlite since the pr opened
  • [x] Fix any broken code from the original pr
  • [x] add .binding.lld module with functions for each platform lld supports
  • [x] complete remaining items on the original PR
  • [x] add documentation for any newly added functions

items remaining in the original PR

  • [x] Make all Travis tests pass, test LLD on 64bit linux
  • [x] Write a test for 32 bits linux? Currently only 64 bits are tested. (not required)
  • [x] Get it to compile on macOS
  • [x] Get it to compile on Windows
  • [x] Get the PyPy test working (not required)
  • [x] Implement LLD for macOS and test it
  • [x] Implement LLD for Windows and test it (all non-required items are going to be checked off)

I will update you as I complete items.

ArachnidAbby avatar Jan 11 '23 13:01 ArachnidAbby

Many thanks for opening this PR!

gmarkall avatar Jan 11 '23 13:01 gmarkall

I know this is still WIP, but I just had a very quick skim through with some initial comments / thoughts - hopefully this shrinks the size of the first proper round of review.

gmarkall avatar Jan 11 '23 14:01 gmarkall

Hey, just putting this comment here to remind myself to use typing.List for annotations to support older versions of python.

ArachnidAbby avatar Jan 11 '23 14:01 ArachnidAbby

@gmarkall Hey I was looking at some of these failing tests and I noticed one for flake8.

I just wanted to ask if you guys follow the 80 characters per line rule or if thats just on by default?

https://dev.azure.com/numba/ff1fe4d0-ed73-4f1c-b894-1d50a27e048f/_apis/build/builds/14107/logs/59

I can fix the line length, but I just want to make sure that's even necessary.

ArachnidAbby avatar Jan 12 '23 01:01 ArachnidAbby

Is there any way for me to run the windows tests locally, specifically the build one. (I am on linux btw)?

I dont want to flood the commit history with a bunch of random changes just to run those tests.

ArachnidAbby avatar Jan 12 '23 05:01 ArachnidAbby

I just wanted to ask if you guys follow the 80 characters per line rule or if thats just on by default? I can fix the line length, but I just want to make sure that's even necessary.

Looks like we configure it explicitly - https://github.com/numba/llvmlite/blob/main/.flake8#L10

If lines are too long, they will need reformatting to reduce their length to pass flake8.

Is there any way for me to run the windows tests locally, specifically the build one. (I am on linux btw)? I dont want to flood the commit history with a bunch of random changes just to run those tests.

The only way to run them will be to do it on a Windows machine. If you don't have one, it's OK to use the CI here to test it.

I think we can consider this a "working PR" - I'd suggest that once we've got most of the way there, it might be a good idea to create a new branch based on the changes in this one but with a tidied history (I can help with that if necessary). Then we can squash out all the commits where you were experimenting with things on CI.

gmarkall avatar Jan 12 '23 14:01 gmarkall

I am in physical pain trying to use Cmake. MSVC is weird or its the way windows does things? It honestly might be best to use clang or mingw instead. At least then it would have similar args to gcc and wouldn't be some poorly renamed garbage

I am completely stuck. I don't use C/Cpp in any person projects because of these types of problems. This is the most frustrated and stuck I have ever been just trying to get a programming language to acknowledge that a god damn function exists.

edit: sorry. I needed to vent. Everything I search for on google gives the wrong results. Trying to ask google how to link lld just gives results for how to link using lld. No combination of search terms works. The llvm documentation is completely useless.

ArachnidAbby avatar Jan 13 '23 05:01 ArachnidAbby

MSVC is weird or its the way windows does things?

MSVC and Windows are certainly different - I don't know about "weird", but there are a lot of differences that tend to need accounting for in some way or other.

It honestly might be best to use clang or mingw instead. At least then it would have similar args to gcc and wouldn't be some poorly renamed garbage

Switching compiler toolchain isn't a change that can be taken lightly - there'd need to be a strong case for it.

Trying to ask google how to link lld just gives results for how to link using lld. No combination of search terms works.

There probably is no single document explaining how to link against lld.

For moving forward, I have a couple of questions / suggestions:

  • Do you know that the libraries you're trying to link against (lldCOFF, etc.) actually exist in the Windows llvmdev package? It may be that llvmdev needs rebuilding on Windows with changes to include these libraries - or some of these may need to be omitted from the link, if it turns out they can't be built.
  • At this point I think the only way you'll be able to move forward and retain your sanity will be to develop interactively on a Windows machine - if it were just something like a test fail where you had a good idea of what to fix, using CI would be OK for it. But for getting things going, using CI is (as you've found) quite tedious and painful. Do you have access to a Windows machine?

gmarkall avatar Jan 13 '23 10:01 gmarkall

* Do you know that the libraries you're trying to link against (lldCOFF, etc.) actually exist in the Windows llvmdev package? It may be that llvmdev needs rebuilding on Windows with changes to include these libraries - or some of these may need to be omitted from the link, if it turns out they can't be built.

* At this point I think the only way you'll be able to move forward and retain your sanity will be to develop interactively on a Windows machine - if it were just something like a test fail where you had a good idea of what to fix, using CI would be OK for it. But for getting things going, using CI is (as you've found) quite tedious and painful. Do you have access to a Windows machine?

I do have access to a windows machine. I just don't have anything for development installed on it. I tried to avoid it so I didn't need to install a bunch of software on my windows machine. But it seems that I need to at this point.

What would I need to install on windows in order to get started. I know I need python and conda, but what else needs to be installed to get started?

I also don't know if llvmdev includes lld. I would assume that it does because it's working on linux and macos builds (at least last I checked). How would I check to see if it is included in the llvmdev windows package?

btw: sorry for leaving that last comment. It was 1:30-2am and I was frustrated that nothing seemed to work. You are definitely right about it being a bad idea to just change compiler toolchains. I was just extremely tired and wasn't thinking that through all the way.

ArachnidAbby avatar Jan 13 '23 13:01 ArachnidAbby

What would I need to install on windows in order to get started. I know I need python and conda, but what else needs to be installed to get started?

You need VS2015 Update 3 or later installed as well: https://llvmlite.readthedocs.io/en/latest/admin-guide/install.html#prerequisites - in practice you can get by with just the Visual Studio Build Tools: https://visualstudio.microsoft.com/downloads/?q=build+tools. CMake is also needed, but you can just install that with conda - conda install cmake.

I also don't know if llvmdev includes lld. I would assume that it does because it's working on linux and macos builds (at least last I checked). How would I check to see if it is included in the llvmdev windows package?

If you install the llvmdev package with conda install llvmdev then look in %CONDA_PREFIX% (in command prompt) or $env:CONDA_PREFIX (in powershell) to see if you can find the libraries with names like lldCOFF.dll, etc. then it should reveal whether they're part of the package. Alternatively you could download the package manually from https://anaconda.org/numba/llvmdev and extract it to see what files are there.

btw: sorry for leaving that last comment. It was 1:30-2am and I was frustrated that nothing seemed to work.

No problem :slightly_smiling_face:

gmarkall avatar Jan 13 '23 15:01 gmarkall

It's just my luck that when trying to boot into my windows install for the first time in a month, it decides to randomly break. All the Bios settings have remained the same and not a single bit has been changed on my damn hardrive. Literally NOTHING has changed and yet it decided to try to boot into recovery mode and then none of the recovery options work. And because it's windows, It's practically impossible for me to get any information as to why it failed.

I am honestly just surprised that windows running on an SSD (that is maybe 2 years old at most) failed before poorly installed arch linux on a 12 year old hardrive.

I'm going to try a VM and see what I can. It might take longer than expected. I just thought I would update you.

Edit: I got the VM setup to hopefully start working as soon as possible. I just need to add my local repo as a shared folder. I also found a vscode plugin that lets me run virtual box from inside of the IDE.

ArachnidAbby avatar Jan 14 '23 01:01 ArachnidAbby

Do you know that the libraries you're trying to link against (lldCOFF, etc.) actually exist in the Windows llvmdev package? It may be that llvmdev needs rebuilding on Windows with changes to include these libraries - or some of these may need to be omitted from the link, if it turns out they can't be built

Okay it turns out they don't exist. But it seems that there is a lld package for conda. When I tried it, it has all the files I need.

I am not very familiar with conda. How could I make lld required for building llvmlite?

edit: Nvm.... they are included? I don't know why I couldn't see them earlier?

ArachnidAbby avatar Jan 15 '23 00:01 ArachnidAbby

@gmarkall I did it!

The windows tests pass. The linux and macos tests fail but thats for setting up miniconda for some reason. I didn't change miniconda for either platform so I am going to ignore that .

It was doing the same thing the other day, then suddenly fixed itself. I am going to assume I don't need to fix that for now.

edit: Hmm macos doesn't build anymore. Give me some time to figure that out.

ArachnidAbby avatar Jan 15 '23 04:01 ArachnidAbby

I have finished the documentation for the new lld module, but it's definitely a good idea to proof read it yourself.

I am unsure of why the linux builds fail on azure but it doesn't even get to the compilation steps. It fails to install miniconda for some reason. I have no reason to believe it wouldn't work. The weird thing is that I don't believe any commits in this PR actually change the miniconda installation steps in any way. But, luckily it compiles and passes all tests locally so I am not too concerned. (It would be nice to get these tests fixed tho. If you could provide help that would be fantastic)

For Macos... I honestly don't understand how it can compile just fine and yet still say the function lld_main doesn't exist. It wouldn't have compiled lld.cpp if there was an issue? And, seeing as the windows VM was 50GB.... I don't know if I want to allocated another 50gb+ for a macos vm. Not to mention the fact that I have never used macos or an apple products. I'm not really sure what to do here if I'm being honest.

Windows builds work on azure and the VM so those seem to be good to go. They pass all tests too! The format checking also looks pretty good for python and c++.

And lastly, would there be a significant enough difference for 32 bit linux and 64 bit linux that would cause lld not to work? the lld homepage says that it supports all the targets that you wanted tests for originally. If there isn't a significant enough difference, I want to remove the item from the checklist for the PR.

@gmarkall

edit: I tried making a better git history on this branch if you want to look at that. https://github.com/Hassium-Software/llvmlite-lld/tree/lld I can probably force push those changes to this PR's branch if you want to use this history instead.

ArachnidAbby avatar Jan 16 '23 05:01 ArachnidAbby

@gmarkall I did it!

The windows tests pass.

Congratulations!

I am unsure of why the linux builds fail on azure but it doesn't even get to the compilation steps.

I'll re-run the builds, that issue looks like something might have been temporarily off.

For Macos... I honestly don't understand how it can compile just fine and yet still say the function lld_main doesn't exist. It wouldn't have compiled lld.cpp if there was an issue? And, seeing as the windows VM was 50GB.... I don't know if I want to allocated another 50gb+ for a macos vm. Not to mention the fact that I have never used macos or an apple products. I'm not really sure what to do here if I'm being honest.

There is a difference between compilation and linking - you can compile a function that calls functions defined in other translation units - at compilation time the compiler doesn't know where the called function will be, so the end result of the compilation and assembly is an object file with a "hole" (a relocation) in it for the address of the called function to be placed. When linking statically (at build time) the linker has to figure out the address of the called function, and the build will fail if it can't. When linking dynamically (at run time), the linker leaves the relocation "unfilled" with the expectation that the dynamic linker must resolve the relocation. Since it hasn't left any unresolved symbols that must be statically resolved, the build succeeds. Then at runtime the dynamic linker can't resolve the dynamic relocation for lld_main, and at that point execution fails.

And lastly, would there be a significant enough difference for 32 bit linux and 64 bit linux that would cause lld not to work? the lld homepage says that it supports all the targets that you wanted tests for originally. If there isn't a significant enough difference, I want to remove the item from the checklist for the PR.

There are definitely differences that can cause 32-bit to fail and 64-bit to succeed (more than I could enumerate... :slightly_smiling_face: ) - I think a 32-bit test would be required rather than assuming that 32-bits is OK.

I tried making a better git history on this branch if you want to look at that. https://github.com/Hassium-Software/llvmlite-lld/tree/lld I can probably force push those changes to this PR's branch if you want to use this history instead.

Thanks - I think it's probably not worth rewriting history just yet for my benefit, but feel free to force-push here if you feel more comfortable with the cleared-up history.

gmarkall avatar Jan 16 '23 11:01 gmarkall

/azp run

gmarkall avatar Jan 16 '23 11:01 gmarkall

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Jan 16 '23 11:01 azure-pipelines[bot]

The Linux build issue is happening on #896 as well, it's not an issue related to this PR.

gmarkall avatar Jan 16 '23 13:01 gmarkall

It is a general problem with CI that the linux builds fail - #899 fixes it.

gmarkall avatar Jan 16 '23 13:01 gmarkall

There is a difference between compilation and linking - you can compile a function that calls functions defined in other translation units - at compilation time the compiler doesn't know where the called function will be, so the end result of the compilation and assembly is an object file with a "hole" (a relocation) in it for the address of the called function to be placed. When linking statically (at build time) the linker has to figure out the address of the called function, and the build will fail if it can't. When linking dynamically (at run time), the linker leaves the relocation "unfilled" with the expectation that the dynamic linker must resolve the relocation. Since it hasn't left any unresolved symbols that must be statically resolved, the build succeeds. Then at runtime the dynamic linker can't resolve the dynamic relocation for lld_main, and at that point execution fails.

I am still lost on what I should do here. How are the other functions used seen by python? I should be able to copy the way the other functions do it right?

edit: Okay I'm getting close. I realized that I just needed to put LLVMPY_ in front of the function name.

ArachnidAbby avatar Jan 16 '23 18:01 ArachnidAbby

All the tests passed!!!

Macos now compiles. I had to add an option when creating the target machine. The option is called force_elf and just adds -elf onto the end (like when the jit option is enabled on windows). I had to add it because the lld::macho::link function didn't seem to work. Also, don't worry, I will add documentation for it, but I want to know your opinion on that change first.

Just 2 questions I have to ask:

  • Are pypy tests still required. If they are, how do I get started on those.
  • How do I create 32 bit tests?

ArachnidAbby avatar Jan 17 '23 01:01 ArachnidAbby

@gmarkall Hey. Just making sure you saw my original questions.

ArachnidAbby avatar Jan 18 '23 23:01 ArachnidAbby

Are pypy tests still required. If they are, how do I get started on those.

It looks like we're not testing with PyPy at the moment, so it should be OK to go without PyPy tests.

How do I create 32 bit tests?

I just noticed Stuart's original comment (https://github.com/numba/llvmlite/pull/419#issuecomment-460192657) suggested not adding 32 bit tests right now, but just ensuring that the tests are appropriately guarded against on 32-bit systems. So, we can probably skip creating these for this PR, and add 32-bit support later if it's warranted.

Accordingly, I'm going to mark this as ready for a more general review now.

gmarkall avatar Jan 19 '23 13:01 gmarkall

BTW, I'd suggest it's probably not worth a squash right now, since we'll end up adding more history through the review process, I expect. Instead let's get this fixed up and then when it's ready to merge, we'll edit the history then to make it look like a better story - would you be happy with this plan?

gmarkall avatar Jan 19 '23 13:01 gmarkall

BTW, I'd suggest it's probably not worth a squash right now, since we'll end up adding more history through the review process, I expect. Instead let's get this fixed up and then when it's ready to merge, we'll edit the history then to make it look like a better story - would you be happy with this plan?

Sounds good to me.

ArachnidAbby avatar Jan 19 '23 16:01 ArachnidAbby

Hey, I was wondering how long the process of review takes (not trying to rush you, just genuinely curious) . Also, are there any other steps I need to take before this PR is ready for review?

ArachnidAbby avatar Jan 31 '23 19:01 ArachnidAbby

@spidertyler2005 Thanks for checking in - all is good from your end, it's just I've got a quite deep backlog and I need to find a decent block of time to give this a proper review - when you were working up to the ready-to-review stage it was easy to answer your questions in the odd minute from time to time, but now it needs my full attention to look at properly. I am aiming to look at this before the 0.40 release.

gmarkall avatar Jan 31 '23 22:01 gmarkall

So yesterday I decided to use the new lld functionality on my current project (which uses llvmlite). Once I implemented the few lines of code needed, I found that lld does not automatically link in system libraries, so symbols like printf and exit could not be resolved. I am wondering if I should add options to automatically link these in. This isn't a necessary addition since linking works if you don't use these symbols or if you include them yourself.

Adding this would make it easier to create binaries regardless of platform. Users of the lld functions would not need to write any platform specific code when wanting to use these common functions. I think it could be worth it, but I want to see what you think. If this is added, then it would also need some platform specific tests that test functions provided by each platform.

This could also be a completely separate, future PR unrelated to this one. I don't really know how to proceed here since it really doesn't break anything related to getting lld wrapped into python (what this PR is focused on). What do you think?

edit: honestly I'm not even sure I know how to do this. I am struggling to figure this out for linux since the documentation for anything involving llvm just doesn't exist (or is garbage).

ArachnidAbby avatar Feb 02 '23 16:02 ArachnidAbby

Thanks for thinking further ahead and testing this out with a real use case!

This could also be a completely separate, future PR unrelated to this one. I don't really know how to proceed here since it really doesn't break anything related to getting lld wrapped into python (what this PR is focused on). What do you think?

I think we should defer thinking about it for another (potential) PR after this one. You've already put a lot of work into this PR and it represents a significant feature and a lot of effort, so it would be best to avoid expanding the scope of this one and get it over the line, rather than adding more to it that will add to the review / revision workload (I've already been struggling to get to it because I know it will take me to some time to properly understand the changes here).

gmarkall avatar Feb 03 '23 14:02 gmarkall

Hey, I can take the time to resolve these conflicts if you want. That way it remains up-to-date. Just respond if you want me to do that

ArachnidAbby avatar Jun 21 '23 22:06 ArachnidAbby