[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3740
+      if (tail_call)
+        call_inst_pc = low_pc;
+      else
----------------
labath wrote:
> vsk wrote:
> > 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.
> 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.
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`.


================
Comment at: lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8
+  // FROM-FUNC1-NEXT: func1
+  // FROM-FUNC1-SAME: [artificial]
+  // FROM-FUNC1-NEXT: main
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80519