[PATCH] D91670: Fix crash after looking up dwo_id=0 in CU index.

[Jorge Gorbe Moya via Phabricator via llvm-commits]

jgorbe marked 3 inline comments as done.
jgorbe added a comment.

Updated diff addressing the new round of feedback.



================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:1
+# REQUIRES: x86-registered-target
+
----------------
jhenderson wrote:
> I'd find it useful if there was a top-level comment in this test explaining the purpose of this test.
Done.


================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:7
+
+# This expected output is very uninteresting, but it's better than a crash.
+# CHECK: ??:0:0
----------------
jhenderson wrote:
> 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?
Sure! Done.


================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:48
+        .section        .debug_addr,"", at progbits
+        .quad   A\I
+        .quad   F\I
----------------
MaskRay wrote:
> `A\I` is undefined. Consider defining the symbol to be more realistic.
Good catch! Went a bit too far trimming the lldb test this is based on :)


================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:93
+        .section        .debug_cu_index,"", at progbits
+        .short  2                       # DWARF version number
+        .short  0                       # Reserved
----------------
labath wrote:
> MaskRay wrote:
> > For version 2, the field should be `.long 2`
> > 
> > (The difference matters if this is big-endian)
> 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.
Fixed.


================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:105
+
+.irpc I,1
+        .long .Lcu_begin\I-.debug_info.dwo
----------------
labath wrote:
> MaskRay wrote:
> > The two loops can be merged.
> Well, if it's going to have just one iteration, then I guess the loops can just be deleted altogether.
Removed all the loops for the DWP case as they had only one iteration.


================
Comment at: llvm/test/tools/llvm-symbolizer/split-dwarf-zero-signature-not-found.s:3
+
+# RUN: llvm-mc --filetype=obj --triple x86_64-pc-linux %s -o %t --defsym MAIN=0
+# RUN: llvm-mc --filetype=obj --triple x86_64-pc-linux %s -o %t.dwp --defsym DWP=0
----------------
MaskRay wrote:
> 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.
Removed the DWP macro and folded all into the same .ifdef/.else/.endif.

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


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

https://reviews.llvm.org/D91670