Support DW_FORM_strx* in llvm-dwp.

[kimanh]

Summary

Currently llvm-dwp only handled DW_FORM_string and DW_FORM_GNU_str_index; with this patch it also starts to handle DW_FORM_strx[1-4]?

[dblaikie]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

7–21

This looks a bit more complicated than it needs to be - you can dwp any two .o files (so you don't need headers, function calls/cross references, etc), or even a single .o file (& a singel one might be sufficient for the purposes of this test case).

26–30

What sort of incompletenes problems does this test case trip over? What /should/ be tested but isn't because it's broken in other ways?

It might be that this patch should be held off on until otehr fixes are made so we're not adding code that can't be well tested, etc.

[tamur]

Simplified the test. llvm-dwp and llvm-dwarfdump works fine for this simple test, but in any case I don't want to overspecify the output.

[tamur]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

26–30

IIRC, dwarfdump had a non-fatal complaint, but they both work fine for this simple test. Still, I don't want to overspecify the output (and I don't want to make the test depend too much on llvm-dwarfdump), so I think just checking that llvm-dwp works correctly and the output has a DW_FORM_strx1 should suffice?

[dblaikie]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

26–30

IIRC, dwarfdump had a non-fatal complaint, but they both work fine for this simple test.

Still, I don't want to overspecify the output (and I don't want to make the test depend too much on llvm-dwarfdump),

Not overspecifying is a worthy goal (& we do lots of things to ensure that - symbolic matches, etc) - though llvm-dwarfdump itself exists predominantly for writing test cases, so that dependence in general is intentional/not something to avoid.

so I think just checking that llvm-dwp works correctly and the output has a DW_FORM_strx1 should suffice?

Specifically the reason llvm-dwp tries to read strings is to produce error messages using the DW_AT_name and DW_AT_dwo_name - so it's probably appropriate to specifically test that such an error message has the right string value (that the parsing was correct, not just that that codepath didn't produce an error).

See llvm/test/tools/llvm-dwp/X86/duplicate.test for existing test coverage.

Also, though I don't feel too strongly about this, I think the current leaning is to write such tests in assembly (though still include source/compile commands, etc - but just not checking in binary object files) and have the test use llvm-mc to assemble them within the test.

[tamur]

I deleted "#include " line in handle_strx.test and ran clang-format on llvm-dwp.cpp

[tamur]

• Introduce a CompileUnitHeader structure, it is needed to know the dwarf version, because, for example, debug_str_offsets sectoins have a header only when dwarf version is 5.
• Introduce a function to parse CompileUnitHeader.
• Introduce a function to compute the size of debug_str_offsets section headers.
• Fix writeStringsAndOffsets to copy debug_str_offsets section headers correctly.
• Get getIndexedString funciton to handle DW_FORM_strx forms correctly.

[tamur]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

26–30

OK, it turns out debug_str_offsets section had problems with respect to header parsing, and dwp did not produce correct strings. With my latest changes, I do see correct strings now.

Almost all tests in llvm/test/tools/llvm-dwp/X86 start from a .dwo file, and I'd like to do it the same way, if that's ok with you.

[dblaikie]

llvm/tools/llvm-dwp/llvm-dwp.cpp

194–197

This should probably be:

Out.emitBytes(Data.getBytes(&Offset, HeaderSize));

I think?

292–293

unnecessary reformatting, maybe?

[dblaikie]

Ah, looks like this overlaps a bit with https://reviews.llvm.org/D74312 - might be worth checking there if there's any necessary issues (rather than orthogonal pieces) that patch addresses that this one doesn't, etc.

[tamur]

Done reviewer suggestions and fixed clang -format complaints.

[tamur]

In D75485#1908787, @dblaikie wrote:

Ah, looks like this overlaps a bit with https://reviews.llvm.org/D74312 - might be worth checking there if there's any necessary issues (rather than orthogonal pieces) that patch addresses that this one doesn't, etc.

