[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik accepted this revision.
shafik added a comment.

LGTM with minor comments. Thank you for these fixes, they are awesome!



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3213
+      bool use_type_size_for_value = false;
+      if (location_form.IsValid()) {
+        has_explicit_location = true;
----------------
It might be helpful to document that `DW_AT_location` is taken over `DW_AT_const_value` and the types of cases this can show up in.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3216
+        if (DWARFFormValue::IsBlockForm(location_form.Form())) {
+          auto data = die.GetData();
+
----------------
`const DWARFDataExtractor&`?

I don't think `auto` adds any value here.




================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3241
+        // string.
+        auto debug_info_data = die.GetData();
+        if (DWARFFormValue::IsBlockForm(const_value_form.Form())) {
----------------
`const DWARFDataExtractor&`?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257
+        } else if (const char *str = const_value_form.AsCString()) {
+          uint32_t string_length = strlen(str) + 1;
+          location = DWARFExpression(
----------------
aprantl wrote:
> If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would make sense...
Why `+1`?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3446
         SymbolFileTypeSP type_sp(
             new SymbolFileType(*this, GetUID(type_die_form.Reference())));
 
----------------
`make_shared`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615