[llvm] r221836 - [dwarfdump] Add support for dumping accelerator tables.
Frédéric Riss
friss at apple.com
Thu Nov 13 13:08:38 PST 2014
> On Nov 13, 2014, at 11:45 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Nov 13, 2014 at 11:40 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>
>> On Nov 13, 2014, at 11:31 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>>
>>
>> On Thu, Nov 13, 2014 at 11:28 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>
>>> On Nov 13, 2014, at 10:15 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>
>>>
>>>
>>> On Wed, Nov 12, 2014 at 4:46 PM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>> For the record, I reverted the feature for now as it failed some tests (for one I foolishly thought that we emitted accelerators on all platforms as the spec makes provision for section names on not Darwin…). I’ll address your review comments in my next try.
>>>
>>>> On Nov 12, 2014, at 4:22 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>>
>>>> I'll leave it up to someone with more Objective-C knowledge to some of this review (Eric - since he implemented the accelerator tables in the first place)
>>>>
>>>> On Wed, Nov 12, 2014 at 3:48 PM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>>> Author: friss
>>>> Date: Wed Nov 12 17:48:10 2014
>>>> New Revision: 221836
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=221836&view=rev <http://llvm.org/viewvc/llvm-project?rev=221836&view=rev>
>>>> Log:
>>>> [dwarfdump] Add support for dumping accelerator tables.
>>>>
>>>> The class used for the dump only allows to dump for the moment, but
>>>> it can (and will) be easily extended to support search also.
>>>>
>>>> Added:
>>>> llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp
>>>> llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h
>>>> llvm/trunk/test/DebugInfo/dwarfdump-accel.test
>>>> Modified:
>>>> llvm/trunk/include/llvm/DebugInfo/DIContext.h
>>>> llvm/trunk/lib/DebugInfo/CMakeLists.txt
>>>> llvm/trunk/lib/DebugInfo/DWARFContext.cpp
>>>> llvm/trunk/lib/DebugInfo/DWARFContext.h
>>>> llvm/trunk/test/DebugInfo/Inputs/gmlt.ll
>>>> llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
>>>>
>>>> Modified: llvm/trunk/include/llvm/DebugInfo/DIContext.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/DIContext.h?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/DIContext.h?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/DebugInfo/DIContext.h (original)
>>>> +++ llvm/trunk/include/llvm/DebugInfo/DIContext.h Wed Nov 12 17:48:10 2014
>>>> @@ -107,7 +107,11 @@ enum DIDumpType {
>>>> DIDT_GnuPubtypes,
>>>> DIDT_Str,
>>>> DIDT_StrDwo,
>>>> - DIDT_StrOffsetsDwo
>>>> + DIDT_StrOffsetsDwo,
>>>> + DIDT_AppleNames,
>>>> + DIDT_AppleTypes,
>>>> + DIDT_AppleNamespaces,
>>>> + DIDT_AppleObjC
>>>> };
>>>>
>>>> // In place of applying the relocations to the data we've read from disk we use
>>>>
>>>> Modified: llvm/trunk/lib/DebugInfo/CMakeLists.txt
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/CMakeLists.txt?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/CMakeLists.txt?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/DebugInfo/CMakeLists.txt (original)
>>>> +++ llvm/trunk/lib/DebugInfo/CMakeLists.txt Wed Nov 12 17:48:10 2014
>>>> @@ -1,6 +1,7 @@
>>>> add_llvm_library(LLVMDebugInfo
>>>> DIContext.cpp
>>>> DWARFAbbreviationDeclaration.cpp
>>>> + DWARFAcceleratorTable.cpp
>>>> DWARFCompileUnit.cpp
>>>> DWARFContext.cpp
>>>> DWARFDebugAbbrev.cpp
>>>>
>>>> Added: llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp?rev=221836&view=auto <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp?rev=221836&view=auto>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp (added)
>>>> +++ llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.cpp Wed Nov 12 17:48:10 2014
>>>> @@ -0,0 +1,110 @@
>>>> +#include "DWARFAcceleratorTable.h"
>>>> +
>>>> +#include "llvm/Support/Dwarf.h"
>>>> +#include "llvm/Support/Format.h"
>>>> +#include "llvm/Support/raw_ostream.h"
>>>> +
>>>> +namespace llvm {
>>>> +
>>>> +bool DWARFAcceleratorTable::extract() {
>>>> + uint32_t Offset = 0;
>>>> +
>>>> + // Check that we can at least read the header.
>>>> + if (!AccelSection.isValidOffset(offsetof(Header, HeaderDataLength)+4))
>>>> + return false;
>>>> +
>>>> + Hdr.Magic = AccelSection.getU32(&Offset);
>>>> + Hdr.Version = AccelSection.getU16(&Offset);
>>>> + Hdr.HashFunction = AccelSection.getU16(&Offset);
>>>> + Hdr.NumBuckets = AccelSection.getU32(&Offset);
>>>> + Hdr.NumHashes = AccelSection.getU32(&Offset);
>>>> + Hdr.HeaderDataLength = AccelSection.getU32(&Offset);
>>>> +
>>>> + // Check that we can read all the hashes and offsets from the
>>>> + // section (see SourceLevelDebugging.rst for the structure of the index).
>>>> + if (!AccelSection.isValidOffset(sizeof(Hdr) + Hdr.HeaderDataLength +
>>>> + Hdr.NumBuckets*4 + Hdr.NumHashes*8))
>>>> + return false;
>>>> +
>>>> + HdrData.DIEOffsetBase = AccelSection.getU32(&Offset);
>>>> + uint32_t NumAtoms = AccelSection.getU32(&Offset);
>>>> +
>>>> + for (unsigned i = 0; i < NumAtoms; ++i) {
>>>> + auto Atom = std::make_pair(AccelSection.getU16(&Offset),
>>>> + DWARFFormValue(AccelSection.getU16(&Offset)));
>>>>
>>>> This might not be portable ^ due to unspecified order of evaluation of function arguments, this might read the fields in the wrong order (dwarfdump built with GCC versus dwarfdump built with Clang would behave differently)
>>>
>>> Good catch! That might actually be one of the failures I’m seeing.
>>>
>>>> + HdrData.Atoms.push_back(Atom);
>>>>
>>>> If not for the point above, I'd probably suggest rolling the make_pair into the push_back, but never mind.
>>>
>>> I’ll do that once I get the getU16 calls on separate lines.
>>>
>>>> Is it worth doing HdrData.reserve(NumAtoms) before this loop?
>>>
>>> As the size is known, it wouldn't cost anything to do it, but the number of Atom definitions is 1 or 3, I doubt it matters.
>>>
>>> Fair enough. It's not like it'll be /bad/ - the container's underlying exponential growth will take care of it if we happen to end up with more in the future, etc.
>>>
>>>
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +void DWARFAcceleratorTable::dump(raw_ostream &OS) {
>>>> + // Dump the header.
>>>> + OS << "Magic = " << format("0x%08x", Hdr.Magic) << '\n';
>>>> + OS << "Version = " << format("0x%04x", Hdr.Version) << '\n';
>>>> + OS << "Hash function = " << format("0x%08x", Hdr.HashFunction) << '\n';
>>>> + OS << "Bucket count = " << Hdr.NumBuckets << '\n';
>>>> + OS << "Hashes count = " << Hdr.NumHashes << '\n';
>>>> + OS << "HeaderData length = " << Hdr.HeaderDataLength << '\n';
>>>> + OS << "DIE offset base = " << HdrData.DIEOffsetBase << '\n';
>>>> + OS << "Number of atoms = " << HdrData.Atoms.size() << '\n';
>>>>
>>>> Looks like dwarfdump is inconsistent in this regard, but you can just do it:
>>>>
>>>> OS << ...
>>>> << ...
>>>> << ...;
>>>>
>>>> rather than separate OS/; on each line.
>>>
>>> Will do
>>>
>>>> +
>>>> + unsigned i = 0;
>>>> + for (const auto &Atom: HdrData.Atoms) {
>>>> + OS << format("Atom[%d] ", i++);
>>>> + OS << " Type: " << dwarf::AtomTypeString(Atom.first);
>>>> + OS << " Form: " << dwarf::FormEncodingString(Atom.second.getForm());
>>>> + OS << "\n";
>>>>
>>>> Which would make code like this ^ just a single statement and you could omit the {} if you want (some people prefer to only omit the braces on single /line/ blocks rather than single statement blocks - up to you)
>>>
>>> Will do
>>>
>>>> + }
>>>> +
>>>> + // Now go through the actual tables and dump them.
>>>> + uint32_t Offset = sizeof(Hdr) + Hdr.HeaderDataLength;
>>>> + unsigned HashesBase = Offset + Hdr.NumBuckets * 4;
>>>> + unsigned OffsetsBase = HashesBase + Hdr.NumHashes * 4;
>>>> +
>>>> + for (unsigned Bucket = 0; Bucket < Hdr.NumBuckets; ++Bucket) {
>>>> + unsigned Index;
>>>> + Index = AccelSection.getU32(&Offset);
>>>>
>>>> Make this into a single line/initialization? ^
>>>
>>> Sure
>>>
>>>> +
>>>> + OS << format("Bucket[%d]\n", Bucket);
>>>> + if (Index == UINT32_MAX) {
>>>> + OS << " EMPTY\n";
>>>> + continue;
>>>> + }
>>>> +
>>>> + for (unsigned HashIdx = Index; HashIdx < Hdr.NumHashes; ++HashIdx) {
>>>> + unsigned HashOffset = HashesBase + HashIdx*4;
>>>> + unsigned OffsetsOffset = OffsetsBase + HashIdx*4;
>>>> + uint32_t Hash = AccelSection.getU32(&HashOffset);
>>>> +
>>>> + if (Hash % Hdr.NumBuckets != Bucket)
>>>> + break;
>>>> +
>>>> + unsigned DataOffset = AccelSection.getU32(&OffsetsOffset);
>>>> + OS << format(" Hash = 0x%08x Offset = 0x%08x\n", Hash, DataOffset);
>>>> + if (!AccelSection.isValidOffset(DataOffset)) {
>>>> + OS << " Invalid section offset\n";
>>>> + continue;
>>>> + }
>>>> + while (unsigned StringOffset = AccelSection.getU32(&DataOffset)) {
>>>> + OS << format(" Name: %08x \"%s\"\n", StringOffset,
>>>> + StringSection.getCStr(&StringOffset));
>>>> + unsigned NumData = AccelSection.getU32(&DataOffset);
>>>> + for (unsigned Data = 0; Data < NumData; ++Data) {
>>>> + OS << format(" Data[%d] => ", Data);
>>>> + unsigned i = 0;
>>>> + for (auto &Atom : HdrData.Atoms) {
>>>> + OS << format("{Atom[%d]: ", i++);
>>>> + if (Atom.second.extractValue(AccelSection, &DataOffset, nullptr))
>>>> + Atom.second.dump(OS, nullptr);
>>>> + else
>>>> + OS << "Error extracting the value";
>>>> + OS << "} ";
>>>> + }
>>>> + OS << '\n';
>>>> + }
>>>> + }
>>>> + }
>>>> + }
>>>> +}
>>>> +}
>>>>
>>>> Added: llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h?rev=221836&view=auto <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h?rev=221836&view=auto>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h (added)
>>>> +++ llvm/trunk/lib/DebugInfo/DWARFAcceleratorTable.h Wed Nov 12 17:48:10 2014
>>>> @@ -0,0 +1,38 @@
>>>> +
>>>> +#include "llvm/ADT/SmallVector.h"
>>>> +#include "llvm/DebugInfo/DWARFFormValue.h"
>>>> +
>>>> +#include <cstdint>
>>>> +
>>>> +namespace llvm {
>>>> +
>>>> +class DWARFAcceleratorTable {
>>>> +
>>>> + struct Header {
>>>> + uint32_t Magic;
>>>> + uint16_t Version;
>>>> + uint16_t HashFunction;
>>>> + uint32_t NumBuckets;
>>>> + uint32_t NumHashes;
>>>> + uint32_t HeaderDataLength;
>>>> + };
>>>> +
>>>> + struct HeaderData {
>>>> + typedef uint16_t AtomType;
>>>> + uint32_t DIEOffsetBase;
>>>> + SmallVector<std::pair<AtomType, DWARFFormValue>, 1> Atoms;
>>>> + };
>>>> +
>>>> + struct Header Hdr;
>>>> + struct HeaderData HdrData;
>>>> + DataExtractor AccelSection;
>>>> + DataExtractor StringSection;
>>>> +public:
>>>> + DWARFAcceleratorTable(DataExtractor AccelSection, DataExtractor StringSection)
>>>> + : AccelSection(AccelSection), StringSection(StringSection) {}
>>>> +
>>>> + bool extract();
>>>> + void dump(raw_ostream &OS);
>>>> +};
>>>> +
>>>> +}
>>>>
>>>> Modified: llvm/trunk/lib/DebugInfo/DWARFContext.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.cpp?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.cpp?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/DebugInfo/DWARFContext.cpp (original)
>>>> +++ llvm/trunk/lib/DebugInfo/DWARFContext.cpp Wed Nov 12 17:48:10 2014
>>>> @@ -9,6 +9,7 @@
>>>>
>>>> #include "DWARFContext.h"
>>>> #include "DWARFDebugArangeSet.h"
>>>> +#include "DWARFAcceleratorTable.h"
>>>>
>>>> #include "llvm/ADT/SmallString.h"
>>>> #include "llvm/ADT/StringSwitch.h"
>>>> @@ -59,6 +60,17 @@ static void dumpPubSection(raw_ostream &
>>>> }
>>>> }
>>>>
>>>> +static void dumpAccelSection(raw_ostream &OS, StringRef Name, StringRef Data,
>>>> + StringRef StringSection, bool LittleEndian) {
>>>> + DataExtractor AccelSection(Data, LittleEndian, 0);
>>>> + DataExtractor StrData(StringSection, LittleEndian, 0);
>>>> + OS << "\n." << Name << " contents:\n";
>>>> + DWARFAcceleratorTable Accel(AccelSection, StrData);
>>>> + if (!Accel.extract())
>>>> + return;
>>>> + Accel.dump(OS);
>>>> +}
>>>> +
>>>> void DWARFContext::dump(raw_ostream &OS, DIDumpType DumpType) {
>>>> if (DumpType == DIDT_All || DumpType == DIDT_Abbrev) {
>>>> OS << ".debug_abbrev contents:\n";
>>>> @@ -218,6 +230,22 @@ void DWARFContext::dump(raw_ostream &OS,
>>>> OS << format("%8.8x\n", strOffsetExt.getU32(&offset));
>>>> }
>>>> }
>>>> +
>>>> + if (DumpType == DIDT_All || DumpType == DIDT_AppleNames)
>>>> + dumpAccelSection(OS, "apple_names", getAppleNamesSection(),
>>>> + getStringSection(), isLittleEndian());
>>>> +
>>>> + if (DumpType == DIDT_All || DumpType == DIDT_AppleTypes)
>>>> + dumpAccelSection(OS, "apple_types", getAppleTypesSection(),
>>>> + getStringSection(), isLittleEndian());
>>>> +
>>>> + if (DumpType == DIDT_All || DumpType == DIDT_AppleNamespaces)
>>>> + dumpAccelSection(OS, "apple_namespaces", getAppleNamespacesSection(),
>>>> + getStringSection(), isLittleEndian());
>>>> +
>>>> + if (DumpType == DIDT_All || DumpType == DIDT_AppleObjC)
>>>> + dumpAccelSection(OS, "apple_objc", getAppleObjCSection(),
>>>> + getStringSection(), isLittleEndian());
>>>> }
>>>>
>>>> const DWARFDebugAbbrev *DWARFContext::getDebugAbbrev() {
>>>> @@ -565,6 +593,11 @@ DWARFContextInMemory::DWARFContextInMemo
>>>> .Case("debug_str.dwo", &StringDWOSection)
>>>> .Case("debug_str_offsets.dwo", &StringOffsetDWOSection)
>>>> .Case("debug_addr", &AddrSection)
>>>> + .Case("apple_names", &AppleNamesSection)
>>>> + .Case("apple_types", &AppleTypesSection)
>>>> + .Case("apple_namespaces", &AppleNamespacesSection)
>>>> + .Case("apple_namespac", &AppleNamespacesSection)
>>>> + .Case("apple_objc", &AppleObjCSection)
>>>> // Any more debug info sections go here.
>>>> .Default(nullptr);
>>>> if (SectionData) {
>>>>
>>>> Modified: llvm/trunk/lib/DebugInfo/DWARFContext.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.h?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.h?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/DebugInfo/DWARFContext.h (original)
>>>> +++ llvm/trunk/lib/DebugInfo/DWARFContext.h Wed Nov 12 17:48:10 2014
>>>> @@ -192,6 +192,10 @@ public:
>>>> virtual StringRef getStringOffsetDWOSection() = 0;
>>>> virtual StringRef getRangeDWOSection() = 0;
>>>> virtual StringRef getAddrSection() = 0;
>>>> + virtual StringRef getAppleNamesSection() = 0;
>>>> + virtual StringRef getAppleTypesSection() = 0;
>>>> + virtual StringRef getAppleNamespacesSection() = 0;
>>>> + virtual StringRef getAppleObjCSection() = 0;
>>>>
>>>> static bool isSupportedVersion(unsigned version) {
>>>> return version == 2 || version == 3 || version == 4;
>>>> @@ -236,6 +240,10 @@ class DWARFContextInMemory : public DWAR
>>>> StringRef StringOffsetDWOSection;
>>>> StringRef RangeDWOSection;
>>>> StringRef AddrSection;
>>>> + StringRef AppleNamesSection;
>>>> + StringRef AppleTypesSection;
>>>> + StringRef AppleNamespacesSection;
>>>> + StringRef AppleObjCSection;
>>>>
>>>> SmallVector<SmallString<32>, 4> UncompressedSections;
>>>>
>>>> @@ -256,6 +264,10 @@ public:
>>>> StringRef getPubTypesSection() override { return PubTypesSection; }
>>>> StringRef getGnuPubNamesSection() override { return GnuPubNamesSection; }
>>>> StringRef getGnuPubTypesSection() override { return GnuPubTypesSection; }
>>>> + StringRef getAppleNamesSection() override { return AppleNamesSection; }
>>>> + StringRef getAppleTypesSection() override { return AppleTypesSection; }
>>>> + StringRef getAppleNamespacesSection() override { return AppleNamespacesSection; }
>>>> + StringRef getAppleObjCSection() override { return AppleObjCSection; }
>>>>
>>>> // Sections for DWARF5 split dwarf proposal.
>>>> const DWARFSection &getInfoDWOSection() override { return InfoDWOSection; }
>>>>
>>>> Modified: llvm/trunk/test/DebugInfo/Inputs/gmlt.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/Inputs/gmlt.ll?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/Inputs/gmlt.ll?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/DebugInfo/Inputs/gmlt.ll (original)
>>>> +++ llvm/trunk/test/DebugInfo/Inputs/gmlt.ll Wed Nov 12 17:48:10 2014
>>>> @@ -95,6 +95,8 @@
>>>> ; CHECK: .debug_pubtypes contents:
>>>> ; CHECK-NOT: Offset
>>>>
>>>> +; CHECK: .apple{{.*}} contents:
>>>> +
>>>> ; Function Attrs: nounwind uwtable
>>>> define void @_Z2f1v() #0 {
>>>> entry:
>>>>
>>>> Added: llvm/trunk/test/DebugInfo/dwarfdump-accel.test
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/dwarfdump-accel.test?rev=221836&view=auto <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/dwarfdump-accel.test?rev=221836&view=auto>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/DebugInfo/dwarfdump-accel.test (added)
>>>> +++ llvm/trunk/test/DebugInfo/dwarfdump-accel.test Wed Nov 12 17:48:10 2014
>>>> @@ -0,0 +1,154 @@
>>>> +RUN: llvm-dwarfdump %p/Inputs/dwarfdump-objc.x86_64.o | FileCheck %s
>>>>
>>>> Does the example need to be this long? Could it involve fewer elements? What particular codepaths is it testing that require this length?
>>>
>>> The binary used is nice because it contains 3 full accelerator tables and one empty section.
>>>
>>> What do you mean by 'full' accelerator tables ?
>>
>> Sorry, by full I actually meant “containing data”. As opposed to the one section that is empty. I meant that I get enough data to actually test the dumping but also that I get to test that an empty section is handled correctly.
>>
>> *nod* testing an empty and non-empty section seems reasonable
>>
>>
>>> Wrt what to test in the full ones, at first I wanted to write a test that matches the DIE tree with some of the accelerator entries, but I just couldn’t figure out what would make most sense to test. I then decided to take the full dump that I verified by hand and to check it in. I could prune the hash table entries by testing only the beginning and the end for example, I don’t think testing coverage would be impacted. Is the test size really an issue?
>>>
>>> Mostly it's about making tests as maintainable as possible - by testing only as much as we need, it means if we change something (the formatting of the dwarfdump output for accelerator tables, for instance) we don't have to trudge through lots of repetitious/uninteresting test output to fix all the test cases.
>>
>>> So if we can reduce tests down to only what's necessary/intended to be tests, that'd be preferred. (&, yes, verifying only the parts of interest if we need to include extra data to test interesting cases - just the same as we don't test every attribute of a DW_TAG_subprogram, or now we don't test that a DW_AT_abstract_definition actually has a ref form and actually points to the DIE, we just use the name printing you added recently - it keeps the tests small and lightweight (if one day we find a better form encoding for these, we won't have to update most of the tests, etc))
>>
>> As I said, I started out with a test that would check that the information is actually correct (ie. that the die offsets actually point to the right DIE) for a selected set of DIEs. However, I wasn’t sure how to select a meaningful set of values to test, thus I decided to go with just the full raw output. I’ll just pick random entries to test for.
>>
>> The input itself could be smaller too, yes? Is there any need to test more than one name? (maybe it's hard to introduce just one name in an Objective C file? Perhaps it'd be easier to test a C file with just a single "void func() { }" - it'll obviously have an empty ObjC section, so you can test that, etc)
>
> I could totally use a smaller input. Reuse one that’s already there or add a new binary that fits my needs. I actually used this one on purpose because it produced most output and thus the most things to test in a single file. Would you prefer to get a very small dwarfdump-accel.test and that I sprinkle a few tests for the various tables into other tests (like namespace.ll for the apple_namespace accelerators) ? That’d be fine by me and give the same coverage, if not more, than the current test.
>
> I think a small standalone test exercising just enough to test what you want to test would be great.
Just to be sure I don’t misinterpret your request before I do the work: I suppose you would actually be fine with reusing the same already-in-tree binary for the test and then testing just a subset of the entries? I find it a bit wasteful to add an archived binary just for that purpose. I struggled as to which entries to test, but I think that any choice of entries will give us basically the same testing coverage.
Fred
> (I'm always on the fence about adding new tests versus testing new features in existing tests - we're not very principled about this stuff and it's always a bit unclear how to test a bunch of related features (test for feature X in x.foo, test for feature Y in y.foo, but what about the tests where X and Y overlap in interesting ways? etc etc) - adding things to existing tests can make the existing tests hard to follow, or liable to lose test coverage if we decide that existing tested feature should be tested in some other way, or removed, etc - not realizing that that test was the only thing testing this feature - but since you've already gone in the direction of a standalone test, I think it's a good idea to stick with that, just make the input as small as it can be while still providing the features you want to test, and only test the features you need to, so the test is more resilient to unrelated changes/easier to update when changes are made)
>
>
> Fred
>
>>
>> Fred
>>
>>
>>> - David
>>>
>>>
>>> Fred
>>>
>>>> +
>>>> +CHECK: .apple_names contents:
>>>> +CHECK: Magic = 0x48415348
>>>> +CHECK: Version = 0x0001
>>>> +CHECK: Hash function = 0x00000000
>>>> +CHECK: Bucket count = 11
>>>> +CHECK: Hashes count = 22
>>>> +CHECK: HeaderData length = 12
>>>> +CHECK: DIE offset base = 0
>>>> +CHECK: Number of atoms = 1
>>>> +CHECK: Atom[0] Type: DW_ATOM_die_offset Form: DW_FORM_data4
>>>> +CHECK: Bucket[0]
>>>> +CHECK: Hash = 0x248050fe Offset = 0x000000fc
>>>> +CHECK: Name: 00000165 "-[TestInterface Retain]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x0000024f}
>>>> +CHECK: Bucket[1]
>>>> +CHECK: Hash = 0x926d42cc Offset = 0x0000010c
>>>> +CHECK: Name: 00000057 "ReadWrite"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000001cb}
>>>> +CHECK: Bucket[2]
>>>> +CHECK: EMPTY
>>>> +CHECK: Bucket[3]
>>>> +CHECK: Hash = 0x99254268 Offset = 0x0000011c
>>>> +CHECK: Name: 0000013f "-[TestInterface setReadWrite:]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000209}
>>>> +CHECK: Hash = 0x946f52b9 Offset = 0x0000012c
>>>> +CHECK: Name: 000000c6 "-[TestInterface ReadOnly]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000109}
>>>> +CHECK: Bucket[4]
>>>> +CHECK: EMPTY
>>>> +CHECK: Bucket[5]
>>>> +CHECK: EMPTY
>>>> +CHECK: Bucket[6]
>>>> +CHECK: Hash = 0x6e8e91a3 Offset = 0x0000013c
>>>> +CHECK: Name: 000001e0 "-[TestInterface NonAtomic]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000357}
>>>> +CHECK: Hash = 0x7d1a5012 Offset = 0x0000014c
>>>> +CHECK: Name: 0000014d "setReadWrite:"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000209}
>>>> +CHECK: Hash = 0xb65f49d3 Offset = 0x0000015c
>>>> +CHECK: Name: 0000020d "setNonAtomic:"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000395}
>>>> +CHECK: Hash = 0x354997e2 Offset = 0x0000016c
>>>> +CHECK: Name: 00000120 "-[TestInterface ReadWrite]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000001cb}
>>>> +CHECK: Bucket[7]
>>>> +CHECK: Hash = 0xce8af9c8 Offset = 0x0000017c
>>>> +CHECK: Name: 0000005e "Retain"
>>>> +CHECK: Data[0] => {Atom[0]: 0x0000024f}
>>>> +CHECK: Hash = 0xa7e0338a Offset = 0x0000018c
>>>> +CHECK: Name: 0000004d "Assign"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000147}
>>>> +CHECK: Hash = 0xa9812410 Offset = 0x0000019c
>>>> +CHECK: Name: 00000105 "setAssign:"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000185}
>>>> +CHECK: Hash = 0x218d07f6 Offset = 0x000001ac
>>>> +CHECK: Name: 000001a2 "-[TestInterface Copy]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000002d3}
>>>> +CHECK: Hash = 0x0456817c Offset = 0x000001bc
>>>> +CHECK: Name: 000001bc "-[TestInterface setCopy:]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000311}
>>>> +CHECK: Hash = 0x7c83b400 Offset = 0x000001cc
>>>> +CHECK: Name: 0000006c "Copy"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000002d3}
>>>> +CHECK: Bucket[8]
>>>> +CHECK: Hash = 0x0f918046 Offset = 0x000001dc
>>>> +CHECK: Name: 000001c5 "setCopy:"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000311}
>>>> +CHECK: Hash = 0xfb097449 Offset = 0x000001ec
>>>> +CHECK: Name: 000001ff "-[TestInterface setNonAtomic:]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000395}
>>>> +CHECK: Hash = 0x71069de3 Offset = 0x000001fc
>>>> +CHECK: Name: 00000042 "ReadOnly"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000109}
>>>> +CHECK: Bucket[9]
>>>> +CHECK: Hash = 0xd55908c6 Offset = 0x0000020c
>>>> +CHECK: Name: 000000fa "-[TestInterface setAssign:]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000185}
>>>> +CHECK: Hash = 0xa584b20e Offset = 0x0000021c
>>>> +CHECK: Name: 0000018c "setRetain:"
>>>> +CHECK: Data[0] => {Atom[0]: 0x0000028d}
>>>> +CHECK: Hash = 0x9429886d Offset = 0x0000022c
>>>> +CHECK: Name: 00000076 "NonAtomic"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000357}
>>>> +CHECK: Hash = 0x287cc300 Offset = 0x0000023c
>>>> +CHECK: Name: 000000de "-[TestInterface Assign]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000147}
>>>> +CHECK: Hash = 0x51ce5684 Offset = 0x0000024c
>>>> +CHECK: Name: 00000181 "-[TestInterface setRetain:]"
>>>> +CHECK: Data[0] => {Atom[0]: 0x0000028d}
>>>> +CHECK: Bucket[10]
>>>> +CHECK: EMPTY
>>>> +
>>>> +
>>>> +CHECK: .apple_types contents:
>>>> +CHECK: Magic = 0x48415348
>>>> +CHECK: Version = 0x0001
>>>> +CHECK: Hash function = 0x00000000
>>>> +CHECK: Bucket count = 4
>>>> +CHECK: Hashes count = 4
>>>> +CHECK: HeaderData length = 20
>>>> +CHECK: DIE offset base = 0
>>>> +CHECK: Number of atoms = 3
>>>> +CHECK: Atom[0] Type: DW_ATOM_die_offset Form: DW_FORM_data4
>>>> +CHECK: Atom[1] Type: DW_ATOM_die_tag Form: DW_FORM_data2
>>>> +CHECK: Atom[2] Type: DW_ATOM_type_flags Form: DW_FORM_data1
>>>> +CHECK: Bucket[0]
>>>> +CHECK: Hash = 0x0b888030 Offset = 0x00000058
>>>> +CHECK: Name: 00000046 "int"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000000f4} {Atom[1]: 0x0024} {Atom[2]: 0x00}
>>>> +CHECK: Bucket[1]
>>>> +CHECK: Hash = 0x0b881d29 Offset = 0x0000006b
>>>> +CHECK: Name: 0000021b "SEL"
>>>> +CHECK: Data[0] => {Atom[0]: 0x000003e0} {Atom[1]: 0x0016} {Atom[2]: 0x00}
>>>> +CHECK: Hash = 0x2c549f3d Offset = 0x0000007e
>>>> +CHECK: Name: 00000067 "NSObject"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000100} {Atom[1]: 0x0013} {Atom[2]: 0x00}
>>>> +CHECK: Bucket[2]
>>>> +CHECK: Hash = 0x16a83cb6 Offset = 0x00000091
>>>> +CHECK: Name: 00000039 "TestInterface"
>>>> +CHECK: Data[0] => {Atom[0]: 0x0000002f} {Atom[1]: 0x0013} {Atom[2]: 0x00}
>>>> +CHECK: Bucket[3]
>>>> +CHECK: EMPTY
>>>> +
>>>> +
>>>> +CHECK: .apple_namespaces contents:
>>>> +CHECK-NOT: Magic
>>>> +
>>>> +
>>>> +CHECK: .apple_objc contents:
>>>> +CHECK: Magic = 0x48415348
>>>> +CHECK: Version = 0x0001
>>>> +CHECK: Hash function = 0x00000000
>>>> +CHECK: Bucket count = 1
>>>> +CHECK: Hashes count = 1
>>>> +CHECK: HeaderData length = 12
>>>> +CHECK: DIE offset base = 0
>>>> +CHECK: Number of atoms = 1
>>>> +CHECK: Atom[0] Type: DW_ATOM_die_offset Form: DW_FORM_data4
>>>> +CHECK: Bucket[0]
>>>> +CHECK: Hash = 0x16a83cb6 Offset = 0x0000002c
>>>> +CHECK: Name: 00000039 "TestInterface"
>>>> +CHECK: Data[0] => {Atom[0]: 0x00000109}
>>>> +CHECK: Data[1] => {Atom[0]: 0x00000147}
>>>> +CHECK: Data[2] => {Atom[0]: 0x00000185}
>>>> +CHECK: Data[3] => {Atom[0]: 0x000001cb}
>>>> +CHECK: Data[4] => {Atom[0]: 0x00000209}
>>>> +CHECK: Data[5] => {Atom[0]: 0x0000024f}
>>>> +CHECK: Data[6] => {Atom[0]: 0x0000028d}
>>>> +CHECK: Data[7] => {Atom[0]: 0x000002d3}
>>>> +CHECK: Data[8] => {Atom[0]: 0x00000311}
>>>> +CHECK: Data[9] => {Atom[0]: 0x00000357}
>>>> +CHECK: Data[10] => {Atom[0]: 0x00000395}
>>>> \ No newline at end of file
>>>>
>>>> Modified: llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp?rev=221836&r1=221835&r2=221836&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp?rev=221836&r1=221835&r2=221836&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp (original)
>>>> +++ llvm/trunk/tools/llvm-dwarfdump/llvm-dwarfdump.cpp Wed Nov 12 17:48:10 2014
>>>> @@ -45,6 +45,10 @@ DumpType("debug-dump", cl::init(DIDT_All
>>>> clEnumValN(DIDT_All, "all", "Dump all debug sections"),
>>>> clEnumValN(DIDT_Abbrev, "abbrev", ".debug_abbrev"),
>>>> clEnumValN(DIDT_AbbrevDwo, "abbrev.dwo", ".debug_abbrev.dwo"),
>>>> + clEnumValN(DIDT_AppleNames, "apple_names", ".apple_names"),
>>>> + clEnumValN(DIDT_AppleTypes, "apple_types", ".apple_types"),
>>>> + clEnumValN(DIDT_AppleNamespaces, "apple_namespaces", ".apple_namespaces"),
>>>> + clEnumValN(DIDT_AppleObjC, "apple_objc", ".apple_objc"),
>>>> clEnumValN(DIDT_Aranges, "aranges", ".debug_aranges"),
>>>> clEnumValN(DIDT_Info, "info", ".debug_info"),
>>>> clEnumValN(DIDT_InfoDwo, "info.dwo", ".debug_info.dwo"),
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141113/9a93e236/attachment.html>
More information about the llvm-commits
mailing list