Thank you, I was not aware https://reviews.llvm.org/D74312 ; I looked at it, yes it seems this patch and that mostly solves the same problems; it seems parts of that patch have already been checked in (maybe as another patch?) but other parts have not seen action for some time. I added the author @SouraVX as a reviewer for comment.

[SouraVX]

In D75485#1908830, @tamur wrote:

In D75485#1908787, @dblaikie wrote:

Ah, looks like this overlaps a bit with https://reviews.llvm.org/D74312 - might be worth checking there if there's any necessary issues (rather than orthogonal pieces) that patch addresses that this one doesn't, etc.

Thank you, I was not aware https://reviews.llvm.org/D74312 ; I looked at it, yes it seems this patch and that mostly solves the same problems; it seems parts of that patch have already been checked in (maybe as another patch?) but other parts have not seen action for some time. I added the author @SouraVX as a reviewer for comment.

Hi thanks for doing this, I was occupied by my other reviews so the debug_str_offsets changes didn't went in. D74425 only addresses info section(Mostly the changes in Header) parsing and writing to dwp.
BTW, have you tested the resultant dwp for debuggability with GDB ?? I also tried to apply locally D74312(info + str_offsets support), As I recall loading of dwp was fine. But their were some complaints reported when you try running/set break etc. If possible can you confirm this ?

Your changes LGTM! but you can wait for @dblaikie comments/approval.

[SouraVX]

@jini.susan.george

[tamur]

BTW, have you tested the resultant dwp for debuggability with GDB ?? I also tried to apply locally D74312(info + str_offsets support), As I recall loading of dwp was fine. But their were some complaints reported when you try running/set break etc. If possible can you confirm this ?

Hi, with this patch I am able to run gdb successsfully on very basic examples. But I am using a custom gdb, that includes some pending patches upstream. llvm-dwp does not have support for loclists and range lists, and I'd not be surprised if there are other problems I am yet unaware of. I am planning to tackle loclists and range lists next, so I don't think we should expect a good gdb experience with .dwp yet.

[dblaikie]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

9–12

I'd probably simplify this to something like:

void f1() {
}

That'll be shorter DWARF and machine code (no need for a function call, a return type, etc).

14

Remove this line that's no longer needed/relevant.

26–30

Could you add a DWARFv5 variation of the duplicate.test (either in that test, or in another/similar one alongside) - this will test that the strings are being parsed correctly by llvm-dwp when it needs to parse them (to extract the dwo_name and source file name).

llvm/tools/llvm-dwp/llvm-dwp.cpp

192

Any idea if this assertion could be hit by malformed input? If that's the case it shouldn't be an assert & should be full error handling - but you could leave it as a FIXME for now.

[ikudrin]

llvm/tools/llvm-dwp/llvm-dwp.cpp

75

There is a named constant for that: DW_LENGTH_DWARF64.

112

• The function should be static.
• The function should check if Info has enough data in it for the full header.

116

Please, Consider using DWARFDataExtractor::getInitialLength() here.

[tamur]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

26–30

I added string parsing to this test.

[tamur]

Friendly ping?

[dblaikie]

llvm/tools/llvm-dwp/llvm-dwp.cpp

137

That's probably a fairly opaque error message compared to returning Err here (possibly extracting the string from Err and adding a prefix, one example of that is here: https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Execution.cpp#L76 - or the Error could be passed up unmodified, potentially, though that might give a less legible message to the user). Same goes for other error failures in this function.

Also, and I know it's annoying to make them, but each of these failure paths should have tests.

[SouraVX]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

2

How about instead of putting raw objects files dw5.dwo, put it as asm file and use llvm-mc to generate object and llvm-dwarfdump and so forth.

[dblaikie]

llvm/test/tools/llvm-dwp/X86/handle_strx.test

2

This was already discussed here: https://reviews.llvm.org/D75485#inline-688717

[kimanh]

We recently ran into this issue too. It seems as if the thread was forgotten about, and if some comments were not addressed yet. Ping @tamur @dblaikie : Are there any plans for this?

[kimanh]

Picking up patch and updating outstanding issues:

