[DWARF] - Simplify HandleExpectedError implementation in DWARFDebugInfoTest

[grimar]

Summary

Current implementation looks confusing. It looks like it should
report/print something on error, but it does not do that.

HandleExpectedError used to check targets created from triple.
I am not sure what was intended behavior,
current one - it does not print anything if target was not created and assumes
that test pass.

That behavior looks correct for me though, but then implementation can be simplified,
like this patch do. I think current behavior is correct because of next:
I tried to configure LLVM with -DLLVM_TARGETS_TO_BUILD=ARM and debugged the testcase under
windows. Triple I had was "i386-pc-windows-msvc". Testcase uses getHostTripleForAddrSize() to get
target triple. As expected for my configuration no tests were executed. For me it is looks fine in that case.

[dblaikie]

I don't quite follow your description - this looks like it would print error messages, if the llvm::Error has a textual message (which not all do). I would expect the behavior should perhaps flip the other way - fix it so it does print a message even if the llvm::Error doesn't have a textual message itself? (I think llvm::Error has a more general tool for textual repreesntation, rather than just the message? Not sure)

[zturner]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

98–99

I recently added some functions to llvm that server this purpose better. I would suggest just deleting this function, and adding this:

#include "llvm/Testing/Support/Error.h"
...
ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());

[grimar]

In D34744#793819, @dblaikie wrote:

I don't quite follow your description - this looks like it would print error messages, if the llvm::Error has a textual message (which not all do). I would expect the behavior should perhaps flip the other way - fix it so it does print a message even if the llvm::Error doesn't have a textual message itself? (I think llvm::Error has a more general tool for textual repreesntation, rather than just the message? Not sure)

What I was mean that

if (!ErrorMsg.empty()) {
  ::testing::AssertionFailure() << "error: " << ErrorMsg;
  return true;
}

creates temporarily object AssertionResult and calls operator<<. That does not print any messages. Object is destroyed after call and all messages
it contains inside after << are dying with it silently. And at first look it seems that method should assert, but this object never used.
So I believe code is equal to what I wrote in this patch.

[grimar]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

98–99

I'll take a look what ASSERT_THAT_EXPECTED do. If I got correctly from quivk look, that might be a better solution, really. Thanks !

[grimar]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

98–99

After closer look I think ASSERT_THAT_EXPECTED will not work here.

ASSERT_THAT_EXPECTED fails the test if ExpectedDG not exist:

error: Value of: llvm::detail::TakeExpected(ExpectedDG)
Expected: succeeded
  Actual: failed  (: error: unable to get target for 'i386-pc-windows-msvc', see
 --version and --triple.)
[  FAILED  ] DWARFDebugInfo.TestDWARF32Version2Addr4AllForms (115792 ms)

But what code expected to do is to pass the test in this case.

[grimar]

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

98–99

So to clarify: if ExpectedDG is exist, test should continue and run, if not - test should terminate with success result, that is how current code works.

[grimar]

• Addressed review comments (introduced isConfigurationSupported() to be a ble to skip test early).

[zturner]

