This is an archive of the discontinued LLVM Phabricator instance.

Add verification for DW_AT_decl_file and DW_AT_call_file.
ClosedPublic

Authored by clayborg on Jul 28 2020, 6:32 PM.

Details

Summary

LTO builds have been creating invalid DWARF and one of the errors was a file index that was out of bounds. "llvm-dwarfdump --verify" will check all file indexes for line tables already, but there are no checks for the validity of file indexes in attributes.

The verification will verify if there is a DW_AT_decl_file/DW_AT_call_file that:

  • there is a line table for the compile unit
  • the file index is valid
  • the encoding is appropriate

Tests are added that test all of the above conditions.

Diff Detail

Event Timeline

clayborg created this revision.Jul 28 2020, 6:32 PM
clayborg requested review of this revision.Jul 28 2020, 6:32 PM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
548

Maybe best to make this not named after the version, e.g. IsZeroIndexed?

558

On first read, I found "by" to feel not quite the right word. Perhaps "with" would be better? (Not 100% sure either way though).

560

Here and below, from a quick glance, it doesn't look like most of the verifier error messages have a trailing full stop, so these probably shouldn't either.

llvm/test/tools/llvm-dwarfdump/X86/verify_attr_file_indexes.yaml
2

Nit: delete blank line at file start.

21

Rather than using mach-o, which has a lot of noise in its YAML, it might be worth switching to ELF. I think yaml2obj's ELF support is more-or-less sufficient now to achieve the desired goal. I think all you'd need to do is to use an ELF file header (you can find some simple examples in the yaml2obj tests), but keep the DWARF block the same.

There are probably ways to reduce the DWARF bit of the YAML too. @Higuoxing is actively working on improving this, so might be able to offer some tips, but I don't know how much could be done there yet.

llvm/test/tools/llvm-dwarfdump/X86/verify_file_encoding.yaml
19

Same comments as other test.

Higuoxing added inline comments.Jul 29 2020, 6:57 AM
llvm/test/tools/llvm-dwarfdump/X86/verify_attr_file_indexes.yaml
21

Yes, this YAML file is able to be reduced by replacing the Mach-O header with ELF header. I've tested it.

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
DWARF:
  debug_str:
    - ''
    - '/tmp/main.c'
    - main
    - inline1
  debug_abbrev:
    - Code:            0x0000000000000001
      Tag:             DW_TAG_compile_unit
      Children:        DW_CHILDREN_yes
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_language
          Form:            DW_FORM_data2
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_stmt_list
          Form:            DW_FORM_sec_offset
    - Code:            0x0000000000000002
      Tag:             DW_TAG_subprogram
      Children:        DW_CHILDREN_yes
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_high_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_decl_file
          Form:            DW_FORM_data1
        - Attribute:       DW_AT_call_line
          Form:            DW_FORM_data1
    - Code:            0x0000000000000003
      Tag:             DW_TAG_inlined_subroutine
      Children:        DW_CHILDREN_no
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_high_pc
          Form:            DW_FORM_data4
        - Attribute:       DW_AT_call_file
          Form:            DW_FORM_data1
        - Attribute:       DW_AT_call_line
          Form:            DW_FORM_data1
  debug_info:
    - Length:          0x0000000000000046
      Version:         4
      AbbrOffset:      0x0000000000000000
      AddrSize:        8
      Entries:
        - AbbrCode:        0x00000001
          Values:
            - Value:           0x0000000000000001
            - Value:           0x0000000000000002
            - Value:           0x0000000000000000
            - Value:           0x0000000000000000
        - AbbrCode:        0x00000002
          Values:
            - Value:           0x000000000000000D
            - Value:           0x0000000000001000
            - Value:           0x0000000000002000
            - Value:           0x0000000000000002
            - Value:           0x0000000000000005
        - AbbrCode:        0x00000003
          Values:
            - Value:           0x0000000000000012
            - Value:           0x0000000000001100
            - Value:           0x0000000000000100
            - Value:           0x0000000000000003
            - Value:           0x000000000000000A
        - AbbrCode:        0x00000000
          Values:          []
        - AbbrCode:        0x00000000
          Values:          []
  debug_line:
    - Length:          40
      Version:         2
      PrologueLength:  34
      MinInstLength:   1
      DefaultIsStmt:   1
      LineBase:        251
      LineRange:       14
      OpcodeBase:      13
      StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
      IncludeDirs:
        - '/tmp'
      Files:
        - Name:            main.c
          DirIdx:          1
          ModTime:         0
          Length:          0
      Opcodes:         []

There are probably ways to reduce the DWARF bit of the YAML too.