• Adding test cases for each error case in parseCompileUnitHeader
• Changing the error message to reuse the message from Error in parseCompileUnitHeader
• Adapted tests (minor change: test error message, and added required header info to unsupported_cu_index_version)

[kimanh]

Commandeering revision.

[kimanh]

Answering leftover TODOs except of the FIXME one.

This change also was rebased to origin/main, so some changes to llvm-dwp are highlighted that are not mine (lightgreen).

llvm/test/tools/llvm-dwp/X86/handle_strx.test

2

I kept tamur's test case as discussed in https://reviews.llvm.org/D75485#inline-688717 but used .s files for the new test cases. Let me know if you want me to change anything.

[ikudrin]

llvm/test/tools/llvm-dwp/X86/invalid_string_form.test

2

Probably, the line was added accidentally.

llvm/test/tools/llvm-dwp/X86/unsupported_cu_index_version.s

4–5

The comment is outdated with the changes in .debug_info.dwo

[dblaikie]

llvm/test/tools/llvm-dwp/X86/invalid_cu_header_unittype.s

5 ↗

(On Diff #338007)

The other messages are specific about what they can't parse - the unit version or the unit length. Hmm, I see because this uses the generic "check the error after parsing a bunch of fields".

Maybe it'd be worth checking that the length is potentially correct? That the length is long enough to include the header, and that the section is long enough to fit the length that's been parsed?

llvm/tools/llvm-dwp/llvm-dwp.cpp

125

I think the norm is probably not to include a newline in the error string - but wherever it's printed can choose where the newline should go.
Also, I think the norm is to not capitalize error messages (think about compiler error messages "error: unexpected thing") - so probably best to change "Cannot" to "cannot" in all these.

[kimanh]

Addressing reviewer comments:

• adding test on the header length, which changed test set
• removing accidentally added line in test
• removing obsolete comment
• update error messages to start with lower letter, and no new line

[kimanh]

Re-uploading as merged commit.

[dblaikie]

Seems OK, I think. Few minor wording/coding changes to deal with.

llvm/test/tools/llvm-dwp/X86/handle_strx.test

2

Yeah, I wouldn't mind if tamur's test was changed to .s, but I don't insist on it.

llvm/tools/llvm-dwp/llvm-dwp.cpp

125

This suggests the header itself is too large for the debug_info section, but what it's testing is that the length specified in the header is too large - so maybe rephrasing this to something like: "compile unit exceeds .debug_info section range"?

160

This is an assertion because the length has been checked ahead of time, yeah? So there's no failure expected?

I guess the assertion was added to suppress an llvm::Error about the Error being unchecked? The problem is that in a non-assertions build that checking will fail because the Error won't be checked because the assertion isn't executed.

Perhaps it'd be OK to call the extraction functions without an Error parameter? (getU32(&Offset) etc) since the bounds have already been checked

273

(are you using/interested in DWARF type units (-fdebug-types-section)? In DWARFv5 those were moved into the .debug_info section, so this would need to be updated to handle them at some ponit)

[kimanh]

• removed Err parameter after already checking the header size
• updated error message for compile unit length
• adapted abbrev offset size (and thus also header size) for dwarf32/dwarf64

[kimanh]

Thanks a lot for the reviews!

After reading up more on the DWARF specs, I noticed that debug abbrev offsets were not updated for dwarf 32-bit, and 64-bit. I've changed that now in the latest patch.

llvm/test/tools/llvm-dwp/X86/handle_strx.test

2

Alright, in that case I prefer to keep it. I'll be updating the code again for type units, so in case your opinion changes I'll take it on in a follow up patch!

llvm/tools/llvm-dwp/llvm-dwp.cpp

160

Yes right, I added the assertion because no failure is expected, and you're right that LLVM would throw an error if that's not checked. That makes sense to remove the Error parameter in that case, thanks!

273

Thanks for pointing this out! I'm currently working on a patch to add support for type units, and also for writing v5 cu/tu index sections.

[dblaikie]

In D75485#2711582, @kimanh wrote:

Thanks a lot for the reviews!

After reading up more on the DWARF specs, I noticed that debug abbrev offsets were not updated for dwarf 32-bit, and 64-bit. I've changed that now in the latest patch.

Pretty sure the dwp code doesn't handle DWARF64 at all - might be best to leave that code to a separate patch with dedicated testing. (if you really want to include it in this patch - please include tests for it)

[kimanh]

• Removing 64-bit DWARF parts

[kimanh]

Ok, sounds good, removed the 64-bit DWARF parts now. If everything looks fine, could you help me to land this patch?

[kimanh]

Thanks for the review and the help with landing the patch :)!

