[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Roman Podoliaka via Phabricator via lldb-commits]

malor added a comment.

In D108510#2971249 , @mib wrote:

> Hi Roman, thanks for your patch. It also looks good to me, so I'll land it 
> for you.

Thank  you, Med!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG54c496dad6f2: [lldb] Allow to register frame recognizers 
applied beyond the first instruction (authored by malor, committed by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108510

Files:
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py

Index: lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
===
--- lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
+++ lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
@@ -205,6 +205,64 @@
 self.expect("frame recognizer info 0",
 substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
 
+@skipUnlessDarwin
+def test_frame_recognizer_not_only_first_instruction(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+
+# Clear internal & plugins recognizers that get initialized at launch.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("command script import " + os.path.join(self.getSourceDir(), "recognizer.py"))
+
+self.expect("frame recognizer list",
+substrs=['no matching results found.'])
+
+# Create a target.
+target, process, thread, _ = lldbutil.run_to_name_breakpoint(self, "foo",
+ exe_name = exe)
+
+# Move the PC one instruction further.
+self.runCmd("next")
+
+# Add a frame recognizer in that target.
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Add a frame recognizer with --first-instruction-only=true.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=true")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Now add a frame recognizer with --first-instruction-only=false.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=false")
+
+# This time it should recognize the frame.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
+
+opts = lldb.SBVariablesOptions()
+opts.SetIncludeRecognizedArguments(True)
+frame = thread.GetSelectedFrame()
+variables = frame.GetVariables(opts)
+
+self.assertEqual(variables.GetSize(), 2)
+self.assertEqual(variables.GetValueAtIndex(0).name, "a")
+self.assertEqual(variables.GetValueAtIndex(0).signed, 42)
+self.assertEqual(variables.GetValueAtIndex(0).GetValueType(), lldb.eValueTypeVariableArgument)
+self.assertEqual(variables.GetValueAtIndex(1).name, "b")
+self.assertEqual(variables.GetValueAtIndex(1).signed, 56)
+self.assertEqual(variables.GetValueAtIndex(1).GetValueType(), lldb.eValueTypeVariableArgument)
+
 @no_debug_info_test
 def test_frame_recognizer_delete_invalid_arg(self):
 self.expect("frame recognizer delete a", error=True,
@@ -226,3 +284,12 @@
 substrs=["error: '-1' is not a valid frame index."])
 self.expect("frame recognizer info 4294967297", error=True,
 substrs=["error: '4294967297' is not a valid frame index."])
+
+@no_debug_info_test
+def test_frame_recognizer_add_invalid_arg(self):
+self.expect("frame recognizer add -f", error=True,
+substrs=["error: last option requires an argument"])
+self.expect("frame recognizer add -f -1", error=True,
+substrs=["error: invalid boolean value '-1' passed for -f option"])
+self.expect("frame recognizer add -f foo", error=True,
+substrs=["error: invalid boolean value 'foo' passed for -f option"])
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -404,6 +404,13 @@
 Desc<"Give the name of a Python class to use for this frame recognizer.">;
   def 

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Hi Roman, thanks for your patch. It also looks good to me, so I'll land it for 
you.


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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Roman Podoliaka via Phabricator via lldb-commits]

malor added a comment.

Thank you, Jim! Would you be able to submit this change?

The clang-tidy premerge check  has failed, but I don't think it's actionable: 
it's complaining about an #include in the file that I'm not modifying -- I saw 
it failing in the same way (although, on different files) in other review 
patches as well.


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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Roman Podoliaka via Phabricator via lldb-commits]

malor updated this revision to Diff 368565.
malor added a comment.

Updated the description of the flag to incorporate Jim's feedback.


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

https://reviews.llvm.org/D108510

Files:
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py

