Repository: quilt/etk (language: rust)

Issue:
Labels are sometimes misaligned when using macros
Hi.  Thanks for your work on this project.  I've found a bug with label addresses when macros insert a large number of bytes into the code.  Here's how it can be reproduced....

Install etk from the master branch
`cargo install --git https://github.com/quilt/etk.git --features cli etk-asm etk-dasm`

Create the following source file, which I'll call `main.etk`
```
    %macro insert_instructions()
        push32 0x01
        pop
        push32 0x02
        pop
        push32 0x03
        pop
        push32 0x04
        pop
        push32 0x05
        pop
        push32 0x06
        pop    
        push32 0x07
        pop 
        push32 0x08
        pop 
    %end

    %push(first)
    jump
    stop
first:
    jumpdest
    pc
    pc
    push1 0x01
    %push(second)
    jumpi
    %insert_instructions()
second:
    jumpdest
 ```
 Assemble with `eas ./main.etk` to get the following output.
 ```
600556005b5858600161011d577f0000000000000000000000000000000000000000000000000000000000000001507f0000000000000000000000000000000000000000000000000000000000000002507f0000000000000000000000000000000000000000000000000000000000000003507f0000000000000000000000000000000000000000000000000000000000000004507f0000000000000000000000000000000000000000000000000000000000000005507f0000000000000000000000000000000000000000000000000000000000000006507f0000000000000000000000000000000000000000000000000000000000000007507f0000000000000000000000000000000000000000000000000000000000000008505b
 ```
 That disassembles to the following.
```
0000    60    PUSH1 0x05
0002    56    *JUMP
0003    00    *STOP
0004    5B    JUMPDEST
0005    58    PC
0006    58    PC
0007    60    PUSH1 0x01
0009    61    PUSH2 0x011d
000C    57    *JUMPI
000D    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000001
002E    50    POP
002F    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000002
0050    50    POP
0051    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000003
0072    50    POP
0073    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000004
0094    50    POP
0095    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000005
00B6    50    POP
00B7    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000006
00D8    50    POP
00D9    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000007
00FA    50    POP
00FB    7F    PUSH32 0x0000000000000000000000000000000000000000000000000000000000000008
011C    50    POP
011D    5B    JUMPDEST
```

As you can see, the first jump instruction is off by 1 byte.

This bug is not present when installing the release version via `cargo install --features cli etk-asm etk-dasm`


Discussion / hints:

Fix patch:
diff --git a/etk-asm/src/asm.rs b/etk-asm/src/asm.rs
index 622308e3..7add835a 100644
--- a/etk-asm/src/asm.rs
+++ b/etk-asm/src/asm.rs
@@ -218,7 +218,7 @@ pub struct Assembler {
     undeclared_labels: HashSet<String>,
 
     /// Pushes that are variable-sized and need to be backpatched.
-    variable_sized_push: Vec<AbstractOp>,
+    variable_sized_push: Vec<PushDef>,
 }
 
 /// A label definition.
@@ -243,6 +243,30 @@ impl LabelDef {
     }
 }
 
+/// A push definition.
+#[derive(Clone, Debug, PartialEq)]
+pub struct PushDef {
+    position: usize,
+    op: AbstractOp,
+}
+
+impl PushDef {
+    /// Create a new `PushDef`.
+    pub fn new(op: AbstractOp, position: usize) -> Self {
+        Self { op, position }
+    }
+
+    /// Get the position of the push.
+    pub fn position(&self) -> usize {
+        self.position
+    }
+
+    /// Get the op from the push.
+    pub fn op(&self) -> &AbstractOp {
+        &self.op
+    }
+}
+
 impl Assembler {
     /// Create a new `Assembler`.
     pub fn new() -> Self {
@@ -358,7 +382,10 @@ impl Assembler {
                             // Here, we set the size of the push to 2 bytes (min possible value),
                             //  as we don't know the final value of the label yet.
                             self.concrete_len += 2;
-                            self.variable_sized_push.push(op.clone());
+                            self.variable_sized_push.push(PushDef {
+                                position: self.concrete_len,
+                                op: op.clone(),
+                            });
                         } else {
                             self.concrete_len += op.size().unwrap();
                         }
@@ -390,8 +417,8 @@ impl Assembler {
     }
 
     fn backpatch_labels(&mut self) -> Result<(), Error> {
-        for op in self.variable_sized_push.iter() {
-            if let AbstractOp::Push(imm) = op {
+        for pushdef in self.variable_sized_push.iter() {
+            if let AbstractOp::Push(imm) = &pushdef.op {
                 let exp = imm
                     .tree
                     .eval_with_context((&self.declared_labels, &self.declared_macros).into());
@@ -403,7 +430,12 @@ impl Assembler {
                     if imm_size > 1 {
                         for label_value in self.declared_labels.values_mut() {
                             let labeldef = label_value.as_ref().unwrap();
+                            if labeldef.position < pushdef.position {
+                                // don't move labels that are declared earlier than this push
+                                continue;
+                            };
                             self.concrete_len += imm_size as usize - 1;
+
                             *label_value = Some(LabelDef {
                                 position: labeldef.position + imm_size as usize - 1,
                                 updated: true,


Test patch:
diff --git a/etk-asm/tests/asm.rs b/etk-asm/tests/asm.rs
index 8ef77aa2..29168436 100644
--- a/etk-asm/tests/asm.rs
+++ b/etk-asm/tests/asm.rs
@@ -321,5 +321,11 @@ fn test_variable_sized_push2() -> Result<(), Error> {
     ingester.ingest_file(source(&["variable-push2", "main3.etk"]))?;
     assert_eq!(output, hex!("610107015801"));
 
+    let mut output = Vec::new();
+    let mut ingester = Ingest::new(&mut output);
+    ingester.ingest_file(source(&["variable-push2", "main4.etk"]))?;
+    let first_push = output.get(0..2).unwrap();
+    assert_eq!(first_push, hex!("6004"));
+
     Ok(())
 }
diff --git a/etk-asm/tests/asm/variable-push2/main4.etk b/etk-asm/tests/asm/variable-push2/main4.etk
new file mode 100644
index 00000000..ac2e8d5e
--- /dev/null
+++ b/etk-asm/tests/asm/variable-push2/main4.etk
@@ -0,0 +1,32 @@
+    %macro insert_instructions()
+        push32 0x01
+        pop
+        push32 0x02
+        pop
+        push32 0x03
+        pop
+        push32 0x04
+        pop
+        push32 0x05
+        pop
+        push32 0x06
+        pop
+        push32 0x07
+        pop
+        push32 0x08
+        pop
+    %end
+
+    %push(first)
+    jump
+    stop
+first:
+    jumpdest
+    pc
+    pc
+    push1 0x01
+    %push(second)
+    jumpi
+    %insert_instructions()
+second:
+    jumpdest
\ No newline at end of file