llvm/test/tools/llvm-dwp/X86/handle_strx.test

+RUN: llvm-dwp %p/../Inputs/handle_strx/dw5.dwo -o %t 2>/dev/null
+RUN: llvm-dwarfdump --verbose %t 2>/dev/null | FileCheck --check-prefix=READ_STRX %s
+
+RUN: not llvm-dwp %p/../Inputs/handle_strx/dw5.dwo %p/../Inputs/handle_strx/dw5.dwo -o %t 2>&1 \
+RUN:   | FileCheck --check-prefix=PARSE_STRX %s
+
+
+dw5.o is compiled from a file that contains a single empty void function,
+with options -gdwarf-5 and -gsplit-dwarf.
+
+READ_STRX: DW_AT_name [DW_FORM_strx1]{{.*}}dw5.cc
+
+PARSE_STRX: error: duplicate DWO ID ({{.*}}) in 'dw5.cc' and 'dw5.cc'
+

llvm/test/tools/llvm-dwp/X86/invalid_cu_header_length.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.o \
+# RUN:         -split-dwarf-file=%t.dwo -dwarf-version=5
+# RUN: not llvm-dwp %t.dwo -o %t.dwp 2>&1 | FileCheck %s
+
+# CHECK: error: compile unit exceeds .debug_info section range: 20 >= 6
+    .section  .debug_info.dwo,"e",@progbits
+    .long 16      # Length of Unit
+    .short 5      # Version
 No newline at end of file

llvm/test/tools/llvm-dwp/X86/invalid_cu_header_length_type.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.o \
+# RUN:         -split-dwarf-file=%t.dwo -dwarf-version=5
+# RUN: not llvm-dwp %t.dwo -o %t.dwp 2>&1 | FileCheck %s
+
+# CHECK: error: cannot parse compile unit length: unexpected end of data at offset 0x2 while reading [0x0, 0x4
+    .section  .debug_info.dwo,"e",@progbits
+    .short  0 # Length of Unit

llvm/test/tools/llvm-dwp/X86/invalid_cu_header_version.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.o \
+# RUN:         -split-dwarf-file=%t.dwo -dwarf-version=5
+# RUN: not llvm-dwp %t.dwo -o %t.dwp 2>&1 | FileCheck %s
+
+# CHECK: error: cannot parse compile unit version: unexpected end of data at offset 0x4 while reading [0x4, 0x6)
+    .section  .debug_info.dwo,"e",@progbits
+    .long 0 # Length of Unit

llvm/test/tools/llvm-dwp/X86/invalid_string_form.test

 RUN: not llvm-dwp %p/../Inputs/invalid_string_form.dwo -o %t 2>&1 | FileCheck %s
 
-CHECK: error: {{.*}}invalid_string_form.dwo': string field encoded without DW_FORM_string or DW_FORM_GNU_str_index
+CHECK: error: {{.*}}: string field must be encoded with one of the following:
+CHECK: DW_FORM_string, DW_FORM_strx, DW_FORM_strx1, DW_FORM_strx2, DW_FORM_strx3, DW_FORM_strx4, or DW_FORM_GNU_str_index.

llvm/test/tools/llvm-dwp/X86/unsupported_cu_index_version.s

 # RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.dwp
 # RUN: not llvm-dwp %t.dwp -o %t 2>&1 | FileCheck %s
 
 # CHECK: error: unsupported cu_index version: 5 (only version 2 is supported)
