[PATCH] D70756: llvm-symbolizer: support DW_FORM_loclistx locations.

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: dblaikie, probinson, wolfgangp, JDevlieghere, aprantl.
jhenderson added a comment.

> C++ styles comments are what clang generates in the assembly output. Anyway, I've replaced them with "#".

They aren't for me ('#' is what clang is generating for me), but I default to X86_64 targets. It looks like it depends on the target triple (aarch64 appears to use '//' as you've observed).

> I'm not really comfortable hacking on the dwarf assembly. I'm worried it's very easy to create a malformed test input this way, and I'm not familiar with the format enough to recognize it.

I can sympathise, but it isn't actually that hard to get rid of a lot of junk. You also now have a working test, so you should be able to try deleting things and seeing if the test still works. Just remember to verify that the test failed beforehand. I've commented inline on some bits that are obviosuly not needed.

I'm not really appropriate to review the loclists stuff, but I've added a few perople more familiar with DWARF as reviewers who may be better placed to do so.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1106
+    } else {
+      consumeError(Loc.takeError());
+    }
----------------
If there's an error, we shouldn't just throw away that error ideally. However, I see that this is a `void` function so propagating the Error up the stack may be non-trivial. If that is the case, please at least comment this with a TODO to say that we should handle the Error more cleanly.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:71-72
+	.asciz	"x"                     # string offset=124
+.Linfo_string6:
+	.asciz	"int"                   # string offset=126
+	.section	.debug_str_offsets,"", at progbits
----------------
You can delete this string and its reference in the .debug_str_offsets table, if you trim the .debug_info as I suggest.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:106
+.Ldebug_loclist_table_end0:
+	.section	.debug_abbrev,"", at progbits
+	.byte	1                       # Abbreviation Code
----------------
You'll be able to delete big chunks of the .debug_abbrev data, that correspond to the stuff I recommend you delete in the .debug_info below.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:241-244
+	.byte	3                       # Abbrev [3] 0x2b:0x5 DW_TAG_formal_parameter
+	.word	49                      # DW_AT_type
+	.byte	0                       # End Of Children Mark
+	.byte	4                       # Abbrev [4] 0x31:0x1 DW_TAG_pointer_type
----------------
I believe this is all irrelevant to the test in question.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:260
+	.byte	5                       # DW_AT_decl_line
+	.word	84                      # DW_AT_type
+	.byte	7                       # Abbrev [7] 0x46:0xd DW_TAG_call_site
----------------
I believe you can delete this attribute, as the type of the variabls is irrelevant for the test.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:261-268
+	.byte	7                       # Abbrev [7] 0x46:0xd DW_TAG_call_site
+	.word	39                      # DW_AT_call_origin
+	.xword	.Ltmp2-.Lfunc_begin0    # DW_AT_call_return_pc
+	.byte	0                       # End Of Children Mark
+	.byte	8                       # Abbrev [8] 0x54:0x4 DW_TAG_base_type
+	.byte	6                       # DW_AT_name
+	.byte	5                       # DW_AT_encoding
----------------
If I'm reading the DWARF right, these have no relevance to the test in question.


================
Comment at: llvm/test/DebugInfo/AArch64/frame-loclistx.s:280-281
+.Ldebug_addr_end0:
+	.ident	"clang version 10.0.0 (git at github.com:llvm/llvm-project.git 57e2b50509793c72f562e8a08523f7ab664adfcd)"
+	.section	".note.GNU-stack","", at progbits
+	.section	.debug_line,"", at progbits
----------------
These two lines don't add anything useful for this test, I believe (they add a STT_FILE elf symbol and an irrelevant section).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70756