Skip to content

fix: moved shl and shr library functions to builtin #1562

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.

Sign up for GitHub

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

Open
Angus-Bethke-Bachmann wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: moved shl and shr library functions to builtin #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ed13a3
fix: moved shl and shr library functions to builtin and made them com…
Angus-Bethke-Bachmann Dec 17, 2025
125b7ab
Merge branch 'master' into anbt/PRG-3569
Angus-Bethke-Bachmann Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 0 additions & 35 deletions libs/stdlib/iec61131-st/bit_shift_functions.st
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,3 @@
(**************************
*
* SHL(IN, n)
*
* This operator implements a bitwise shift of an operand to the left.
* IN is shifted by n bit to the left and is filled from the right with zeros.
*
*************************)
{external}
FUNCTION SHL<T: ANY_BIT> : T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION

(**************************
*
* SHR(IN, n)
*
* This operator implements a bitwise shift of an operand to the right.
* IN is shifted by n bit to the right.
* If an unsigned data type is used, filling from the left with zeros ensues.
* In the case of signed data types, an arithmetic shifting is implemented,
* i.e. it is filled with the value of the highest bit.
*
*************************)
{external}
FUNCTION SHR<T: ANY_BIT> : T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION

(**************************
*
* ROL(IN, n)
Expand Down
56 changes: 0 additions & 56 deletions libs/stdlib/src/bit_shift_functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,5 @@
//! Defines shift operations

#[allow(non_snake_case)]
#[no_mangle]
/// Shift left operation on bytes
pub fn SHL__BYTE(input: u8, n: u32) -> u8 {
input << n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift left operation on word
pub fn SHL__WORD(input: u16, n: u32) -> u16 {
input << n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift left operation on dword
pub fn SHL__DWORD(input: u32, n: u32) -> u32 {
input << n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift left operation on lword
pub fn SHL__LWORD(input: u64, n: u32) -> u64 {
input << n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift right operation on bytes
pub fn SHR__BYTE(input: u8, n: u32) -> u8 {
input >> n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift right operation on word
pub fn SHR__WORD(input: u16, n: u32) -> u16 {
input >> n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift right operation on dword
pub fn SHR__DWORD(input: u32, n: u32) -> u32 {
input >> n
}

#[allow(non_snake_case)]
#[no_mangle]
/// Shift right operation on lword
pub fn SHR__LWORD(input: u64, n: u32) -> u64 {
input >> n
}

Comment on lines -3 to -58
Copy link
Member

@volsa volsa Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghaith why the migration from SH{L,R} as a standard library function to a built-in one? Shouldn't we try to keep the built-ins as small as possible?

#[allow(non_snake_case)]
#[no_mangle]
/// Rotate left operation on bytes
Expand Down
45 changes: 43 additions & 2 deletions libs/stdlib/tests/bit_shift_functions_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
mod common;

use common::add_std;
use plc_source::SourceCode;

use crate::common::compile_and_run;

Expand Down Expand Up @@ -30,7 +31,7 @@ fn shift_left_test() {
l := SHL(LWORD#2#0001_1001,59);
END_PROGRAM
";
let sources = add_std!(src, "bit_shift_functions.st");
let sources = SourceCode::new(src, "main.st");
let mut maintype = MainType::default();
let _res: u32 = compile_and_run(sources, &mut maintype);
assert_eq!(maintype.byte, 0b1100_1000);
Expand Down Expand Up @@ -58,7 +59,7 @@ fn shift_right_test() {
l := SHR(LWORD#16#1_0000_0000_0001,3);
END_PROGRAM
";
let sources = add_std!(src, "bit_shift_functions.st");
let sources = SourceCode::new(src, "main.st");
let mut maintype = MainType::default();
let _res: u32 = compile_and_run(sources, &mut maintype);
assert_eq!(maintype.byte, 0x2);
Expand Down Expand Up @@ -116,3 +117,43 @@ fn rotate_right_test() {
assert_eq!(maintype.dword, 0x3000_0000);
assert_eq!(maintype.lword, 0x3000_0000_0000_0000);
}

#[derive(Default, Debug)]
#[repr(C)]
struct MainTypePrg3569 {
a: u32,
}

#[test]
fn bug_prg_3569_shl_must_be_usable_with_int() {
let src = "
PROGRAM main
VAR
a : INT;
END_VAR
a := SHL(21,2);
END_PROGRAM
";
let sources = SourceCode::new(src, "main.st");
let mut maintype = MainTypePrg3569::default();
let _res: u32 = compile_and_run(sources, &mut maintype);
let shift_left = 21 << 2;
assert_eq!(maintype.a, shift_left);
}

#[test]
fn bug_prg_3569_shr_must_be_usable_with_int() {
let src = "
PROGRAM main
VAR
a : INT;
END_VAR
a := SHR(21,2);
END_PROGRAM
";
let sources = SourceCode::new(src, "main.st");
let mut maintype = MainTypePrg3569::default();
let _res: u32 = compile_and_run(sources, &mut maintype);
let shift_right = 21 >> 2;
assert_eq!(maintype.a, shift_right);
}
Comment on lines +121 to +159
Copy link
Member

@volsa volsa Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use lit for these integration tests? For context, we have decided to use lit for all of our integration tests moving forward. At some point we would also like to migrate the existing ones but that hasn't happened yet.

For reference https://github.com/PLC-lang/rusty/tree/master/tests/lit

8 changes: 0 additions & 8 deletions libs/stdlib/tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,6 @@ pub fn compile_with_native<T: Compilable>(context: &CodegenContext, source: T) -
("WCHAR_TO_WSTRING", iec61131std::string_conversion::WCHAR_TO_WSTRING as usize),
("WCHAR_TO_CHAR", iec61131std::string_conversion::WCHAR_TO_CHAR as usize),
("CHAR_TO_WCHAR", iec61131std::string_conversion::CHAR_TO_WCHAR as usize),
("SHL__BYTE", iec61131std::bit_shift_functions::SHL__BYTE as usize),
("SHL__WORD", iec61131std::bit_shift_functions::SHL__WORD as usize),
("SHL__DWORD", iec61131std::bit_shift_functions::SHL__DWORD as usize),
("SHL__LWORD", iec61131std::bit_shift_functions::SHL__LWORD as usize),
("SHR__BYTE", iec61131std::bit_shift_functions::SHR__BYTE as usize),
("SHR__WORD", iec61131std::bit_shift_functions::SHR__WORD as usize),
("SHR__DWORD", iec61131std::bit_shift_functions::SHR__DWORD as usize),
("SHR__LWORD", iec61131std::bit_shift_functions::SHR__LWORD as usize),
("ROL__BYTE", iec61131std::bit_shift_functions::ROL__BYTE as usize),
("ROL__WORD", iec61131std::bit_shift_functions::ROL__WORD as usize),
("ROL__DWORD", iec61131std::bit_shift_functions::ROL__DWORD as usize),
Expand Down
52 changes: 52 additions & 0 deletions src/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,58 @@ lazy_static! {
}
}
),
(
"SHL",
BuiltIn {
decl: "
FUNCTION SHL<T: ANY> : T
Copy link
Member

@volsa volsa Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By declaring ANY (instead of ANY_BIT as was declared previously), SHL('foo', 2) now becomes valid which will result in a panic

thread '<unnamed>' panicked at src/builtins.rs:624:74:
Found ArrayValue(ArrayValue { name: "", address: 0x7c7718007c00, is_const: false, is_const_array: false, is_const_data_array: false, is_null: false, llvm_value: "  %0 = load [4 x i8], [4 x i8]* @utf08_literal_0, align 1", llvm_type: ArrayType { array_type: Type { address: 0x7c771801f650, llvm_type: "[4 x i8]" } } }) but expected the IntValue variant
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrack

In general I'm not sure if ANY this is the correct solution here. I believe the underlying issue is a resolver "bug". I took a quick look and it seems as if the derive_generic_types call is the culprit here (the call-flow is annotate_call_statement -> update_generic_call_statement -> derive_generic_types). The function is also the reason why we get a BOOL annotation on the generic type T, at least that's what I get when debugging

#[test]
fn temp() {
    let id_provider = IdProvider::default();
    let (unit, mut index) = index_with_ids(
        r#"
        FUNCTION SHLL<T: ANY_BIT>: T
            VAR_INPUT
                IN : T;
                n : UDINT;
            END_VAR
        END_FUNCTION

        FUNCTION main
            SHLL(5, 2);
        END_FUNCTION
        "#,
        id_provider.clone(),
    );

    let annotations = annotate_with_ids(&unit, &mut index, id_provider);

    let stmt = &unit.implementations.iter().find(|imp| imp.name == "main").unwrap().statements[0];
    let AstStatement::CallStatement(CallStatement { operator, parameters: Some(parameters), .. }) =
        stmt.get_stmt()
    else {
        unreachable!();
    };

    // SHLL(5, 2);
    // ^^^^^^^^^^
    insta::assert_debug_snapshot!(annotations.get(stmt), @r#"
    Some(
        Value {
            resulting_type: "BOOL",
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type(stmt, &index), @r#"
    Some(
        DataType {
            name: "BOOL",
            initial_value: None,
            information: Integer {
                name: "BOOL",
                signed: false,
                size: 8,
                semantic_size: Some(
                    1,
                ),
            },
            nature: Bit,
            location: SourceLocation {
                span: None,
            },
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type_hint(stmt, &index), @"None");

    let AstStatement::ExpressionList(arguments) = &parameters.stmt else {
        unreachable!();
    };

    // SHLL(5, 2);
    //      ^
    insta::assert_debug_snapshot!(annotations.get(&arguments[0]), @r#"
    Some(
        Value {
            resulting_type: "DINT",
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type(&arguments[0], &index), @r#"
    Some(
        DataType {
            name: "DINT",
            initial_value: None,
            information: Integer {
                name: "DINT",
                signed: true,
                size: 32,
                semantic_size: None,
            },
            nature: Signed,
            location: SourceLocation {
                span: None,
            },
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type_hint(&arguments[0], &index), @r#"
    Some(
        DataType {
            name: "BOOL",
            initial_value: None,
            information: Integer {
                name: "BOOL",
                signed: false,
                size: 8,
                semantic_size: Some(
                    1,
                ),
            },
            nature: Bit,
            location: SourceLocation {
                span: None,
            },
        },
    )
    "#);

    // SHLL(5, 2);
    //         ^
    insta::assert_debug_snapshot!(annotations.get(&arguments[1]), @r#"
    Some(
        Value {
            resulting_type: "DINT",
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type(&arguments[1], &index), @r#"
    Some(
        DataType {
            name: "DINT",
            initial_value: None,
            information: Integer {
                name: "DINT",
                signed: true,
                size: 32,
                semantic_size: None,
            },
            nature: Signed,
            location: SourceLocation {
                span: None,
            },
        },
    )
    "#);
    insta::assert_debug_snapshot!(annotations.get_type_hint(&arguments[1], &index), @r#"
    Some(
        DataType {
            name: "UDINT",
            initial_value: None,
            information: Integer {
                name: "UDINT",
                signed: false,
                size: 32,
                semantic_size: None,
            },
            nature: Unsigned,
            location: SourceLocation {
                span: None,
            },
        },
    )
    "#);

VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION
",
annotation: None,
validation: Some(|validator, operator, parameters, _, _| {
validate_argument_count(validator, operator, &parameters, 2);
}),
generic_name_resolver: no_generic_name_resolver,
code: |generator, params, _| {
let left = generator.generate_expression(params[0])?.into_int_value();
let right = generator.generate_expression(params[1])?.into_int_value();

let shl = generator.llvm.builder.build_left_shift(left, right, "")?;

Ok(ExpressionValue::RValue(shl.as_basic_value_enum()))
}
},
),
(
"SHR",
BuiltIn {
decl: "
FUNCTION SHR<T: ANY> : T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION
",
annotation: None,
validation: Some(|validator, operator, parameters, _, _| {
validate_argument_count(validator, operator, &parameters, 2);
}),
generic_name_resolver: no_generic_name_resolver,
code: |generator, params, _| {
let left = generator.generate_expression(params[0])?.into_int_value();
let right = generator.generate_expression(params[1])?.into_int_value();

let shl = generator.llvm.builder.build_right_shift(left, right, false, "")?;

Ok(ExpressionValue::RValue(shl.as_basic_value_enum()))
}
},
),
]);
}

Expand Down
33 changes: 0 additions & 33 deletions xtask/res/combined.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,39 +914,6 @@ FUNCTION DAY_OF_WEEK : SINT
VAR_INPUT
in : DATE;
END_VAR
END_FUNCTION (* *************************
*
* SHL(IN, n)
*
* This operator implements a bitwise shift of an operand to the left.
* IN is shifted by n bit to the left and is filled from the right with zeros.
*
************************ *)
{external}
FUNCTION SHL < T : ANY_BIT > : T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION

(**************************
*
* SHR(IN, n)
*
* This operator implements a bitwise shift of an operand to the right.
* IN is shifted by n bit to the right.
* If an unsigned data type is used, filling from the left with zeros ensues.
* In the case of signed data types, an arithmetic shifting is implemented,
* i.e. it is filled with the value of the highest bit.
*
************************ *)
{external}
FUNCTION SHR < T : ANY_BIT > : T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION

(**************************
Expand Down
Loading