[DebugInfo][NFCI] Add unittest for DWARFAbbreviationDeclarationSet

[bulbazord]

Summary

The classes relevant to DWARFDebugAbbrev do not have any unittests
verifying their behavior. Seeing as there is not much error handling
around these classes right now, I want to add some testing as I plan on
making changes to these classes in the near future.

[aprantl]

That sounds like a good idea!

[jhenderson]

Thanks for the tests - always useful. I've got a couple of comments/suggestions for you.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

26

You don't necessarily need this in this patch in isolation, but the patterns you're using to define your abbrevs are likely to be fairly repetitive. I'm assuming you're going to add new unit tests to test the error handling? If so, you may want to look at adding one or more basic generator functions to factor out the test setup (and in which case you may want to do it as part of this patch, or a prerequisite patch). We have similar stuff for the, admittedly more complex, debug info and debug line sections IIRC.

45

I'm not convinced by using sizeof(void *) for the address size, rather than hard-coding values of 4 and 8: the address size is the size of the address in the section data. This is independent of the machine that the test code is built on (i.e. a 32-bit machine should be able to read 64-bit addresses and vice versa).

If it's irrelevant to the test, then hard coding in a value of 4 or 8 is fine. Otherwise, you should test with both 4 and 8-byte addreses.

53

I think the LLVM policy on auto implies this shouldn't be auto, since it's not obvious what the actual type is if you aren't familiar with the interface.

[bulbazord]

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

26

Yes, I'll be adding more tests, so yanking the setup into a separate function is appropriate. It probably does not need to be very complex though. I'll do so in this patch. Thanks for the suggestion.

45

It is indeed irrelevant to the to the test. I'll hardcode 8.

[bulbazord]

• Yank common setup into its own function
• sizeof(void *) -> sizeof(uint64_t)
• Don't use auto where confusing

[jhenderson]

It would have been better to wait until I'd had a chance to review your latest updates, as there are a few (admittedly minor) points I'd have picked up on that now need you to go and update the in-tree code. I'm also adding a note that I haven't reviewed the coverage of these tests, just the general approach and style.

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

23

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

58

Comments should be full sentences with correct punctuation (https://llvm.org/docs/CodingStandards.html#commenting).

63

If the purpose of this check is to ensure that a valid pointer has come back, this should be ASSERT_TRUE, to prevent the test from continuing and crashing if it isn't. Same goes for a few other cases throughout the tests.

89

Same comment as above.

[bulbazord]

In D151001#4363438, @jhenderson wrote:

It would have been better to wait until I'd had a chance to review your latest updates, as there are a few (admittedly minor) points I'd have picked up on that now need you to go and update the in-tree code. I'm also adding a note that I haven't reviewed the coverage of these tests, just the general approach and style.

Apologies, I was too eager to land this. I've uploaded a new phabricator diff to address your comments. We can continue iterating there, sorry about this. D151233

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h

public:
   }
 
   const_iterator end() const {
     return Decls.end();
   }
 
   std::string getCodeRange() const;
 
+  uint32_t getFirstAbbrCode() const { return FirstAbbrCode; }
+
 private:
   void clear();
 };
 
 class DWARFDebugAbbrev {
   using DWARFAbbreviationDeclarationSetMap =
       std::map<uint64_t, DWARFAbbreviationDeclarationSet>;
 

llvm/unittests/DebugInfo/DWARF/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   TargetParser
   )
 
 add_llvm_unittest(DebugInfoDWARFTests
   DwarfGenerator.cpp
   DwarfUtils.cpp
   DWARFAcceleratorTableTest.cpp
   DWARFDataExtractorTest.cpp
+  DWARFDebugAbbrevTest.cpp
   DWARFDebugArangeSetTest.cpp
   DWARFDebugFrameTest.cpp
   DWARFDebugInfoTest.cpp
   DWARFDebugLineTest.cpp
   DWARFDieTest.cpp
   DWARFDieManualExtractTest.cpp
   DWARFExpressionCopyBytesTest.cpp
   DWARFExpressionCompactPrinterTest.cpp
   DWARFFormValueTest.cpp
   DWARFListTableTest.cpp
   DWARFLocationExpressionTest.cpp
   )
 
 target_link_libraries(DebugInfoDWARFTests PRIVATE LLVMTestingSupport)
 
 set_property(TARGET DebugInfoDWARFTests PROPERTY FOLDER "Tests/UnitTests/DebugInfoTests")

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

