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

Factor out rustc_apfloat into a standalone crate #55993

Closed
sunfishcode opened this issue Nov 15, 2018 · 19 comments · Fixed by #113843
Closed

Factor out rustc_apfloat into a standalone crate #55993

sunfishcode opened this issue Nov 15, 2018 · 19 comments · Fixed by #113843
Labels
A-licensing Area: compiler licensing C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@sunfishcode
Copy link
Member

librustc_apfloat is a Rust translation of LLVM's APFloat library, written in C++, and provides software implementations of f32, f64, and other floating-point types, which are useful for performing host-independent floating-point computation. We made a standalone crate of this for use in Cranelift, however other compiler projects could make use of such a library as well, and in general it'd be good to have only one copy of this code.

Where's the best place for the code live?

What should the crate be called?

  • rustc_apfloat -- the current name. "apfloat" isn't very descriptive, except that it's the name of the upstream code this crate is based on. And the crate isn't specific to rustc :-).
  • software_float -- a more descriptive name, and not yet claimed on crates.io
  • other ideas?

cc @eddyb @lachlansneff

@eddyb
Copy link
Member

eddyb commented Nov 16, 2018

software_float is a bit long, I'd rather expect something like softfloat.

cc @rust-lang/compiler

@eddyb
Copy link
Member

eddyb commented Nov 16, 2018

Also, I think we need to figure out copyright. I remember talking to a few people about it and our licensing setup deals with LLVM, but I'm not sure about a crate in isolation.
cc @alexcrichton

@bstrie
Copy link
Contributor

bstrie commented Nov 16, 2018

What's there about copyright to figure out? The copyright header of that LLVM file implies it's licensed under the Illinois Open Source License, which is an FSF- and OSI-approved permissive license. Surely the Rust translation should be licensed as the same, unless somebody has a good reason for wanting to change it?

@eddyb
Copy link
Member

eddyb commented Nov 16, 2018

@bstrie rustc_apfloat is not currently explicitly licensed in any way different from any of the other Rust code in the repo, but apparently this is okay because of the COPYRIGHT file or something.
IANAL so no clue what we'd need to do here, I just remember being told things are okay.

@bstrie
Copy link
Contributor

bstrie commented Nov 16, 2018

Ah, I see. I'm also not a lawyer, but I'm fairly confident that it isn't compliant with the license for us to create a derivative work of apfloat that doesn't use the Illinois license, at least not without explicit permission of the copyright holder. Those license headers claiming that rustc_apfloat is MIT/Apache seem somewhat dubious.

On the one hand I want to say that we should just change the license headers to be compliant (under this interpretation this doesn't count as relicensing, since we didn't have permission to use a different license in the first place); shipping more Illinois-licensed code when we already ship all of LLVM isn't a problem.

However--and here's the thing you might really need a lawyer for--if there's anyone who's contributed to rustc_apfloat while it's been erroneously licensed, I have absolutely no idea what license their contributions are actually licensed under. The conservative response would be to reach out to anyone who's ever contributed to it and ask them if they're okay with their contributions being licensed as Illinois.

@eddyb
Copy link
Member

eddyb commented Nov 17, 2018

@bstrie Wasn't it decided that we can just remove copyright headers?
I assumed what we'd have to figure out is the license specified in the Cargo.toml.

@eddyb
Copy link
Member

eddyb commented Nov 17, 2018

if there's anyone who's contributed to rustc_apfloat while it's been erroneously licensed, I have absolutely no idea what license their contributions are actually licensed under

Looking at https://github.com/rust-lang/rust/commits/master/src/librustc_apfloat, it's mostly sweeping changes across the the codebase, with only a few apfloat-specific changes.

