[lldb/DWARF] Add support for pre-standard GNU call site attributes

[labath]

Summary

The code changes are very straight-forward -- just handle both DW_AT_GNU
and DW_AT_call versions of all tags and attributes. There is just one
small gotcha: in the GNU version, DW_AT_low_pc was used both for the
"return pc" and the "call pc" values, depending on whether the tag was
describing a tail call, while the official scheme uses different
attributes for the two things.

Depends on D80518.

[vsk]

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

3740

I think this needs to be call_inst_pc = low_pc - 1, see DwarfCompileUnit::constructCallSiteEntryDIE for the rationale, and StackFrameList::SynthesizeTailCallFrames for where we use this information. The relevant part of the comment from SynthesizeTailCallFrames is:

"We do not want to subtract 1 from this PC, as it's the actual address of the tail-calling branch instruction. This address is provided by the compiler via DW_AT_call_pc."

In GNU+Dwarf4 mode, that's no longer true, the DW_AT_low_pc is a fake "return address" for the tail call (really: the address of the instruction after the tail-calling jump).

On x86_64, this test doesn't seem to stress this case, but the test breaks on Darwin/arm64 without the adjustment.

[vsk]

Also, I think it'd be good to enable most/all of the tests under test/API/functionalities/tail_call_frames under -ggdb along with this change.

[labath]

• add -1
• add -ggdb flavours for inline tail call tests, drop -glldb from the others. The choice is fairly arbitrary. I don't believe we should be doing combinatorial testing at this level, so I did not want to enable all of them. Enabling inline tests was easy because I had muscle memory for it. For the rest, dropping -glldb means that the gnu flavour will be tested on platforms that default to -ggdb.

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

3740

Heh, you're right. I should've looked at what the code does instead of just trying to reverse engineer the logic from the output. I've now added the -1.
After looking that this code some more, I've come to realize that it's usage of the lack of DW_AT_call_return_pc to indicate a tail call is not correct -- I don't see anything preventing a producer from generating this attribute even for tail calls. I'm going to try refactoring this in another patch to store the tail-call-ness more explicitly. That should also make this part slightly cleaner.

[vsk]

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

3740

The parsing code uses DW_AT_call_tail_call to determine whether or not there's a tail call, which is already explicit. However, the CallEdge representation does take an invalid return_pc address to mean there's a tail call. My $0.02 is that that's legit, and a producer that emits DW_AT_call_return_pc at a tail call site is behaving badly. If we care to support that, we could do it by changing the condition on line 3710 to attr == DW_AT_call_return_pc && !tail_call.

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Are these test updates necessary because lldb doesn't print '[opt]' and '[artificial]' next to frame descriptions in a consistent way across platforms? Or is it just that you don't think matching '[opt]' is relevant to the test?

[labath]

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

3740

You're right again. I confused a tail call with a (regular) call to a noreturn function.

The real reason why I wanted to do that change was to avoid the -1 thingy here. If we carried the information about a tail call and the information whether the address points before/after the call instruction explicitly, I believe that the -1 here wouldn't be necessary, and the code in SynthesizeTailCallFrames could just set behaves_like_frame_zero depending on what kind of address it gets. I have a feeling that would be cleaner, but I have to try it out...

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Right, I wanted to mention that as it's not very obvious, but I forgot...

The [opt] thingy is not printed at all with -ggdb because the attribute we get this information from -- DW_AT_APPLE_optimized -- is only emitted for -glldb. The optimization flag did not seem very relevant for these tests (I mean, technically the compiler could emit call site attributes even in non-optimized mode) so instead of forking the expectations I chose to simply remove it.

[vsk]

Thanks, lgtm!

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

3740

I think you could do this by adding a flag to the CallEdge class, like:

bool have_one_after_call_inst_pc = ...;
union {
  addr_t call_inst_pc;
  addr_t one_after_call_inst_pc;
};

and then set behaves_like_frame_zero = !call_edge.have_one_after_call_inst_pc. But I'm not convinced this successfully separates the concerns re: parsing DWARF vs. adjusting PC values.

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Sounds good.

[dblaikie]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

