[DebugInfo/DWARF] - Report .eh_frame sections of version != 1.

[grimar]

Summary

Specification (https://refspecs.linuxbase.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html#AEN1349)
says that the value of Version field for .eh_frame should be 1.

Though we accept other values and might perform an attempt to read
it as a .debug_frame because of that, what is wrong.

This patch adds a version check.

[probinson]

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

[grimar]

In D81469#2082410, @probinson wrote:

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Sounds reasonable.

But Error DWARFDebugFrame::parse(DWARFDataExtractor Data) returns errors for a few cases already,
e.g. for the case when something is wrong with the augmentation string. Instead it could provide a way to
report a warning (perhaps a warning callback) and continue dumping.

Reimplementing the DWARFDebugFrame::parse() to support warnings should be done independently from this change I think?

[grimar]

One more way to have a single test right now is to use yaml2obj instead of llvm-mc.

With it I'll need to specify a binary blob for the section content though.
(yaml2obj doesn't fully support the .eh_frame section yet I believe)

I.e. it will look like:

Sections:
  - Name: .eh_frame
    Type: SHT_PROGBITS
## A comment describing the content.
    Content: "001122334455"

Should I do it?

[MaskRay]

In D81469#2084492, @grimar wrote:

One more way to have a single test right now is to use yaml2obj instead of llvm-mc.

With it I'll need to specify a binary blob for the section content though.
(yaml2obj doesn't fully support the .eh_frame section yet I believe)

I.e. it will look like:

Sections:
  - Name: .eh_frame
    Type: SHT_PROGBITS
## A comment describing the content.
    Content: "001122334455"

Should I do it?

Can you just use .byte .long to construct the content of .eh_frame? It will be more reasonable than hex pairs in YAML.

[grimar]

In D81469#2085356, @MaskRay wrote:

In D81469#2084492, @grimar wrote:

One more way to have a single test right now is to use yaml2obj instead of llvm-mc.

With it I'll need to specify a binary blob for the section content though.
(yaml2obj doesn't fully support the .eh_frame section yet I believe)

I.e. it will look like:

Sections:
  - Name: .eh_frame
    Type: SHT_PROGBITS
## A comment describing the content.
    Content: "001122334455"

Should I do it?

Can you just use .byte .long to construct the content of .eh_frame? It will be more reasonable than hex pairs in YAML.

This is what this patch already does I think? Did you mean using something like
we do in LLD, i.e like the following?

# RUN: echo '.section .eh_frame,"a",@unwind; .long 6; .long 0; .long 2' | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %tbar.o

It is probably not that nicely readable, though I do not have a strong objection to do in this way if you think it is better than what is done in this patch already.

[grimar]

Ping

[probinson]

In D81469#2084372, @grimar wrote:

In D81469#2082410, @probinson wrote:

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Sounds reasonable.

But Error DWARFDebugFrame::parse(DWARFDataExtractor Data) returns errors for a few cases already,
e.g. for the case when something is wrong with the augmentation string. Instead it could provide a way to
report a warning (perhaps a warning callback) and continue dumping.

Reimplementing the DWARFDebugFrame::parse() to support warnings should be done independently from this change I think?

That's fair. LGTM, although it would be nice if you could leave a TODO comment.

[grimar]

In D81469#2093648, @probinson wrote:

In D81469#2084372, @grimar wrote:

In D81469#2082410, @probinson wrote:

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Sounds reasonable.

But Error DWARFDebugFrame::parse(DWARFDataExtractor Data) returns errors for a few cases already,
e.g. for the case when something is wrong with the augmentation string. Instead it could provide a way to
report a warning (perhaps a warning callback) and continue dumping.

Reimplementing the DWARFDebugFrame::parse() to support warnings should be done independently from this change I think?

That's fair. LGTM, although it would be nice if you could leave a TODO comment.

Will do, thanks!

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

uint64_t Id = Data.getRelocatedValue((IsDWARF64 && !IsEH) ? 8 : 4, &Offset,
                                          /*SectionIndex=*/nullptr, &Err);
     if (Err)
       return Err;
 
     if (Id == getCIEId(IsDWARF64, IsEH)) {
       uint8_t Version = Data.getU8(&Offset);
       const char *Augmentation = Data.getCStr(&Offset);
       StringRef AugmentationString(Augmentation ? Augmentation : "");
+      // TODO: we should provide a way to report a warning and continue dumping.
+      if (IsEH && Version != 1)
+        return createStringError(errc::not_supported,
+                                 "unsupported CIE version: %" PRIu8, Version);
+
       uint8_t AddressSize = Version < 4 ? Data.getAddressSize() :
                                           Data.getU8(&Offset);
       Data.setAddressSize(AddressSize);
       uint8_t SegmentDescriptorSize = Version < 4 ? 0 : Data.getU8(&Offset);
       uint64_t CodeAlignmentFactor = Data.getULEB128(&Offset);
       int64_t DataAlignmentFactor = Data.getSLEB128(&Offset);
       uint64_t ReturnAddressRegister =
           Version == 1 ? Data.getU8(&Offset) : Data.getULEB128(&Offset);

llvm/test/DebugInfo/X86/eh-frame-invalid-version-zero.s

+## Check we do not support .eh_frame sections of version 0.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-frame %t 2>&1 | FileCheck %s
+
+# CHECK: unsupported CIE version: 0
+
+.section .eh_frame,"a",@unwind
+ .long .Lend - .LCIEptr ## Length
+.LCIEptr:
+ .long 0x00000000       ## CIE ID
+ .byte 0                ## Version
+.Lend:

llvm/test/DebugInfo/X86/eh-frame-invalid-version.s

+## Check we do not support .eh_frame sections of versions greater than 1.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-frame %t 2>&1 | FileCheck %s
+
+# CHECK: unsupported CIE version: 2
+
+.section .eh_frame,"a",@unwind
+ .long .Lend - .LCIEptr ## Length
+.LCIEptr:
+ .long 0x00000000       ## CIE ID
+ .byte 2                ## Version
+.Lend: