[PATCH] D71932: [DWARF] Better detect errors in Address Range Tables.

Thu Jul 23 01:06:49 PDT 2020

jhenderson added inline comments.

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp:167-171
+      return createStringError(
+          errc::invalid_argument,
+          "address range table at offset 0x%" PRIx64
+          " has an invalid tuple (length = 0) at offset 0x%" PRIx64,
+          Offset, *offset_ptr - tuple_size);
----------------
ikudrin wrote:
> dblaikie wrote:
> > jhenderson wrote:
> > > @ikudrin, I know this was committed a while back, but I only just ran into this.
> > > 
> > > Why was this check added? There's nothing in the DWARF spec that I know of that requires a non-zero length for all non-terminating entries. In our Sony linker, prior to the new tombstoning functionality that has recently been implemented, we would patch a reference in the .debug_aranges to have a zero length, and -1 for the address. This used to be fine, but recent versions of llvm-dwarfdump now can't dump this. Even without the Sony changes, there are occasionally functions in the DWARF with zero length (see also my post earlier today on the llvm-dev list for .debug_loc and zero length entry descriptions).
> > Yep, +1 we should allow empty ranges here. Even with better tombstoning - a) backwards compatibility with existing implementations (ld.bfd) b) even with proper tombstoning, it'd effectively create an empty range (-2, -2) - though arguably in that case we should detect the tombstoning first, before considering the range size/other issues
> > 
> > Here's a reproduction with commonly accessible tools:
> > 
> > ```
> > $ cat range2.cpp
> > void f1() {
> > }
> > void f2() {
> > }
> > int main() {
> >   f1();
> > }
> > $ clang++ range2.cpp -g -fuse-ld=bfd -ffunction-sections -gdwarf-aranges -Wl,-gc-sections
> > $ llvm-dwarfdump -verify a.out
> > Verifying a.out:        file format elf64-x86-64
> > Verifying .debug_abbrev...
> > Verifying .debug_info Unit Header Chain...
> > error: DIE address ranges are not contained in its parent's ranges:
> > 0x0000000b: DW_TAG_compile_unit
> >               DW_AT_producer    ("clang version 12.0.0 (git at github.com:llvm/llvm-project.git 80e6844e28f10c1c2c31b301c3ef980c7a11ee6b)")
> >               DW_AT_language    (DW_LANG_C_plus_plus_14)
> >               DW_AT_name        ("range2.cpp")
> >               DW_AT_stmt_list   (0x00000000)
> >               DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
> >               DW_AT_low_pc      (0x0000000000000000)
> >               DW_AT_ranges      (0x00000000
> >                  [0x0000000000401110, 0x0000000000401116)
> >                  [0x0000000000000001, 0x0000000000000001)
> >                  [0x0000000000401120, 0x000000000040112d))
> > 
> > 0x00000043:   DW_TAG_subprogram
> >                 DW_AT_low_pc    (0x0000000000000000)
> >                 DW_AT_high_pc   (0x0000000000000006)
> >                 DW_AT_frame_base        (DW_OP_reg6)
> >                 DW_AT_linkage_name      ("_Z2f2v")
> >                 DW_AT_name      ("f2")
> >                 DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/range2.cpp")
> >                 DW_AT_decl_line (3)
> >                 DW_AT_external  (true)
> > 
> > Verifying .debug_info references...
> > Verifying .debug_types Unit Header Chain...
> > Errors detected.
> > ```
> > Similarly, with GCC (which produces debug_aranges by default):
> > ```
> > $ g++ range2.cpp -g -fuse-ld=bfd -ffunction-sections -Wl,-gc-sections
> > $ llvm-dwarfdump -verify a.out
> > Verifying a.out:        file format elf64-x86-64
> > Verifying .debug_abbrev...
> > Verifying .debug_info Unit Header Chain...
> > error: DIE address ranges are not contained in its parent's ranges:
> > 0x0000000b: DW_TAG_compile_unit
> >               DW_AT_producer    ("GNU C++14 10.0.0 20200111 (experimental) -mtune=generic -march=x86-64 -g -fuse-ld=bfd -ffunction-sections")
> >               DW_AT_language    (DW_LANG_C_plus_plus)
> >               DW_AT_name        ("range2.cpp")
> >               DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
> >               DW_AT_ranges      (0x00000000
> >                  [0x0000000000401102, 0x0000000000401109)
> >                  [0x0000000000000001, 0x0000000000000001)
> >                  [0x0000000000401109, 0x0000000000401119))
> >               DW_AT_low_pc      (0x0000000000000000)
> >               DW_AT_stmt_list   (0x00000000)
> > 
> > 0x0000004e:   DW_TAG_subprogram
> >                 DW_AT_external  (true)
> >                 DW_AT_name      ("f2")
> >                 DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/range2.cpp")
> >                 DW_AT_decl_line (3)
> >                 DW_AT_decl_column       (0x06)
> >                 DW_AT_linkage_name      ("_Z2f2v")
> >                 DW_AT_low_pc    (0x0000000000000000)
> >                 DW_AT_high_pc   (0x0000000000000007)
> >                 DW_AT_frame_base        (DW_OP_call_frame_cfa)
> >                 DW_AT_GNU_all_call_sites        (true)
> > 
> > Verifying .debug_info references...
> > Verifying .debug_types Unit Header Chain...
> > Errors detected.
> > ```
> > Why was this check added? There's nothing in the DWARF spec that I know of that requires a non-zero length for all non-terminating entries.
> 
> I tried to follow the Standard here, see 6.1.2 at DWARFv5, p. 148:
> > Each descriptor is a triple consisting of a segment selector, the beginning address within that segment of a range of text or data covered by some entry owned by the corresponding compilation unit, followed by the **non-zero** length of that range.
> 
> It looks like the DWARF spec should be updated to reflect the practice.
> 
> @jhenderson, it would be great if you prepare the fix. I'll be AFK for a couple of weeks.
Huh, you're right. We must have missed that bit in the spec when we implemented our internal solution for stripped references. I guess the trouble is that if you do remove the non-zero length bit from the spec, a system with a zero address would treat it as a terminator, unless we also removed the terminator from the spec (I don't see why we couldn't - an aranges table has a known length). I'll post on https://bugs.llvm.org/show_bug.cgi?id=46805 that point, to see if I can get some feedback from other DWARF folks.

I may not get around to the fix for a bit - I've got a few other things on my plate currently, but this is certainly going to stay on my radar so if I do get a chance I'll come back to it. I don't think it's an urgent enough issue to warrant a release branch patch, unless somebody else feels it is.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71932/new/

https://reviews.llvm.org/D71932