Emit DWARF3 call frame information when DWARF3+ debug info is requested

[olista01]

Summary

Currently, llvm always emits a DWARF CIE with a version of 1, even when emitting DWARF 3 or 4, which both support CIE version 3. This patch makes it emit the newer CIE version when we are emitting DWARF 3 or 4. This will not reduce compatibility, as we already emit other DWARF3/4 features, and is worth doing as the DWARF3 spec removed some ambiguities in the interpretation of call frame information.

It also fixes a minor bug where the "return address" field of the CIE was encoded as a ULEB128, which is only valid when the CIE version is 3. There are no test changes for this, because (as far as I can tell) none of the platforms that we test have a return address register with a DWARF register number >127.

[dblaikie]

So - is this just correcting the number to reflect the version we're actually emitting? Should we just hardcode it to the higher number, then?

Are there any other cases of differences between CIE versions other than the one place you fixed in this patch? I'd sort of consider leaving that code to just use "emitIntValue" and assert that the value is small enough, with a comment about the right implementation should the value be larger (and a request to add a test case when that happens). As it stands this code would just do bad things if you used the old CIE version and some target that didn't fit in an int?

[olista01]

Should we just hardcode it to the higher number, then?

We can't do that, because CIE version 3 was only introduced in DWARF3, so we need to fall back to CIE version 1 when emitting DWARF2.

Are there any other cases of differences between CIE versions other than the one place you fixed in this patch?

The only place I can find where we rely on CIE version 3 features in the call frame information is llvm/lib/MC/MCDwarf.cpp:1086, but as far a I can tell it is not possible to represent a negative offset with the DWARF2 call frame instructions.

I'd sort of consider leaving that code to just use "emitIntValue" and assert that the value is small enough, with a comment about the right implementation should the value be larger (and a request to add a test case when that happens). As it stands this code would just do bad things if you used the old CIE version and some target that didn't fit in an int?

I have just re-read the DWARF spec, and the return_address_register field addresses a column in the rule table, which may not correspond to a DWARF register number.

With my patch as it currently is, we can represent columns up to 255 in DWARF2, and up to infinity (2^32 in llvm's implementation) in DWARF3. If we used emitIntValue for DWARF3, we could only represent columns up to 127, register numbers 128-255 would unintentionally consume the next byte (due to the LEB128 encoding), resulting in very invalid DWARF.

Asserting when the value cannot be correctly encoded is a good idea, I'll upload a new patch.

[olista01]

Added assertion when DWARF2 cannot represent the return address column

[olista01]

Ping?

[echristo]

Not to hijack Dave's review, but looks good to me in general.

-eric

lib/MC/MCDwarf.cpp

1275–1276

Comment on the version that we're adding as 1 for < 3, and 3 for >= 3 would be nice.

1308

I think the general style is "if one half has braces they both do" but I'm not wedded to it.

[olista01]

Comments to explain version numbers, code style changes

[olista01]

Uploading the correct type of diff this time

[echristo]

LGTM.

[olista01]

Eric: do you want me to wait for Dave, or am I OK to commit this?

[dblaikie]

I believe that was Eric signing off on this, which is fine.

My main misgiving is that I'd like to see all this range handling built into a reusable device and reused by both asm and source range building... (ie: the range handling in MCDwarf/MCContext and the range handling in DwarfDebug) one day. :/

[dblaikie]

Oops, rambling about the wrong code review. Only reason I didn't sign off on this one is I know next to nothing about CFI.

[olista01]

Thanks, committed revision 211272.

lib/MC/MCDwarf.cpp

const MCSymbol &FrameEmitterImpl::EmitCIE(MCObjectStreamer &streamer,
 
   // CIE ID
   unsigned CIE_ID = IsEH ? 0 : -1;
   if (verboseAsm) streamer.AddComment("CIE ID Tag");
   streamer.EmitIntValue(CIE_ID, 4);
 
   // Version
   if (verboseAsm) streamer.AddComment("DW_CIE_VERSION");
+  // For DWARF2, we use CIE version 1
+  // For DWARF3+, we use CIE version 3
   uint8_t CIEVersion = context.getDwarfVersion() <= 2 ? 1 : 3;
   streamer.EmitIntValue(CIEVersion, 1);
 
   // Augmentation String
   SmallString<8> Augmentation;
   if (IsEH) {
     if (verboseAsm) streamer.AddComment("CIE Augmentation");
     Augmentation += "z";
     if (personality)
       Augmentation += "P";
const MCSymbol &FrameEmitterImpl::EmitCIE(MCObjectStreamer &streamer,
   streamer.EmitSLEB128IntValue(getDataAlignmentFactor(streamer));
 
   // Return Address Register
   if (verboseAsm) streamer.AddComment("CIE Return Address Column");
   if (CIEVersion == 1) {
     assert(MRI->getRARegister() <= 255 &&
            "DWARF 2 encodes return_address_register in one byte");
     streamer.EmitIntValue(MRI->getDwarfRegNum(MRI->getRARegister(), true), 1);
-  } else
+  } else {
     streamer.EmitULEB128IntValue(
         MRI->getDwarfRegNum(MRI->getRARegister(), true));
+  }
 
   // Augmentation Data Length (optional)
 
   unsigned augmentationLength = 0;
   if (IsEH) {
     if (personality) {
       // Personality Encoding
       augmentationLength += 1;