[PATCH] D63468: llvm-symbolizer: Add a FRAME command.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 4 inline comments as done.
pcc added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:987
+  case DW_TAG_ptr_to_member_type:
+    return 2 * PointerSize;
+  case DW_TAG_const_type:
----------------
eugenis wrote:
>  I'm not very familiar with this: is that true for all targets?
It's true for pointers to member functions on all Itanium targets:
http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/ItaniumCXXABI.cpp#510

I forgot about pointers to data members,which are a single pointer width. I'll add a case for that.

It's not true with the Microsoft ABI, but in that case the debug info format will be CodeView not DWARF anyway.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1006
+      if (Child.getTag() != DW_TAG_subrange_type)
+        continue;
+
----------------
eugenis wrote:
> I think this is not correct for multi-dimensional arrays.
> 
> This is a int [10][20] array:
> 
> 0x00000053:   DW_TAG_array_type
>                 DW_AT_type	(0x00000065 "int")
> 
> 0x00000058:     DW_TAG_subrange_type
>                   DW_AT_type	(0x0000006c "__ARRAY_SIZE_TYPE__")
>                   DW_AT_count	(0x0a)
> 
> 0x0000005e:     DW_TAG_subrange_type
>                   DW_AT_type	(0x0000006c "__ARRAY_SIZE_TYPE__")
>                   DW_AT_count	(0x14)
> 
Good catch. That's a... surprising way to represent that.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1014
+        if (Optional<uint64_t> UpperBound =
+                UpperBoundAttr->getAsUnsignedConstant())
+          return (*UpperBound + 1) * *BaseSize;
----------------
eugenis wrote:
> Do you want to look at lower bound, too?
Might as well. I don't think this can actually happen in C/C++ but it could in Fortran or other languages with one-based arrays.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:144
+  else
+    OS << "??\n";
+  return *this;
----------------
eugenis wrote:
> Should we simply not print anything when tag offset is missing? It's completely optional, after all. It's a bit confusing to see "??" in the output for normal (non-hwasan) binary and makes me think that some debug info got lost or stripped.
> 
I guess the other fields are technically optional as well (e.g. debug info could represent location without frame offset, or no location at all, or without type information) so  it seems simplest to handle all of them the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63468