x86: fix helper function that displaces address by a given amount.

Change the function from a non-const method to a static member to avoid
giving an impression that it modifies the old address.

Add a gtest that constructs addresses with some initial displacement and
checks that they are equal to addresses constructed in the same way only
without displacement, and then displaced.

The added gtest reveals errors in the displace function. Some of the
added test cases crashed on checks, others failed test assertions. This
shouldn't affect real world as the use of the helper function in the
compiler is limited to a few working cases.

Checks failed because some of the address constructors do not expect ESP
as base register. It is possible to construct such addresses with
another constructor like `x86::Address(x86::ESP, 0))`, but an attempt to
displace this address would try to reassemble it using a forbidden
constructor and hit the check.

Some of the failed test assertions were for Address::Absolute, such as:

  Expected equality of these values:
    x86::Address::displace(x86::Address::Absolute(0), 42)
      Which is: 42(%ebp)
    x86::Address::Absolute(42)
      Which is: (%ebp)

Other failed test assertions were due to the fact that one and the same
address can be encoded in more than one way, e.g. 32-bit displacement
normally requires mod 11b, but can also be achieved with mod 00b if EBP
is used as r/m or base. Example of a test failure:

  Expected equality of these values:
    x86::Address::displace(x86::Address(EAX, TIMES_1, 42), -42)
      Which is: 0(%ebp,%eax,1)
    x86::Address(EAX, TIMES_1, 0)
      Which is: 0(,%eax,1)

Bug: 65872996
Test: m test-art-host-gtest  # changes covered by the new test
Test: art/test.py --host -r
Change-Id: I8dcdc968d7bd9da2c0a16ef0afeb13cf9a168359

3 files changed, 93 insertions, 9 deletions

diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index 52852f2ce6..dfed2f76e3 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -1510,7 +1510,7 @@ void CodeGeneratorX86::LoadFromMemoryNoBarrier(DataType::Type dst_type,
         __ movd(dst.AsRegisterPairHigh<Register>(), temp);
       } else {
         DCHECK_NE(src.GetBaseRegister(), dst.AsRegisterPairLow<Register>());
-        Address src_high = src.displaceBy(kX86WordSize);
+        Address src_high = Address::displace(src, kX86WordSize);
         __ movl(dst.AsRegisterPairLow<Register>(), src);
         if (instr != nullptr) {
           MaybeRecordImplicitNullCheck(instr);
@@ -5873,11 +5873,11 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction,
         int64_t v = CodeGenerator::GetInt64ValueOf(value.GetConstant());
         __ movl(field_addr, Immediate(Low32Bits(v)));
         codegen_->MaybeRecordImplicitNullCheck(instruction);
-        __ movl(field_addr.displaceBy(kX86WordSize), Immediate(High32Bits(v)));
+        __ movl(Address::displace(field_addr, kX86WordSize), Immediate(High32Bits(v)));
       } else {
         __ movl(field_addr, value.AsRegisterPairLow<Register>());
         codegen_->MaybeRecordImplicitNullCheck(instruction);
-        __ movl(field_addr.displaceBy(kX86WordSize), value.AsRegisterPairHigh<Register>());
+        __ movl(Address::displace(field_addr, kX86WordSize), value.AsRegisterPairHigh<Register>());
       }
       maybe_record_implicit_null_check_done = true;
       break;
@@ -5899,7 +5899,7 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction,
         int64_t v = CodeGenerator::GetInt64ValueOf(value.GetConstant());
         __ movl(field_addr, Immediate(Low32Bits(v)));
         codegen_->MaybeRecordImplicitNullCheck(instruction);
-        __ movl(field_addr.displaceBy(kX86WordSize), Immediate(High32Bits(v)));
+        __ movl(Address::displace(field_addr, kX86WordSize), Immediate(High32Bits(v)));
         maybe_record_implicit_null_check_done = true;
       } else {
         __ movsd(field_addr, value.AsFpuRegister<XmmRegister>());
diff --git a/compiler/utils/x86/assembler_x86.h b/compiler/utils/x86/assembler_x86.h
index a9050e6df1..5ac566787f 100644
--- a/compiler/utils/x86/assembler_x86.h
+++ b/compiler/utils/x86/assembler_x86.h
@@ -94,6 +94,13 @@ class Operand : public ValueObject {
         && ((encoding_[0] & 0x07) == reg);  // Register codes match.
   }
 
+  inline bool operator==(const Operand &op) const {
+    return length_ == op.length_ &&
+        memcmp(encoding_, op.encoding_, length_) == 0 &&
+        disp_ == op.disp_ &&
+        fixup_ == op.fixup_;
+  }
+
  protected:
   // Operand can be sub classed (e.g: Address).
   Operand() : length_(0), disp_(0), fixup_(nullptr) { }
@@ -198,13 +205,43 @@ class Address : public Operand {
     SetFixup(fixup);
   }
 
-  Address displaceBy(int offset) {
-    if (rm() == ESP) {
-      // SIB addressing mode
-      return Address(base(), index(), scale(), disp() + offset, GetFixup());
+  // Break the address into pieces and reassemble it again with a new displacement.
+  // Note that it may require a new addressing mode if displacement size is changed.
+  static Address displace(const Address &addr, int32_t disp) {
+    const int32_t new_disp = addr.disp() + disp;
+    const bool sib = addr.rm() == ESP;
+    const bool ebp = EBP == (sib ? addr.base() : addr.rm());
+    Address new_addr;
+    if (addr.mod() == 0 && ebp) {
+      // Special case: mod 00b and EBP in r/m or SIB base => 32-bit displacement.
+      new_addr.SetModRM(0, addr.rm());
+      if (sib) {
+        new_addr.SetSIB(addr.scale(), addr.index(), addr.base());
+      }
+      new_addr.SetDisp32(new_disp);
+    } else if (new_disp == 0 && !ebp) {
+      // Mod 00b (excluding a special case for EBP) => no displacement.
+      new_addr.SetModRM(0, addr.rm());
+      if (sib) {
+        new_addr.SetSIB(addr.scale(), addr.index(), addr.base());
+      }
+    } else if (new_disp >= -128 && new_disp <= 127) {
+      // Mod 01b => 8-bit displacement.
+      new_addr.SetModRM(1, addr.rm());
+      if (sib) {
+        new_addr.SetSIB(addr.scale(), addr.index(), addr.base());
+      }
+      new_addr.SetDisp8(new_disp);
     } else {
-      return Address(rm(), disp() + offset, GetFixup());
+      // Mod 10b => 32-bit displacement.
+      new_addr.SetModRM(2, addr.rm());
+      if (sib) {
+        new_addr.SetSIB(addr.scale(), addr.index(), addr.base());
+      }
+      new_addr.SetDisp32(new_disp);
     }
+    new_addr.SetFixup(addr.GetFixup());
+    return new_addr;
   }
 
   Register GetBaseRegister() {
@@ -226,6 +263,10 @@ class Address : public Operand {
     return Absolute(addr.Int32Value());
   }
 
+  inline bool operator==(const Address& addr) const {
+    return static_cast<const Operand&>(*this) == static_cast<const Operand&>(addr);
+  }
+
  private:
   Address() {}
 
diff --git a/compiler/utils/x86/assembler_x86_test.cc b/compiler/utils/x86/assembler_x86_test.cc
index 030479c63b..c6a181ad91 100644
--- a/compiler/utils/x86/assembler_x86_test.cc
+++ b/compiler/utils/x86/assembler_x86_test.cc
@@ -1324,4 +1324,47 @@ TEST_F(AssemblerX86Test, Notl) {
   DriverStr(RepeatR(&x86::X86Assembler::notl, "notl %{reg}"), "notl");
 }
 
+// Test that displacing an existing address is the same as constructing a new one with the same
+// initial displacement.
+TEST_F(AssemblerX86Test, AddressDisplaceBy) {
+  // Test different displacements, including some 8-bit and 32-bit ones, so that changing
+  // displacement may require a different addressing mode.
+  static const std::vector<int32_t> displacements = {0, 42, -42, 140, -140};
+  // Test with all scale factors.
+  static const std::vector<ScaleFactor> scales = {TIMES_1, TIMES_2, TIMES_4, TIMES_8};
+
+  for (int32_t disp0 : displacements) {  // initial displacement
+    for (int32_t disp : displacements) {  // extra displacement
+      for (const x86::Register *reg : GetRegisters()) {
+        // Test non-SIB addressing.
+        EXPECT_EQ(x86::Address::displace(x86::Address(*reg, disp0), disp),
+                  x86::Address(*reg, disp0 + disp));
+
+        // Test SIB addressing with EBP base.
+        if (*reg != x86::ESP) {
+          for (ScaleFactor scale : scales) {
+            EXPECT_EQ(x86::Address::displace(x86::Address(*reg, scale, disp0), disp),
+                      x86::Address(*reg, scale, disp0 + disp));
+          }
+        }
+
+        // Test SIB addressing with different base.
+        for (const x86::Register *index : GetRegisters()) {
+          if (*index == x86::ESP) {
+            continue;  // Skip ESP as it cannot be used with this address constructor.
+          }
+          for (ScaleFactor scale : scales) {
+            EXPECT_EQ(x86::Address::displace(x86::Address(*reg, *index, scale, disp0), disp),
+                      x86::Address(*reg, *index, scale, disp0 + disp));
+          }
+        }
+
+        // Test absolute addressing.
+        EXPECT_EQ(x86::Address::displace(x86::Address::Absolute(disp0), disp),
+                  x86::Address::Absolute(disp0 + disp));
+      }
+    }
+  }
+}
+
 }  // namespace art