[libunwind] Fix CIE v1 return address parsing

[rprichard]

Summary

• For CIE version 1 (e.g. in DWARF 2.0.0), the return_address_register field is a ubyte [0..255].
• For CIE version 3 (e.g. in DWARF 3), the field is instead a ULEB128 constant.

Previously, libunwind accepted a CIE version of 1 or 3, but always
parsed the field as ULEB128.

Clang always outputs CIE version 1 into .eh_frame. (It can output CIE
version 3 or 4, but only into .debug_frame.)

[rprichard]

Stylistic changes.

[rprichard]

For reference, here's where Clang selects the CIE version:
https://github.com/llvm/llvm-project/blob/74c14202d90b46dda64a2542602855727b7d7f60/llvm/lib/MC/MCDwarf.cpp#L1592-L1605

And here's where Clang emits the return address register:
https://github.com/llvm/llvm-project/blob/74c14202d90b46dda64a2542602855727b7d7f60/llvm/lib/MC/MCDwarf.cpp#L1675-L1681

LLVM has another CIE parser (for dumping tools, I think):
https://github.com/llvm/llvm-project/blob/74c14202d90b46dda64a2542602855727b7d7f60/llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp#L430-L431

[mstorsjo]

LGTM

[smeenai]

Would it make sense to pick this to 11.0? CC @hans

[hans]

In D83741#2155008, @smeenai wrote:

Would it make sense to pick this to 11.0? CC @hans

Sounds good to me. Please go ahead and cherry-pick with the -x option, or let me know if you'd prefer me to do it.

[smeenai]

In D83741#2155821, @hans wrote:

In D83741#2155008, @smeenai wrote:

Would it make sense to pick this to 11.0? CC @hans

Sounds good to me. Please go ahead and cherry-pick with the -x option, or let me know if you'd prefer me to do it.

Sorry, wasn't sure if this was directed to me or @rprichard (the diff author). I'm happy to do the cherry-pick, but I don't have a way of testing it.

[hans]

Sorry, wasn't sure if this was directed to me or @rprichard (the diff author). I'm happy to do the cherry-pick, but I don't have a way of testing it.

I meant you, since I interpreted your question as a request. But if it's more of a question, maybe @rprichard is the right person to answer. Is there a need for this in 11.x, or should we just wait and let it ship in the next release?

[rprichard]

There's only an issue if the return address register is in the range [128..255], and my impression is that probably never happens. It's also not a regression.

[hans]

In D83741#2165675, @rprichard wrote:

There's only an issue if the return address register is in the range [128..255], and my impression is that probably never happens. It's also not a regression.

Okay, let's not worry about it for the release then. Thanks!

libunwind/src/DwarfParser.hpp

#endif
   while (addressSpace.get8(p) != 0)
     ++p;
   ++p;
   // parse code aligment factor
   cieInfo->codeAlignFactor = (uint32_t)addressSpace.getULEB128(p, cieContentEnd);
   // parse data alignment factor
   cieInfo->dataAlignFactor = (int)addressSpace.getSLEB128(p, cieContentEnd);
   // parse return address register
-  uint64_t raReg = addressSpace.getULEB128(p, cieContentEnd);
+  uint64_t raReg = (version == 1) ? addressSpace.get8(p++)
+                                  : addressSpace.getULEB128(p, cieContentEnd);
   assert(raReg < 255 && "return address register too large");
   cieInfo->returnAddressRegister = (uint8_t)raReg;
   // parse augmentation data based on augmentation string
   const char *result = NULL;
   if (addressSpace.get8(strStart) == 'z') {
     // parse augmentation data length
     addressSpace.getULEB128(p, cieContentEnd);
     for (pint_t s = strStart; addressSpace.get8(s) != '\0'; ++s) {