[MC] Fix undefined behavior in MCInstPrinter::formatHex

[JDevlieghere]

Summary

Passing INT64_MIN to MCInstPrinter::formatHex triggers undefined behavior because the negation of -9223372036854775808 cannot be represented in type 'int64_t' (aka 'long long'). This patch puts a workaround in place to just print the hex value directly, without the leading -. A possible alternative involves using a small helper functions that uses (implementation) defined conversions to achieve the desirable value:

static int64_t helper(int64_t V) {
  auto U = static_cast<uint64_t>(V);
  return V < 0 ? -U : U;
}

The underlying problem is that MCInstPrinter::formatHex(int64_t) returns a format_object<int64_t> and should really return a format_object<uint64_t>. However, that's not possible because formatImm needs to be able to print both as decimal (where a signed is required) and hex (where we'd prefer to always have an unsigned).

format_object<int64_t> formatImm(int64_t Value) const {
  return PrintImmHex ? formatHex(Value) : formatDec(Value);
}

[shafik]

It is not obvious what the change in result is expected to be so a test would be helpful.

[dblaikie]

I'm confused - the patch description mentions passing unsigned min to a function that takes a signed value - if there's UB, it's in the caller so can't really be fixed in the callee, right? (or is the UB still present with passing int32 max too?)

[JDevlieghere]

• Add test
• Fix description to say INT64_MIN instead of UINT64_MIN

[JDevlieghere]

• Update test (Forgot to stage before updating the patch)

[dblaikie]

Couldn't this value be hardcoded? The maximum value of a 64 bit signed integer is a known value, right? We could hardcode that string?

[JDevlieghere]

• Hard-code the value

[JDevlieghere]

I don't know what it is with me today, but this patch seems to be suffering a lot from (un)staged changes...

[dblaikie]

Seems fairly reasonable to me - test might be able to be cleaned up a bit. Code might be cleaned up a bit (either now or in a follow-up commit)

llvm/lib/MC/MCInstPrinter.cpp

88–93 ↗

(On Diff #218999)

There's a lot of else-after-return here, and maybe a special case might work here:

if (Value < 0) {
  if (Value == min)
    return constant
  return format("-0x%"... -Value);
}
return format(...)

(this means only one test for the positive case, rather than 2)

But all that might be better as a separate cleanup patch.

llvm/unittests/MC/MCInstPrinter.cpp

60–63 ↗

(On Diff #218999)

Could probably put this in a test fixture, rather than a singleton? (but if there's nearby prior-art, I'm OK with it) (oh, the Printer class itself could be the test fixture class)

llvm/trunk/lib/MC/MCInstPrinter.cpp

static bool needsLeadingZero(uint64_t Value)
   return false;
 }
 
 format_object<int64_t> MCInstPrinter::formatDec(int64_t Value) const {
   return format("%" PRId64, Value);
 }
 
 format_object<int64_t> MCInstPrinter::formatHex(int64_t Value) const {
-  switch(PrintHexStyle) {
+  switch (PrintHexStyle) {
   case HexStyle::C:
-    if (Value < 0)
+    if (Value < 0) {
+      if (Value == std::numeric_limits<int64_t>::min())
+        return format<int64_t>("-0x8000000000000000", Value);
       return format("-0x%" PRIx64, -Value);
-    else
-      return format("0x%" PRIx64, Value);
+    }
   case HexStyle::Asm:
     if (Value < 0) {
-      if (needsLeadingZero((uint64_t)(-Value)))
+      if (Value == std::numeric_limits<int64_t>::min())
+        return format<int64_t>("-8000000000000000h", Value);
+      if (needsLeadingZero(-(uint64_t)(Value)))
         return format("-0%" PRIx64 "h", -Value);
-      else
-        return format("-%" PRIx64 "h", -Value);
-    } else {
-      if (needsLeadingZero((uint64_t)(Value)))
-        return format("0%" PRIx64 "h", Value);
-      else
-        return format("%" PRIx64 "h", Value);
-    }
-  }
+    }
   llvm_unreachable("unsupported print style");
 }
 
 format_object<uint64_t> MCInstPrinter::formatHex(uint64_t Value) const {
   switch(PrintHexStyle) {
   case HexStyle::C:
      return format("0x%" PRIx64, Value);
   case HexStyle::Asm:
     if (needsLeadingZero(Value))
       return format("0%" PRIx64 "h", Value);
     else
       return format("%" PRIx64 "h", Value);
   }
   llvm_unreachable("unsupported print style");
 }

llvm/trunk/unittests/MC/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   MC
   MCDisassembler
   Support
   )
 
 add_llvm_unittest(MCTests
   Disassembler.cpp
   DwarfLineTables.cpp
+  MCInstPrinter.cpp
   StringTableBuilderTest.cpp
   TargetRegistry.cpp
   )

llvm/trunk/unittests/MC/MCInstPrinter.cpp

+//===- llvm/unittest/MC/MCInstPrinter.cpp ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/MC/MCInstPrinter.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetMachine.h"
+#include "llvm/Target/TargetOptions.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+class MCInstPrinterTest : public ::testing::Test {
+public:
+  std::unique_ptr<MCRegisterInfo> MRI;
+  std::unique_ptr<MCAsmInfo> MAI;
+  std::unique_ptr<const MCInstrInfo> MII;
+  std::unique_ptr<MCInstPrinter> Printer;
+
+  MCInstPrinterTest() {
+    llvm::InitializeAllTargetInfos();
+    llvm::InitializeAllTargetMCs();
+
+    std::string TripleName = "x86_64-pc-linux";
+    std::string ErrorStr;
+
+    const Target *TheTarget =
+        TargetRegistry::lookupTarget(TripleName, ErrorStr);
+
+    // If we didn't build x86, do not run the test.
+    if (!TheTarget)
+      return;
+
+    MRI.reset(TheTarget->createMCRegInfo(TripleName));
+    MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName));
+    MII.reset(TheTarget->createMCInstrInfo());
+    Printer.reset(TheTarget->createMCInstPrinter(
+        Triple(TripleName), MAI->getAssemblerDialect(), *MAI, *MII, *MRI));
+  }
+
+  template <typename T> std::string formatHex(T i) {
+    std::string Buffer;
+    raw_string_ostream OS(Buffer);
+    OS << Printer->formatHex(i);
+    OS.flush();
+    return Buffer;
+  }
+};
+} // namespace
+
+TEST_F(MCInstPrinterTest, formatHex) {
+  if (!Printer)
+    return;
+
+  EXPECT_EQ("0x1", formatHex<int64_t>(1));
+  EXPECT_EQ("0x7fffffffffffffff",
+            formatHex(std::numeric_limits<int64_t>::max()));
+  EXPECT_EQ("-0x8000000000000000",
+            formatHex(std::numeric_limits<int64_t>::min()));
+}