Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream the llvm patches #4259

Closed
3 of 4 tasks
erickt opened this issue Dec 22, 2012 · 54 comments
Closed
3 of 4 tasks

Upstream the llvm patches #4259

erickt opened this issue Dec 22, 2012 · 54 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. P-medium Medium priority

Comments

@erickt
Copy link
Contributor

erickt commented Dec 22, 2012

We have a handful of llvm patches that need to be pushed upstream. llvm just had a release, so have a nice window to get our patches reviewed and integrated for the next release.

Remaining patches to upstream

@fabiand
Copy link
Contributor

fabiand commented Mar 8, 2013

Is there any progress or timeline when you want to push your changes to upstream?

@sanxiyn
Copy link
Member

sanxiyn commented Mar 26, 2013

Referencing #5368.

@brson
Copy link
Contributor

brson commented Mar 26, 2013

@fabiand we actually are carrying no key llvm patches at the moment besides those dealing with ARM. I don't know the ETA for upstreaming those.

@fabiand
Copy link
Contributor

fabiand commented Mar 26, 2013

@brson That's good to know. Is there currently a build switch to build rust without pulling and building llvm? (So using existing llvm binaries?)

@brson
Copy link
Contributor

brson commented Mar 26, 2013

@fabiand configure has an --llvm-root flag for specifying a custom LLVM but I don't know if it works

@fabiand
Copy link
Contributor

fabiand commented Mar 26, 2013

@brson, thanks - It doesn't seem to work. The linker can't find the installed version. But I did not yet nail it down further. THere was one patch needed for llvm3.2 (wllvm32 branch)

@brson
Copy link
Contributor

brson commented Mar 31, 2013

Not for 0.6.

@isanbard
Copy link

I just wanted to jump in and say that LLVM is in the midst of planning their next release. If you have any patches for LLVM, please let us know! :-) Either by creating a PR or sending them to llvm-commits.

Thanks. :-)

@bstrie
Copy link
Contributor

bstrie commented Apr 22, 2013

Nominated for milestone due to time-sensitivity.

@pnkfelix
Copy link
Member

visiting for triage email 2013-07-22.

@brson @graydon : Any way we can up the priority on this task? Do we think we missed the window of time described in isanbard's comment?

From my reading of the releases at the LLVM download page and the LLVM release process docs, it seems like they release every 6 months, and we might expect the next release to take place to occur in December. (Maybe isanbard was referring to the 3.3 release, which came out on June 17th.) So this may not be super-duper high priority, but that might be all the more reason to actually start working this through our own pipeline...

@isanbard
Copy link

Unfortunately, you did miss the window for the 3.3 release. The next major release is around the end of the year. Though there may be a "dot" release before that. (I believe one of our contributors is in the middle of doing this.)

@bstrie
Copy link
Contributor

bstrie commented Jul 25, 2013

Are we also certain that we won't have more patches for LLVM, what with the new scheduler, actual GC, TBAA, etc.?

@thestinger
Copy link
Contributor

We don't need any patches for a TBAA pass, it can be loaded as a dynamic library by LLVM.

@brson
Copy link
Contributor

brson commented Jul 28, 2013

There will probably one more patch for #1226 (unless upstream has already done it). Additionally, I think that the patches we do have are probably not up to the quality standards llvm expects (missing tests, etc.).

@brson
Copy link
Contributor

brson commented Jul 28, 2013

@pnkfelix I haven't been very motivated to pursue this, sadly. It will be significant work, we still have at least some known new patches to write (mentioned above), the patches we do have need to be improved, and from what I understand some of our arm patches are somewhat special-cased and likely need further design work.

@brson
Copy link
Contributor

brson commented Jul 28, 2013

Also, last time I tried to upgrade LLVM I hit errors on windows, and as that so often goes, I just gave up rather than try to debug there.

@omasanori
Copy link
Contributor

Certainly we, the Rust project, have to maintain our own fork, it's still good for the upstream, other projects and package maintainers to merge patches into the upstream as soon as it gets mature, isn't it?
(you have many tasks so it might have a lower priority, of cource.)

@emberian
Copy link
Member

emberian commented Aug 5, 2013

@omasanori we don't need our own fork, the only reason we do is to work around issues in llvm

@omasanori
Copy link
Contributor

@cmr Certainly it's not a complete fork.

@thestinger
Copy link
Contributor

The intention is for Rust to be using a vanilla LLVM; we shouldn't have our own fork. The current patches can be reworked until they're accepted upstream or just dropped by removing the dependency on them.

@alexcrichton
Copy link
Member

As of right now, here's a list of patches we have to upstream.

Of those, two can be ignored. One is already upstream (Use a mutex instead of an RWMutex), and the other is a fix to get around a bug in 3.3 that was already fixed upstream (Skip i8 case in fastisel)

@lucab
Copy link
Contributor

lucab commented Sep 2, 2013

A fellow Debian and LLVM upstream developer agreed to take a look at our patch queue and integrating the non-debatable ones upstream. I'll keep you posted on the status.

@emberian
Copy link
Member

emberian commented Sep 2, 2013

☀️ wonderful!

@fabiand
Copy link
Contributor

fabiand commented Sep 2, 2013

🎉 (icons!) I'd love to get an official package in place for Fedora 21

@alexcrichton
Copy link
Member

@lucab, awesome! If you want to connect me to them or vice versa, I've been dealing with the LLVM upgrades recently so I could try to help out where needed.