Some of the more specific ones, like fixing comments, need to be upstreamed to LLVM as well, IMO.
I should've enforced a stricter policy of keeping the two in sync (modulo language/library minutiae).
(but I don't necessarily have a way to force apfloat changes to require my review)

@eddyb
Copy link
Member

eddyb commented Nov 17, 2018

Note to self: write an email to Mozilla legal about this situation.
(please ping me on IRC/Discord next week until this comment gets removed)

@jonas-schievink jonas-schievink added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2019
@eddyb
Copy link
Member

eddyb commented Jun 16, 2019

Oops, this really slipped through the cracks. I remembered about it at the All-Hands in Berlin (back in February IIRC), but forgot about it again.

Also, this is now completed: https://llvm.org/foundation/relicensing/ - LLVM moved to the Apache2 license.
So that might change the legal calculus a bit. But it seems likely we can just use Apache2 now.

@eddyb
Copy link
Member

eddyb commented Oct 1, 2019

Once we move this out of tree we might want to have (as per #63416 (comment)):

  • one branch for the port (where LLVM->Rust backports would go into)
  • one branch for the crates.io version, based on the other branch, with some other patches on top

This would even allow upstreaming some of the changes to LLVM, but at our own pace.

@Centril Centril added A-cranelift Things relevant to the [future] cranelift backend and removed A-cranelift Things relevant to the [future] cranelift backend labels Mar 12, 2020
@eddyb eddyb mentioned this issue Mar 16, 2021
6 tasks
@RalfJung
Copy link
Member

@eddyb wrote

But I haven't been able to do as much as I want with it because it's been stuck in licensing limbo for years.

I don't dare to touch it at all, really, while the licensing situation is up in the air, in the fear that it will prolong the limbo (originally I was assured there were no licensing issues and whoever told me that was wrong, so I don't want that to happen again) and I'd strongly advise anyone else against messing with it.

However, in this thread here it sounds like things are fine as LLVM moved to Apache2 licensing. Are there further licensing issues not yet mentioned in this thread?

@est31
Copy link
Member

est31 commented Mar 22, 2021

Not a lawyer but these are a few open problems/questions I would have:

  • LLVM moved not to Apache2 itself but to "Apache-2.0 WITH LLVM-exception" to use its SPDX identifier. I would guess that it's downward compatible to Apache-2.0, but ideally that exception is kept as it has definite GPL 2.0 compatibility, while the compatibility of Apache-2.0 with GPL 2.0 is an open question.
  • From my understanding, rustc_apfloat bases on an older version of llvm's apfloat code that existed before the relicensing. From what I understand, the relicensing did not extend to historic versions of llvm arbitrarily far back into the past. Is the historic version sufficiently different from the oldest relicensed version for it to be a problem?
  • Since its merger, rustc_apfloat has had modifications here and there (although few I guess?). I'm not sure under which license those modifications were, and where that'd put the combined work.

@eddyb
Copy link
Member

eddyb commented Mar 24, 2021

Since its merger, rustc_apfloat has had modifications here and there (although few I guess?). I'm not sure under which license those modifications were, and where that'd put the combined work.

At least we can split out the modifications somewhat, I'm more interested in the original port.
I could resync the port to a newer version of LLVM if it helps, and treat any changes on top as if they never happened (and anything that's not just aesthetic changes, can be redone as-needed).

Are there further licensing issues not yet mentioned in this thread?

All I know is people more qualified than me are going to look into it (or are already doing that). I haven't heard anything to suggest it's solved, though.

@bstrie
Copy link
Contributor

bstrie commented May 19, 2021

The current situation where people seem too afraid to approve changes to rustc-apfloat seems untenable. Instead of not approving any changes, get approval from the people submitting changes that they consent to having their changes optionally released under some set of licenses that you come up (say, Illinois, MIT, Apache 2).

@cbeuw
Copy link
Contributor

cbeuw commented Mar 25, 2023

Similar to @bstrie's suggestion. Can we change the rustbot comment to something like "You agree that the license of your contributions to rustc_apfloat may be changed to Apache 2.0 with LLVM exceptions at the core team's pleasure without notification"

@RalfJung
Copy link
Member

Cc #96784 which also mentions apfloat licensing

@eddyb
Copy link
Member

eddyb commented Jul 13, 2023

It's been a while (my bad) since potential solutions were discussed, but I come bearing good news.

With @pnkfelix and @wesleywiser coordinating things and also e.g. painstakingly verifying the licensing side (unsurprisingly, LLVM's colossal relicensing effort didn't go perfectly smooth - I'm very grateful they're handling those aspects, so that we'll never end up again, hopefully), I finally managed to find some time to execute on the plan we discussed late last year: start fresh all the way back from the original port.


My main focus this time was correctness: we kept stumbling onto bugs, and, personally, I don't think rustc devs should be in the business of randomly fixing softfloat bugs (or Rust users randomly running into them).

This is a library of 100% pure integer math, there are better ways.


Here's a rundown of the notable commits from the WIP rustc_apfloat repo:

At this point I think it's safe to say this version of rustc_apfloat is strictly superior to what's in compiler/rustc_apfloat right now (okay, sure, maybe it has more typos, and we still need to bring in @wesleywiser's detailed documentation of all the licensing history, before it can actually be used by anyone).

Pushing the port forward another year or two (or even all the way up to the present) should be quite doable (I've previously overestimated the complexity added by new float formats - bfloat16 was just one line in src/ieee.rs, NaN-related tests seem to the be bulk of the changes), but I need to stop after 30h awake for now.


As a teaser, this is what seems to be the only remaining issue:


EDIT: the repo has been transferred to rust-lang and I made a PR for the commits not in main:

@eddyb
Copy link
Member

eddyb commented Jul 17, 2023

Small update regarding the port advancing PR:

Specifically, these new commits are significant:

At this point, all previously-known rustc_apfloat bugs have been fixed
(shame about the new one though)

@eddyb
Copy link
Member

eddyb commented Jul 18, 2023

Both rust-lang/rustc_apfloat PRs have now landed:

I've also tried to describe as much I could in the README.
(it may be a bit too verbose though, feel free to file issues/PRs or bring it up on Zulip)

Next steps are probably to perf+crater switching to the new version, and releasing on crates.io if everything goes well (ideally fully replacing the old CraneStation/rustc_apfloat crate published by @lachlansneff - IMO the existing versions should be yanked because they're labeled as MIT and MIT/Apache2, and I doubt either of those choices is legitimate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-licensing Area: compiler licensing C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants