Improve detection of global vs local variables

[sammccall]

Summary

Improve detection of global vs local variables.

Currently when a global variable is optimized out or otherwise has an unknown
location (DW_AT_location is empty) it gets reported as local.

I added two new heuristics:

• if a mangled name is present, the variable is global (or static)
• if DW_AT_location is present but invalid, the variable is global (or static)

[abidh]

Please add @clayborg as reviewer. Also paste the dwarf generated for this case from both gcc and clang.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3911 ↗

(On Diff #78700)

Why not put the checks in the same line. Something like...
bool is_static_lifetime = has_explicit_mangled || (has_explicit_location && !location.IsValid());

You probably need logical operator and not the bitwise.

3925 ↗

(On Diff #78700)

similar issue here

[sammccall]

Address review comments.

[sammccall]

@abidh: thanks for the pointer, still getting the hang of how reviews work here :-)

Also reordered the heuristics in the comment to match the code.

Test case:

struct Obj{};
namespace N { extern Obj& o; }
using N::o;
int main(){}

clang++ -g -O0 -o test.clang test.cc; readelf --debug-dump=info test.clang

 Compilation Unit @ offset 0x0:
  Length:        0x71 (32-bit)
  Version:       4
  Abbrev Offset: 0x0
  Pointer Size:  8
<0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
   <c>   DW_AT_producer    : (indirect string, offset: 0x0): clang version 4.0.0 	
   <10>   DW_AT_language    : 4	(C++)
   <12>   DW_AT_name        : (indirect string, offset: 0x15): test.cc	
   <16>   DW_AT_stmt_list   : 0x0	
   <1a>   DW_AT_comp_dir    : (indirect string, offset: 0x1d): /usr/local/google/home/sammccall/src/debugexample	
   <1e>   DW_AT_low_pc      : 0x400590	
   <26>   DW_AT_high_pc     : 0x8	
<1><2a>: Abbrev Number: 2 (DW_TAG_namespace)
   <2b>   DW_AT_name        : (indirect string, offset: 0x4f): N	
   <2f>   DW_AT_decl_file   : 1	
   <30>   DW_AT_decl_line   : 2	
<2><31>: Abbrev Number: 3 (DW_TAG_variable)
   <32>   DW_AT_name        : (indirect string, offset: 0x51): o	
   <36>   DW_AT_type        : <0x42>	
   <3a>   DW_AT_external    : 1	
   <3a>   DW_AT_decl_file   : 1	
   <3b>   DW_AT_decl_line   : 2	
   <3c>   DW_AT_declaration : 1	
   <3c>   DW_AT_location    : 0 byte block: 	()
   <3d>   DW_AT_linkage_name: (indirect string, offset: 0x57): _ZN1N1oE	
<2><41>: Abbrev Number: 0
<1><42>: Abbrev Number: 4 (DW_TAG_reference_type)
   <43>   DW_AT_type        : <0x47>	
<1><47>: Abbrev Number: 5 (DW_TAG_structure_type)
   <48>   DW_AT_name        : (indirect string, offset: 0x53): Obj	
   <4c>   DW_AT_byte_size   : 1	
   <4d>   DW_AT_declaration : 1	
<1><4d>: Abbrev Number: 6 (DW_TAG_imported_declaration)
   <4e>   DW_AT_decl_file   : 1	
   <4f>   DW_AT_decl_line   : 3	
   <50>   DW_AT_import      : <0x31>	[Abbrev Number: 3 (DW_TAG_variable)]
<1><54>: Abbrev Number: 7 (DW_TAG_subprogram)
   <55>   DW_AT_low_pc      : 0x400590	
   <5d>   DW_AT_high_pc     : 0x8	
   <61>   DW_AT_frame_base  : 1 byte block: 56 	(DW_OP_reg6 (rbp))
   <63>   DW_AT_name        : (indirect string, offset: 0x60): main	
   <67>   DW_AT_decl_file   : 1	
   <68>   DW_AT_decl_line   : 4	
   <69>   DW_AT_type        : <0x6d>	
   <6d>   DW_AT_external    : 1	
<1><6d>: Abbrev Number: 8 (DW_TAG_base_type)
   <6e>   DW_AT_name        : (indirect string, offset: 0x65): int	
   <72>   DW_AT_encoding    : 5	(signed)
   <73>   DW_AT_byte_size   : 4	
<1><74>: Abbrev Number: 0

g++ -g -O0 -o test.gcc test.cc; readelf --debug-dump=info test.gcc

 Compilation Unit @ offset 0x0:
  Length:        0x7f (32-bit)
  Version:       4
  Abbrev Offset: 0x0
  Pointer Size:  8
<0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
   <c>   DW_AT_producer    : (indirect string, offset: 0x32): GNU C++ 4.8.4 -mtune=generic -march=x86-64 -g -O0 -fstack-protector	
   <10>   DW_AT_language    : 4	(C++)
   <11>   DW_AT_name        : (indirect string, offset: 0x7f): test.cc	
   <15>   DW_AT_comp_dir    : (indirect string, offset: 0x0): /usr/local/google/home/sammccall/src/debugexample	
   <19>   DW_AT_low_pc      : 0x40057d	
   <21>   DW_AT_high_pc     : 0xb	
   <29>   DW_AT_stmt_list   : 0x0	
<1><2d>: Abbrev Number: 2 (DW_TAG_structure_type)
   <2e>   DW_AT_name        : Obj	
   <32>   DW_AT_byte_size   : 1	
   <33>   DW_AT_decl_file   : 1	
   <34>   DW_AT_decl_line   : 1	
<1><35>: Abbrev Number: 3 (DW_TAG_namespace)
   <36>   DW_AT_name        : N	
   <38>   DW_AT_decl_file   : 1	
   <39>   DW_AT_decl_line   : 2	
   <3a>   DW_AT_sibling     : <0x4c>	
<2><3e>: Abbrev Number: 4 (DW_TAG_variable)
   <3f>   DW_AT_name        : o	
   <41>   DW_AT_decl_file   : 1	
   <42>   DW_AT_decl_line   : 2	
   <43>   DW_AT_linkage_name: (indirect string, offset: 0x76): _ZN1N1oE	
   <47>   DW_AT_type        : <0x4c>	
   <4b>   DW_AT_external    : 1	
   <4b>   DW_AT_declaration : 1	
<2><4b>: Abbrev Number: 0
<1><4c>: Abbrev Number: 5 (DW_TAG_const_type)
   <4d>   DW_AT_type        : <0x51>	
<1><51>: Abbrev Number: 6 (DW_TAG_reference_type)
   <52>   DW_AT_byte_size   : 8	
   <53>   DW_AT_type        : <0x2d>	
<1><57>: Abbrev Number: 7 (DW_TAG_imported_declaration)
   <58>   DW_AT_decl_file   : 1	
   <59>   DW_AT_decl_line   : 3	
   <5a>   DW_AT_import      : <0x3e>	[Abbrev Number: 4 (DW_TAG_variable)]
<1><5e>: Abbrev Number: 8 (DW_TAG_subprogram)
   <5f>   DW_AT_external    : 1	
   <5f>   DW_AT_name        : (indirect string, offset: 0x87): main	
   <63>   DW_AT_decl_file   : 1	
   <64>   DW_AT_decl_line   : 4	
   <65>   DW_AT_type        : <0x7b>	
   <69>   DW_AT_low_pc      : 0x40057d	
   <71>   DW_AT_high_pc     : 0xb	
   <79>   DW_AT_frame_base  : 1 byte block: 9c 	(DW_OP_call_frame_cfa)
   <7b>   DW_AT_GNU_all_call_sites: 1	
<1><7b>: Abbrev Number: 9 (DW_TAG_base_type)
   <7c>   DW_AT_byte_size   : 4	
   <7d>   DW_AT_encoding    : 5	(signed)
   <7e>   DW_AT_name        : int	
<1><82>: Abbrev Number: 0

Happy to add an automated test, LMK if it should be lit or follow unittests/SymbolFile/PDB, etc.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3925 ↗

(On Diff #78700)

(I believe |= is fine in c++, because bools are 1 or 0. But an explicit test is clearer)

[clayborg]

Looks fine.

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (num_attributes > 0) {
           (parent_context_die.Tag() == DW_TAG_class_type ||
            parent_context_die.Tag() == DW_TAG_structure_type);
 
       ValueType scope = eValueTypeInvalid;
 
       const DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
       SymbolContextScope *symbol_context_scope = NULL;
 
+      bool has_explicit_mangled = mangled != nullptr;
       if (!mangled) {
         // LLDB relies on the mangled name (DW_TAG_linkage_name or
         // DW_AT_MIPS_linkage_name) to
         // generate fully qualified names of global variables with commands like
         // "frame var j".
         // For example, if j were an int variable holding a value 4 and declared
         // in a namespace
         // B which in turn is contained in a namespace A, the command "frame var
         // j" returns
         // "(int) A::B::j = 4". If the compiler does not emit a linkage name, we
         // should be able
         // to generate a fully qualified name from the declaration context.
         if (parent_tag == DW_TAG_compile_unit &&
             Language::LanguageIsCPlusPlus(die.GetLanguage())) {
           DWARFDeclContext decl_ctx;
 
           die.GetDWARFDeclContext(decl_ctx);
           mangled = decl_ctx.GetQualifiedNameAsConstString().GetCString();
         }
       }
 
-      // DWARF doesn't specify if a DW_TAG_variable is a local, global
-      // or static variable, so we have to do a little digging by
-      // looking at the location of a variable to see if it contains
-      // a DW_OP_addr opcode _somewhere_ in the definition. I say
-      // somewhere because clang likes to combine small global variables
-      // into the same symbol and have locations like:
-      // DW_OP_addr(0x1000), DW_OP_constu(2), DW_OP_plus
-      // So if we don't have a DW_TAG_formal_parameter, we can look at
-      // the location to see if it contains a DW_OP_addr opcode, and
-      // then we can correctly classify  our variables.
       if (tag == DW_TAG_formal_parameter)
         scope = eValueTypeVariableArgument;
       else {
-        bool op_error = false;
+        // DWARF doesn't specify if a DW_TAG_variable is a local, global
+        // or static variable, so we have to do a little digging:
+        // 1) DW_AT_linkage_name implies static lifetime (but may be missing)
+        // 2) An empty DW_AT_location is an (optimized-out) static lifetime var.
+        // 3) DW_AT_location containing a DW_OP_addr implies static lifetime.
+        // Clang likes to combine small global variables into the same symbol
+        // with locations like: DW_OP_addr(0x1000), DW_OP_constu(2), DW_OP_plus
+        // so we need to look through the whole expression.
+        bool is_static_lifetime =
+            has_explicit_mangled ||
+            (has_explicit_location && !location.IsValid());
         // Check if the location has a DW_OP_addr with any address value...
         lldb::addr_t location_DW_OP_addr = LLDB_INVALID_ADDRESS;
         if (!location_is_const_value_data) {
+          bool op_error = false;
           location_DW_OP_addr = location.GetLocation_DW_OP_addr(0, op_error);
           if (op_error) {
             StreamString strm;
             location.DumpLocationForAddress(&strm, eDescriptionLevelFull, 0, 0,
                                             NULL);
             GetObjectFile()->GetModule()->ReportError(
                 "0x%8.8x: %s has an invalid location: %s", die.GetOffset(),
                 die.GetTagAsCString(), strm.GetData());
           }
+          if (location_DW_OP_addr != LLDB_INVALID_ADDRESS)
+            is_static_lifetime = true;
         }
         SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
 
-        if (location_DW_OP_addr != LLDB_INVALID_ADDRESS) {
+        if (is_static_lifetime) {
           if (is_external)
             scope = eValueTypeVariableGlobal;
           else
             scope = eValueTypeVariableStatic;
 
           if (debug_map_symfile) {
             // When leaving the DWARF in the .o files on darwin,
             // when we have a global variable that wasn't initialized,