@bstrie
Copy link
Contributor

bstrie commented Oct 8, 2013

@lucab Any updates? Also, I'm curious what the cutoff is for getting patches into the next stable LLVM release.

@lucab
Copy link
Contributor

lucab commented Oct 9, 2013

@sylvestre said he would have taken care of this, not sure about the status right now...

@isanbard
Copy link

isanbard commented Oct 9, 2013

This is the documentation on how to write them:

http://llvm.org/docs/DeveloperPolicy.html#test-cases

It's not very extensive, but there are many examples in the code base of tests (see the test/ directories). Just use good testing practices to figure out what to test.

@brson
Copy link
Contributor

brson commented Jan 27, 2014

Nominating. Is it a goal to be able to use system LLVM for 1.0?

@thestinger
Copy link
Contributor

@brson: it would need to happen to have a distribution like Fedora or Debian to package it, so I think it should be

@sylvestre
Copy link
Contributor

Indeed. I haven't followed that closely but is the bootstrap "issue" been fixed ?

@sanxiyn
Copy link
Member

sanxiyn commented Jan 28, 2014

@sylvestre What is the "bootstrap issue"?

@sylvestre
Copy link
Contributor

As explained here https://github.com/mozilla/rust
"Since the Rust compiler is written in Rust, it must be built by a precompiled "snapshot" version of itself (made in an earlier state of development). As such, source builds require a connection to the Internet, to fetch snapshots, and an OS that can execute the available snapshot binaries."
In Debian or Ubuntu build system, the network is not available and we want to be able to bootstrap the build from scratch. For example, OpenJDK is built using gcj.

@thestinger
Copy link
Contributor

The snapshot can be downloaded in advance and this definitely works as Rust won't keep downloading it on each build if snapshots.txt hasn't been updated. I don't think it's an issue.

@lucab
Copy link
Contributor

lucab commented Jan 28, 2014

@sylvestre I'm collecting some preliminary ideas on that at https://wiki.debian.org/Teams/RustPackaging/Bootstrap but that's not the point of this bug report. The LLVM upstreaming issue is a separate one which also the Fedora guys would like to have addressed.

I believe this is not an hard requirements for both distro, but will make package review harder (nobody wants yet another full LLVM copy in the archive).

@sylvestre
Copy link
Contributor

OK, thanks mate :)

About the bug, if there is no tests, it is going to be impossible to see the patches applied upstream.

@thestinger
Copy link
Contributor

The no-split-stack attribute was explicitly rejected upstream, and I have no doubt that fixed-stack-segment would also be rejected. It's not just about tests as these things have to be implemented in the right way to be merged rather than just what Rust happens to need at the moment.

If stop using the segmented stack support, then most of this problem will go away.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 4, 2014

We discussed what our requirements with respect to this issue in the weekly meeting. Our current stance is this: We want to be compatible with a reference host-provided LLVM for Rust 1.0, and we believe that this is an achievable goal (at least on x86 hosts).

So, we will attempt to support and/or shepherd-in changes that move us towards the goal of being able to work with a host-provided LLVM. (e.g. @alexcrichton has some ideas on how to deal with no_split_stack.)

Bu: working atop a host-provided LLVM is not something that should block the Rust 1.0 release.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 4, 2014

(removing the I-nominated tag since we have now had the delayed discussion of the matter.)

@emberian
Copy link
Member

@alexcrichton what ideas are those?

@alexcrichton
Copy link
Member

Any no_split_stack function can be written in C. I would much rather redouble efforts to upstream the patch, I don't believe that LLVM won't take it.

@thestinger
Copy link
Contributor

@alexcrichton: They expressed that they wanted split_stack instead of no_split_stack.

@alexcrichton
Copy link
Member

The win64 segmented stack patch was just upstreamed, I've updated the issue description to reflect remaining work.

@alexcrichton
Copy link
Member

The segmented stacks for ARM patches have been upstreamed.

@alexcrichton
Copy link
Member

The equivalent of a no-split-stack patch has been upstreamed. On the next LLVM update we will have to change our codegen to attach the "split-stack" attribute to all functions we generate.

@alexcrichton
Copy link
Member

The last remaining patch is just an optimization, and I don't think it's critical that we upstream it. We can keep it around locally for awhile, however.

We've got a bit of work on our hands to get compatible with upstream LLVM 3.5, but it'll be hairy regardless because we'll have to start dealing with c++11 compilers and such.

With all the patches upstreamed, I'm going to close this issue. Hurray!

@brson
Copy link
Contributor

brson commented Apr 11, 2014

\o/
On Apr 10, 2014 4:18 PM, "Alex Crichton" [email protected] wrote:

Closed #4259 #4259.

Reply to this email directly or view it on GitHubhttps://github.com//issues/4259
.

@omasanori
Copy link
Contributor

👍

@cnd
Copy link
Contributor

cnd commented Apr 11, 2014

👍

@lucab
Copy link
Contributor

lucab commented Apr 11, 2014

@alexcrichton This (and the libuv too) are great news. Thanks for all your efforts!

calebcartwright pushed a commit to calebcartwright/rust that referenced this issue Mar 30, 2022
* Fix newlines in JSON output

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line.

When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too).

Fixes rust-lang#4259

* Update tests
# Conflicts:
#	tests/writemode/target/output.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. P-medium Medium priority
Projects
None yet
Development

No branches or pull requests