Index: lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
===
--- lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
+++ lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
@@ -205,6 +205,64 @@
 self.expect("frame recognizer info 0",
 substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
 
+@skipUnlessDarwin
+def test_frame_recognizer_not_only_first_instruction(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+
+# Clear internal & plugins recognizers that get initialized at launch.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("command script import " + os.path.join(self.getSourceDir(), "recognizer.py"))
+
+self.expect("frame recognizer list",
+substrs=['no matching results found.'])
+
+# Create a target.
+target, process, thread, _ = lldbutil.run_to_name_breakpoint(self, "foo",
+ exe_name = exe)
+
+# Move the PC one instruction further.
+self.runCmd("next")
+
+# Add a frame recognizer in that target.
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Add a frame recognizer with --first-instruction-only=true.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=true")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Now add a frame recognizer with --first-instruction-only=false.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=false")
+
+# This time it should recognize the frame.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
+
+opts = lldb.SBVariablesOptions()
+opts.SetIncludeRecognizedArguments(True)
+frame = thread.GetSelectedFrame()
+variables = frame.GetVariables(opts)
+
+self.assertEqual(variables.GetSize(), 2)
+self.assertEqual(variables.GetValueAtIndex(0).name, "a")
+self.assertEqual(variables.GetValueAtIndex(0).signed, 42)
+self.assertEqual(variables.GetValueAtIndex(0).GetValueType(), lldb.eValueTypeVariableArgument)
+self.assertEqual(variables.GetValueAtIndex(1).name, "b")
+self.assertEqual(variables.GetValueAtIndex(1).signed, 56)
+self.assertEqual(variables.GetValueAtIndex(1).GetValueType(), lldb.eValueTypeVariableArgument)
+
 @no_debug_info_test
 def test_frame_recognizer_delete_invalid_arg(self):
 self.expect("frame recognizer delete a", error=True,
@@ -226,3 +284,12 @@
 substrs=["error: '-1' is not a valid frame index."])
 self.expect("frame recognizer info 4294967297", error=True,
 substrs=["error: '4294967297' is not a valid frame index."])
+
+@no_debug_info_test
+def test_frame_recognizer_add_invalid_arg(self):
+self.expect("frame recognizer add -f", error=True,
+substrs=["error: last option requires an argument"])
+self.expect("frame recognizer add -f -1", error=True,
+substrs=["error: invalid boolean value '-1' passed for -f option"])
+self.expect("frame recognizer add -f foo", error=True,
+substrs=["error: invalid boolean value 'foo' passed for -f option"])
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -404,6 +404,13 @@
 Desc<"Give the name of a Python class to use for this frame recognizer.">;
   def frame_recognizer_regex : Option<"regex", "x">,
 Desc<"Function name and module name are actually regular expressions.">;
+  def frame_recognizer_first_instruction_only : 

Re: [Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Jim Ingham via lldb-commits]

> On Aug 24, 2021, at 11:57 AM, Roman Podoliaka via Phabricator 
>  wrote:
> 
> malor added a comment.
> 
> In D108510#2961332 , @jingham wrote:
> 
>> One of the key reasons to use "frame recognizers" is to provide argument 
>> values to functions that don't have debug information.  That generally only 
>> works when stopped at the first instruction, since otherwise you have to 
>> follow the value as it moves through the code, which in the absence of debug 
>> information isn't entirely trivial.
>> 
>> A naive user might think reporting `expr (SomeType *) $arg1` is going to 
>> always work, but in fact it only works reliably on the first instruction.
>> 
>> I don't think that possible incorrect usage means we should only allow frame 
>> recognizers on the first instruction.  But I do think we should say 
>> something more in the help for this flag, like "remember that $arg is 
>> only reliable on the first instruction" to warn folks in advance of this 
>> pitfall.
> 
> Thank you, Jim! That makes sense. I figured it was something along those 
> lines.
> 
> Funnily enough, the way //I// ran into this issue is that I tried to add a 
> frame recognizer for a binary that //had// debug information. In that case, 
> doing something like `breakpoint set -n foo` actually sets the breakpoint 
> *right after* the prologue of a function, so execution won't stop at the 
> first instruction but at something like `foo+4`, and thus the frame 
> recognizer won't be applied.

If you are relying on "first instruction" behavior for breakpoints you can pass 
"--skip-prologue 0" when you set the breakpoint.  For source debugging, you 
generally want to stop after the prologue because debug info doesn't generally 
describe variable locations in the prologue, so the locals variable values will 
likely be wrong.


> 
> I think we should give users a choice and keep the current behavior of only 
> applying recognizers to the first instruction as the default. I'll upload a 
> new revision with the updated documentation.
> 

SGTM

Jim

> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D108510/new/
> 
> https://reviews.llvm.org/D108510
> 

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Roman Podoliaka via Phabricator via lldb-commits]

malor added a comment.

In D108510#2961332 , @jingham wrote:

> One of the key reasons to use "frame recognizers" is to provide argument 
> values to functions that don't have debug information.  That generally only 
> works when stopped at the first instruction, since otherwise you have to 
> follow the value as it moves through the code, which in the absence of debug 
> information isn't entirely trivial.
>
> A naive user might think reporting `expr (SomeType *) $arg1` is going to 
> always work, but in fact it only works reliably on the first instruction.
>
> I don't think that possible incorrect usage means we should only allow frame 
> recognizers on the first instruction.  But I do think we should say something 
> more in the help for this flag, like "remember that $arg is only reliable 
> on the first instruction" to warn folks in advance of this pitfall.