-
-## To reach the test point, no real data is needed in .debug_info.dwo,
-## but the section should not be empty.
     .section .debug_info.dwo, "e", @progbits
-    .space 1
-
+    .long  .Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
+.Ldebug_info_dwo_start0:
+    .short 5                       # DWARF version number
+    .byte 5                        # DWARF Unit type
+    .byte 8                        # Address Size (in bytes)
+    .long 0                        # Offset Into Abbrev. Section
+    .quad -346972125991005518
+.Ldebug_info_dwo_end0:
     .section .debug_cu_index, "", @progbits
 ## Header:
     .short 5                        # Version
     .space 2                        # Padding
     .long 2                         # Section count
     .long 1                         # Unit count
     .long 2                         # Slot count
 ## Hash Table of Signatures:

llvm/tools/llvm-dwp/llvm-dwp.cpp

 //
 //===----------------------------------------------------------------------===//
 #include "DWPError.h"
 #include "DWPStringPool.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
+#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"
static cl::list<std::string> ExecFilenames(
     cl::desc("Specify the executable/library files to get the list of *.dwo from"),
     cl::value_desc("filename"), cl::cat(DwpCategory));
 
 static cl::opt<std::string> OutputFilename(cl::Required, "o",
                                            cl::desc("Specify the output file."),
                                            cl::value_desc("filename"),
                                            cl::cat(DwpCategory));
 
