diff --git a/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs b/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs index 49208375474..78017072fe2 100644 --- a/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs +++ b/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs @@ -243,6 +243,8 @@ lazy_static! { E139, Error, include_str!("./error_codes/E139.md"), // Linker invocation failed (spawn / cmdline) E140, Error, include_str!("./error_codes/E140.md"), // ':=' used for an output parameter E141, Error, include_str!("./error_codes/E141.md"), // Member access on a non-auto-deref pointer base + // E142..E149 are used on later branches, E150 matches the same warning there + E150, Warning, include_str!("./error_codes/E150.md"), // ABS on an unsigned value has no effect ); } diff --git a/compiler/plc_diagnostics/src/diagnostics/error_codes/E150.md b/compiler/plc_diagnostics/src/diagnostics/error_codes/E150.md new file mode 100644 index 00000000000..c8fe1bbf76b --- /dev/null +++ b/compiler/plc_diagnostics/src/diagnostics/error_codes/E150.md @@ -0,0 +1,25 @@ +# E150 - ABS on a value of unsigned type has no effect + +`ABS` returns the absolute value of its argument. A value of an unsigned type cannot be +negative, so `ABS` returns the argument unchanged. Such a call usually points at a +mistake, for example an argument expression that already underflowed: + +```iecst +FUNCTION delta : UINT +VAR_INPUT + a, b : UINT; +END_VAR + // WRONG: a - b underflows for a < b, ABS does not correct the result + delta := ABS(a - b); +END_FUNCTION +``` + +If the distance between two unsigned values is needed, compare them first: + +```iecst +IF a >= b THEN + delta := a - b; +ELSE + delta := b - a; +END_IF +``` diff --git a/src/builtins.rs b/src/builtins.rs index 46f8543f302..e6cf12a99f6 100644 --- a/src/builtins.rs +++ b/src/builtins.rs @@ -469,6 +469,69 @@ lazy_static! { } } ), + // The standard library provides no ABS implementations for unsigned types; these + // builtins provide them so the generic resolver binds e.g. ABS(UINT#1) to + // ABS__UINT instead of inventing an external declaration that nothing implements + ( + "ABS__USINT", + BuiltIn { + decl: "FUNCTION ABS__USINT : USINT + VAR_INPUT + IN : USINT; + END_VAR + END_FUNCTION + ", + annotation: None, + validation: Some(validate_abs_unsigned_call), + generic_name_resolver: no_generic_name_resolver, + code: generate_abs_identity, + } + ), + ( + "ABS__UINT", + BuiltIn { + decl: "FUNCTION ABS__UINT : UINT + VAR_INPUT + IN : UINT; + END_VAR + END_FUNCTION + ", + annotation: None, + validation: Some(validate_abs_unsigned_call), + generic_name_resolver: no_generic_name_resolver, + code: generate_abs_identity, + } + ), + ( + "ABS__UDINT", + BuiltIn { + decl: "FUNCTION ABS__UDINT : UDINT + VAR_INPUT + IN : UDINT; + END_VAR + END_FUNCTION + ", + annotation: None, + validation: Some(validate_abs_unsigned_call), + generic_name_resolver: no_generic_name_resolver, + code: generate_abs_identity, + } + ), + ( + "ABS__ULINT", + BuiltIn { + decl: "FUNCTION ABS__ULINT : ULINT + VAR_INPUT + IN : ULINT; + END_VAR + END_FUNCTION + ", + annotation: None, + validation: Some(validate_abs_unsigned_call), + generic_name_resolver: no_generic_name_resolver, + code: generate_abs_identity, + } + ), // TODO: MOD and AND/OR/XOR/NOT ANY_BIT ( NOT also supports boolean ) - FIXME: these are all keywords and therefore conflicting ( "GT", @@ -1010,6 +1073,63 @@ fn validate_argument_count( } } +/// Validates a call bound to one of the unsigned ABS builtins. ABS of an unsigned value +/// returns the value unchanged, such a call is almost always a mistake (e.g. an argument +/// expression that already underflowed). +fn validate_abs_unsigned_call( + validator: &mut Validator, + operator: &AstNode, + parameters: Option<&AstNode>, + annotations: &dyn AnnotationMap, + index: &Index, +) { + validate_argument_count(validator, operator, ¶meters, 1); + + let Some(param) = parameters.map(flatten_expression_list).and_then(|it| it.first().copied()) else { + return; + }; + let param = extract_actual_parameter(param); + + // arguments whose actual type does not satisfy ANY_NUM already report E062, + // stay silent for those + if !annotations.get_type(param, index).is_some_and(|it| it.has_nature(TypeNature::Num, index)) { + return; + } + + // the argument's type hint carries the concrete type derived for T + let Some(data_type) = + annotations.get_type_hint(param, index).or_else(|| annotations.get_type(param, index)) + else { + return; + }; + + if index.get_intrinsic_type(data_type).get_type_information().is_unsigned_int() { + validator.push_diagnostic( + Diagnostic::new(format!( + "ABS on a value of unsigned type '{}' has no effect", + data_type.get_name() + )) + .with_error_code("E150") + .with_location(operator), + ); + } +} + +/// Codegen for the unsigned ABS builtins: |x| = x for unsigned values, the argument is +/// generated as-is +fn generate_abs_identity<'ink, 'b>( + generator: &'b ExpressionCodeGenerator<'ink, 'b>, + params: &[&AstNode], + location: SourceLocation, +) -> Result, CodegenError> { + if let [param] = params { + let param = extract_actual_parameter(param); + generator.generate_expression(param).map(ExpressionValue::RValue) + } else { + Err(Diagnostic::codegen_error("Expected exactly one parameter for ABS", location).into()) + } +} + fn validate_constant_parameters( validator: &mut Validator, parameters: &Option<&AstNode>, diff --git a/src/codegen/generators/pou_generator.rs b/src/codegen/generators/pou_generator.rs index 4d680b44d7a..07cc2f7b79d 100644 --- a/src/codegen/generators/pou_generator.rs +++ b/src/codegen/generators/pou_generator.rs @@ -79,7 +79,9 @@ pub fn generate_implementation_stubs<'ink>( .filter_map(|name| index.find_implementation_by_name(name).map(|it| (name, it))) .collect::>(); for (name, implementation) in implementations { - if !implementation.is_generic() { + // builtins (e.g. ABS__UINT) are generated inline by their codegen hook and must + // not leave an external declaration behind + if !implementation.is_generic() && index.get_builtin_function(name).is_none() { let curr_f = pou_generator.generate_implementation_stub( implementation, module, diff --git a/src/codegen/tests/expression_tests.rs b/src/codegen/tests/expression_tests.rs index c6c1cea3044..b64779fd128 100644 --- a/src/codegen/tests/expression_tests.rs +++ b/src/codegen/tests/expression_tests.rs @@ -468,6 +468,60 @@ fn builtin_function_call_move() { "#); } +#[test] +fn builtin_function_call_abs_unsigned_ints() { + // the generic declaration mimics the one the standard library includes provide; + // the unsigned monomorphs bind to the builtins and generate the argument as-is + let result = codegen( + "FUNCTION ABS : T + VAR_INPUT + IN : T; + END_VAR + END_FUNCTION + + PROGRAM main + VAR + a : USINT; + b : UINT; + c : UDINT; + d : ULINT; + END_VAR + a := ABS(a); + b := ABS(b); + c := ABS(c); + d := ABS(IN := d); + END_PROGRAM", + ); + + filtered_assert_snapshot!(result, @r#" + ; ModuleID = '' + source_filename = "" + target datalayout = "[filtered]" + target triple = "[filtered]" + + %main = type { i8, i16, i32, i64 } + + @main_instance = global %main zeroinitializer + + define void @main(ptr %0) { + entry: + %a = getelementptr inbounds nuw %main, ptr %0, i32 0, i32 0 + %b = getelementptr inbounds nuw %main, ptr %0, i32 0, i32 1 + %c = getelementptr inbounds nuw %main, ptr %0, i32 0, i32 2 + %d = getelementptr inbounds nuw %main, ptr %0, i32 0, i32 3 + %load_a = load i8, ptr %a, align [filtered] + store i8 %load_a, ptr %a, align [filtered] + %load_b = load i16, ptr %b, align [filtered] + store i16 %load_b, ptr %b, align [filtered] + %load_c = load i32, ptr %c, align [filtered] + store i32 %load_c, ptr %c, align [filtered] + %load_d = load i64, ptr %d, align [filtered] + store i64 %load_d, ptr %d, align [filtered] + ret void + } + "#); +} + #[test] fn builtin_function_call_sizeof() { let result = codegen( diff --git a/src/validation/statement.rs b/src/validation/statement.rs index dbbfe60c62e..fb65ca71539 100644 --- a/src/validation/statement.rs +++ b/src/validation/statement.rs @@ -1986,11 +1986,16 @@ fn validate_call( ); } - // Check if we're dealing with a builtin function and if so call its validation function - if let Some(validation) = builtins::get_builtin(fn_ident.get_flat_reference_name().unwrap_or_default()) - .and_then(BuiltIn::get_validation) - { - validation(validator, fn_ident, fn_args, context.annotations, context.index); + // Check if we're dealing with a builtin function and if so call its validation function. + // A generic call can also bind to a builtin under its resolved name (e.g. ABS(UINT#1) + // binds to ABS__UINT), so dispatch the validation of the bound builtin as well. + let flat_name = fn_ident.get_flat_reference_name().unwrap_or_default(); + let call_name = + context.annotations.get_call_name(fn_ident).filter(|it| !it.eq_ignore_ascii_case(flat_name)); + for name in std::iter::once(flat_name).chain(call_name) { + if let Some(validation) = builtins::get_builtin(name).and_then(BuiltIn::get_validation) { + validation(validator, fn_ident, fn_args, context.annotations, context.index); + } } let Some(pou) = context.find_pou(fn_ident) else { diff --git a/src/validation/tests/builtin_validation_tests.rs b/src/validation/tests/builtin_validation_tests.rs index 9d32903b120..6a0a2716807 100644 --- a/src/validation/tests/builtin_validation_tests.rs +++ b/src/validation/tests/builtin_validation_tests.rs @@ -133,3 +133,166 @@ fn shr_must_validate_types() { assert_snapshot!(&diagnostics); } + +/// Mimics the generic ABS declaration the standard library includes provide +const ABS_DECLARATION: &str = " + FUNCTION ABS : T + VAR_INPUT + IN : T; + END_VAR + END_FUNCTION +"; + +#[test] +fn abs_on_unsigned_arguments_reports_a_warning() { + let diagnostics = parse_and_validate_buffered(&format!( + "{ABS_DECLARATION} + FUNCTION main : DINT + VAR + a : USINT; + b : UINT; + c : UDINT; + d : ULINT; + END_VAR + ABS(a); + ABS(b); + ABS(c); + ABS(d); + ABS(b - UINT#10); + END_FUNCTION + ", + )); + + assert_snapshot!(diagnostics, @" + warning[E150]: ABS on a value of unsigned type 'USINT' has no effect + ┌─ :15:13 + │ + 15 │ ABS(a); + │ ^^^ ABS on a value of unsigned type 'USINT' has no effect + + warning[E150]: ABS on a value of unsigned type 'UINT' has no effect + ┌─ :16:13 + │ + 16 │ ABS(b); + │ ^^^ ABS on a value of unsigned type 'UINT' has no effect + + warning[E150]: ABS on a value of unsigned type 'UDINT' has no effect + ┌─ :17:13 + │ + 17 │ ABS(c); + │ ^^^ ABS on a value of unsigned type 'UDINT' has no effect + + warning[E150]: ABS on a value of unsigned type 'ULINT' has no effect + ┌─ :18:13 + │ + 18 │ ABS(d); + │ ^^^ ABS on a value of unsigned type 'ULINT' has no effect + + warning[E150]: ABS on a value of unsigned type 'UDINT' has no effect + ┌─ :19:13 + │ + 19 │ ABS(b - UINT#10); + │ ^^^ ABS on a value of unsigned type 'UDINT' has no effect + "); +} + +#[test] +fn abs_on_signed_or_float_arguments_reports_nothing() { + let diagnostics = parse_and_validate_buffered(&format!( + "{ABS_DECLARATION} + FUNCTION main : DINT + VAR + a : SINT; + b : INT; + c : DINT; + d : LINT; + e : REAL; + f : LREAL; + u : UINT; + END_VAR + ABS(a); + ABS(b); + ABS(c); + ABS(d); + ABS(e); + ABS(f); + // mixing an unsigned with a signed argument derives a signed type + ABS(u + b); + END_FUNCTION + ", + )); + + assert!(diagnostics.is_empty(), "expected no diagnostics but got:\n{diagnostics}"); +} + +#[test] +fn abs_on_a_bit_type_reports_no_unsigned_warning() { + let diagnostics = parse_and_validate_buffered(&format!( + "{ABS_DECLARATION} + FUNCTION main : DINT + VAR + a : BYTE; + END_VAR + ABS(a); + END_FUNCTION + ", + )); + + assert_snapshot!(diagnostics, @" + error[E062]: Invalid type nature for generic argument. BYTE is no ANY_NUMBER + ┌─ :12:17 + │ + 12 │ ABS(a); + │ ^ Invalid type nature for generic argument. BYTE is no ANY_NUMBER + "); +} + +#[test] +fn abs_with_invalid_argument_count_on_an_unsigned_monomorph() { + let diagnostics = parse_and_validate_buffered(&format!( + "{ABS_DECLARATION} + FUNCTION main : DINT + VAR + u : UINT; + END_VAR + ABS(u, u); + END_FUNCTION + ", + )); + + assert_snapshot!(diagnostics, @" + error[E032]: this POU takes 1 argument but 2 arguments were supplied + ┌─ :12:13 + │ + 12 │ ABS(u, u); + │ ^^^ this POU takes 1 argument but 2 arguments were supplied + + warning[E150]: ABS on a value of unsigned type 'UINT' has no effect + ┌─ :12:13 + │ + 12 │ ABS(u, u); + │ ^^^ ABS on a value of unsigned type 'UINT' has no effect + "); +} + +#[test] +fn abs_monomorph_called_directly_reports_the_warning() { + let diagnostics = parse_and_validate_buffered( + " + FUNCTION main : DINT + VAR + u : UDINT; + END_VAR + ABS__UDINT(u); + END_FUNCTION + ", + ); + + assert_snapshot!(diagnostics, @" + warning[E150]: ABS on a value of unsigned type 'UDINT' has no effect + ┌─ :6:13 + │ + 6 │ ABS__UDINT(u); + │ ^^^^^^^^^^ ABS on a value of unsigned type 'UDINT' has no effect + "); +} diff --git a/src/validation/tests/statement_validation_tests.rs b/src/validation/tests/statement_validation_tests.rs index e0ab6364aba..0f4e7019f79 100644 --- a/src/validation/tests/statement_validation_tests.rs +++ b/src/validation/tests/statement_validation_tests.rs @@ -1085,7 +1085,7 @@ fn builtin_functions_named_arguments_invalid_parameter_names() { ", ); - assert_snapshot!(diagnostics, @r" + assert_snapshot!(diagnostics, @" error[E089]: Invalid call parameters ┌─ :10:22 │ @@ -1122,11 +1122,11 @@ fn builtin_functions_named_arguments_invalid_parameter_names() { 14 │ arr2 := MOVE(SOURCE := arr); │ ^^^^^^ Could not resolve reference to SOURCE - error[E037]: Invalid assignment: cannot assign 'SEL with wrong parameter names a := SEL(WRONG := sel, IN0 := a, IN1 := b); a := SEL(G := sel, INVALID := a,' to 'ARRAY[0..5] OF INT' + error[E037]: Invalid assignment: cannot assign '// MOVE with wrong parameter name arr2 := MOVE(SOURCE := arr); // SIZEOF with wrong parameter name' to 'ARRAY[0..5] OF INT' ┌─ :14:13 │ 14 │ arr2 := MOVE(SOURCE := arr); - │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'SEL with wrong parameter names a := SEL(WRONG := sel, IN0 := a, IN1 := b); a := SEL(G := sel, INVALID := a,' to 'ARRAY[0..5] OF INT' + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign '// MOVE with wrong parameter name arr2 := MOVE(SOURCE := arr); // SIZEOF with wrong parameter name' to 'ARRAY[0..5] OF INT' error[E089]: Invalid call parameters ┌─ :17:25 diff --git a/tests/lit/single/builtin/abs_unsigned.st b/tests/lit/single/builtin/abs_unsigned.st new file mode 100644 index 00000000000..f8100b09903 --- /dev/null +++ b/tests/lit/single/builtin/abs_unsigned.st @@ -0,0 +1,56 @@ +// RUN: (%COMPILE %s && %RUN) | %CHECK %s +// +// ABS with an unsigned argument binds to the ABS__U* builtins and returns the +// value unchanged instead of calling a non-existent library monomorph +// (previously a crash at runtime). Signed and float arguments still call the +// library implementations. + +FUNCTION main +VAR + s : SINT := SINT#-128; + i : INT := INT#-3; + d : DINT := DINT#-10; + l : LINT := LINT#-1234567890123; + u8 : USINT := USINT#200; + u16 : UINT := UINT#5; + u32 : UDINT := UDINT#4000000000; + u64 : ULINT := ULINT#42; + r : REAL := REAL#-2.0; + lr : LREAL := LREAL#-9.5; +END_VAR + +// ABS(INT_MIN) wraps to INT_MIN +// CHECK: s=-128 +printf('s=%d$N', ABS(s)); + +// CHECK: i=3 +printf('i=%d$N', ABS(i)); + +// CHECK: d=10 +printf('d=%d$N', ABS(d)); + +// CHECK: l=1234567890123 +printf('l=%lld$N', ABS(l)); + +// CHECK: u8=200 +printf('u8=%d$N', ABS(u8)); + +// CHECK: u16=5 +printf('u16=%d$N', ABS(u16)); + +// CHECK: u32=4000000000 +printf('u32=%u$N', ABS(u32)); + +// CHECK: u64=42 +printf('u64=%llu$N', ABS(u64)); + +// CHECK: r=2.0 +printf('r=%.1f$N', ABS(r)); + +// CHECK: lr=9.5 +printf('lr=%.1f$N', ABS(lr)); + +// explicit calls to the library monomorphs still link against the stdlib +// CHECK: lib=7 +printf('lib=%d$N', ABS__DINT(DINT#-7)); +END_FUNCTION diff --git a/tests/lit/single/builtin/abs_unsigned_warning.st b/tests/lit/single/builtin/abs_unsigned_warning.st new file mode 100644 index 00000000000..aab8dd1f535 --- /dev/null +++ b/tests/lit/single/builtin/abs_unsigned_warning.st @@ -0,0 +1,25 @@ +// RUN: %COMPILE %s 2>&1 | %CHECK %s +// +// ABS of an unsigned value has no effect and reports a warning for each call. + +FUNCTION main +VAR + u8 : USINT; + u16 : UINT; + u32 : UDINT; + u64 : ULINT; + res : ULINT; +END_VAR + +// CHECK: {{.*}}warning[E150]{{.*}}ABS on a value of unsigned type 'USINT' has no effect{{.*}} +res := ABS(u8); + +// CHECK: {{.*}}warning[E150]{{.*}}ABS on a value of unsigned type 'UINT' has no effect{{.*}} +res := ABS(u16); + +// CHECK: {{.*}}warning[E150]{{.*}}ABS on a value of unsigned type 'UDINT' has no effect{{.*}} +res := ABS(u32); + +// CHECK: {{.*}}warning[E150]{{.*}}ABS on a value of unsigned type 'ULINT' has no effect{{.*}} +res := ABS(u64); +END_FUNCTION