From 387afc805ec2a4904e1c60fe28fad46bf2e54ebf Mon Sep 17 00:00:00 2001
From: Gabor Greif <gabor@dfinity.org>
Date: Wed, 7 Oct 2020 19:42:20 +0200
Subject: [PATCH] debug: Normalise value prior to right shifts (#2276)

* normalise value prior to right shifts

by first left-aligning (shift left by 32 bits)
then shifting back (respecting signedness)

* Update crates/debug/src/transform/expression.rs

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

* Update crates/debug/src/transform/expression.rs

* Update crates/debug/src/transform/expression.rs

* update translation of DW_OP_shr in test

* add translation test for DW_OP_shra

* explain normalisation

* optimise the expression by performing only one right shift

We assume that the expression evaluator permits collapsing
two shifts as long as they go in the same direction.

Review feedback.

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
---
 crates/debug/src/transform/expression.rs | 47 ++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/crates/debug/src/transform/expression.rs b/crates/debug/src/transform/expression.rs
index 991de99ce0..3d9985b2fa 100644
--- a/crates/debug/src/transform/expression.rs
+++ b/crates/debug/src/transform/expression.rs
@@ -547,8 +547,6 @@ where
                 | Operation::And { .. }
                 | Operation::Or { .. }
                 | Operation::Xor { .. }
-                | Operation::Shr { .. }
-                | Operation::Shra { .. }
                 | Operation::Shl { .. }
                 | Operation::Plus { .. }
                 | Operation::Minus { .. }
@@ -604,6 +602,24 @@ where
                     // Don't re-enter the loop here (i.e. continue), because the
                     // DW_OP_deref still needs to be kept.
                 }
+                Operation::Shr { .. } | Operation::Shra { .. } => {
+                    // Insert value normalisation part.
+                    // The semantic value is 32 bits (TODO: check unit)
+                    // but the target architecture is 64-bits. So we'll
+                    // clean out the upper 32 bits (in a sign-correct way)
+                    // to avoid contamination of the result with randomness.
+                    let mut writer = ExpressionWriter::new();
+                    writer.write_op(gimli::constants::DW_OP_plus_uconst)?;
+                    writer.write_uleb128(32)?; // increase shift amount
+                    writer.write_op(gimli::constants::DW_OP_swap)?;
+                    writer.write_op(gimli::constants::DW_OP_const1u)?;
+                    writer.write_u8(32)?;
+                    writer.write_op(gimli::constants::DW_OP_shl)?;
+                    writer.write_op(gimli::constants::DW_OP_swap)?;
+                    code_chunk.extend(writer.into_vec());
+                    // Don't re-enter the loop here (i.e. continue), because the
+                    // DW_OP_shr* still needs to be kept.
+                }
                 Operation::Address { .. }
                 | Operation::AddressIndex { .. }
                 | Operation::Call { .. }
@@ -939,6 +955,31 @@ mod tests {
             }
         );
 
+        let e = expression!(
+            DW_OP_WASM_location,
+            0x0,
+            1,
+            DW_OP_lit16,
+            DW_OP_shra,
+            DW_OP_stack_value
+        );
+        let ce = compile_expression(&e, DWARF_ENCODING, None)
+            .expect("non-error")
+            .expect("expression");
+        assert_eq!(
+            ce,
+            CompiledExpression {
+                parts: vec![
+                    CompiledExpressionPart::Local {
+                        label: val1,
+                        trailing: false
+                    },
+                    CompiledExpressionPart::Code(vec![64, 35, 32, 22, 8, 32, 36, 22, 38, 159])
+                ],
+                need_deref: false,
+            }
+        );
+
         let e = expression!(
             DW_OP_lit1,
             DW_OP_dup,
@@ -979,7 +1020,7 @@ mod tests {
                         conditionally: true,
                         target: targets[0].clone(),
                     },
-                    CompiledExpressionPart::Code(vec![22, 37]),
+                    CompiledExpressionPart::Code(vec![22, 35, 32, 22, 8, 32, 36, 22, 37]),
                     CompiledExpressionPart::Jump {
                         conditionally: false,
                         target: targets[1].clone(),