[PATCH] D38871: [ExecutionEngine] Modify static_casts to not be tautological in some COFF i386 relocations

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

I'd wait for @compnerd to take a look before acting on my comments, since I don't have much context on this code.



================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h:147
                     RE.Addend);
-      assert(static_cast<int32_t>(Result) <= INT32_MAX &&
+      assert(static_cast<int64_t>(Result) <= INT32_MAX &&
              "relocation overflow");
----------------
A VA is unsigned, so "relocation underflow" doesn't make much sense to me. I'd just assert `Result <= UINT32_MAX` instead.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h:164
           Sections[0].getLoadAddress();
-      assert(static_cast<int32_t>(Result) <= INT32_MAX &&
+      assert(static_cast<int64_t>(Result) <= INT32_MAX &&
              "relocation overflow");
----------------
Same here.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFI386.h:205
       // 32-bit offset of the target from the beginning of its section.
-      assert(static_cast<int32_t>(RE.Addend) <= INT32_MAX &&
-             "relocation overflow");
-      assert(static_cast<int32_t>(RE.Addend) >= INT32_MIN &&
-             "relocation underflow");
+      assert(RE.Addend <= INT32_MAX && "relocation overflow");
+      assert(RE.Addend >= INT32_MIN && "relocation underflow");
----------------
Same here (though you'll need a cast to `uint64_t`).


https://reviews.llvm.org/D38871