Simplify guest linker configuration #559

shkoo · 2023-05-11T20:32:02Z

Remove custom linker script in favor of -Ttext= linker arg
Stack pointer and .text are now based on values in memory.rs instead of a separate linker script. .data and .bss follow .text.
When using "std" on the guest, we no longer need to include #![no_main] or risc0_zkvm::entry!(...); just a standard "pub fn main" will do. (Unfortunately we still have to force inclusion of the risc0 zkvm crate with "use risc0_zkvm as _" if we don't use anything else from it)

tzerrell

When building full projects, would users now need to use cargo risczero build/test instead of cargo build/test? Or are these just utilities for building/testing pure guest code (i.e. in the absence of host code)?

github-actions · 2023-05-12T00:01:47Z

Benchmark for Linux-cuda `5a16c82`

Click to hide benchmark

Test	Base	PR	%
fib/100/execute	318.2±0.18ms	317.2±1.10ms	-0.31%
fib/100/prove	2.1±0.56s	1507.4±4.87ms	-28.22%
fib/100/total	2.4±0.01s	1834.6±14.62ms	-23.56%
fib/1000/execute	332.9±0.40ms	330.0±0.32ms	-0.87%
fib/1000/prove	1968.6±160.41ms	1543.5±3.85ms	-21.59%
fib/1000/total	2.5±0.01s	1877.0±4.51ms	-24.92%
fib/10000/execute	480.7±0.84ms	450.7±3.77ms	-6.24%
fib/10000/prove	10.6±0.06s	5.5±0.65s	+-48.11%
fib/10000/total	11.1±0.04s	6.1±0.02s	+-45.05%

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default `5a16c82`

Click to hide benchmark

Test	Base	PR	%
fib/100/execute	704.0±1.58ms	685.4±1.07ms	-2.64%
fib/100/prove	3.8±0.04s	3.7±0.07s	-2.63%
fib/100/total	4.5±0.04s	4.4±0.05s	-2.22%
fib/1000/execute	699.2±9.26ms	694.0±1.12ms	-0.74%
fib/1000/prove	3.8±0.06s	3.8±0.05s	0.00%
fib/1000/total	4.5±0.05s	4.5±0.06s	0.00%
fib/10000/execute	767.4±0.64ms	748.4±1.34ms	-2.48%
fib/10000/prove	15.4±0.14s	15.2±0.11s	-1.30%
fib/10000/total	16.1±0.11s	16.1±0.05s	0.00%

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

shkoo · 2023-05-12T01:25:05Z

When building full projects, would users now need to use cargo risczero build/test instead of cargo build/test? Or are these just utilities for building/testing pure guest code (i.e. in the absence of host code)?

These are just utilities for building/testing pure guest code in the absence of host code.

Going this direction could support the other use case too though, if that's the direction we want to go!

risc0/build/src/lib.rs

tzerrell · 2023-05-12T16:27:55Z

These are just utilities for building/testing pure guest code in the absence of host code.

Going this direction could support the other use case too though, if that's the direction we want to go!

Great, that seems like a useful feature to have available! IMO, in the absence of technical benefits/problems to the two approaches, using cargo build for a "full" project is better than cargo risczero build -- but if we gain technical benefits to the latter it could be worthwhile. That's beside the point for this PR regardless though.

risc0/zkvm/src/guest/mod.rs

shkoo · 2023-05-12T17:59:53Z

After discussion with Frank and Jeremy, I'm going to refactor and work on doing the following:

Put TEXT at 0x400, stack at top of address space, everything else packed after TEXT.
Check with @tzerrell to make sure the no_main changes are OK

tzerrell · 2023-05-12T18:19:43Z

* Check with @tzerrell to make sure the no_main changes are OK

The impact of the no_main changes in the examples in the current version of this PR seemed reasonable to me. What I wonder (and probably this is worth a discussion with you) is what other ramifications there are beyond what's apparent in the examples -- in particular if there is any impact on integration with Bonsai, as that is simultaneously the most important and the most different from examples.

shkoo · 2023-05-13T00:37:53Z

The impact of the no_main changes in the examples in the current version of this PR seemed reasonable to me. What I wonder (and probably this is worth a discussion with you) is what other ramifications there are beyond what's apparent in the examples -- in particular if there is any impact on integration with Bonsai, as that is simultaneously the most important and the most different from examples.

I changed the entry code to allow "risc0_zkvm::entry!" even when in "std" mode, and reverted the changes in "examples" since we don't need them anymore, so we should be good on that front.

This should have no effect on any bonsai integration.

This should be ready to review now!

github-actions · 2023-05-13T00:53:34Z

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

risc0/build/src/lib.rs

risc0/zkvm/platform/src/syscall.rs

risc0/build/src/lib.rs

flaub

A few things:

We should ban #[doc(hidden)] or have clear justification for its use.
Each PR should try to aim to address a single thing. This seems to have a mix of linker script changes along with risczero build support, which doesn't seem directly dependent on one another.
Without an explicit linker script, it's unclear where the DATA section is. There's an _end symbol that needs more documentation or a reference/link to how this symbol is defined. Do we know if this will work with all compilers?

nategraf · 2023-05-15T17:29:01Z

On 1 and 2, both were originally included in the draft PR I put up mostly for visibility. Maybe we split that off?

Remove custom linker script in favor of -Ttext= linkeer arg Stack pointer and .text are now based on values in memory.rs instead of a separate linker script. .data and .bss follow .text. When using "std" on the guest, we no longer need to include #![no_main] or risc0_zkvm::entry!(...); just a standard "pub fn main" will do. (Unfortunately we still have to force inclusion of the risc0 zkvm crate with "use risc0_zkvm as _" if we don't use anything else from it)

…o-test

github-actions · 2023-05-16T19:59:45Z

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

github-actions · 2023-05-22T18:38:11Z

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default `08cdac9`

Click to hide benchmark

Test	Base	PR	%
fib/100/execute	29.0±0.04ms	29.0±0.03ms	0.00%
fib/100/prove	3.7±0.04s	3.7±0.07s	0.00%
fib/100/total	3.7±0.04s	3.7±0.09s	0.00%
fib/1000/execute	39.0±0.06ms	38.3±0.17ms	-1.79%
fib/1000/prove	3.7±0.06s	3.7±0.06s	0.00%
fib/1000/total	3.8±0.06s	3.7±0.07s	-2.63%
fib/10000/execute	139.2±0.24ms	133.0±0.15ms	-4.45%
fib/10000/prove	15.4±0.11s	15.2±0.06s	-1.30%
fib/10000/total	15.4±0.12s	15.4±0.15s	0.00%

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

github-actions · 2023-05-22T22:52:14Z

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

* Simplify guest linker configuration Remove custom linker script in favor of -Ttext= linkeer arg Stack pointer and .text are now based on values in memory.rs instead of a separate linker script. .data and .bss follow .text. When using "std" on the guest, we no longer need to include #![no_main] or risc0_zkvm::entry!(...); just a standard "pub fn main" will do. (Unfortunately we still have to force inclusion of the risc0 zkvm crate with "use risc0_zkvm as _" if we don't use anything else from it) * Add comments with links to how the linker works --------- Co-authored-by: nils <shkoo@users.noreply.github.com>

shkoo force-pushed the nils/cargo-risczero-test branch from 9935bf7 to 8f0f15a Compare May 11, 2023 20:48

shkoo marked this pull request as ready for review May 11, 2023 21:08

shkoo requested review from mothran and nategraf May 11, 2023 21:08

tzerrell reviewed May 11, 2023

View reviewed changes

flaub reviewed May 12, 2023

View reviewed changes

risc0/build/src/lib.rs Outdated Show resolved Hide resolved

flaub reviewed May 12, 2023

View reviewed changes

risc0/zkvm/src/guest/mod.rs Show resolved Hide resolved

shkoo marked this pull request as draft May 12, 2023 17:55

shkoo marked this pull request as ready for review May 13, 2023 00:38

flaub reviewed May 14, 2023

View reviewed changes

risc0/build/src/lib.rs Outdated Show resolved Hide resolved

flaub reviewed May 14, 2023

View reviewed changes

risc0/zkvm/platform/src/syscall.rs Show resolved Hide resolved

flaub reviewed May 14, 2023

View reviewed changes

risc0/build/src/lib.rs Outdated Show resolved Hide resolved

flaub requested changes May 14, 2023

View reviewed changes

shkoo force-pushed the nils/cargo-risczero-test branch from c03ef83 to 28bef28 Compare May 16, 2023 17:11

shkoo changed the title ~~Simplify guest builds, now with cargo risczero build~~ Simplify guest linker configuration May 16, 2023

Merge branch 'main' of github.com:risc0/risc0 into nils/cargo-risczer…

b9ea509

…o-test

shkoo requested a review from flaub May 16, 2023 17:30

Add comments with links to how the linker works

40ad291

Merge branch 'main' into nils/cargo-risczero-test

e370c1c

flaub approved these changes May 22, 2023

View reviewed changes

Merge branch 'main' into nils/cargo-risczero-test

47aa8d5

shkoo enabled auto-merge (squash) May 22, 2023 22:14

shkoo merged commit e5221ce into main May 22, 2023
12 checks passed

shkoo deleted the nils/cargo-risczero-test branch May 22, 2023 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify guest linker configuration #559

Simplify guest linker configuration #559

shkoo commented May 11, 2023 •

edited

tzerrell left a comment

github-actions bot commented May 12, 2023

shkoo commented May 12, 2023

tzerrell commented May 12, 2023

shkoo commented May 12, 2023

tzerrell commented May 12, 2023 •

edited

shkoo commented May 13, 2023

github-actions bot commented May 13, 2023

flaub left a comment

nategraf commented May 15, 2023

github-actions bot commented May 16, 2023

github-actions bot commented May 22, 2023

github-actions bot commented May 22, 2023

Simplify guest linker configuration #559

Simplify guest linker configuration #559

Conversation

shkoo commented May 11, 2023 • edited

tzerrell left a comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2023

Benchmark for Linux-cuda 5a16c82

Benchmark for Linux-default

Benchmark for macOS-default 5a16c82

Benchmark for macOS-metal

shkoo commented May 12, 2023

tzerrell commented May 12, 2023

shkoo commented May 12, 2023

tzerrell commented May 12, 2023 • edited

shkoo commented May 13, 2023

github-actions bot commented May 13, 2023

Benchmark for Linux-cuda

Benchmark for Linux-default

Benchmark for macOS-default

Benchmark for macOS-metal

flaub left a comment

Choose a reason for hiding this comment

nategraf commented May 15, 2023

github-actions bot commented May 16, 2023

Benchmark for Linux-cuda

Benchmark for Linux-default

Benchmark for macOS-default

Benchmark for macOS-metal

github-actions bot commented May 22, 2023

Benchmark for Linux-cuda

Benchmark for Linux-default

Benchmark for macOS-default 08cdac9

Benchmark for macOS-metal

github-actions bot commented May 22, 2023

Benchmark for Linux-cuda

Benchmark for Linux-default

Benchmark for macOS-default

Benchmark for macOS-metal

shkoo commented May 11, 2023 •

edited

Benchmark for Linux-cuda `5a16c82`

Benchmark for macOS-default `5a16c82`

tzerrell commented May 12, 2023 •

edited

Benchmark for macOS-default `08cdac9`