Skip to content

Commit

Permalink
fix: Write diagnostics to stderr (vs stdout)
Browse files Browse the repository at this point in the history
Before this change (even when logging was not enabled) SP1 was writing diagnostic messages to stdout, breaking the implicit contract that only the application logic should decide what gets written to stdout.
This contract is important, e.g. for passing the output for further processing in shell-driven use cases:

```sh
cat ./input-data.json | my-app | another-app
```

This change aims to fix this issue, and to safeguard against it (to a reasonable extent) in the future.

I've left alone tests, examples and build scripts, and tried to make the change as narrow as possible and to affect only the production code paths.
  • Loading branch information
imikushin committed Jan 24, 2025
1 parent 5972269 commit 39c5f85
Show file tree
Hide file tree
Showing 59 changed files with 109 additions and 23 deletions.
10 changes: 5 additions & 5 deletions crates/core/executor/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,12 @@ impl<'a> Executor<'a> {
});

if let Some(trace_buf) = trace_buf {
println!("Profiling enabled");
eprintln!("Profiling enabled");

let sample_rate = std::env::var("TRACE_SAMPLE_RATE")
.ok()
.and_then(|rate| {
println!("Profiling sample rate: {rate}");
eprintln!("Profiling sample rate: {rate}");
rate.parse::<u32>().ok()
})
.unwrap_or(1);
Expand Down Expand Up @@ -1341,7 +1341,7 @@ impl<'a> Executor<'a> {
// See https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md#instruction-aliases
return Err(ExecutionError::Unimplemented());
} else {
println!("unreachable: {:?}", instruction.opcode);
eprintln!("unreachable: {:?}", instruction.opcode);
unreachable!()
}

Expand Down Expand Up @@ -2000,10 +2000,10 @@ impl<'a> Executor<'a> {
if !buf.is_empty() {
match fd {
1 => {
println!("stdout: {buf}");
eprintln!("stdout: {buf}");
}
2 => {
println!("stderr: {buf}");
eprintln!("stderr: {buf}");
}
_ => {}
}
Expand Down
2 changes: 2 additions & 0 deletions crates/core/executor/src/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,8 @@ fn pad_to_be(val: &BigUint, len: usize) -> Vec<u8> {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;

#[test]
Expand Down
1 change: 1 addition & 0 deletions crates/core/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#![allow(clippy::missing_errors_doc)]
#![allow(clippy::explicit_iter_loop)]
#![allow(clippy::struct_excessive_bools)]
#![warn(clippy::print_stdout)]
#![warn(missing_docs)]

mod air;
Expand Down
4 changes: 2 additions & 2 deletions crates/core/executor/src/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,9 @@ impl Profiler {

pb.finish();

println!("Writing profile, this can take awhile");
eprintln!("Writing profile, this can take awhile");
serde_json::to_writer(writer, &profile_builder.to_serializable())?;
println!("Profile written successfully");
eprintln!("Profile written successfully");

Ok(())
}
Expand Down
4 changes: 2 additions & 2 deletions crates/core/executor/src/syscalls/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ impl Syscall for WriteSyscall {
// If the string does not match any known command, print it to stdout.
let flush_s = update_io_buf(ctx, fd, s);
if !flush_s.is_empty() {
flush_s.into_iter().for_each(|line| println!("stdout: {}", line));
flush_s.into_iter().for_each(|line| eprintln!("stdout: {}", line));
}
}
}
} else if fd == 2 {
let s = core::str::from_utf8(slice).unwrap();
let flush_s = update_io_buf(ctx, fd, s);
if !flush_s.is_empty() {
flush_s.into_iter().for_each(|line| println!("stderr: {}", line));
flush_s.into_iter().for_each(|line| eprintln!("stderr: {}", line));
}
} else if fd <= LOWEST_ALLOWED_FD {
if std::env::var("SP1_ALLOW_DEPRECATED_HOOKS")
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/alu/add_sub/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use p3_baby_bear::BabyBear;
use p3_matrix::dense::RowMajorMatrix;
use rand::{thread_rng, Rng};
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/alu/bitwise/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use p3_baby_bear::BabyBear;
use p3_matrix::dense::RowMajorMatrix;
use rand::{thread_rng, Rng};
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/alu/divrem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,7 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use crate::{
io::SP1Stdin,
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/alu/lt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use std::borrow::BorrowMut;

Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/alu/sll/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use std::borrow::BorrowMut;

Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/alu/sr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use std::borrow::BorrowMut;

use crate::{
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/bytes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ impl<F: Field> ByteChip<F> {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use p3_baby_bear::BabyBear;
use std::time::Instant;

Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/bytes/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ pub const fn shr_carry(input: u8, rotation: u8) -> (u8, u8) {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;

/// Tests the `shr_carry` function.
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/global/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;
use crate::programs::tests::*;
use p3_baby_bear::BabyBear;
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
deprecated,
incomplete_features
)]
#![warn(clippy::print_stdout)]
#![warn(unused_extern_crates)]

pub mod air;
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/memory/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;
use crate::programs::tests::*;
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/memory/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use crate::programs::tests::*;
use crate::{
memory::MemoryLocalChip, riscv::RiscvAir,
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/operations/field/field_den.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use num::BigUint;
use p3_air::BaseAir;
use p3_field::{Field, PrimeField32};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use num::BigUint;
use p3_air::BaseAir;
use p3_field::{Field, PrimeField32};
Expand Down
2 changes: 2 additions & 0 deletions crates/core/machine/src/operations/field/field_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,8 @@ impl<V: Copy, P: FieldParameters> FieldOpCols<V, P> {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use num::BigUint;
use p3_air::BaseAir;
use p3_field::{Field, PrimeField32};
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/program/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ where

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use std::sync::Arc;

Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/riscv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ impl<F: PrimeField32> fmt::Debug for RiscvAir<F> {

#[cfg(test)]
#[allow(non_snake_case)]
#[allow(clippy::print_stdout)]
pub mod tests {

use crate::{
Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/shape/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ pub fn create_dummy_record(shape: &Shape<RiscvAirId>) -> ExecutionRecord {

#[cfg(test)]
pub mod tests {
#![allow(clippy::print_stdout)]

use hashbrown::HashSet;
use sp1_stark::{Dom, MachineProver, StarkGenericConfig};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub fn sha_extend(w: &mut [u32]) {

#[cfg(test)]
pub mod extend_tests {
#![allow(clippy::print_stdout)]

use p3_baby_bear::BabyBear;

Expand Down
1 change: 1 addition & 0 deletions crates/core/machine/src/utils/logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub fn setup_logger() {
.with_thread_names(false)
.with_env_filter(env_filter)
.with_span_events(FmtSpan::CLOSE)
.with_writer(std::io::stderr)
.finish()
.init();
}
Expand Down
2 changes: 2 additions & 0 deletions crates/cuda/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![warn(clippy::print_stdout)]

use std::{
error::Error as StdError,
future::Future,
Expand Down
2 changes: 2 additions & 0 deletions crates/curves/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![warn(clippy::print_stdout)]

pub mod edwards;
pub mod params;
// pub mod polynomial;
Expand Down
1 change: 1 addition & 0 deletions crates/curves/src/weierstrass/secp256k1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub fn secp256k1_sqrt(n: &BigUint) -> BigUint {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;
use crate::utils::biguint_from_limbs;
Expand Down
6 changes: 4 additions & 2 deletions crates/derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

#![warn(clippy::print_stdout)]

extern crate proc_macro;

use proc_macro::TokenStream;
Expand Down Expand Up @@ -322,9 +324,9 @@ pub fn cycle_tracker(_attr: TokenStream, item: TokenStream) -> TokenStream {

let result = quote! {
#visibility fn #name #generics (#inputs) #output #where_clause {
println!("cycle-tracker-start: {}", stringify!(#name));
eprintln!("cycle-tracker-start: {}", stringify!(#name));
let result = (|| #block)();
println!("cycle-tracker-end: {}", stringify!(#name));
eprintln!("cycle-tracker-end: {}", stringify!(#name));
result
}
};
Expand Down
2 changes: 2 additions & 0 deletions crates/primitives/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! sp1-primitives contains types and functions that are used in both sp1-core and sp1-zkvm.
//! Because it is imported in the zkvm entrypoint, it should be kept minimal.
#![warn(clippy::print_stdout)]

use lazy_static::lazy_static;
use p3_baby_bear::{BabyBear, DiffusionMatrixBabyBear};
use p3_field::AbstractField;
Expand Down
2 changes: 2 additions & 0 deletions crates/prover/src/build.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::print_stdout)] // okay to print to stdout: this is a build script

use std::{borrow::Borrow, path::PathBuf};

use p3_baby_bear::BabyBear;
Expand Down
2 changes: 2 additions & 0 deletions crates/prover/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#![allow(clippy::too_many_arguments)]
#![allow(clippy::new_without_default)]
#![allow(clippy::collapsible_else_if)]
#![warn(clippy::print_stdout)]

pub mod build;
pub mod components;
Expand Down Expand Up @@ -1316,6 +1317,7 @@ pub fn compress_program_from_input<C: SP1ProverComponents>(

#[cfg(test)]
pub mod tests {
#![allow(clippy::print_stdout)]

use std::{
collections::BTreeSet,
Expand Down
3 changes: 2 additions & 1 deletion crates/prover/src/shapes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ pub fn build_vk_map<C: SP1ProverComponents + 'static>(
let panic_tx = panic_tx.clone();
s.spawn(move || {
while let Ok((i, shape)) = shape_rx.lock().unwrap().recv() {
println!("shape: {:?}", shape);
eprintln!("shape: {:?}", shape);
let is_shrink = matches!(shape, SP1CompressProgramShape::Shrink(_));
let prover = prover.clone();
let shape_clone = shape.clone();
Expand Down Expand Up @@ -466,6 +466,7 @@ impl<C: SP1ProverComponents> SP1Prover<C> {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use super::*;

Expand Down
2 changes: 2 additions & 0 deletions crates/recursion/circuit/src/challenger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,8 @@ pub fn split_32<C: Config>(builder: &mut Builder<C>, val: Var<C::N>, n: usize) -

#[cfg(test)]
pub(crate) mod tests {
#![allow(clippy::print_stdout)]

use std::iter::zip;

use crate::{
Expand Down
2 changes: 2 additions & 0 deletions crates/recursion/circuit/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Copied from [`sp1_recursion_program`].
#![warn(clippy::print_stdout)]

use challenger::{
CanCopyChallenger, CanObserveVariable, DuplexChallengerVariable, FieldChallengerVariable,
MultiField32ChallengerVariable, SpongeChallengerShape,
Expand Down
2 changes: 2 additions & 0 deletions crates/recursion/compiler/src/circuit/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,8 @@ impl<C: Config<F: PrimeField64>> Reg<C> for Address<C::F> {

#[cfg(test)]
mod tests {
#![allow(clippy::print_stdout)]

use std::{collections::VecDeque, io::BufRead, iter::zip, sync::Arc};

use p3_baby_bear::DiffusionMatrixBabyBear;
Expand Down
1 change: 1 addition & 0 deletions crates/recursion/compiler/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![allow(clippy::type_complexity)]
#![allow(clippy::needless_range_loop)]
#![warn(clippy::print_stdout)]

extern crate alloc;

Expand Down
2 changes: 1 addition & 1 deletion crates/recursion/core/src/chips/batch_fri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ impl<F: PrimeField32, const DEGREE: usize> MachineAir<F> for BatchFRIChip<DEGREE
);

#[cfg(debug_assertions)]
println!(
eprintln!(
"batch fri trace dims is width: {:?}, height: {:?}",
trace.width(),
trace.height()
Expand Down
Loading

0 comments on commit 39c5f85

Please sign in to comment.