+// Returns the size of debug_str_offsets section headers in bytes.
+static uint64_t debugStrOffsetsHeaderSize(DataExtractor StrOffsetsData,
+                                          uint16_t DwarfVersion) {
+  if (DwarfVersion <= 4)
+    return 0; // There is no header before dwarf 5.
+  uint64_t Offset = 0;
+  uint64_t Length = StrOffsetsData.getU32(&Offset);
+  if (Length == llvm::dwarf::DW_LENGTH_DWARF64)
+    return 16; // unit length: 12 bytes, version: 2 bytes, padding: 2 bytes.
+  return 8;    // unit length: 4 bytes, version: 2 bytes, padding: 2 bytes.
+}
+
+// Holds data for Skeleton and Split Compilation Unit Headers as defined in
+// Dwarf 5 specification, 7.5.1.2 and Dwarf 4 specification 7.5.1.1.
+struct CompileUnitHeader {
+  // unit_length field. Note that the type is uint64_t even in 32-bit dwarf.
+  uint64_t Length = 0;
+
+  // version field.
+  uint16_t Version = 0;
+
+  // unit_type field. Initialized only if Version >= 5.
+  uint8_t UnitType = 0;
+
+  // address_size field.
+  uint8_t AddrSize = 0;
+
+  // debug_abbrev_offset field. Note that the type is uint64_t even in 32-bit
+  // dwarf. It is assumed to be 0.
+  uint64_t DebugAbbrevOffset = 0;
+
+  // dwo_id field. This resides in the header only if Version >= 5.
+  // In earlier versions, it is read from DW_AT_GNU_dwo_id.
+  Optional<uint64_t> Signature = None;
+
+  // Derived from the length of Length field.
+  dwarf::DwarfFormat Format = dwarf::DwarfFormat::DWARF32;
+
+  // The size of the Header in bytes. This is derived while parsing the header,
+  // and is stored as a convenience.
+  uint8_t HeaderSize = 0;
+};
+
+// Parse and return the header of the compile unit.
+static Expected<CompileUnitHeader> parseCompileUnitHeader(StringRef Info) {
+  CompileUnitHeader Header;
+  Error Err = Error::success();
+  uint64_t Offset = 0;
+  DWARFDataExtractor InfoData(Info, true, 0);
+  std::tie(Header.Length, Header.Format) =
+      InfoData.getInitialLength(&Offset, &Err);
+  if (Err)
+    return make_error<DWPError>("cannot parse compile unit length: " +
+                                llvm::toString(std::move(Err)));
+
+  if (!InfoData.isValidOffset(Offset + (Header.Length - 1))) {
+    return make_error<DWPError>(
+        "compile unit exceeds .debug_info section range: " +
+        utostr(Offset + Header.Length) + " >= " + utostr(InfoData.size()));
+  }
+
+  Header.Version = InfoData.getU16(&Offset, &Err);
+  if (Err)
+    return make_error<DWPError>("cannot parse compile unit version: " +
+                                llvm::toString(std::move(Err)));
+
+  uint64_t MinHeaderLength;
+  if (Header.Version >= 5) {
+    // Size: Version (2), UnitType (1), AddrSize (1), DebugAbbrevOffset (4),
+    // Signature (8)
+    MinHeaderLength = 16;
+  } else {
+    // Size: Version (2), DebugAbbrevOffset (4), AddrSize (1)
+    MinHeaderLength = 7;
+  }
+  if (Header.Length < MinHeaderLength) {
+    return make_error<DWPError>(
+        "compile unit length is too small: expected at least " +
+        utostr(MinHeaderLength) + " got " + utostr(Header.Length) + ".");
+  }
+  if (Header.Version >= 5) {
+    Header.UnitType = InfoData.getU8(&Offset);
+    Header.AddrSize = InfoData.getU8(&Offset);
+    Header.DebugAbbrevOffset = InfoData.getU32(&Offset);
+    Header.Signature = InfoData.getU64(&Offset);
+  } else {
+    // Note that, address_size and debug_abbrev_offset fields have switched
+    // places between dwarf version 4 and 5.
+    Header.DebugAbbrevOffset = InfoData.getU32(&Offset);
+    Header.AddrSize = InfoData.getU8(&Offset);
+  }
+
+  Header.HeaderSize = Offset;
+  return Header;
+}
+
 static void writeStringsAndOffsets(MCStreamer &Out, DWPStringPool &Strings,
                                    MCSection *StrOffsetSection,
                                    StringRef CurStrSection,
-                                   StringRef CurStrOffsetSection) {
+                                   StringRef CurStrOffsetSection,
+                                   const CompileUnitHeader &Header) {
   // Could possibly produce an error or warning if one of these was non-null but
   // the other was null.
   if (CurStrSection.empty() || CurStrOffsetSection.empty())
     return;
 
   DenseMap<uint64_t, uint32_t> OffsetRemapping;
 
   DataExtractor Data(CurStrSection, true, 0);
   uint64_t LocalOffset = 0;
   uint64_t PrevOffset = 0;
   while (const char *s = Data.getCStr(&LocalOffset)) {
     OffsetRemapping[PrevOffset] =
         Strings.getOffset(s, LocalOffset - PrevOffset);
     PrevOffset = LocalOffset;
   }
 
   Data = DataExtractor(CurStrOffsetSection, true, 0);
 
   Out.SwitchSection(StrOffsetSection);
 
+  uint64_t HeaderSize = debugStrOffsetsHeaderSize(Data, Header.Version);
   uint64_t Offset = 0;
   uint64_t Size = CurStrOffsetSection.size();
+  // FIXME: This can be caused by bad input and should be handled as such.
+  assert(HeaderSize <= Size && "StrOffsetSection size is less than its header");
+  // Copy the header to the output.
+  Out.emitBytes(Data.getBytes(&Offset, HeaderSize));
   while (Offset < Size) {
     auto OldOffset = Data.getU32(&Offset);
     auto NewOffset = OffsetRemapping[OldOffset];
     Out.emitIntValue(NewOffset, 4);
   }
 }
 
 static uint64_t getCUAbbrev(StringRef Abbrev, uint64_t AbbrCode) {
   uint64_t CurCode;
   uint64_t Offset = 0;

 
 struct CompileUnitIdentifiers {
   uint64_t Signature = 0;
   const char *Name = "";
   const char *DWOName = "";
 };
 
 static Expected<const char *>
-getIndexedString(dwarf::Form Form, DataExtractor InfoData,
-                 uint64_t &InfoOffset, StringRef StrOffsets, StringRef Str) {
+getIndexedString(dwarf::Form Form, DataExtractor InfoData, uint64_t &InfoOffset,
+                 StringRef StrOffsets, StringRef Str, uint16_t Version) {
   if (Form == dwarf::DW_FORM_string)
     return InfoData.getCStr(&InfoOffset);
-  if (Form != dwarf::DW_FORM_GNU_str_index)
+  uint64_t StrIndex;
+  switch (Form) {
+  case dwarf::DW_FORM_strx1:
+    StrIndex = InfoData.getU8(&InfoOffset);
+    break;
+  case dwarf::DW_FORM_strx2:
+    StrIndex = InfoData.getU16(&InfoOffset);
+    break;
+  case dwarf::DW_FORM_strx3:
+    StrIndex = InfoData.getU24(&InfoOffset);
+    break;
+  case dwarf::DW_FORM_strx4:
+    StrIndex = InfoData.getU32(&InfoOffset);
+    break;
+  case dwarf::DW_FORM_strx:
+  case dwarf::DW_FORM_GNU_str_index:
+    StrIndex = InfoData.getULEB128(&InfoOffset);
+    break;
+  default:
     return make_error<DWPError>(
-        "string field encoded without DW_FORM_string or DW_FORM_GNU_str_index");
-  auto StrIndex = InfoData.getULEB128(&InfoOffset);
+        "string field must be encoded with one of the following: "
+        "DW_FORM_string, DW_FORM_strx, DW_FORM_strx1, DW_FORM_strx2, "
+        "DW_FORM_strx3, DW_FORM_strx4, or DW_FORM_GNU_str_index.");
+  }
   DataExtractor StrOffsetsData(StrOffsets, true, 0);
   uint64_t StrOffsetsOffset = 4 * StrIndex;
+  StrOffsetsOffset += debugStrOffsetsHeaderSize(StrOffsetsData, Version);
+
   uint64_t StrOffset = StrOffsetsData.getU32(&StrOffsetsOffset);
   DataExtractor StrData(Str, true, 0);
   return StrData.getCStr(&StrOffset);
 }
 
 static Expected<CompileUnitIdentifiers> getCUIdentifiers(StringRef Abbrev,
                                                          StringRef Info,
                                                          StringRef StrOffsets,
                                                          StringRef Str) {
-  uint64_t Offset = 0;
+  Expected<CompileUnitHeader> HeaderOrError = parseCompileUnitHeader(Info);
+  if (!HeaderOrError)
+    return HeaderOrError.takeError();
+  CompileUnitHeader &Header = *HeaderOrError;
   DataExtractor InfoData(Info, true, 0);
-  dwarf::DwarfFormat Format = dwarf::DwarfFormat::DWARF32;
-  uint64_t Length = InfoData.getU32(&Offset);
-  CompileUnitIdentifiers ID;
-  Optional<uint64_t> Signature = None;
-  // If the length is 0xffffffff, then this indictes that this is a DWARF 64
-  // stream and the length is actually encoded into a 64 bit value that follows.
-  if (Length == 0xffffffffU) {
-    Format = dwarf::DwarfFormat::DWARF64;
-    Length = InfoData.getU64(&Offset);
-  }
-  uint16_t Version = InfoData.getU16(&Offset);
-  if (Version >= 5) {
-    auto UnitType = InfoData.getU8(&Offset);
-    if (UnitType != dwarf::DW_UT_split_compile)
-      return make_error<DWPError>(
-          std::string("unit type DW_UT_split_compile type not found in "
-                      "debug_info header. Unexpected unit type 0x" +
-                      utostr(UnitType) + " found"));
-  }
-  InfoData.getU32(&Offset); // Abbrev offset (should be zero)
-  uint8_t AddrSize = InfoData.getU8(&Offset);
-  if (Version >= 5)
-    Signature = InfoData.getU64(&Offset);
-  uint32_t AbbrCode = InfoData.getULEB128(&Offset);
+  uint64_t Offset = Header.HeaderSize;
+  if (Header.Version >= 5 && Header.UnitType != dwarf::DW_UT_split_compile)
+                    utostr(Header.UnitType) + " found"));
+
+  CompileUnitIdentifiers ID;
 