I would like not to reduce the DWARF for now since most of the fields are required.

llvm/test/tools/llvm-dwarfdump/X86/verify_file_encoding.yaml
19

We can use ELF YAML for this test as well.

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
DWARF:
  debug_str:
    - ''
    - '/tmp/main.c'
    - main
    - ''
    - inline1
  debug_abbrev:
    - Code:            0x0000000000000001
      Tag:             DW_TAG_compile_unit
      Children:        DW_CHILDREN_yes
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_language
          Form:            DW_FORM_data2
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
    - Code:            0x0000000000000002
      Tag:             DW_TAG_subprogram
      Children:        DW_CHILDREN_yes
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_high_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_decl_file
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_call_line
          Form:            DW_FORM_data1
    - Code:            0x0000000000000003
      Tag:             DW_TAG_inlined_subroutine
      Children:        DW_CHILDREN_no
      Attributes:
        - Attribute:       DW_AT_name
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_low_pc
          Form:            DW_FORM_addr
        - Attribute:       DW_AT_high_pc
          Form:            DW_FORM_data4
        - Attribute:       DW_AT_call_file
          Form:            DW_FORM_strp
        - Attribute:       DW_AT_call_line
          Form:            DW_FORM_data1
  debug_info:
    - Length:          0x0000000000000048
      Version:         4
      AbbrOffset:      0x0000000000000000
      AddrSize:        8
      Entries:
        - AbbrCode:        0x00000001
          Values:
            - Value:           0x0000000000000001
            - Value:           0x0000000000000002
            - Value:           0x0000000000000000
        - AbbrCode:        0x00000002
          Values:
            - Value:           0x000000000000000D
            - Value:           0x0000000000001000
            - Value:           0x0000000000002000
            - Value:           0x0000000000000012
            - Value:           0x0000000000000005
        - AbbrCode:        0x00000003
          Values:
            - Value:           0x0000000000000013
            - Value:           0x0000000000001100
            - Value:           0x0000000000000100
            - Value:           0x0000000000000012
            - Value:           0x000000000000000A
        - AbbrCode:        0x00000000
          Values:          []
        - AbbrCode:        0x00000000
          Values:          []
Higuoxing added inline comments.Jul 29 2020, 7:29 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
82

I think uint64_t makes more sense here.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
556

I think the brackets for else can be removed.

562

The brackets for else can be removed as well.

566

This break should be put into the bracket?

- } break;
+   break;
+ }
clayborg updated this revision to Diff 281711.Jul 29 2020, 12:56 PM

Fixed inline comments:

  • switched yaml files over to use ELF
  • remove unneeded brackets
  • renamed some variables
clayborg marked 11 inline comments as done.Jul 29 2020, 12:57 PM

Marked things as done.

jhenderson accepted this revision.Jul 30 2020, 1:26 AM

Aside from the braces issue mentioned inline, LGTM.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
556

My reading of recent updates to https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements is that if the if or else requires braces, the other half should too, so I think we actually want them here.

562

Same comment as above.

This revision is now accepted and ready to land.Jul 30 2020, 1:26 AM
Higuoxing added inline comments.Jul 30 2020, 1:34 AM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
556

Seems that I missed it. Sorry for the misdirection.

clayborg updated this revision to Diff 282022.Jul 30 2020, 12:45 PM

Added else braces back.

Anymore issues with this patch?

jhenderson accepted this revision.Aug 3 2020, 12:35 AM

Not from me! LGTM again.

aprantl added inline comments.Aug 3 2020, 10:23 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
88

What happens if Filenames.size() == 0? Can that happen in malformed DWARF input? Otherwise can we add an assert that it isn't zero?

Should it return an Optional<uint64_t>?

clayborg updated this revision to Diff 283124.Aug 4 2020, 9:59 PM

Made DWARFDebugLine::Prologue::getLastValidFileIndex() and DWARFDebugLine::getLastValidFileIndex() return an Optional<uint64_t> and modified the error message for when a file index is out of range to specify the valid range, or specify that there are no files in the prologue file table. Added a test to cover this case as well.

clayborg marked an inline comment as done.Aug 4 2020, 10:00 PM
clayborg added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
88

Yep, switched it over and added a test to cover this as well.

jhenderson accepted this revision.Aug 5 2020, 12:33 AM

Latest update LGTM.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
84

Do you actually need the llvm:: prefix here?

llvm/test/tools/llvm-dwarfdump/X86/verify_attr_file_indexes_no_files.yaml
19 ↗(On Diff #283124)

Nit: add a blank line before the YAML.

This revision was automatically updated to reflect the committed changes.