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

RISC-V implementation contains likely GPL-3 code #319

Closed
brson opened this issue Oct 20, 2019 · 23 comments · Fixed by #499
Closed

RISC-V implementation contains likely GPL-3 code #319

brson opened this issue Oct 20, 2019 · 23 comments · Fixed by #499

Comments

@brson
Copy link
Contributor

brson commented Oct 20, 2019

From riscv32.rs:

intrinsics! {
    // Implementation from gcc
    // https://raw.githubusercontent.com/gcc-mirror/gcc/master/libgcc/config/epiphany/mulsi3.c
    pub extern "C" fn __mulsi3(a: u32, b: u32) -> u32 {
        let (mut a, mut b) = (a, b);
        let mut r = 0;

        while a > 0 {
            if a & 1 > 0 {
                r += b;
            }
            a >>= 1;
            b <<= 1;
        }

        r
    }
}

That link contains a GPL-3 license.

It's not ok to read GPL code and then reimplement it for Rust, let alone copy it verbatim.

@mati865
Copy link

mati865 commented Oct 20, 2019

Compiler Rt provides Apache-2.0 version of this function but in assembly: https://github.com/llvm-mirror/compiler-rt/blob/7a739a0dfb6d408c6d587e5c7b52abd89fc3fdd3/lib/builtins/riscv/mulsi3.S

@gnzlbg
Copy link

gnzlbg commented Oct 20, 2019

This was submitted in #251 by @dvc94ch and reviewed by @japaric .

I don't think we can just delete the code without providing a replacement because I recall that Debian ships rust for riscv-32 (cc @infinity0, is that correct?).

@infinity0
Copy link

@gnzlbg no, Debian doesn't ship any riscv-32. we are beginning to ship riscv-64 for some things like gcc, but are blocked on llvm for rustc. Would be nice to eventually get riscv-64 for rustc though.

@skade
Copy link

skade commented Oct 29, 2019

Is this the only instance of this happening in RISC-V? @dvc94ch, can you confirm that this is the only instance of copied/ported gcc code?

@skade
Copy link

skade commented Oct 29, 2019

Note: The copyright holder of this code is Embecosm.

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

@brson how do you suggest impl compatible implementations? To me __mulsi3 does not constitute a specification. So anyone who has ever looked at gcc code can't impl a c compiler for the rest of their career? I find it absurd

@bjorn3
Copy link
Member

bjorn3 commented Oct 29, 2019

This code is equivalent to the llvm compiler-builtins riscv asm implementation: https://github.com/llvm/llvm-project/blob/57b08b0944046a6a57ee9b7b479181f548a5b9b4/compiler-rt/lib/builtins/riscv/mulsi3.S#L15

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

Also if someone wants to sue I can take the heat. I'm moving to Phuket in Feb I doubt that's in a US or European jurisdiction

@gnzlbg
Copy link

gnzlbg commented Oct 29, 2019

@dvc94ch I'm not a lawyer and this isn't legal advice, but the problem isn't you getting sued. The problem would be if a company which has a product that uses Rust gets sued, since it's their job to check that their product licenses comply with the regulations they operate in. It doesn't really matter what happens afterwards (*) since that alone would probably severely hurt the Rust project and make it unviable in the eyes of many for any commercial application. Particularly if we knew about the issue and didn't do anything about it.

EDIT: also @dvc94ch, the purpose of finding if there is more code like this anywhere is to be able to fix things and protect the project, not to assign blame. Stuff like this happens on every big project, that's kind of unavoidable. The main difference is whether the project appropriately reacts to these issues or "ignores" them.

(*) Such a company could try to sue the Rust project, and the Rust project could maybe try to sue you, but that's kind of pointless if there isn't much money to be obtained from any of that, and also none of that would repair any damage done to the Rust project.

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

Well it's not plagiarism, since the source was accredited. Copyright or patent infringement, are vague concept, which are completely divorced from questions of morality

@gnzlbg
Copy link

gnzlbg commented Oct 29, 2019

Nobody is talking about plagiarism. We tell Rust users that they can compile a Rust binary and by default no GPL licensed source code will be embedded into the binary, which allows them to sell the binary without having to provide any source code. Your patch silently includes GNU licensed source code in all Rust binaries on RISC-V, making anyone producing those binaries without releasing the source code liable in many jurisdictions.

You have been asked twice whether this is the only case where you have submitted GNU licensed source code to the Rust project, and you have deflected the question twice.

If you don't want or can't help fixing this issue, that's ok, please just say so, so that we can factor that in the discussion of possible solutions.

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

The cabi references the spec, targets do not reference the cortexm targets, rt I think references cortexm rt and the official sdk, e310x references the spec, board support crates reference their schematics

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

I'll submit a PR to remove it

@gnzlbg
Copy link

gnzlbg commented Oct 29, 2019

I'll submit a PR to remove it

Waait! Let's wait and see what the others think. We might need to replace this code with something else, or maybe that GCC code came from somewhere else (e.g. LLVM also has it) or that GCC code is available under multiple licenses or probably other things.

The cabi references the spec, targets do not reference the cortexm targets, rt I think references cortexm rt and the official sdk, e310x references the spec, board support crates reference their schematics

Maybe we should open other issues to re-check these as well and make sure that they are ok ?

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

You are welcome to do that if that is what wish to spend your time doing. I personally can think of more interesting things to do.

@japaric
Copy link
Member

japaric commented Oct 29, 2019

__mulsi