+  uint32_t AbbrCode = InfoData.getULEB128(&Offset);
   DataExtractor AbbrevData(Abbrev, true, 0);
   uint64_t AbbrevOffset = getCUAbbrev(Abbrev, AbbrCode);
   auto Tag = static_cast<dwarf::Tag>(AbbrevData.getULEB128(&AbbrevOffset));
   if (Tag != dwarf::DW_TAG_compile_unit)
     return make_error<DWPError>("top level DIE is not a compile unit");
   // DW_CHILDREN
   AbbrevData.getU8(&AbbrevOffset);
   uint32_t Name;
   dwarf::Form Form;
   while ((Name = AbbrevData.getULEB128(&AbbrevOffset)) |
          (Form = static_cast<dwarf::Form>(AbbrevData.getULEB128(&AbbrevOffset))) &&
          (Name != 0 || Form != 0)) {
     switch (Name) {
     case dwarf::DW_AT_name: {
-      Expected<const char *> EName =
-          getIndexedString(Form, InfoData, Offset, StrOffsets, Str);
+      Expected<const char *> EName = getIndexedString(
+          Form, InfoData, Offset, StrOffsets, Str, Header.Version);
       if (!EName)
         return EName.takeError();
       ID.Name = *EName;
       break;
     }
     case dwarf::DW_AT_GNU_dwo_name:
     case dwarf::DW_AT_dwo_name: {
-      Expected<const char *> EName =
-          getIndexedString(Form, InfoData, Offset, StrOffsets, Str);
+      Expected<const char *> EName = getIndexedString(
+          Form, InfoData, Offset, StrOffsets, Str, Header.Version);
       if (!EName)
         return EName.takeError();
       ID.DWOName = *EName;
       break;
     }
     case dwarf::DW_AT_GNU_dwo_id:
-      Signature = InfoData.getU64(&Offset);
+      Header.Signature = InfoData.getU64(&Offset);
       break;
     default:
-      DWARFFormValue::skipValue(Form, InfoData, &Offset,
-                                dwarf::FormParams({Version, AddrSize, Format}));
+      DWARFFormValue::skipValue(
+          Form, InfoData, &Offset,
+          dwarf::FormParams({Header.Version, Header.AddrSize, Header.Format}));
     }
   }
