vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.
Thanks, lgtm!
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3740
+ if (tail_call)
+ call_inst_pc = low_pc;
+ else
----------------
labath wrote:
> vsk wrote:
> > 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`.
> 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...
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.
================
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
----------------
labath wrote:
> vsk wrote:
> > 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?
> 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.
Sounds good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80519/new/
https://reviews.llvm.org/D80519
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