lgtm. I have some other ideas for how to improve this test in the future, but mostly orthogonal to this effort (for example, why can't we just enumerate the cases that we expect to work based on the TargetRegistry, and then only run those tests?)

Might be a nice cleanup if you feel like it (or there might be some reason I can't think of why it wouldn't work the way I imagine), but feel free to do it in a follow-up (or not at all, purely optional)

[grimar]

In D34744#795395, @zturner wrote:

lgtm. I have some other ideas for how to improve this test in the future, but mostly orthogonal to this effort (for example, why can't we just enumerate the cases that we expect to work based on the TargetRegistry, and then only run those tests?)

Might be a nice cleanup if you feel like it (or there might be some reason I can't think of why it wouldn't work the way I imagine), but feel free to do it in a follow-up (or not at all, purely optional)

Thanks for review. Idea looks interesting for me. I am not so familar with gtest yet. I read gtest doc and reviewed implementation today, but did not yet found how to filter out tests with some custom predicate.
I see that gtest allows filtering by name, that is probably not what can help here. I think that all what is needed is to set TestInfo::is_disabled_ flag basing on custom user predicate somehow.
I'll try to revisit it more attentively a bit later, probably I am missed something.

[grimar]

r306812

unittests/DebugInfo/DWARF/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   AsmPrinter
   DebugInfoDWARF
   MC
   Object
   ObjectYAML
   Support
+  TestingSupport
   )
 
 set(DebugInfoSources
   DwarfGenerator.cpp
   DWARFDebugInfoTest.cpp
   DWARFFormValueTest.cpp
   )
 
 add_llvm_unittest(DebugInfoDWARFTests
   ${DebugInfoSources}
   )

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/ObjectYAML/DWARFEmitter.h"
 #include "llvm/ObjectYAML/DWARFYAML.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <climits>
 #include <cstdint>
 #include <cstring>
 #include <string>
 
 using namespace llvm;
 using namespace dwarf;
Triple getHostTripleForAddrSize(uint8_t AddrSize) {
 
   if (AddrSize == 8 && PT.isArch32Bit())
     return PT.get64BitArchVariant();
   if (AddrSize == 4 && PT.isArch64Bit())
     return PT.get32BitArchVariant();
   return PT;
 }
 
-/// Take any llvm::Expected and check and handle any errors.
-///
-/// \param Expected a llvm::Excepted instance to check.
-/// \returns true if there were errors, false otherwise.
-template <typename T>
-static bool HandleExpectedError(T &Expected) {
-  std::string ErrorMsg;
-  handleAllErrors(Expected.takeError(), [&](const ErrorInfoBase &EI) {
-    ErrorMsg = EI.message();
-  });
-  if (!ErrorMsg.empty()) {
-    ::testing::AssertionFailure() << "error: " << ErrorMsg;
-    return true;
-  }
-  return false;
+static bool isConfigurationSupported(Triple &T) {
+  initLLVMIfNeeded();
+  std::string Err;
+  return TargetRegistry::lookupTarget(T.getTriple(), Err);
 }
 
 template <uint16_t Version, class AddrType, class RefAddrType>
 void TestAllForms() {
-  // Test that we can decode all DW_FORM values correctly.
+  Triple Triple = getHostTripleForAddrSize(sizeof(AddrType));
+  if (!isConfigurationSupported(Triple))
+    return;
 
-  const uint8_t AddrSize = sizeof(AddrType);
+  // Test that we can decode all DW_FORM values correctly.
   const AddrType AddrValue = (AddrType)0x0123456789abcdefULL;
   const uint8_t BlockData[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
   const uint32_t BlockSize = sizeof(BlockData);
   const RefAddrType RefAddr = 0x12345678;
   const uint8_t Data1 = 0x01U;
   const uint16_t Data2 = 0x2345U;
   const uint32_t Data4 = 0x6789abcdU;
   const uint64_t Data8 = 0x0011223344556677ULL;
   const uint64_t Data8_2 = 0xAABBCCDDEEFF0011ULL;
   const int64_t SData = INT64_MIN;
   const int64_t ICSData = INT64_MAX; // DW_FORM_implicit_const SData
   const uint64_t UData[] = {UINT64_MAX - 1, UINT64_MAX - 2, UINT64_MAX - 3,
                             UINT64_MAX - 4, UINT64_MAX - 5, UINT64_MAX - 6,
                             UINT64_MAX - 7, UINT64_MAX - 8, UINT64_MAX - 9};
 #define UDATA_1 18446744073709551614ULL
   const uint32_t Dwarf32Values[] = {1, 2, 3, 4, 5, 6, 7, 8};
   const char *StringValue = "Hello";
   const char *StrpValue = "World";
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
+
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
   dwarfgen::DIE CUDie = CU.getUnitDIE();
   uint16_t Attr = DW_AT_lo_user;
 
   //----------------------------------------------------------------------
   // Test address forms
   //----------------------------------------------------------------------
TEST(DWARFDebugInfo, TestDWARF32Version5Addr8AllForms) {
   // addresses.
   typedef uint64_t AddrType;
   // DW_FORM_ref_addr are 4 bytes in DWARF32 for version 3 and later
   typedef uint32_t RefAddrType;
   TestAllForms<5, AddrType, RefAddrType>();
 }
 
 template <uint16_t Version, class AddrType> void TestChildren() {
+  Triple Triple = getHostTripleForAddrSize(sizeof(AddrType));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test that we can decode DW_FORM_ref_addr values correctly in DWARF 2 with
   // 4 byte addresses. DW_FORM_ref_addr values should be 4 bytes when using
   // 8 byte addresses.
 
-  const uint8_t AddrSize = sizeof(AddrType);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
   dwarfgen::DIE CUDie = CU.getUnitDIE();
 
   CUDie.addAttribute(DW_AT_name, DW_FORM_strp, "/tmp/main.c");
   CUDie.addAttribute(DW_AT_language, DW_FORM_data2, DW_LANG_C);
 
   dwarfgen::DIE SubprogramDie = CUDie.addChild(DW_TAG_subprogram);

 TEST(DWARFDebugInfo, TestDWARF32Version4Addr8Children) {
   // Test that we can decode all forms for DWARF32, version 4, with 8 byte
   // addresses.
   typedef uint64_t AddrType;
   TestChildren<4, AddrType>();
 }
 
 template <uint16_t Version, class AddrType> void TestReferences() {
-  // Test that we can decode DW_FORM_refXXX values correctly in DWARF.
+  Triple Triple = getHostTripleForAddrSize(sizeof(AddrType));
+  if (!isConfigurationSupported(Triple))
+    return;
 
-  const uint8_t AddrSize = sizeof(AddrType);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
+  // Test that we can decode DW_FORM_refXXX values correctly in DWARF.
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU1 = DG->addCompileUnit();
   dwarfgen::CompileUnit &CU2 = DG->addCompileUnit();
 
   dwarfgen::DIE CU1Die = CU1.getUnitDIE();
   CU1Die.addAttribute(DW_AT_name, DW_FORM_strp, "/tmp/main.c");
   CU1Die.addAttribute(DW_AT_language, DW_FORM_data2, DW_LANG_C);
 

 TEST(DWARFDebugInfo, TestDWARF32Version4Addr8References) {
   // Test that we can decode all forms for DWARF32, version 4, with 8 byte
   // addresses.
   typedef uint64_t AddrType;
   TestReferences<4, AddrType>();
 }
 
 template <uint16_t Version, class AddrType> void TestAddresses() {
+  Triple Triple = getHostTripleForAddrSize(sizeof(AddrType));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test the DWARF APIs related to accessing the DW_AT_low_pc and
   // DW_AT_high_pc.
-  const uint8_t AddrSize = sizeof(AddrType);
   const bool SupportsHighPCAsOffset = Version >= 4;
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
   dwarfgen::DIE CUDie = CU.getUnitDIE();
 
   CUDie.addAttribute(DW_AT_name, DW_FORM_strp, "/tmp/main.c");
   CUDie.addAttribute(DW_AT_language, DW_FORM_data2, DW_LANG_C);
 
   // Create a subprogram DIE with no low or high PC.

 TEST(DWARFDebugInfo, TestDWARF32Version4Addr8Addresses) {
   // Test that we can decode address values in DWARF32, version 4, with 8 byte
   // addresses.
   typedef uint64_t AddrType;
   TestAddresses<4, AddrType>();
 }
 
 TEST(DWARFDebugInfo, TestRelations) {
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test the DWARF APIs related to accessing the DW_AT_low_pc and
   // DW_AT_high_pc.
   uint16_t Version = 4;
-
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
 
   enum class Tag: uint16_t  {
     A = dwarf::DW_TAG_lo_user,
     B,
     C,
     C1,
TEST(DWARFDebugInfo, TestDWARFDie) {
   // or child;
   DWARFDie DefaultDie;
   EXPECT_FALSE(DefaultDie.getParent().isValid());
   EXPECT_FALSE(DefaultDie.getFirstChild().isValid());
   EXPECT_FALSE(DefaultDie.getSibling().isValid());
 }
 
 TEST(DWARFDebugInfo, TestChildIterators) {
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test the DWARF APIs related to iterating across the children of a DIE using
   // the DWARFDie::iterator class.
   uint16_t Version = 4;
-
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
 
   enum class Tag: uint16_t  {
     A = dwarf::DW_TAG_lo_user,
     B,
   };
 
TEST(DWARFDebugInfo, TestEmptyChildren) {
 
   // Verify that the CU Die that says it has children, but doesn't, actually
   // has begin and end iterators that are equal. We want to make sure we don't
   // see the Null DIEs during iteration.
   EXPECT_EQ(CUDie.begin(), CUDie.end());
 }
 
 TEST(DWARFDebugInfo, TestAttributeIterators) {
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test the DWARF APIs related to iterating across all attribute values in a
   // a DWARFDie.
   uint16_t Version = 4;
-
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
   const uint64_t CULowPC = 0x1000;
   StringRef CUPath("/tmp/main.c");
 
   // Scope to allow us to re-use the same DIE names
   {
     auto CUDie = CU.getUnitDIE();
TEST(DWARFDebugInfo, TestAttributeIterators) {
   ASSERT_NE(E, ++I);
   EXPECT_EQ(I->Attr, DW_AT_low_pc);
   EXPECT_EQ(CULowPC, *I->Value.getAsAddress());
 
   EXPECT_EQ(E, ++I);
 }
 
 TEST(DWARFDebugInfo, TestFindRecurse) {
-  uint16_t Version = 4;
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
 
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
+  uint16_t Version = 4;
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
 
   StringRef SpecDieName = "spec";
   StringRef SpecLinkageName = "spec_linkage";
   StringRef AbsDieName = "abs";
   // Scope to allow us to re-use the same DIE names
   {
TEST(DWARFDebugInfo, TestDwarfToFunctions) {
   EXPECT_EQ(InvalidU64, toAddress(FormValOpt, InvalidU64));
   EXPECT_EQ(InvalidU64, toSectionOffset(FormValOpt, InvalidU64));
   EXPECT_EQ(InvalidS64, toSigned(FormValOpt, InvalidU64));
 
   // Test
 }
 
 TEST(DWARFDebugInfo, TestFindAttrs) {
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
+
   // Test the DWARFDie::find() and DWARFDie::findRecursively() that take an
   // ArrayRef<dwarf::Attribute> value to make sure they work correctly.
   uint16_t Version = 4;
-
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
 
   StringRef DieMangled("_Z3fooi");
   // Scope to allow us to re-use the same DIE names
   {
     auto CUDie = CU.getUnitDIE();
     auto FuncSpecDie = CUDie.addChild(DW_TAG_subprogram);
TEST(DWARFDebugInfo, TestFindAttrs) {
   // DWARFDie::findRecursively() since it should recurse through the
   // DW_AT_specification DIE.
   auto NameOpt = FuncDie.findRecursively(Attrs);
   EXPECT_TRUE(NameOpt.hasValue());
   EXPECT_EQ(DieMangled, toString(NameOpt, ""));
 }
 
 TEST(DWARFDebugInfo, TestImplicitConstAbbrevs) {
-  uint16_t Version = 5;
+  Triple Triple = getHostTripleForAddrSize(sizeof(void *));
+  if (!isConfigurationSupported(Triple))
+    return;
 
-  const uint8_t AddrSize = sizeof(void *);
-  initLLVMIfNeeded();
-  Triple Triple = getHostTripleForAddrSize(AddrSize);
+  uint16_t Version = 5;
   auto ExpectedDG = dwarfgen::Generator::create(Triple, Version);
-  if (HandleExpectedError(ExpectedDG))
-    return;
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   dwarfgen::CompileUnit &CU = DG->addCompileUnit();
   dwarfgen::DIE CUDie = CU.getUnitDIE();
   const dwarf::Attribute Attr = DW_AT_lo_user;
   const int64_t Val1 = 42;
   const int64_t Val2 = 43;
 
   auto FirstVal1DIE = CUDie.addChild(DW_TAG_class_type);
TEST(DWARFDebugInfo, TestDwarfVerifyCUDontShareLineTable) {
   ASSERT_TRUE((bool)ErrOrSections);
   DWARFContextInMemory DwarfContext(*ErrOrSections, 8);
   VerifyError(DwarfContext, "error: two compile unit DIEs, 0x0000000b and "
                             "0x0000001f, have the same DW_AT_stmt_list section "
                             "offset:");
 }
 
 TEST(DWARFDebugInfo, TestErrorReportingPolicy) {
-  initLLVMIfNeeded();
-  auto ExpectedDG = dwarfgen::Generator::create(Triple("x86_64-pc-linux"),
-                                                4 /*DwarfVersion*/);
-  if (HandleExpectedError(ExpectedDG))
+  Triple Triple("x86_64-pc-linux");
+  if (!isConfigurationSupported(Triple))
       return;
+
+  auto ExpectedDG = dwarfgen::Generator::create(Triple, 4 /*DwarfVersion*/);
+  ASSERT_THAT_EXPECTED(ExpectedDG, Succeeded());
   dwarfgen::Generator *DG = ExpectedDG.get().get();
   AsmPrinter *AP = DG->getAsmPrinter();
   MCContext *MC = DG->getMCContext();
 
   // Emit two compressed sections with broken headers.
   AP->OutStreamer->SwitchSection(
       MC->getELFSection(".zdebug_foo", 0 /*Type*/, 0 /*Flags*/));
   AP->OutStreamer->EmitBytes("0");