[PATCH] D91670: Fix crash after looking up dwo_id=0 in CU index.
Jorge Gorbe Moya via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 11:40:41 PST 2020
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
More information about the llvm-commits
mailing list