As an aside, now that lldb understands these attributes - perhaps we should emit them under -glldb as well as -ggdb? (@aprantl might be interested in making that call)

[labath]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

FWIW, I think that would be great as it would reduce the effects of the debugger tuning argument, making the compiler output more "portable".

Though, we may want to wait with that until I look at the -1 issue. I believe that the way this is implemented now means we will end up pointing to the middle of a call instruction in an artificial frame, which would be a slight regression. It's not the end of the world, but I believe we can do something slightly better.

[labath]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Ok, I take that back. The instruction pointer handling is not terribly consistent right now anyway:

(lldb) up
frame #1: 0x0000000000401210 a.out`func12(...)
(lldb) register read rip
     rip = 0x0000000000401300

So, I wouldn't worry too much about preserving behavior here.

[vsk]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

I don't see any concrete benefit to supporting -ggdb on Darwin. Actually, changing llvm to emit the GNU opcodes on Darwin seems bad to me, as it could force Darwin tools authors to support two sets of call-site related opcodes.

[dblaikie]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Not sure what it would mean to "support -ggdb on Darwin" - it's supported in that some peolpe might be using gdb on Darwin & compile that way. But I meant emitting these when targeting lldb - which someone might be doing on any platform & they might still want to use DWARFv4 for whatever reason - or does DWARFv4 + -glldb already use the DWARFv5 call site opcodes? If so, then, sure, that sounds OK to me & I don't suppose there's much reason to emit the GNU ones instead. If DWARFv4 + -glldb doesn't emit any call site info, it seems like an improvement to emit the GNU extension (consumers should always be written to ignore tags/attributes they don't know - and if so they'll be no worse off than if the call site info hadn't been emitted) or the v5 attributes maybe (though I don't know that that's quite as valid - I guess a consumer could rightly reject tags/attributes that aren't in the extension number space, or in the standard number space for the version being parsed)

[vsk]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Yep, DWARFv4 + -glldb emits DWARFv5 call site opcodes -- and I should have written (as I meant), 'there's no need to change this to behave like -ggdb under -gdwarf-4 mode on Darwin'. It should (remain!) possible to use -ggdb on Darwin.

[dblaikie]

lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp

8 ↗

(On Diff #267179)

Ah, fair enough - marginally questionable emitting future standard tags/attributes in previous versions - but I'm not too fussed so long as it's not new forms. As that's the direction chosen, yeah, no point emitting the DWARFv4 gdb extension tags/attributes under -glldb.

lldb/source/Expression/DWARFExpression.cpp

static offset_t GetOpcodeDataSize(const DataExtractor &data,
   case DW_OP_implicit_value: // 0x9e ULEB128 size followed by block of that size
                              // (DWARF4)
   {
     uint64_t block_len = data.Skip_LEB128(&offset);
     offset += block_len;
     return offset - data_offset;
   }
 
+  case DW_OP_GNU_entry_value:
   case DW_OP_entry_value: // 0xa3 ULEB128 size + variable-length block
   {
     uint64_t subexpr_len = data.GetULEB128(&offset);
     return (offset - data_offset) + subexpr_len;
   }
 
   default:
     break;
case DW_OP_GNU_const_index: {
                                     "compile unit being specified");
         return false;
       }
       uint64_t index = opcodes.GetULEB128(&offset);
       lldb::addr_t value = ReadAddressFromDebugAddrSection(dwarf_cu, index);
       stack.push_back(Scalar(value));
     } break;
 