Thank you, Jim! That makes sense. I figured it was something along those lines.

Funnily enough, the way //I// ran into this issue is that I tried to add a 
frame recognizer for a binary that //had// debug information. In that case, 
doing something like `breakpoint set -n foo` actually sets the breakpoint 
*right after* the prologue of a function, so execution won't stop at the first 
instruction but at something like `foo+4`, and thus the frame recognizer won't 
be applied.

I think we should give users a choice and keep the current behavior of only 
applying recognizers to the first instruction as the default. I'll upload a new 
revision with the updated documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

One of the key reasons to use "frame recognizers" is to provide argument values 
to functions that don't have debug information.  That generally only works when 
stopped at the first instruction, since otherwise you have to follow the value 
as it moves through the code, which in the absence of debug information isn't 
entirely trivial.

A naive user might think reporting `expr (SomeType *) $arg1` is going to always 
work, but in fact it only works reliably on the first instruction.

I don't think that possible incorrect usage means we should only allow frame 
recognizers on the first instruction.  But I do think we should say something 
more in the help for this flag, like "remember that $arg is only reliable on 
the first instruction" to warn folks in advance of this pitfall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108510

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

[Lldb-commits] [PATCH] D108510: [lldb] Allow to register frame recognizers applied beyond the first instruction

[Roman Podoliaka via Phabricator via lldb-commits]

malor created this revision.
malor added reviewers: jingham, mib.
malor added a project: LLDB.
Herald added subscribers: dang, JDevlieghere.
malor requested review of this revision.
Herald added a subscriber: lldb-commits.

It is currently possible to register a frame recognizer, but it will be applied 
if and only if the frame's PC points to the very first instruction of the 
specified function, which limits usability of this feature.

The implementation already supports changing this behaviour by passing an 
additional flag, but it's not possible to set it via the command interface. Fix 
that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108510

Files:
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py

Index: lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
===
--- lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
+++ lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
@@ -205,6 +205,64 @@
 self.expect("frame recognizer info 0",
 substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
 
+@skipUnlessDarwin
+def test_frame_recognizer_not_only_first_instruction(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+
+# Clear internal & plugins recognizers that get initialized at launch.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("command script import " + os.path.join(self.getSourceDir(), "recognizer.py"))
+
+self.expect("frame recognizer list",
+substrs=['no matching results found.'])
+
+# Create a target.
+target, process, thread, _ = lldbutil.run_to_name_breakpoint(self, "foo",
+ exe_name = exe)
+
+# Move the PC one instruction further.
+self.runCmd("next")
+
+# Add a frame recognizer in that target.
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Add a frame recognizer with --first-instruction-only=true.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=true")
+
+# It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 not recognized by any recognizer'])
+
+# Now add a frame recognizer with --first-instruction-only=false.
+self.runCmd("frame recognizer clear")
+
+self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=false")
+
+# This time it should recognize the frame.
+self.expect("frame recognizer info 0",
+substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
+
+opts = lldb.SBVariablesOptions()
+opts.SetIncludeRecognizedArguments(True)
+frame = thread.GetSelectedFrame()
+variables = frame.GetVariables(opts)
+
+self.assertEqual(variables.GetSize(), 2)
+self.assertEqual(variables.GetValueAtIndex(0).name, "a")
+self.assertEqual(variables.GetValueAtIndex(0).signed, 42)
+self.assertEqual(variables.GetValueAtIndex(0).GetValueType(), lldb.eValueTypeVariableArgument)
+self.assertEqual(variables.GetValueAtIndex(1).name, "b")
+self.assertEqual(variables.GetValueAtIndex(1).signed, 56)
+self.assertEqual(variables.GetValueAtIndex(1).GetValueType(), lldb.eValueTypeVariableArgument)
+
 @no_debug_info_test
 def test_frame_recognizer_delete_invalid_arg(self):
 self.expect("frame recognizer delete a", error=True,
@@ -226,3 +284,12 @@
 substrs=["error: '-1' is not a valid frame index."])
 self.expect("frame recognizer info 4294967297", error=True,
 substrs=["error: '4294967297' is not a valid frame index."])
+
+@no_debug_info_test
+def test_frame_recognizer_add_invalid_arg(self):
+self.expect("frame recognizer add -f", error=True,
+substrs=["error: last option requires an argument"])
+self.expect("frame recognizer add -f -1", error=True,
+substrs=["error: invalid boolean value '-1' passed for -f option"])
+self.expect("frame recognizer add -f foo", error=True,
+substrs=["error: invalid boolean value 'foo' passed for -f option"])
Index: