[ARM][AArch64] Test case debugging output

[MarkMurrayARM]

Summary

Add extra output for the mismatch cases to show where bitmap
mismatches are ocurring.

[DavidSpickett]

I see the idea to give some immediate improvement but I think there's more to be done to inform the user properly:

• Convert all of the runs of ASSERT_TRUE to individual tests using a test fixture (or two), for example [0]
• Creating a custom assertion message that includes the CPU name and the feature [1]
• Treating the features as a set, so we can say in one go "the features <list> are missing", instead of a one by one check
• Printing feature names when we do find a missing bit, you an ask the target parser for that

But I get that that's more than you bargained for.

I think the messages as they stand only help local development, where you know what you broke because you're adding a CPU.
If I'm working on seemingly unrelated code, I don't see how these messages give me any more info than the single gtest failure we have now.

Like ok the ARM cpus test failed on this bit. Ok. What does that mean?

Also I'm not a fan of random errs messages in buildbot logs but that's mostly my preference. I don't think there's a rule for it.

You could add context to these errs messages but still I'm not a fan of errs as a mechanism for reporting this. That's what failure messages are for.

[0] https://github.com/llvm/llvm-project/blob/32541685b2a980290c320adb289b56395a88d4c3/lldb/unittests/Process/Utility/LinuxProcMapsTest.cpp
[1] https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#using-a-predicate-formatter

[DavidSpickett]

Also the commit msg " Test case debugging output" should include which test cases, "target parser test cases" would do fine.

Target parser is nominally my area so if you don't have the time to pursue this (these tests don't fail often at all, so I get it) I can have a go at doing what I proposed. Then you can review for me. After all, you're the one adding the CPUs these days so you're seeing the failures.

[MarkMurrayARM]

@DavidSpickett, I would very much appreciate you taking this over, thanks! I needed a way of find out *how* these tests were failing, not just *that* they were failing. I will abandon this change.

[DavidSpickett]

No problem, I'll have something for review in a few days.

llvm/unittests/Support/TargetParserTest.cpp

 //===----------- TargetParser.cpp - Target Parser -------------------------===//
 //
 // 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/Support/TargetParser.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include <string>
 
 using namespace llvm;
 
 namespace {
 const char *ARMArch[] = {
     "armv2",       "armv2a",       "armv3",       "armv3m",       "armv4",
     "armv4t",      "armv5",        "armv5t",      "armv5e",       "armv5te",
     "armv5tej",    "armv6",        "armv6j",      "armv6k",       "armv6hl",
     "armv6t2",     "armv6kz",      "armv6z",      "armv6zk",      "armv6-m",
     "armv6m",      "armv6sm",      "armv6s-m",    "armv7-a",      "armv7",
     "armv7a",      "armv7ve",      "armv7hl",     "armv7l",       "armv7-r",
     "armv7r",      "armv7-m",      "armv7m",      "armv7k",       "armv7s",
     "armv7e-m",    "armv7em",      "armv8-a",     "armv8",        "armv8a",
     "armv8l",      "armv8.1-a",    "armv8.1a",    "armv8.2-a",    "armv8.2a",
     "armv8.3-a",   "armv8.3a",     "armv8.4-a",   "armv8.4a",     "armv8.5-a",
     "armv8.5a",     "armv8.6-a",   "armv8.6a",     "armv8-r",     "armv8r",
     "armv8-m.base", "armv8m.base", "armv8-m.main", "armv8m.main", "iwmmxt",
     "iwmmxt2",      "xscale",      "armv8.1-m.main",
 };
 
+void printDifferingBits(uint64_t a, uint64_t b)
+{
+  llvm::errs() << "Differing bits are:";
+  uint64_t bit = 1;
+  for (int i = 0; i < 64; i++) {
+    if ((a & bit) != (b & bit))
+      llvm::errs() << " " << i;
+    bit <<= 1;
+  }
+  llvm::errs() << "\n";
+}
+
 bool testARMCPU(StringRef CPUName, StringRef ExpectedArch,
                 StringRef ExpectedFPU, uint64_t ExpectedFlags,
                 StringRef CPUAttr) {
   ARM::ArchKind AK = ARM::parseCPUArch(CPUName);
   bool pass = ARM::getArchName(AK).equals(ExpectedArch);
   unsigned FPUKind = ARM::getDefaultFPU(CPUName, AK);
   pass &= ARM::getFPUName(FPUKind).equals(ExpectedFPU);
 
   uint64_t ExtKind = ARM::getDefaultExtensions(CPUName, AK);
-  if (ExtKind > 1 && (ExtKind & ARM::AEK_NONE))
+  if (ExtKind > 1 && (ExtKind & ARM::AEK_NONE)) {
     pass &= ((ExtKind ^ ARM::AEK_NONE) == ExpectedFlags);
-  else
+    if (!pass) {
+      llvm::errs() << "ExpectedFlags = " << ExpectedFlags
+        << " do not equal ExtKind = " << ExtKind
+        << "in the ARM::AEK_NONE case. " << "\n";
+      printDifferingBits(ExtKind, ExpectedFlags);
+    }
+  } else {
     pass &= (ExtKind == ExpectedFlags);
+    if (!pass) {
+      llvm::errs() << "ExpectedFlags = " << ExpectedFlags
+        << " do not equal ExtKind = " << ExtKind << "\n";
+      printDifferingBits(ExtKind, ExpectedFlags);
+    }
+  }
   pass &= ARM::getCPUAttr(AK).equals(CPUAttr);
 
   return pass;
 }
 
 TEST(TargetParserTest, testARMCPU) {
   EXPECT_TRUE(testARMCPU("invalid", "invalid", "invalid",
                          ARM::AEK_NONE, ""));

 
 bool testAArch64CPU(StringRef CPUName, StringRef ExpectedArch,
                     StringRef ExpectedFPU, uint64_t ExpectedFlags,
                     StringRef CPUAttr) {
   AArch64::ArchKind AK = AArch64::parseCPUArch(CPUName);
   bool pass = AArch64::getArchName(AK).equals(ExpectedArch);
 
   uint64_t ExtKind = AArch64::getDefaultExtensions(CPUName, AK);
-  if (ExtKind > 1 && (ExtKind & AArch64::AEK_NONE))
+  if (ExtKind > 1 && (ExtKind & AArch64::AEK_NONE)) {
     pass &= ((ExtKind ^ AArch64::AEK_NONE) == ExpectedFlags);
-  else
+    if (!pass) {
+      llvm::errs() << "ExpectedFlags = 0x" << ExpectedFlags
+        << " do not equal ExtKind = 0x" << ExtKind
+        << "in the ARM::AEK_NONE case. " << "\n";
+      printDifferingBits(ExtKind, ExpectedFlags);
+    }
+  } else {
     pass &= (ExtKind == ExpectedFlags);
-
+    if (!pass) {
+      llvm::errs() << "ExpectedFlags = 0x" << ExpectedFlags
+        << " do not equal ExtKind = 0x" << ExtKind << "\n";
+      printDifferingBits(ExtKind, ExpectedFlags);
+    }
+  }
   pass &= AArch64::getCPUAttr(AK).equals(CPUAttr);
 
   return pass;
 }
 
 TEST(TargetParserTest, testAArch64CPU) {
   EXPECT_TRUE(testAArch64CPU(
       "invalid", "invalid", "invalid",