Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

> On Oct 19, 2018, at 5:37 PM, Vedant Kumar wrote: > > Hi Stella, > > The logs are really helpful, thanks. This part is unexpected: > > python Finding frames between main and sink(), retn-pc=0x4005b8 > python GetCallEdges: Attempting to parse call site info for main >

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

Hi Stella, The logs are really helpful, thanks. This part is unexpected: python Finding frames between main and sink(), retn-pc=0x4005b8 python GetCallEdges: Attempting to parse call site info for main python CollectCallEdges: Found call site info in main python

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

Hey Vedant, I’ve attached the logs from Linux. Most of the tests now pass on Windows with the exception of TestSteppingOutWithArtificialFrames and TestTailCallFrameSBAPI. Both of these attempt to get a specific frame by calling GetFrameAtIndex which only works partially on Windows right now.

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

Once we’re no longer using DIA this should be a lot easier to control on our side On Tue, Oct 16, 2018 at 11:17 AM Vedant Kumar wrote: > > > On Oct 16, 2018, at 10:59 AM, Stella Stamenova > wrote: > > The windows error is because the names are different, as you expected: > > AssertionError:

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

> On Oct 16, 2018, at 10:59 AM, Stella Stamenova wrote: > > The windows error is because the names are different, as you expected: > > AssertionError: 'void sink(void)' != 'sink()' > > You can probably update the test to look for a different name on Windows > (though if I recall

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

The windows error is because the names are different, as you expected: AssertionError: 'void sink(void)' != 'sink()' You can probably update the test to look for a different name on Windows (though if I recall correctly, different versions of the DIA sdk provide different detail on the names,

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

> On Oct 15, 2018, at 4:46 PM, Frédéric Riss wrote: > > > >> On Oct 15, 2018, at 4:40 PM, Vedant Kumar > > wrote: >> >> >> >>> On Oct 15, 2018, at 3:47 PM, Stella Stamenova via Phabricator >>> mailto:revi...@reviews.llvm.org>> wrote: >>> >>> stella.stamenova added

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

> On Oct 15, 2018, at 4:40 PM, Vedant Kumar wrote: > > > >> On Oct 15, 2018, at 3:47 PM, Stella Stamenova via Phabricator >> wrote: >> >> stella.stamenova added a comment. >> >> In https://reviews.llvm.org/D50478#1262717, @vsk wrote: >> >>> In https://reviews.llvm.org/D50478#1262710,

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

> On Oct 15, 2018, at 3:47 PM, Stella Stamenova via Phabricator > wrote: > > stella.stamenova added a comment. > > In https://reviews.llvm.org/D50478#1262717, @vsk wrote: > >> In https://reviews.llvm.org/D50478#1262710, @stella.stamenova wrote: >> >>> Unfortunately, the bots are broken

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

stella.stamenova added a comment. In https://reviews.llvm.org/D50478#1262717, @vsk wrote: > In https://reviews.llvm.org/D50478#1262710, @stella.stamenova wrote: > > > Unfortunately, the bots are broken because of the FileCheck issue, so I > > can't confirm with them, but I see a number of these

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

zturner added a subscriber: aprantl. zturner added a comment. https://reviews.llvm.org/D53002 Is the thread I'm referring to. Repository: rLLDB LLDB https://reviews.llvm.org/D50478 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

https://reviews.llvm.org/D53002 Is the thread I'm referring to. On Thu, Oct 11, 2018 at 2:59 PM Zachary Turner via Phabricator < revi...@reviews.llvm.org> wrote: > zturner added a subscriber: vsk. > zturner added a comment. > > See the other email thread. The bots have been broken since

Re: [Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

See the other email thread. The bots have been broken since September, all of them complain about missing FileCheck executable On Thu, Oct 11, 2018 at 2:58 PM Vedant Kumar via Phabricator < revi...@reviews.llvm.org> wrote: > vsk added a comment. > > In https://reviews.llvm.org/D50478#1262710,

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

zturner added a subscriber: vsk. zturner added a comment. See the other email thread. The bots have been broken since September, all of them complain about missing FileCheck executable Repository: rLLDB LLDB https://reviews.llvm.org/D50478 ___

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added a comment. In https://reviews.llvm.org/D50478#1262710, @stella.stamenova wrote: > Unfortunately, the bots are broken because of the FileCheck issue, so I can't > confirm with them, but I see a number of these tests fail in our local > testing. Some fail on both Windows and Linux and

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB343900: Add support for artificial tail call frames (authored by vedantk, committed by ). Herald added subscribers: teemperor, abidh. Changed prior to commit:

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D50478 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:304 +public: + CallEdge(const char *mangled_name, lldb::addr_t return_pc); + aprantl wrote: > vsk wrote: > > vsk wrote: > > > aprantl wrote: > > > > Does this also work for C functions?

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:304 +public: + CallEdge(const char *mangled_name, lldb::addr_t return_pc); + vsk wrote: > vsk wrote: > > aprantl wrote: > > > Does this also work for C functions? If yes, would

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:304 +public: + CallEdge(const char *mangled_name, lldb::addr_t return_pc); + aprantl wrote: > Does this also work for C functions? If yes, would `symbol_name` be a more > accurate

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 168155. vsk marked an inline comment as done. vsk added a comment. - Address feedback from Adrian. https://reviews.llvm.org/D50478 Files: lldb/include/lldb/API/SBFrame.h lldb/include/lldb/Core/FormatEntity.h lldb/include/lldb/Symbol/Block.h

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl added a comment. This looks pretty good! I have one last question about CallEdge inside. Comment at: lldb/include/lldb/Symbol/Function.h:304 +public: + CallEdge(const char *mangled_name, lldb::addr_t return_pc); + Does this also work for C functions?

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added a comment. Friendly ping. https://reviews.llvm.org/D50478 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added inline comments. Comment at: lldb/source/Target/StackFrameList.cpp:331 +dfs(next_callee); +if (ambiguous) + return; aprantl wrote: > On what path can this happen? Aren't all paths that set `ambiguous=true` > returning

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 167148. vsk marked an inline comment as done. vsk added a comment. - Use std::make_shared(...), instead of StackFrameSP(new ...). https://reviews.llvm.org/D50478 Files: lldb/include/lldb/API/SBFrame.h lldb/include/lldb/Core/FormatEntity.h

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl added inline comments. Comment at: lldb/source/Target/StackFrameList.cpp:331 +dfs(next_callee); +if (ambiguous) + return; On what path can this happen? Aren't all paths that set `ambiguous=true` returning immediately?

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 167038. vsk added a comment. The bug I thought I saw in CallEdge::GetReturnPCAddress turned out to be an issue I introduced in dsymutil. It's not correct for dsymutil to relocate the return PC value in TAG_call_site entries, because the correct section slide

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk planned changes to this revision. vsk added a comment. While testing this out, I found an issue with CallEdge::GetReturnPCAddress. Getting the load address there adds an unnecessary slide to the PC. I'll try to have that worked out next week. https://reviews.llvm.org/D50478

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 166534. vsk added a comment. As discussed offline, print out a note when stepping out of a frame with artificial ancestors explaining that they were skipped while stepping out. See the added test: functionalities/tail_call_frames/thread_step_out_message.

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 166394. vsk added a comment. Teach SBThread::StepOut and SBThread::ReturnFromFrame to behave as-if artificial frames were not present. This preserves the current behavior of "finish" and "thread return". The alternatives -- stepping out into an artificial

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk planned changes to this revision. vsk added a comment. In https://reviews.llvm.org/D50478#1241264, @jingham wrote: > Can you add a test that makes sure that when you stop in a frame that has > artificial frames above it, and then you do "finish", or "step out" past the > end of frame 0,

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

jingham added a comment. Really "step out past the end of the frame" isn't really necessary, since that doesn't pay any attention to the frames above it, it just runs the code till the frame ID changes. But "finish" does look at the parent frame, so it might get confused.

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

jingham added a comment. Can you add a test that makes sure that when you stop in a frame that has artificial frames above it, and then you do "finish", or "step out" past the end of frame 0, the presence of the artificial frame doesn't confuse us? I am pretty sure that will just work, but it

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 166378. vsk added a comment. I've added SB API support (SBFrame::IsArtificial), a SB API test, fleshed out the remaining tests, and rebased. PTAL, thanks! https://reviews.llvm.org/D50478 Files: lldb/include/lldb/API/SBFrame.h

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:331 + /// \ref resolved. + union { +const char *mangled_name; vsk wrote: > aprantl wrote: > > `llvm::PointerUnion` ? > It's not possible to use PointerUnion here because `const

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 162416. vsk marked 4 inline comments as done. vsk added a comment. Thanks for the feedback! https://reviews.llvm.org/D50478 Files: lldb/include/lldb/Symbol/Block.h lldb/include/lldb/Symbol/Function.h lldb/include/lldb/Symbol/SymbolFile.h

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:331 + /// \ref resolved. + union { +const char *mangled_name; aprantl wrote: > `llvm::PointerUnion` ? It's not possible to use PointerUnion here because `const char *` has 1-byte

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

aprantl added inline comments. Comment at: lldb/include/lldb/Symbol/Function.h:331 + /// \ref resolved. + union { +const char *mangled_name; `llvm::PointerUnion` ? https://reviews.llvm.org/D50478 ___

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

sgraenitz added a comment. Interesting topic and very well-written patch. Semantics look reasonable, but can't say much about the details. Added a few minor notes inline. Comment at: lldb/include/lldb/Symbol/Function.h:456 +

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk added a comment. Ping. https://reviews.llvm.org/D50478 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

vsk updated this revision to Diff 160666. vsk retitled this revision from "WIP: Basic tail call frame support" to "Add support for artificial tail call frames". vsk edited the summary of this revision. vsk added reviewers: aprantl, probinson, JDevlieghere, jingham, friss, zturner. vsk removed