+//===- llvm/unittest/DebugInfo/DWARFDebugAbbrevTest.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/DebugInfo/DWARF/DWARFDebugAbbrev.h"
+#include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/LEB128.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace dwarf;
+
+enum Order : bool { InOrder, OutOfOrder };
+
+void WriteAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
+                                   Order Ord) {
+  encodeULEB128(FirstCode, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
+  OS << static_cast<uint8_t>(DW_CHILDREN_yes);
+  encodeULEB128(DW_AT_name, OS);
+  encodeULEB128(DW_FORM_strp, OS);
+  encodeULEB128(0, OS);
+  encodeULEB128(0, OS);
+
+  uint32_t SecondCode = Ord == Order::InOrder ? FirstCode + 1 : FirstCode - 1;
+
+  encodeULEB128(SecondCode, OS);
+  encodeULEB128(DW_TAG_subprogram, OS);
+  OS << static_cast<uint8_t>(DW_CHILDREN_no);
+  encodeULEB128(DW_AT_name, OS);
+  encodeULEB128(DW_FORM_strp, OS);
+  encodeULEB128(0, OS);
+  encodeULEB128(0, OS);
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccess) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 5;
+
+  WriteAbbreviationDeclarations(OS, FirstCode, InOrder);
+  encodeULEB128(0, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
+  EXPECT_TRUE(DataWasExtracted);
+  // The Abbreviation Declarations are in order and contiguous, so we want to
+  // make sure that FirstAbbrCode was correctly set
+  EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), FirstCode);
+
+  const DWARFAbbreviationDeclaration *Abbrev5 =
+      AbbrevSet.getAbbreviationDeclaration(FirstCode);
+  EXPECT_TRUE(Abbrev5);
+  EXPECT_EQ(Abbrev5->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(Abbrev5->hasChildren());
+  EXPECT_EQ(Abbrev5->getNumAttributes(), 1u);
+
+  const DWARFAbbreviationDeclaration *Abbrev6 =
+      AbbrevSet.getAbbreviationDeclaration(FirstCode + 1);
+  EXPECT_TRUE(Abbrev6);
+  EXPECT_EQ(Abbrev6->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(Abbrev6->hasChildren());
+  EXPECT_EQ(Abbrev6->getNumAttributes(), 1u);
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccessOutOfOrder) {
+  SmallString<64> RawData;
+  raw_svector_ostream OS(RawData);
+  uint32_t FirstCode = 2;
+
+  WriteAbbreviationDeclarations(OS, FirstCode, OutOfOrder);
+  encodeULEB128(0, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  const bool DataWasExtracted = AbbrevSet.extract(Data, &Offset);
+  EXPECT_TRUE(DataWasExtracted);
+  // The declarations are out of order, ensure that FirstAbbrCode is UINT32_MAX
+  EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), UINT32_MAX);
+
+  const DWARFAbbreviationDeclaration *Abbrev2 =
+      AbbrevSet.getAbbreviationDeclaration(FirstCode);
+  EXPECT_TRUE(Abbrev2);
+  EXPECT_EQ(Abbrev2->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(Abbrev2->hasChildren());
+  EXPECT_EQ(Abbrev2->getNumAttributes(), 1u);
+
+  const DWARFAbbreviationDeclaration *Abbrev1 =
+      AbbrevSet.getAbbreviationDeclaration(FirstCode - 1);
+  EXPECT_TRUE(Abbrev1);
+  EXPECT_EQ(Abbrev1->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(Abbrev1->hasChildren());
+  EXPECT_EQ(Abbrev1->getNumAttributes(), 1u);
+}