As mentioned in #251 (comment) only RISC-V targets without the +m extension may use the __mulsi intrinsic. Of the 5 RISC-V targets built into the compiler (see rustc --print target-list | grep riscv; checked with nightly-2019-10-27) only the riscv32i-unknown-none-elf target may use this intrinsic (*). By "may use" I mean that this symbol may appear in binaries built for this target; for all other targets the __mulsi symbol will be GC-ed by the linker.

So if we remove this intrinsic then the only affected target will be riscv32i-unknown-none-elf: linking may fail in some cases, specially when building without optimizations.

(*) you can check yourself with:

// foo.rs
#![crate_type = "lib"]
#![no_std]

static mut X: i32 = 0;
static mut Y: i32 = 0;
static mut Z: i32 = 0;

#[no_mangle]
unsafe fn foo() {
    let z = X * Y; // <- mulsi
    Z = z;
}

#[no_mangle]
unsafe fn bar() {
    X += 1;
    Y += 1;
    Z += 1;
}
$ rustup component add llvm-tools-preview
$ T = riscv32i-unknown-none-elf
$ rustup target add $T
$ rustc --target $T -O --emit=obj foo.rs
$ $(find $(rustc --print sysroot) -name llvm-nm) -C foo.o
00000000 b foo::X::h5c225369fd0fbfc7
00000004 b foo::Y::hec3ce46c3c468fa7
00000008 b foo::Z::h93ab4cd4e1fe8b9c
         U __mulsi3
00000000 T bar
00000000 T foo

@dvc94ch
Copy link
Contributor

dvc94ch commented Oct 29, 2019

This seems to be Booth's multiplication algorithm, which would not make it a gcc invention.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 26, 2019

This seems to be Booth's multiplication algorithm, which would not make it a gcc invention.

This is not Booth; it is a simpler method that long predates Booth. (A separate Wikipedia article also mentions it in the context of shift-and-add multiplication algorithms.)

(I'm not trying to use that as an argument for keeping the code as written. I mostly just wanted to point it out since I just wasted some time trying to understand how the code here could possibly correspond to Booth's multiplication algorithm.)

@dangerrust
Copy link

First of all, that code is so simple that it can never be copyrighted nor thralled to a licence. You cannot just write some extremely trivial code and call it "copyrighted!" It would make it impossible for programming to be, for how will we know whether some code is an independent invention or a theft?

Second of all, that algorithm being so simple, it is already well-established and now I can say with double confidence, it is completely uncopyrightable. We call that algorithm in my country "The Serf-farmer Multiplication." The GPL-3 licence improperly placed on it is, I think, an insult and an an arrogant lie.

I advice that the issue be closed because it was noticed by the compliance department of my employer. When they inspected more closely the claim of GPL-3 presence they concluded as I note above that this is not true. Still, I fear that some might be less attentive and reject the builds to RISC-V because of this copyright concern-troll.

@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 6, 2021

Also unless you're running a riscv core on an fpga you almost certainly have a hardware multiplier. So this wouldn't be used on any commercially available chip.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 7, 2021

I think the best argument that there is no danger here is not based on algorithmic simplicity, but rather on how the original code was actually licensed.

The linked code in question explicitly says that it grants additional permissions, specifically:

Under Section 7 of GPL version 3, you are granted additional
permissions described in the GCC Runtime Library Exception, version
3.1, as published by the Free Software Foundation.

The GCC Runtime Library Exception, version 3.1, is documented here.

The crux of the exception, from my reading, is these two paragraphs:

[...]
A Compilation Process is "Eligible" if it is done using GCC, alone or with other GPL-compatible software, or if it is done without using any work based on GCC. For example, using non-GPL-compatible Software to optimize any GCC intermediate representations would not qualify as an Eligible Compilation Process.

1. Grant of Additional Permission.

You have permission to propagate a work of Target Code formed by combining the Runtime Library with Independent Modules, even if such propagation would otherwise violate the terms of GPLv3, provided that all Target Code was generated by Eligible Compilation Processes. You may then convey such a combination under terms of your choice, consistent with the licensing of the Independent Modules.
[...]

@skade , you and I looked at this ages ago, but I overlooked this detail at that time. Do you think we can close this (modulo update below) based on the reasoning I've provided here? Or do you think we should take more explicit action to ensure that the conditions described above are met? (Namely, I think rustc should be considered an "eligible compilation process", but that's a detail that maybe is better established by an actual lawyer.)

Update: Okay, there is one action item we would need to do before closing if this is the route we take: We would need to explicitly put appropriate license text into the riscv32.rs file in compiler-builtins. (I assume "appropriate license text" here would be GPLv3 with the GCC Runtime Library Exception v3.1.)

@joshtriplett
Copy link
Member

I don't think we should be making compiler-builtins GPL-with-runtime-library-exception. It seems like there's a version from LLVM that we could switch to, and keep the existing compiler-builtins license.

@nbdd0121
Copy link
Contributor

I tried to write it myself without looking at any existing code. I ended up producing this:

pub extern "C" fn __mulsi3(mut a: u32, mut b: u32) -> u32 {
    let mut ret = 0;
    while a != 0 {
        if a & 1 != 0 {
            ret += b;
        }
        a >>= 1;
        b <<= 1;
    }
    ret
}

when compared to the allegedly GPL-3 code, there are only subtle differences from what's currently implemented (variable names, != in place of > and mut in signature).

I am pretty certain that most people given the task to write such a function would end up with very similar code. So I think it's very reasonable to claim that the code isn't copyrightable and the issue could be closed.

tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Feb 23, 2025
Rename the `musl-bitwise-tests` feature to `test-musl-serialized`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.