Fix crash after looking up dwo_id=0 in CU index.

[jgorbe]

Summary

In the current state, if getFromHash(0) is called and there's no CU
with dwo_id=0, the lookup will stop at an empty slot, then the check
Rows[H].getSignature() != S won't fail, because the empty slot has
a 0 in the signature field, and a pointer to the empty slot will be
returned.

This patch fixes this by using the index field in the hash entry to
check for empty slots: signature = 0 can match a valid hash but
according to the spec the index for an occupied slot will always be
non-zero.

[dblaikie]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

6–7

Checking for "anything other than crashing" is a bit loose - could you check for the specific failure you expect when the dwo_id 0 can't be found? (I guess the symbolizer won't include some details in the symbolized stack trace, for instance)

[MaskRay]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

6

If llvm-symbolizer segfaults, I think it is not guaranteed that "Stack dump" will be printed.
Since a segfaulting process has a non-zero exit code, simply writing RUN: llvm-symbolizer --obj=%t --dwp=%t.dwp 0x0 performs the check.

This is probably sufficient for a regression test. However, if you have other interesting output to check, consider adding them to make the test better.

[jgorbe]

Changed test expectation from "not a stack dump" to the actual output from llvm-symbolizer for the test case.

[jgorbe]

Thanks for the comments!

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

6

Changed to expect the output of the fixed symbolizer as suggested by the other comment. I'm not sure what option is better, this test case is basically a regression test.

6–7

The output is extremely uninteresting, but done.

[dblaikie]

Looks good, thanks!

[MaskRay]

then the check Rows[H].getSignature() != S won't fail

The condition fails.

[MaskRay]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

4

Since MAIN and DWP are mutual exclusive, you can just define one macro and use

.ifdef
...
.else
...
.endif

I usually write x86_64-pc-linux as x86_64 to represent that this is a generic ELF behavior, rather than a Linux specific one.

94

For version 2, the field should be .long 2

(The difference matters if this is big-endian)

[MaskRay]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

49

A\I is undefined. Consider defining the symbol to be more realistic.

106

The two loops can be merged.

[jhenderson]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

2

I'd find it useful if there was a top-level comment in this test explaining the purpose of this test.

8

A number of our newer binary utility tests use the pattern of ## for comments, to clearly distinguish them from lit and FileCheck lines. Could you do that in this test too?

[labath]

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

94

This is kind of my fault, as this source is adapted from one of my tests. DWP version 2 (unofficial fission proposal) does indeed use a single long field, but then the official standardized format (version 5) uses two short fields -- and I have mixed the two up.

106

Well, if it's going to have just one iteration, then I guess the loops can just be deleted altogether.

[jgorbe]

Addressed reviewer comments.

[jgorbe]

Updated diff addressing the new round of feedback.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

2

Done.

4

Removed the DWP macro and folded all into the same .ifdef/.else/.endif.

Changed triple from x86_64-pc-linux to x86-64

8

Sure! Done.

49

Good catch! Went a bit too far trimming the lldb test this is based on :)

94

Fixed.

106

Removed all the loops for the DWP case as they had only one iteration.

[MaskRay]

LGTM.

[MaskRay]

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

[jgorbe]

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

Yeah, I could have expressed this better in the summary. What I meant with "the check won't fail" was that the check Rows[H].getSignature() != S is not enough to cause the lookup to fail and return a nullptr (as it should). I'll fix it in the actual commit message.

[jhenderson]

Not looked at the whole thing in depth, but my comments have been addressed, so happy for this to land from my point of view, if others are happy.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

49

It's probably not applicable here, but FYI, yaml2obj now has some DWARF support which allows you to craft sections using YAML rather than assembly, without needing to specify all details. Take a look at some of the tests that are using it for examples of how you might use it.

[jgorbe]

Thanks everyone for the reviews! I'm committing this now.

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

49

Thanks for the tip! I'll have a look so I can use it in future tests :)

[MaskRay]

In D91670#2403635, @jgorbe wrote:

then the check Rows[H].getSignature() != S won't fail

The condition fails. It should return nullptr but returned a Rows entry.

Yeah, I could have expressed this better in the summary. What I meant with "the check won't fail" was that the check Rows[H].getSignature() != S is not enough to cause the lookup to fail and return a nullptr (as it should). I'll fix it in the actual commit message.

You can edit the summary to be synchronized with the commit. If you upload differentials with arc diff --verbatim (--verbatim allows you to modify the summary while uploading a new patch).

llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp

DWARFUnitIndex::getFromOffset(uint32_t Offset) const {
   return E;
 }
 
 const DWARFUnitIndex::Entry *DWARFUnitIndex::getFromHash(uint64_t S) const {
   uint64_t Mask = Header.NumBuckets - 1;
 
   auto H = S & Mask;
   auto HP = ((S >> 32) & Mask) | 1;
-  while (Rows[H].getSignature() != S && Rows[H].getSignature() != 0)
+  // The spec says "while 0 is a valid hash value, the row index in a used slot
+  // will always be non-zero". Loop until we find a match or an empty slot.
+  while (Rows[H].getSignature() != S && Rows[H].Index != nullptr)
     H = (H + HP) & Mask;
 
-  if (Rows[H].getSignature() != S)
+  // If the slot is empty, we don't care whether the signature matches (it could
+  // be zero and still match the zeros in the empty slot).
+  if (Rows[H].Index == nullptr)
     return nullptr;
 
   return &Rows[H];
 }

llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s

+## This test checks that looking up a zero hash in the .debug_cu_index hash
+## table works correctly when there's no CU with signature = 0.
+##
+## LLVM used to check just the signature bits to decide if the hash lookup ended
+## at a match or at an empty slot. This is wrong when signature = 0 because
+## empty slots have all zeros in the signature field too, and LLVM would return
+## the empty slot as a valid result.
+
+# REQUIRES: x86-registered-target
+
+# RUN: llvm-mc --filetype=obj --triple x86_64 %s -o %t --defsym MAIN=0
+# RUN: llvm-mc --filetype=obj --triple x86_64 %s -o %t.dwp
+# RUN: llvm-symbolizer --obj=%t --dwp=%t.dwp 0x0 | FileCheck %s
+
+## This expected output is very uninteresting, but it's better than a crash.
+# CHECK: ??:0:0
+
+        .section        .debug_abbrev,"",@progbits
+        .byte   1                       # Abbreviation Code
+        .byte   17                      # DW_TAG_compile_unit
+        .byte   0                       # DW_CHILDREN_no
+        .ascii  "\260B"                 # DW_AT_GNU_dwo_name
+        .byte   8                       # DW_FORM_string
+        .ascii  "\261B"                 # DW_AT_GNU_dwo_id
+        .byte   7                       # DW_FORM_data8
+        .ascii  "\263B"                 # DW_AT_GNU_addr_base
+        .byte   23                      # DW_FORM_sec_offset
+        .byte   85                      # DW_AT_ranges
+        .byte   23                      # DW_FORM_sec_offset
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   0                       # EOM(3)
+
+## Create two CUs, with dwo_ids 0 and 1 respectively.
+.ifdef MAIN
+.irpc I,01
+        .data
+A\I:
+        .long \I
+
+        .text
+F\I:
+        nop
+
+        .section        .debug_info,"",@progbits
+.Lcu_begin\I:
+        .long   .Ldebug_info_end\I-.Ldebug_info_start\I # Length of Unit
+.Ldebug_info_start\I:
+        .short  4                       # DWARF version number
+        .long   .debug_abbrev           # Offset Into Abbrev. Section
+        .byte   8                       # Address Size (in bytes)
+        .byte   1                       # Abbrev [1] 0xb:0x25 DW_TAG_compile_unit
+        .asciz  "A.dwo"                 # DW_AT_GNU_dwo_name
+        .quad   \I                      # DW_AT_GNU_dwo_id
+        .long   .debug_addr             # DW_AT_GNU_addr_base
+        .long   .Lranges\I              # DW_AT_ranges
+.Ldebug_info_end\I:
+
+        .section        .debug_addr,"",@progbits
+        .quad   A\I
+        .quad   F\I
+
+        .section        .debug_ranges,"",@progbits
+.Lranges\I:
+        .quad   F\I
+        .quad   F\I+1
+        .quad   0
+        .quad   0
+.endr
+.else
+## Deliberately omit compile unit 0 in the DWP. We want to check the case where
+## a signature = 0 matches an empty hash slot in .debug_cu_index and the index
+## in the parallel table has to be checked.
+        .section        .debug_abbrev.dwo,"e",@progbits
+.Labbrev1:
+        .byte   1                       # Abbreviation Code
+        .byte   17                      # DW_TAG_compile_unit
+        .byte   0                       # DW_CHILDREN_no
+        .byte   37                      # DW_AT_producer
+        .byte   8                       # DW_FORM_string
+        .byte   3                       # DW_AT_name
+        .byte   8                       # DW_FORM_string
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   0                       # EOM(3)
+.Labbrev_end1:
+
+        .section        .debug_info.dwo,"e",@progbits
+.Lcu_begin1:
+        .long   .Ldebug_info_end1-.Ldebug_info_start1 # Length of Unit
+.Ldebug_info_start1:
+        .short  4                       # DWARF version number
+        .long   0                       # Offset Into Abbrev. Section
+        .byte   8                       # Address Size (in bytes)
+        .byte   1                       # Abbrev DW_TAG_compile_unit
+        .asciz  "Hand-written DWARF"    # DW_AT_producer
+        .byte   '1', '.', 'c', 0        # DW_AT_name
+.Ldebug_info_end1:
+
+        .section        .debug_cu_index,"",@progbits
+        .long   2                       # DWARF version number
+        .long   2                       # Section count
+        .long   1                       # Unit count
+        .long   8                       # Slot count
+
+        .quad   1, 0, 0, 0, 0, 0, 0, 0  # Hash table
+        .long   1, 0, 0, 0, 0, 0, 0, 0  # Index table
+
+        .long   1                       # DW_SECT_INFO
+        .long   3                       # DW_SECT_ABBREV
+
+        .long .Lcu_begin1-.debug_info.dwo
+        .long .Labbrev1-.debug_abbrev.dwo
+
+        .long .Ldebug_info_end1-.Lcu_begin1
+        .long .Labbrev_end1-.Labbrev1
+
+.endif