[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-14 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM with Adrian's and my comments addressed.




Comment at: lldb/include/lldb/Symbol/Function.h:439
   /// The section offset based address for this function.
+  /// \param[in] generic_trampoline
+  /// If this function is a generic trampoline. A generic trampoline 

aprantl wrote:
> Is the "generic" qualifier necessary here? If we later add support for 
> trampolines with a jump target maybe, but without that this seems to just 
> create opportunity for confusion, particularly for Swift where the word 
> "generic" has very different connotations.
I wondered that too, but assumed we were still planning to support trampolines 
with a target. If not, then +1 on making this just "trampoline". 



Comment at: lldb/source/Target/ThreadPlanStepRange.cpp:520-522
+  if (log)
+log->PutCString("ThreadPlanStepRange got asked if it explains the "
+"stop for some reason other than step.");

```
LLDB_LOG(GetLog(LLDBLog::Step), "ThreadPlanStepRange got asked if it explains 
the stop for some reason other than step.");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-10 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added inline comments.



Comment at: lldb/include/lldb/Symbol/Function.h:439
   /// The section offset based address for this function.
+  /// \param[in] generic_trampoline
+  /// If this function is a generic trampoline. A generic trampoline 

Is the "generic" qualifier necessary here? If we later add support for 
trampolines with a jump target maybe, but without that this seems to just 
create opportunity for confusion, particularly for Swift where the word 
"generic" has very different connotations.



Comment at: lldb/source/Target/ThreadPlanStepThroughGenericTrampoline.cpp:2
+//===-- ThreadPlanStepThroughGenericTrampoline.cpp
+//-===//
+//

formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-06 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 updated this revision to Diff 511549.
augusto2112 marked 3 inline comments as done.
augusto2112 added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

Files:
  lldb/include/lldb/Symbol/Function.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStepOverRange.h
  lldb/include/lldb/Target/ThreadPlanStepRange.h
  lldb/include/lldb/Target/ThreadPlanStepThroughGenericTrampoline.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Symbol/Function.cpp
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadPlanShouldStopHere.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Target/ThreadPlanStepOverRange.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp
  lldb/source/Target/ThreadPlanStepThrough.cpp
  lldb/source/Target/ThreadPlanStepThroughGenericTrampoline.cpp
  lldb/test/API/lang/c/trampoline_stepping/Makefile
  lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
  lldb/test/API/lang/c/trampoline_stepping/main.c

Index: lldb/test/API/lang/c/trampoline_stepping/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/main.c
@@ -0,0 +1,52 @@
+void foo(void) {}
+
+__attribute__((debug_trampoline))
+void bar(void) {
+  foo();
+}
+
+__attribute__((debug_trampoline))
+void baz(void) {
+  bar();
+}
+
+__attribute__((nodebug))
+void nodebug(void) {}
+
+__attribute__((debug_trampoline))
+void nodebug_then_trampoline(void) {
+  nodebug();
+  baz();
+}
+
+__attribute__((debug_trampoline))
+void doesnt_call_trampoline(void) {}
+
+void direct_trampoline_call(void) {
+  bar(); // Break here for direct 
+  bar();
+}
+
+void chained_trampoline_call(void) {
+  baz(); // Break here for chained
+  baz();
+}
+
+void trampoline_after_nodebug(void) {
+  nodebug_then_trampoline(); // Break here for nodebug then trampoline
+  nodebug_then_trampoline();
+}
+
+void unused_target(void) {
+  doesnt_call_trampoline(); // Break here for unused
+}
+
+
+int main(void) {
+  direct_trampoline_call();
+  chained_trampoline_call();
+  trampoline_after_nodebug();
+  unused_target();
+  return 0;
+}
+
Index: lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
@@ -0,0 +1,104 @@
+"""Test that stepping in/out of trampolines works as expected.
+"""
+
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestTrampoline(TestBase):
+def setup(self, bkpt_str):
+self.build()
+
+_, _, thread, _ = lldbutil.run_to_source_breakpoint(
+self, bkpt_str, lldb.SBFileSpec('main.c'))
+return thread
+
+def test_direct_call(self):
+thread = self.setup('Break here for direct')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping in will take us directly to the trampoline target.
+thread.StepInto()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('foo', name)
+
+# Check that stepping out takes us back to the trampoline caller.
+thread.StepOut()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping over the end of the trampoline target 
+# takes us back to the trampoline caller.
+thread.StepInto()
+thread.StepOver()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+
+def test_chained_call(self):
+thread = self.setup('Break here for chained')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('chained_trampoline_call', name)
+
+# Check that stepping in will take us directly to the trampoline target.
+thread.StepInto()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('foo', name)
+
+# Check that stepping out takes us back to the trampoline caller.
+thread.StepOut()
+name = thread.frames[0].GetFunctionName()
+

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-03 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

> Be handy to mark this abandoned when you get a chance

Augusto had already abandoned it when he wrote that reply, maybe you're looking 
at a different patch or stale tab?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-03 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added a comment.

In D147292#4238834 , @augusto2112 
wrote:

> In D147292#4238820 , @JDevlieghere 
> wrote:
>
>> There seems to be overlap in the code added in this patch and D146679 
>> . Does one supersede the other or is there 
>> a dependency? If it's the latter you should extract that into a separate 
>> patch and make it a parent revision of this and D146679 
>> .
>
> Yes, sorry, I'm abandoning the other one in favor of this one.

Be handy to mark this abandoned when you get a chance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-02 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added inline comments.



Comment at: lldb/source/Core/Module.cpp:780
+  bool is_trampoline =
+  Target::GetGlobalProperties().GetEnableTrampolineSupport() &&
+  sc.function && sc.function->IsGenericTrampoline();

You can hoist `target::GetGlobalProperties().GetEnableTrampolineSupport()` out 
of the loop. Right now it will need to be recomputed every time in because 
someone could change the setting in the meantime and although it's a little far 
fetched, that's probably not what you want anyway. 



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2443
 
+bool is_generic_trampoline = die.IsGenericTrampoline();
+

Let's make this `const` for consistence with `func_user_id` (and remove the 
newline as they're related). 



Comment at: lldb/source/Target/ThreadPlanStepRange.cpp:509-527
+  bool return_value;
+
+  if (stop_info_sp) {
+StopReason reason = stop_info_sp->GetStopReason();
+
+if (reason == eStopReasonTrace) {
+  return_value = true;

I know you copy pasted this but this can be simplified quite a bit. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-01 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 added a comment.

In D147292#4238820 , @JDevlieghere 
wrote:

> There seems to be overlap in the code added in this patch and D146679 
> . Does one supersede the other or is there 
> a dependency? If it's the latter you should extract that into a separate 
> patch and make it a parent revision of this and D146679 
> .

Yes, sorry, I'm abandoning the other one in favor of this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-04-01 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

There seems to be overlap in the code added in this patch and D146679 
. Does one supersede the other or is there a 
dependency? If it's the latter you should extract that into a separate patch 
and make it a parent revision of this and D146679 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147292

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

2023-03-30 Thread Augusto Noronha via Phabricator via lldb-commits 
augusto2112 created this revision.
augusto2112 added reviewers: aprantl, jingham, dblaikie.
Herald added a reviewer: shafik.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds support for the DW_AT_trampoline attribute whose value
is a boolean. Which is a "generic trampoline". Stepping into a generic
trampoline by default will step through the function, checking
at every branch, until we stop in a function which makes sense to stop
at (a function with debug info, which isn't a trampoline, for example).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147292

Files:
  lldb/include/lldb/Symbol/Function.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStepOverRange.h
  lldb/include/lldb/Target/ThreadPlanStepRange.h
  lldb/include/lldb/Target/ThreadPlanStepThroughGenericTrampoline.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Symbol/Function.cpp
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadPlanShouldStopHere.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Target/ThreadPlanStepOverRange.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp
  lldb/source/Target/ThreadPlanStepThrough.cpp
  lldb/source/Target/ThreadPlanStepThroughGenericTrampoline.cpp
  lldb/test/API/lang/c/trampoline_stepping/Makefile
  lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
  lldb/test/API/lang/c/trampoline_stepping/main.c

Index: lldb/test/API/lang/c/trampoline_stepping/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/main.c
@@ -0,0 +1,52 @@
+void foo(void) {}
+
+__attribute__((transparent_stepping))
+void bar(void) {
+  foo();
+}
+
+__attribute__((transparent_stepping))
+void baz(void) {
+  bar();
+}
+
+__attribute__((nodebug))
+void nodebug(void) {}
+
+__attribute__((transparent_stepping))
+void nodebug_then_trampoline(void) {
+  nodebug();
+  baz();
+}
+
+__attribute__((transparent_stepping))
+void doesnt_call_trampoline(void) {}
+
+void direct_trampoline_call(void) {
+  bar(); // Break here for direct 
+  bar();
+}
+
+void chained_trampoline_call(void) {
+  baz(); // Break here for chained
+  baz();
+}
+
+void trampoline_after_nodebug(void) {
+  nodebug_then_trampoline(); // Break here for nodebug then trampoline
+  nodebug_then_trampoline();
+}
+
+void unused_target(void) {
+  doesnt_call_trampoline(); // Break here for unused
+}
+
+
+int main(void) {
+  direct_trampoline_call();
+  chained_trampoline_call();
+  trampoline_after_nodebug();
+  unused_target();
+  return 0;
+}
+
Index: lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
@@ -0,0 +1,104 @@
+"""Test that stepping in/out of trampolines works as expected.
+"""
+
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestTrampoline(TestBase):
+def setup(self, bkpt_str):
+self.build()
+
+_, _, thread, _ = lldbutil.run_to_source_breakpoint(
+self, bkpt_str, lldb.SBFileSpec('main.c'))
+return thread
+
+def test_direct_call(self):
+thread = self.setup('Break here for direct')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping in will take us directly to the trampoline target.
+thread.StepInto()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('foo', name)
+
+# Check that stepping out takes us back to the trampoline caller.
+thread.StepOut()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping over the end of the trampoline target 
+# takes us back to the trampoline caller.
+thread.StepInto()
+thread.StepOver()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+
+def test_chained_call(self):
+thread = self.setup('Break here for chained')
+
+# Sanity check that we start out in the correct function.
+name =