-  if (!Signature)
+  if (!Header.Signature)
     return make_error<DWPError>("compile unit missing dwo_id");
-  ID.Signature = *Signature;
+  ID.Signature = *Header.Signature;
   return ID;
 }
 
 struct UnitIndexEntry {
   DWARFUnitIndex::Entry::SectionContribution Contributions[8];
   std::string Name;
   std::string DWOName;
   StringRef DWPName;
for (const auto &Section : Obj.sections())
               UncompressedSections, ContributionOffsets, CurEntry,
               CurStrSection, CurStrOffsetSection, CurTypesSection, InfoSection,
               AbbrevSection, CurCUIndexSection, CurTUIndexSection))
         return Err;
 
     if (InfoSection.empty())
       continue;
 
+    Expected<CompileUnitHeader> CompileUnitHeaderOrErr =
+        parseCompileUnitHeader(InfoSection);
+    if (!CompileUnitHeaderOrErr)
+      return CompileUnitHeaderOrErr.takeError();
+    CompileUnitHeader &CompileUnitHeader = *CompileUnitHeaderOrErr;
+
     writeStringsAndOffsets(Out, Strings, StrOffsetSection, CurStrSection,
-                           CurStrOffsetSection);
+                           CurStrOffsetSection, CompileUnitHeader);
 
     if (CurCUIndexSection.empty()) {
       Expected<CompileUnitIdentifiers> EID = getCUIdentifiers(
           AbbrevSection, InfoSection, CurStrOffsetSection, CurStrSection);
       if (!EID)
         return createFileError(Input, EID.takeError());
       const auto &ID = *EID;
       auto P = IndexEntries.insert(std::make_pair(ID.Signature, CurEntry));