Skip to content

Commit

Permalink
fix: correct range for overlfowing/underflowing integer assignment (n…
Browse files Browse the repository at this point in the history
…oir-lang/noir#5416)

feat: Detect subgraphs that are completely independent from inputs or outputs (noir-lang/noir#5402)
chore: Add remaining slice methods to the interpreter (noir-lang/noir#5422)
chore: add test and benchmarks for poseidon2 (noir-lang/noir#5386)
fix: Complete call stacks with no_predicates (noir-lang/noir#5418)
feat: lsp rename/find-all-references for traits (noir-lang/noir#5409)
feat: lsp "go to definition" for modules (noir-lang/noir#5406)
fix: lsp find struct reference in return locations and paths (noir-lang/noir#5404)
feat: Sync from aztec-packages (noir-lang/noir#5408)
  • Loading branch information
AztecBot committed Jul 6, 2024
2 parents 0f0a8e5 + 1316471 commit 299a61d
Show file tree
Hide file tree
Showing 46 changed files with 1,190 additions and 101 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
bf3a75a3f9c6926baaa1408767dd929de2f8a8f9
30c50f52a6d58163e39006b73f4eb5003afc239b
26 changes: 24 additions & 2 deletions noir/noir-repo/compiler/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub struct CustomDiagnostic {
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum DiagnosticKind {
Error,
Bug,
Warning,
}

Expand Down Expand Up @@ -62,6 +63,19 @@ impl CustomDiagnostic {
}
}

pub fn simple_bug(
primary_message: String,
secondary_message: String,
secondary_span: Span,
) -> CustomDiagnostic {
CustomDiagnostic {
message: primary_message,
secondaries: vec![CustomLabel::new(secondary_message, secondary_span)],
notes: Vec::new(),
kind: DiagnosticKind::Bug,
}
}

pub fn in_file(self, file_id: fm::FileId) -> FileDiagnostic {
FileDiagnostic::new(file_id, self)
}
Expand All @@ -81,6 +95,10 @@ impl CustomDiagnostic {
pub fn is_warning(&self) -> bool {
matches!(self.kind, DiagnosticKind::Warning)
}

pub fn is_bug(&self) -> bool {
matches!(self.kind, DiagnosticKind::Bug)
}
}

impl std::fmt::Display for CustomDiagnostic {
Expand Down Expand Up @@ -120,10 +138,13 @@ pub fn report_all<'files>(
silence_warnings: bool,
) -> ReportedErrors {
// Report warnings before any errors
let (warnings, mut errors): (Vec<_>, _) =
diagnostics.iter().partition(|item| item.diagnostic.is_warning());
let (warnings_and_bugs, mut errors): (Vec<_>, _) =
diagnostics.iter().partition(|item| !item.diagnostic.is_error());

let (warnings, mut bugs): (Vec<_>, _) =
warnings_and_bugs.iter().partition(|item| item.diagnostic.is_warning());
let mut diagnostics = if silence_warnings { Vec::new() } else { warnings };
diagnostics.append(&mut bugs);
diagnostics.append(&mut errors);

let error_count =
Expand Down Expand Up @@ -170,6 +191,7 @@ fn convert_diagnostic(
) -> Diagnostic<fm::FileId> {
let diagnostic = match (cd.kind, deny_warnings) {
(DiagnosticKind::Warning, false) => Diagnostic::warning(),
(DiagnosticKind::Bug, ..) => Diagnostic::bug(),
_ => Diagnostic::error(),
};

Expand Down
29 changes: 27 additions & 2 deletions noir/noir-repo/compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@ pub enum RuntimeError {
IndexOutOfBounds { index: usize, array_size: usize, call_stack: CallStack },
#[error("Range constraint of {num_bits} bits is too large for the Field size")]
InvalidRangeConstraint { num_bits: u32, call_stack: CallStack },
#[error("{value} does not fit within the type bounds for {typ}")]
IntegerOutOfBounds { value: FieldElement, typ: NumericType, call_stack: CallStack },
#[error("The value `{value:?}` cannot fit into `{typ}` which has range `{range}`")]
IntegerOutOfBounds {
value: FieldElement,
typ: NumericType,
range: String,
call_stack: CallStack,
},
#[error("Expected array index to fit into a u64")]
TypeConversion { from: String, into: String, call_stack: CallStack },
#[error("{name:?} is not initialized")]
Expand Down Expand Up @@ -56,6 +61,7 @@ pub enum RuntimeError {
#[derive(Debug, Clone, Serialize, Deserialize)]
pub enum SsaReport {
Warning(InternalWarning),
Bug(InternalBug),
}

impl From<SsaReport> for FileDiagnostic {
Expand All @@ -78,6 +84,19 @@ impl From<SsaReport> for FileDiagnostic {
Diagnostic::simple_warning(message, secondary_message, location.span);
diagnostic.in_file(file_id).with_call_stack(call_stack)
}
SsaReport::Bug(bug) => {
let message = bug.to_string();
let (secondary_message, call_stack) = match bug {
InternalBug::IndependentSubgraph { call_stack } => {
("There is no path from the output of this brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(),call_stack)
}
};
let call_stack = vecmap(call_stack, |location| location);
let file_id = call_stack.last().map(|location| location.file).unwrap_or_default();
let location = call_stack.last().expect("Expected RuntimeError to have a location");
let diagnostic = Diagnostic::simple_bug(message, secondary_message, location.span);
diagnostic.in_file(file_id).with_call_stack(call_stack)
}
}
}
}
Expand All @@ -90,6 +109,12 @@ pub enum InternalWarning {
VerifyProof { call_stack: CallStack },
}

#[derive(Debug, PartialEq, Eq, Clone, Error, Serialize, Deserialize)]
pub enum InternalBug {
#[error("Input to brillig function is in a separate subgraph to output")]
IndependentSubgraph { call_stack: CallStack },
}

#[derive(Debug, PartialEq, Eq, Clone, Error)]
pub enum InternalError {
#[error("ICE: Both expressions should have degree<=1")]
Expand Down
20 changes: 16 additions & 4 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub mod ir;
mod opt;
pub mod ssa_gen;

pub(crate) struct ArtifactsAndWarnings(Artifacts, Vec<SsaReport>);

/// Optimize the given program by converting it into SSA
/// form and performing optimizations there. When finished,
/// convert the final SSA into an ACIR program and return it.
Expand All @@ -49,10 +51,10 @@ pub mod ssa_gen;
pub(crate) fn optimize_into_acir(
program: Program,
options: &SsaEvaluatorOptions,
) -> Result<Artifacts, RuntimeError> {
) -> Result<ArtifactsAndWarnings, RuntimeError> {
let ssa_gen_span = span!(Level::TRACE, "ssa_generation");
let ssa_gen_span_guard = ssa_gen_span.enter();
let ssa = SsaBuilder::new(
let mut ssa = SsaBuilder::new(
program,
options.enable_ssa_logging,
options.force_brillig_output,
Expand Down Expand Up @@ -89,13 +91,15 @@ pub(crate) fn optimize_into_acir(
.run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:")
.finish();

let ssa_level_warnings = ssa.check_for_underconstrained_values();
let brillig = time("SSA to Brillig", options.print_codegen_timings, || {
ssa.to_brillig(options.enable_brillig_logging)
});

drop(ssa_gen_span_guard);

time("SSA to ACIR", options.print_codegen_timings, || ssa.into_acir(&brillig))
let artifacts = time("SSA to ACIR", options.print_codegen_timings, || ssa.into_acir(&brillig))?;
Ok(ArtifactsAndWarnings(artifacts, ssa_level_warnings))
}

// Helper to time SSA passes
Expand Down Expand Up @@ -149,6 +153,10 @@ impl SsaProgramArtifact {
}
self.names.push(circuit_artifact.name);
}

fn add_warnings(&mut self, mut warnings: Vec<SsaReport>) {
self.warnings.append(&mut warnings);
}
}

pub struct SsaEvaluatorOptions {
Expand Down Expand Up @@ -179,14 +187,18 @@ pub fn create_program(
let func_sigs = program.function_signatures.clone();

let recursive = program.recursive;
let (generated_acirs, generated_brillig, error_types) = optimize_into_acir(program, options)?;
let ArtifactsAndWarnings((generated_acirs, generated_brillig, error_types), ssa_level_warnings) =
optimize_into_acir(program, options)?;
assert_eq!(
generated_acirs.len(),
func_sigs.len(),
"The generated ACIRs should match the supplied function signatures"
);

let mut program_artifact = SsaProgramArtifact::new(generated_brillig, error_types);

// Add warnings collected at the Ssa stage
program_artifact.add_warnings(ssa_level_warnings);
// For setting up the ABI we need separately specify main's input and return witnesses
let mut is_main = true;
for (acir, func_sig) in generated_acirs.into_iter().zip(func_sigs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,8 @@ impl<'a> Context<'a> {
}

warnings.extend(return_warnings);

// Add the warnings from the alter Ssa passes
Ok(self.acir_context.finish(input_witness, return_witnesses, warnings))
}

Expand Down
56 changes: 35 additions & 21 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,37 @@ impl NumericType {
}
}

/// Returns true if the given Field value is within the numeric limits
/// for the current NumericType.
pub(crate) fn value_is_within_limits(self, field: FieldElement, negative: bool) -> bool {
/// Returns None if the given Field value is within the numeric limits
/// for the current NumericType. Otherwise returns a string describing
/// the limits, as a range.
pub(crate) fn value_is_outside_limits(
self,
field: FieldElement,
negative: bool,
) -> Option<String> {
match self {
NumericType::Unsigned { bit_size } => {
let max = 2u128.pow(bit_size) - 1;
if negative {
return false;
return Some(format!("0..={}", max));
}
if field <= max.into() {
None
} else {
Some(format!("0..={}", max))
}
let max = 2u128.pow(bit_size) - 1;
field <= max.into()
}
NumericType::Signed { bit_size } => {
let max =
if negative { 2u128.pow(bit_size - 1) } else { 2u128.pow(bit_size - 1) - 1 };
field <= max.into()
let min = 2u128.pow(bit_size - 1);
let max = 2u128.pow(bit_size - 1) - 1;
let target_max = if negative { min } else { max };
if field <= target_max.into() {
None
} else {
Some(format!("-{}..={}", min, max))
}
}
NumericType::NativeField => true,
NumericType::NativeField => None,
}
}
}
Expand Down Expand Up @@ -224,21 +238,21 @@ mod tests {
use super::*;

#[test]
fn test_u8_is_within_limits() {
fn test_u8_value_is_outside_limits() {
let u8 = NumericType::Unsigned { bit_size: 8 };
assert!(!u8.value_is_within_limits(FieldElement::from(1_i128), true));
assert!(u8.value_is_within_limits(FieldElement::from(0_i128), false));
assert!(u8.value_is_within_limits(FieldElement::from(255_i128), false));
assert!(!u8.value_is_within_limits(FieldElement::from(256_i128), false));
assert!(u8.value_is_outside_limits(FieldElement::from(1_i128), true).is_some());
assert!(u8.value_is_outside_limits(FieldElement::from(0_i128), false).is_none());
assert!(u8.value_is_outside_limits(FieldElement::from(255_i128), false).is_none());
assert!(u8.value_is_outside_limits(FieldElement::from(256_i128), false).is_some());
}

#[test]
fn test_i8_is_within_limits() {
fn test_i8_value_is_outside_limits() {
let i8 = NumericType::Signed { bit_size: 8 };
assert!(!i8.value_is_within_limits(FieldElement::from(129_i128), true));
assert!(i8.value_is_within_limits(FieldElement::from(128_i128), true));
assert!(i8.value_is_within_limits(FieldElement::from(0_i128), false));
assert!(i8.value_is_within_limits(FieldElement::from(127_i128), false));
assert!(!i8.value_is_within_limits(FieldElement::from(128_i128), false));
assert!(i8.value_is_outside_limits(FieldElement::from(129_i128), true).is_some());
assert!(i8.value_is_outside_limits(FieldElement::from(128_i128), true).is_none());
assert!(i8.value_is_outside_limits(FieldElement::from(0_i128), false).is_none());
assert!(i8.value_is_outside_limits(FieldElement::from(127_i128), false).is_none());
assert!(i8.value_is_outside_limits(FieldElement::from(128_i128), false).is_some());
}
}
Loading

0 comments on commit 299a61d

Please sign in to comment.