+    case DW_OP_GNU_entry_value:
     case DW_OP_entry_value: {
       if (!Evaluate_DW_OP_entry_value(stack, exe_ctx, reg_ctx, opcodes, offset,
                                       error_ptr, log)) {
         LLDB_ERRORF(error_ptr, "Could not evaluate %s.",
                     DW_OP_value_to_name(op));
         return false;
       }
       break;

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

 }
 
 /// Collect call site parameters in a DW_TAG_call_site DIE.
 static CallSiteParameterArray
 CollectCallSiteParameters(ModuleSP module, DWARFDIE call_site_die) {
   CallSiteParameterArray parameters;
   for (DWARFDIE child = call_site_die.GetFirstChild(); child.IsValid();
        child = child.GetSibling()) {
-    if (child.Tag() != DW_TAG_call_site_parameter)
+    if (child.Tag() != DW_TAG_call_site_parameter &&
+        child.Tag() != DW_TAG_GNU_call_site_parameter)
       continue;
 
     llvm::Optional<DWARFExpression> LocationInCallee;
     llvm::Optional<DWARFExpression> LocationInCaller;
 
     DWARFAttributes attributes;
     const size_t num_attributes = child.GetAttributes(attributes);
 
auto parse_simple_location =
                              DataExtractor(data, block_offset, block_length),
                              child.GetCU());
     };
 
     for (size_t i = 0; i < num_attributes; ++i) {
       dw_attr_t attr = attributes.AttributeAtIndex(i);
       if (attr == DW_AT_location)
         LocationInCallee = parse_simple_location(i);
-      if (attr == DW_AT_call_value)
+      if (attr == DW_AT_call_value || attr == DW_AT_GNU_call_site_value)
         LocationInCaller = parse_simple_location(i);
     }
 
     if (LocationInCallee && LocationInCaller) {
       CallSiteParameter param = {*LocationInCallee, *LocationInCaller};
       parameters.push_back(param);
     }
   }
   return parameters;
 }
 
 /// Collect call graph edges present in a function DIE.
 std::vector<std::unique_ptr<lldb_private::CallEdge>>
 SymbolFileDWARF::CollectCallEdges(ModuleSP module, DWARFDIE function_die) {
   // Check if the function has a supported call site-related attribute.
   // TODO: In the future it may be worthwhile to support call_all_source_calls.
-  uint64_t has_call_edges =
-      function_die.GetAttributeValueAsUnsigned(DW_AT_call_all_calls, 0);
+  bool has_call_edges =
+      function_die.GetAttributeValueAsUnsigned(DW_AT_call_all_calls, 0) ||
+      function_die.GetAttributeValueAsUnsigned(DW_AT_GNU_all_call_sites, 0);
   if (!has_call_edges)
     return {};
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   LLDB_LOG(log, "CollectCallEdges: Found call site info in {0}",
            function_die.GetPubname());
 
   // Scan the DIE for TAG_call_site entries.
   // TODO: A recursive scan of all blocks in the subprogram is needed in order
   // to be DWARF5-compliant. This may need to be done lazily to be performant.
   // For now, assume that all entries are nested directly under the subprogram
   // (this is the kind of DWARF LLVM produces) and parse them eagerly.
   std::vector<std::unique_ptr<CallEdge>> call_edges;
   for (DWARFDIE child = function_die.GetFirstChild(); child.IsValid();
        child = child.GetSibling()) {
-    if (child.Tag() != DW_TAG_call_site)
+    if (child.Tag() != DW_TAG_call_site && child.Tag() != DW_TAG_GNU_call_site)
       continue;
 
     llvm::Optional<DWARFDIE> call_origin;
     llvm::Optional<DWARFExpression> call_target;
     addr_t return_pc = LLDB_INVALID_ADDRESS;
     addr_t call_inst_pc = LLDB_INVALID_ADDRESS;
+    addr_t low_pc = LLDB_INVALID_ADDRESS;
+    bool tail_call = false;
 
     DWARFAttributes attributes;
     const size_t num_attributes = child.GetAttributes(attributes);
     for (size_t i = 0; i < num_attributes; ++i) {
       DWARFFormValue form_value;
       if (!attributes.ExtractFormValueAtIndex(i, form_value)) {
         LLDB_LOG(log, "CollectCallEdges: Could not extract TAG_call_site form");
         break;
       }
 
       dw_attr_t attr = attributes.AttributeAtIndex(i);
 
+      if (attr == DW_AT_call_tail_call || attr == DW_AT_GNU_tail_call)
+        tail_call = form_value.Boolean();
+
       // Extract DW_AT_call_origin (the call target's DIE).
-      if (attr == DW_AT_call_origin) {
+      if (attr == DW_AT_call_origin || attr == DW_AT_abstract_origin) {
         call_origin = form_value.Reference();
         if (!call_origin->IsValid()) {
           LLDB_LOG(log, "CollectCallEdges: Invalid call origin in {0}",
                    function_die.GetPubname());
           break;
         }
       }
 
+      if (attr == DW_AT_low_pc)
+        low_pc = form_value.Address();
+
       // Extract DW_AT_call_return_pc (the PC the call returns to) if it's
       // available. It should only ever be unavailable for tail call edges, in
       // which case use LLDB_INVALID_ADDRESS.
       if (attr == DW_AT_call_return_pc)
         return_pc = form_value.Address();
 
       // Extract DW_AT_call_pc (the PC at the call/branch instruction). It
       // should only ever be unavailable for non-tail calls, in which case use
       // LLDB_INVALID_ADDRESS.
       if (attr == DW_AT_call_pc)
         call_inst_pc = form_value.Address();
 
       // Extract DW_AT_call_target (the location of the address of the indirect
       // call).
-      if (attr == DW_AT_call_target) {
+      if (attr == DW_AT_call_target || attr == DW_AT_GNU_call_site_target) {
         if (!DWARFFormValue::IsBlockForm(form_value.Form())) {
           LLDB_LOG(log,
                    "CollectCallEdges: AT_call_target does not have block form");
           break;
         }
 
         auto data = child.GetData();
         uint32_t block_offset = form_value.BlockData() - data.GetDataStart();
         uint32_t block_length = form_value.Unsigned();
         call_target = DWARFExpression(
             module, DataExtractor(data, block_offset, block_length),
             child.GetCU());
       }
     }
+    // In DWARF v4 low_pc is either the address of the call instruction or the
+    // "return to" address, depending on whether the call is a tail call.
+    if (child.Tag() == DW_TAG_GNU_call_site) {
+      if (tail_call)
+        call_inst_pc = low_pc;
+      else
+        return_pc = low_pc;
+    }
+
     if (!call_origin && !call_target) {
       LLDB_LOG(log, "CollectCallEdges: call site without any call target");
       continue;
     }
 
     // Adjust any PC forms. It needs to be fixed up if the main executable
     // contains a debug map (i.e. pointers to object files), because we need a
     // file address relative to the executable's text section.

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

 from lldbsuite.test import lldbinline
 from lldbsuite.test.decorators import *
 from lldbsuite.test import lldbplatformutil
 
 supported_archs = ["x86_64", "aarch64"]
 decorators = [skipIf(archs=no_match(supported_archs)),
          skipIf(compiler="clang", compiler_version=['<', '10.0']),
          skipUnlessHasCallSiteInfo,
          skipIf(dwarf_version=['<', '4'])]
 
 lldbinline.MakeInlineTest(__file__, globals(), decorators=decorators,
         name="BasicEntryValues_V5",
         build_dict=dict(CXXFLAGS_EXTRAS="-O2 -glldb"))
+
+lldbinline.MakeInlineTest(__file__, globals(), decorators=decorators,
+        name="BasicEntryValues_GNU",
+        build_dict=dict(CXXFLAGS_EXTRAS="-O2 -ggdb"))

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

 __attribute__((noinline)) void func1(int &sink) {
   // First use works around a compiler "bug" where an unused variable gets
   // no location descriptions. The second use overwrites the function arguments
   // with other values.
   use<int &>(sink);
   use<int &>(dummy);
 
   ++global;
-  //% self.filecheck("image lookup -v -a $pc", "main.cpp", "-check-prefix=FUNC1-DESC")
-  // FUNC1-DESC: name = "sink", type = "int &", location = DW_OP_entry_value
+  //% prefix = "FUNC1-GNU" if "GNU" in self.name else "FUNC1-V5"
+  //% self.filecheck("image lookup -v -a $pc", "main.cpp", "-check-prefix="+prefix)
+  // FUNC1-GNU: name = "sink", type = "int &", location = DW_OP_GNU_entry_value
+  // FUNC1-V5: name = "sink", type = "int &", location = DW_OP_entry_value
 }
 
 __attribute__((noinline)) void func2(int &sink, int x) {
   use<int &, int>(sink, x);
   use<int &, int>(dummy, 0);
 
   ++global;
   //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC2-EXPR1")