[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Move best-practices.txt contain to resources/test.rst

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added a comment.

I will ask for commit access and commit it soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

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

[Lldb-commits] [PATCH] D108889: Use dSYM SymbolFile Sections file addr instead of ObjectFile Section file addr when they differ in ObjectFileMachO

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Nice catch, LGTM but I'll defer to Greg to accept.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108889

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

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Move best-practices.txt contain to resources/test.rst

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Thanks for the quick turnaround! This LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

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

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 369376.
xgupta added a comment.

restore a blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

Files:
  lldb/docs/resources/test.rst
  lldb/docs/testsuite/best-practices.txt

Index: lldb/docs/testsuite/best-practices.txt
===
--- lldb/docs/testsuite/best-practices.txt
+++ /dev/null
@@ -1,93 +0,0 @@
-This document attempts to point out some best practices that prove to be helpful
-when building new test cases in the tot/test directory.  Everyone is welcomed to
-add/modify contents into this file.
-
-o Do not use hard-coded line numbers in your test case.  Instead, try to tag the
-  line with some distinguishing pattern, and use the function line_number()
-  defined in lldbtest.py which takes filename and string_to_match as arguments
-  and returns the line number.
-
-As an example, take a look at test/breakpoint_conditions/main.c which has these
-two lines:
-
-return c(val); // Find the line number of c's parent call here.
-
-and
-
-return val + 3; // Find the line number of function "c" here.
-
-The Python test case TestBreakpointConditions.py uses the comment strings to
-find the line numbers during setUp(self) and use them later on to verify that
-the correct breakpoint is being stopped on and that its parent frame also has
-the correct line number as intended through the breakpoint condition.
-
-o Take advantage of the unittest framework's decorator features to properly
-  mark your test class or method for platform-specific tests.
-
-As an example, take a look at test/forward/TestForwardDeclaration.py which has
-these lines:
-
-@unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
-def test_with_dsym_and_run_command(self):
-"""Display *bar_ptr when stopped on a function with forward declaration of struct bar."""
-self.buildDsym()
-self.forward_declaration()
-
-This tells the test harness that unless we are running "darwin", the test should
-be skipped.  This is because we are asking to build the binaries with dsym debug
-info, which is only available on the darwin platforms.
-
-o Cleanup after yourself.  A classic example of this can be found in test/types/
-  TestFloatTypes.py:
-
-def test_float_types_with_dsym(self):
-"""Test that float-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'float.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.float_type()
-
-...
-
-def test_double_type_with_dsym(self):
-"""Test that double-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'double.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.double_type()
-
-This tests different data structures composed of float types to verify that what
-the debugger prints out matches what the compiler does for different variables
-of these types.  We're using a dictionary to pass the build parameters to the
-build system.  After a particular test instance is done, it is a good idea to
-clean up the files built.  This eliminates the chance that some leftover files
-can interfere with the build phase for the next test instance and render it
-invalid.
-
-TestBase.setTearDownCleanup(self, dictionary) defined in lldbtest.py is created
-to cope with this use case by taking the same build parameters in order to do
-the cleanup when we are finished with a test instance, during
-TestBase.tearDown(self).
-
-o Class-wise cleanup after yourself.
-
-TestBase.tearDownClass(cls) provides a mechanism to invoke the platform-specific
-cleanup after finishing with a test class. A test class can have more than one
-test methods, so the tearDownClass(cls) method gets run after all the test
-methods have been executed by the test harness.
-
-The default cleanup action performed by the plugins/darwin.py module invokes the
-"make clean" os command.
-
-If this default cleanup is not enough, individual class can provide an extra
-cleanup hook with a class method named classCleanup , for example,
-in test/breakpoint_command/TestBreakpointCommand.py:
-
-@classmethod
-def classCleanup(cls):
-system(["/bin/sh", "-c", "rm -f output.txt"])
-
-The 'output.txt' file gets generated during the test run, so it makes sense to
-explicitly spell out the action in the same TestBreakpointCommand.py file to do
-the cleanup instead of artificially adding it as part of the default cleanup
-action which serves to cleanup those intermediate and a.out files. 
Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -319,6 +319,71 @@
 # Good. Will print expected_string and the contents of 

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 369375.
xgupta added a comment.

remove best-practice.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

Files:
  lldb/docs/index.rst
  lldb/docs/resources/test.rst
  lldb/docs/testsuite/best-practices.txt

Index: lldb/docs/testsuite/best-practices.txt
===
--- lldb/docs/testsuite/best-practices.txt
+++ /dev/null
@@ -1,93 +0,0 @@
-This document attempts to point out some best practices that prove to be helpful
-when building new test cases in the tot/test directory.  Everyone is welcomed to
-add/modify contents into this file.
-
-o Do not use hard-coded line numbers in your test case.  Instead, try to tag the
-  line with some distinguishing pattern, and use the function line_number()
-  defined in lldbtest.py which takes filename and string_to_match as arguments
-  and returns the line number.
-
-As an example, take a look at test/breakpoint_conditions/main.c which has these
-two lines:
-
-return c(val); // Find the line number of c's parent call here.
-
-and
-
-return val + 3; // Find the line number of function "c" here.
-
-The Python test case TestBreakpointConditions.py uses the comment strings to
-find the line numbers during setUp(self) and use them later on to verify that
-the correct breakpoint is being stopped on and that its parent frame also has
-the correct line number as intended through the breakpoint condition.
-
-o Take advantage of the unittest framework's decorator features to properly
-  mark your test class or method for platform-specific tests.
-
-As an example, take a look at test/forward/TestForwardDeclaration.py which has
-these lines:
-
-@unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
-def test_with_dsym_and_run_command(self):
-"""Display *bar_ptr when stopped on a function with forward declaration of struct bar."""
-self.buildDsym()
-self.forward_declaration()
-
-This tells the test harness that unless we are running "darwin", the test should
-be skipped.  This is because we are asking to build the binaries with dsym debug
-info, which is only available on the darwin platforms.
-
-o Cleanup after yourself.  A classic example of this can be found in test/types/
-  TestFloatTypes.py:
-
-def test_float_types_with_dsym(self):
-"""Test that float-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'float.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.float_type()
-
-...
-
-def test_double_type_with_dsym(self):
-"""Test that double-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'double.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.double_type()
-
-This tests different data structures composed of float types to verify that what
-the debugger prints out matches what the compiler does for different variables
-of these types.  We're using a dictionary to pass the build parameters to the
-build system.  After a particular test instance is done, it is a good idea to
-clean up the files built.  This eliminates the chance that some leftover files
-can interfere with the build phase for the next test instance and render it
-invalid.
-
-TestBase.setTearDownCleanup(self, dictionary) defined in lldbtest.py is created
-to cope with this use case by taking the same build parameters in order to do
-the cleanup when we are finished with a test instance, during
-TestBase.tearDown(self).
-
-o Class-wise cleanup after yourself.
-
-TestBase.tearDownClass(cls) provides a mechanism to invoke the platform-specific
-cleanup after finishing with a test class. A test class can have more than one
-test methods, so the tearDownClass(cls) method gets run after all the test
-methods have been executed by the test harness.
-
-The default cleanup action performed by the plugins/darwin.py module invokes the
-"make clean" os command.
-
-If this default cleanup is not enough, individual class can provide an extra
-cleanup hook with a class method named classCleanup , for example,
-in test/breakpoint_command/TestBreakpointCommand.py:
-
-@classmethod
-def classCleanup(cls):
-system(["/bin/sh", "-c", "rm -f output.txt"])
-
-The 'output.txt' file gets generated during the test run, so it makes sense to
-explicitly spell out the action in the same TestBreakpointCommand.py file to do
-the cleanup instead of artificially adding it as part of the default cleanup
-action which serves to cleanup those intermediate and a.out files. 
Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -319,6 +319,71 @@
 # Good. Will print expected_string and 

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added a comment.

In D108812#2971657 , @JDevlieghere 
wrote:

> I'm not convinced this warrants a whole new section. Especially since this is 
> focussed on lldb developers rather than end users. Could we move this under 
> the resources/test page?

Agree. I also think so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

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

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 369374.
xgupta added a comment.

Move testsuite/best-practice comtain to resources/test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

Files:
  lldb/docs/index.rst
  lldb/docs/resources/test.rst
  lldb/docs/testsuite/best-practices.rst
  lldb/docs/testsuite/best-practices.txt

Index: lldb/docs/testsuite/best-practices.txt
===
--- lldb/docs/testsuite/best-practices.txt
+++ /dev/null
@@ -1,93 +0,0 @@
-This document attempts to point out some best practices that prove to be helpful
-when building new test cases in the tot/test directory.  Everyone is welcomed to
-add/modify contents into this file.
-
-o Do not use hard-coded line numbers in your test case.  Instead, try to tag the
-  line with some distinguishing pattern, and use the function line_number()
-  defined in lldbtest.py which takes filename and string_to_match as arguments
-  and returns the line number.
-
-As an example, take a look at test/breakpoint_conditions/main.c which has these
-two lines:
-
-return c(val); // Find the line number of c's parent call here.
-
-and
-
-return val + 3; // Find the line number of function "c" here.
-
-The Python test case TestBreakpointConditions.py uses the comment strings to
-find the line numbers during setUp(self) and use them later on to verify that
-the correct breakpoint is being stopped on and that its parent frame also has
-the correct line number as intended through the breakpoint condition.
-
-o Take advantage of the unittest framework's decorator features to properly
-  mark your test class or method for platform-specific tests.
-
-As an example, take a look at test/forward/TestForwardDeclaration.py which has
-these lines:
-
-@unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
-def test_with_dsym_and_run_command(self):
-"""Display *bar_ptr when stopped on a function with forward declaration of struct bar."""
-self.buildDsym()
-self.forward_declaration()
-
-This tells the test harness that unless we are running "darwin", the test should
-be skipped.  This is because we are asking to build the binaries with dsym debug
-info, which is only available on the darwin platforms.
-
-o Cleanup after yourself.  A classic example of this can be found in test/types/
-  TestFloatTypes.py:
-
-def test_float_types_with_dsym(self):
-"""Test that float-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'float.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.float_type()
-
-...
-
-def test_double_type_with_dsym(self):
-"""Test that double-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'double.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.double_type()
-
-This tests different data structures composed of float types to verify that what
-the debugger prints out matches what the compiler does for different variables
-of these types.  We're using a dictionary to pass the build parameters to the
-build system.  After a particular test instance is done, it is a good idea to
-clean up the files built.  This eliminates the chance that some leftover files
-can interfere with the build phase for the next test instance and render it
-invalid.
-
-TestBase.setTearDownCleanup(self, dictionary) defined in lldbtest.py is created
-to cope with this use case by taking the same build parameters in order to do
-the cleanup when we are finished with a test instance, during
-TestBase.tearDown(self).
-
-o Class-wise cleanup after yourself.
-
-TestBase.tearDownClass(cls) provides a mechanism to invoke the platform-specific
-cleanup after finishing with a test class. A test class can have more than one
-test methods, so the tearDownClass(cls) method gets run after all the test
-methods have been executed by the test harness.
-
-The default cleanup action performed by the plugins/darwin.py module invokes the
-"make clean" os command.
-
-If this default cleanup is not enough, individual class can provide an extra
-cleanup hook with a class method named classCleanup , for example,
-in test/breakpoint_command/TestBreakpointCommand.py:
-
-@classmethod
-def classCleanup(cls):
-system(["/bin/sh", "-c", "rm -f output.txt"])
-
-The 'output.txt' file gets generated during the test run, so it makes sense to
-explicitly spell out the action in the same TestBreakpointCommand.py file to do
-the cleanup instead of artificially adding it as part of the default cleanup
-action which serves to cleanup those intermediate and a.out files. 
Index: lldb/docs/testsuite/best-practices.rst
===
--- /dev/null
+++ 

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

I'm not convinced this warrants a whole new section. Especially since this is 
focussed on lldb developers rather than end users. Could we move this under the 
resources/test page?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

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

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta added inline comments.



Comment at: lldb/docs/testsuite/best-practices.rst:53
+
+Cleanup after yourself
+--

teemperor wrote:
> This section here seems very outdated, can we just delete it?
sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

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

[Lldb-commits] [PATCH] D108812: [LLDB][Docs] Renew best-practices.txt

[Shivam Gupta via Phabricator via lldb-commits]

xgupta updated this revision to Diff 369328.
xgupta added a comment.

Remove an outdated paragraph


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108812

Files:
  lldb/docs/index.rst
  lldb/docs/testsuite/best-practices.rst
  lldb/docs/testsuite/best-practices.txt

Index: lldb/docs/testsuite/best-practices.txt
===
--- lldb/docs/testsuite/best-practices.txt
+++ /dev/null
@@ -1,93 +0,0 @@
-This document attempts to point out some best practices that prove to be helpful
-when building new test cases in the tot/test directory.  Everyone is welcomed to
-add/modify contents into this file.
-
-o Do not use hard-coded line numbers in your test case.  Instead, try to tag the
-  line with some distinguishing pattern, and use the function line_number()
-  defined in lldbtest.py which takes filename and string_to_match as arguments
-  and returns the line number.
-
-As an example, take a look at test/breakpoint_conditions/main.c which has these
-two lines:
-
-return c(val); // Find the line number of c's parent call here.
-
-and
-
-return val + 3; // Find the line number of function "c" here.
-
-The Python test case TestBreakpointConditions.py uses the comment strings to
-find the line numbers during setUp(self) and use them later on to verify that
-the correct breakpoint is being stopped on and that its parent frame also has
-the correct line number as intended through the breakpoint condition.
-
-o Take advantage of the unittest framework's decorator features to properly
-  mark your test class or method for platform-specific tests.
-
-As an example, take a look at test/forward/TestForwardDeclaration.py which has
-these lines:
-
-@unittest2.skipUnless(sys.platform.startswith("darwin"), "requires Darwin")
-def test_with_dsym_and_run_command(self):
-"""Display *bar_ptr when stopped on a function with forward declaration of struct bar."""
-self.buildDsym()
-self.forward_declaration()
-
-This tells the test harness that unless we are running "darwin", the test should
-be skipped.  This is because we are asking to build the binaries with dsym debug
-info, which is only available on the darwin platforms.
-
-o Cleanup after yourself.  A classic example of this can be found in test/types/
-  TestFloatTypes.py:
-
-def test_float_types_with_dsym(self):
-"""Test that float-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'float.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.float_type()
-
-...
-
-def test_double_type_with_dsym(self):
-"""Test that double-type variables are displayed correctly."""
-d = {'CXX_SOURCES': 'double.cpp'}
-self.buildDsym(dictionary=d)
-self.setTearDownCleanup(dictionary=d)
-self.double_type()
-
-This tests different data structures composed of float types to verify that what
-the debugger prints out matches what the compiler does for different variables
-of these types.  We're using a dictionary to pass the build parameters to the
-build system.  After a particular test instance is done, it is a good idea to
-clean up the files built.  This eliminates the chance that some leftover files
-can interfere with the build phase for the next test instance and render it
-invalid.
-
-TestBase.setTearDownCleanup(self, dictionary) defined in lldbtest.py is created
-to cope with this use case by taking the same build parameters in order to do
-the cleanup when we are finished with a test instance, during
-TestBase.tearDown(self).
-
-o Class-wise cleanup after yourself.
-
-TestBase.tearDownClass(cls) provides a mechanism to invoke the platform-specific
-cleanup after finishing with a test class. A test class can have more than one
-test methods, so the tearDownClass(cls) method gets run after all the test
-methods have been executed by the test harness.
-
-The default cleanup action performed by the plugins/darwin.py module invokes the
-"make clean" os command.
-
-If this default cleanup is not enough, individual class can provide an extra
-cleanup hook with a class method named classCleanup , for example,
-in test/breakpoint_command/TestBreakpointCommand.py:
-
-@classmethod
-def classCleanup(cls):
-system(["/bin/sh", "-c", "rm -f output.txt"])
-
-The 'output.txt' file gets generated during the test run, so it makes sense to
-explicitly spell out the action in the same TestBreakpointCommand.py file to do
-the cleanup instead of artificially adding it as part of the default cleanup
-action which serves to cleanup those intermediate and a.out files. 
Index: lldb/docs/testsuite/best-practices.rst
===
--- /dev/null
+++ lldb/docs/testsuite/best-practices.rst
@@ -0,0 +1,75 @@
+Best practices in writing 

[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] [lldb] 54c496d - [lldb] Allow to register frame recognizers applied beyond the first instruction

[Med Ismail Bennani via lldb-commits]

Author: Roman Podoliaka
Date: 2021-08-29T17:28:46+02:00
New Revision: 54c496dad6f28f1ab663f1b30401fe460709d50d

URL: 
https://github.com/llvm/llvm-project/commit/54c496dad6f28f1ab663f1b30401fe460709d50d
DIFF: 
https://github.com/llvm/llvm-project/commit/54c496dad6f28f1ab663f1b30401fe460709d50d.diff

LOG: [lldb] Allow to register frame recognizers applied beyond the first 
instruction

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.

Reviewed By: jingham

Differential Revision: https://reviews.llvm.org/D108510

Added: 


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

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectFrame.cpp 
b/lldb/source/Commands/CommandObjectFrame.cpp
index d90e357bf1aa3..1fd36e65ae151 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionGroupVariable.h"
@@ -739,6 +740,17 @@ class CommandObjectFrameRecognizerAdd : public 
CommandObjectParsed {
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
+  case 'f': {
+bool value, success;
+value = OptionArgParser::ToBoolean(option_arg, true, );
+if (success) {
+  m_first_instruction_only = value;
+} else {
+  error.SetErrorStringWithFormat(
+  "invalid boolean value '%s' passed for -f option",
+  option_arg.str().c_str());
+}
+  } break;
   case 'l':
 m_class_name = std::string(option_arg);
 break;
@@ -763,6 +775,7 @@ class CommandObjectFrameRecognizerAdd : public 
CommandObjectParsed {
   m_symbols.clear();
   m_class_name = "";
   m_regex = false;
+  m_first_instruction_only = true;
 }
 
 llvm::ArrayRef GetDefinitions() override {
@@ -774,6 +787,7 @@ class CommandObjectFrameRecognizerAdd : public 
CommandObjectParsed {
 std::string m_module;
 std::vector m_symbols;
 bool m_regex;
+bool m_first_instruction_only;
   };
 
   CommandOptions m_options;
@@ -883,13 +897,13 @@ bool CommandObjectFrameRecognizerAdd::DoExecute(Args 
,
 auto func =
 RegularExpressionSP(new 
RegularExpression(m_options.m_symbols.front()));
 GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
-recognizer_sp, module, func);
+recognizer_sp, module, func, m_options.m_first_instruction_only);
   } else {
 auto module = ConstString(m_options.m_module);
 std::vector symbols(m_options.m_symbols.begin(),
  m_options.m_symbols.end());
 GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
-recognizer_sp, module, symbols);
+recognizer_sp, module, symbols, m_options.m_first_instruction_only);
   }
 #endif
 

diff  --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index a766d79065f4c..5cef8f93b6989 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -404,6 +404,13 @@ let Command = "frame recognizer add" in {
 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 : 
Option<"first-instruction-only", "f">, Arg<"Boolean">,
+Desc<"If true, only apply this recognizer to frames whose PC currently 
points to the "
+"first instruction of the specified function. If false, the recognizer "
+"will always be applied, regardless of the current position within the 
specified function. The "
+"implementor should keep in mind that some features, e.g. accessing 
function argument "
+"values via $arg, are not guaranteed to work reliably in this case, so 
extra care must "
+"be taken to make the recognizer operate correctly. Defaults to true.">;
 }
 
 let Command = "history" in {

diff  --git a/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py 
b/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
index b025a2255f07b..284aeed701699 100644
--- 

[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] D108233: WIP: Add minidump save-core functionality to ELF object files

[Andrej Korman via Phabricator via lldb-commits]

Aj0SK added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:549
+bool is_interesting = false;
+for (size_t interesting_address : interesting_addresses)
+  if (interesting_address >= addr && interesting_address < addr + size) {

clayborg wrote:
> Shouldn't this be the main loop that would replace the "while 
> (range_info.GetRange().GetRangeBase() != LLDB_INVALID_ADDRESS)" loop? I would 
> think that the main loop wiould be something like:
> 
> ```
> std::set visited_region_base_addresses;
> for (size_t interesting_address : interesting_addresses) {
>   error = process_sp->GetMemoryRegionInfo(interesting_address, range_info);
>   // Skip failed memory region requests or any regions with no permissions.
>   if (error.Fail() || range_info.GetPermissions() == 0)
> continue;
>   const addr_t addr = range_info.GetRange().GetRangeBase();
>   // Skip any regions we have already saved out.
>   if (visited_region_base_addresses.insert(addr).second == false)
> continue;
>   const addr_t size = range_info.GetRange().GetByteSize();
>   if (size == 0)
> continue;
>   auto data_up = std::make_unique(size, 0);
>   const size_t bytes_read = process_sp->ReadMemory(addr, data_up->GetBytes(), 
> size, error);
>   if (bytes_read == 0)
> continue;
>   // We have a good memory region with valid bytes to store.
>   LocationDescriptor memory_dump;
>   memory_dump.DataSize = static_cast(size);
>   memory_dump.RVA 
> =static_cast(GetCurrentDataEndOffset());
>   MemoryDescriptor memory_desc;
>   memory_desc.StartOfMemoryRange = 
> static_cast(addr);
>   memory_desc.Memory = memory_dump;
>   mem_descriptors.push_back(memory_desc);
>   m_data.AppendData(data_up->GetBytes(), bytes_read);
> }
> ```
> 
> 
This kind of an approach was something I also thought about and in fact, I was 
mainly inspired by this [[ 
https://github.com/llvm-mirror/lldb/blob/d01083a850f577b85501a0902b52fd0930de72c7/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp#L6051
 | approach ]]. Also, the previous comment about the need of saving the loaded 
modules addresses in a set doesn't apply to my solution, am I correct? Just to 
clear it a little bit, I like your solution though and it's better when I think 
about it. Thanks for the suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108233

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

[Lldb-commits] [PATCH] D108889: Use dSYM SymbolFile Sections file addr instead of ObjectFile Section file addr when they differ in ObjectFileMachO

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I should note two more things.  First, I make this code which was previously 
checking if the ObjectFile was a memory image unconditional if the dSYM's 
segment file address differs from the ObjectFile's segment file address.  I 
can't see how this is possible in any case but a shared cache -- so I could 
check that the ObjectFile is in the shared cache -- but however we got to this 
point, the only correct thing to do is to use the dSYM (SymbolFile)'s file 
addresses, it's immaterial.

Second, man, I can't think of a way to test this.  You need to be adding a dSYM 
to a binary in the system's shared cache on a darwin system.  We have access to 
the dSYMs for system binaries inside Apple, but we can't use those for 
automated testing in any way, and which dSYM you need depends on the system.  
This is a drag because this regression has snuck past us at least once in the 
past too, looking over the history of the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108889

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

[Lldb-commits] [PATCH] D108889: Use dSYM SymbolFile Sections file addr instead of ObjectFile Section file addr when they differ in ObjectFileMachO

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: clayborg.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
jasonmolenda requested review of this revision.

This is addressing a regression introduced by Greg's change in 
https://reviews.llvm.org/D86122 .  His change was fine, but the original code 
was written unclearly as to what it was doing.

When a binary is in the system's "shared cache", the file addresses will be 
different from the dSYM's file addresses.  The binary (both binary file & dSYM) 
usually start with a file address of 0, but when the binary is integrated into 
the "shared cache" of all common binaries, the segments are rearranged and now 
the TEXT segment has a non-0 file address.  This is pretty unusual normally -- 
when the UUID matches for a binary and its dSYMs, they should have identical 
file addresses for their segments, for example.

When we add a dSYM SymbolFile to an ObjectFile that is in the shared cache, 
we're going to use the symbol table and debug information from the dSYM 
(SymbolFile) -- we need the Sections to have file addresses that match the 
symbol table and debug info in the dSYM.  In other words, the dSYM's file 
addresses must win.

This is handled in ObjectFileMachO::ProcessSegmentCommand.  This was previously 
behind a check of "does this section have a valid base address" or something 
that didn't make sense.  Greg noticed this and changed it to "Is this 
ObjectFile a memory image".  This seems like the right thing to do but 
"IsInMemory()" doesn't apply to the way we create shared cache binaries when 
lldb and the inferior are using the same shared cache.  lldb thinks of these as 
the same as an on-disk ObjectFile that we've mmap'ed into our space, except 
it's contents are represented by a DataBufferUnowned.  With this "file addr 
fixup" code behind a "IsInMemory()" check, now we weren't fixing the file 
addresses and none of the debug info was ever found because the fileaddrs were 
contained by any of the Sections.

I shouldn't intermix changes, but I was also annoyed by how many places we 
check if a binary is in the shared cache by checking flags in ObjectFileMachO, 
and put it in a little method.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108889

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -84,6 +84,8 @@
 
   bool IsDynamicLoader() const;
 
+  bool IsSharedCacheBinary() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1127,6 +1127,10 @@
   return m_header.filetype == MH_DYLINKER;
 }
 
+bool ObjectFileMachO::IsSharedCacheBinary() const {
+  return m_header.flags & MH_DYLIB_IN_CACHE;
+}
+
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
@@ -1377,7 +1381,7 @@
   if (m_length == 0 || seg_cmd.filesize == 0)
 return;
 
-  if ((m_header.flags & MH_DYLIB_IN_CACHE) && !IsInMemory()) {
+  if (IsSharedCacheBinary() && !IsInMemory()) {
 // In shared cache images, the load commands are relative to the
 // shared cache file, and not the specific image we are
 // examining. Let's fix this up so that it looks like a normal
@@ -1675,14 +1679,15 @@
 if (add_to_unified)
   context.UnifiedList.AddSection(segment_sp);
   } else if (unified_section_sp) {
+// If this is a dSYM and the file addresses in the dSYM differ from the
+// file addresses in the ObjectFile, we must use the file base address for
+// the Section from the dSYM for the DWARF to resolve correctly.  This
+// only happens with binaries in the shared cache in practice; normally a
+// mismatch like this would give a binary & dSYM that do not match UUIDs.
+// When a binary is included in the shared cache, its segments are
+// rearranged to optimize the shared cache, so its file addresses will
+// differ from what the ObjectFile had originally, and what the dSYM has.
 if (is_dsym && unified_section_sp->GetFileAddress() != load_cmd.vmaddr) {
-  // Check to see if the module was read from memory?
-  if (module_sp->GetObjectFile()->IsInMemory()) {
-// We have a module that is in memory and needs to have its file
-// address adjusted. We need to do this because when we load a file
-// from memory, its addresses will be slid already, 

[Lldb-commits] [PATCH] D108888: Improve error messaging on process connect errors

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: clayborg.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
jasonmolenda requested review of this revision.

process connect (aka `gdb-remote`) isn't the clearest in its error messaging.  
We have everything we need to distinguish between three common failure modes:  
Nothing is listening to the port (or we could not reach the port), the remote 
side did not respond to the initial handshake packet quickly enough, or the 
remote connection accepts the connection and then drops it immediately (e.g. a 
debugserver might reject the connection because it is only accepting 
connections from certain hosts).

With this patch, we get

  (lldb) gdb-remote 7000
  error: Failed to connect port
  
  (lldb) gdb-remote 4000
  error: failed to get reply to handshake packet within timeout of 6.0 seconds
  
  (lldb) gdb-remote work-imacpro:4000
  error: Connection shut down by remote side while waiting for reply to initial 
handshake packet

and I think it's a little easier to understand the nature of the problem.  Just 
fixing a little thing that's bugged me for a while.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D10

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -82,6 +82,8 @@
 
   // Start the read thread after we send the handshake ack since if we fail to
   // send the handshake ack, there is no reason to continue...
+  std::chrono::steady_clock::time_point start_of_handshake =
+  std::chrono::steady_clock::now();
   if (SendAck()) {
 // Wait for any responses that might have been queued up in the remote
 // GDB server and flush them all
@@ -97,8 +99,24 @@
 if (QueryNoAckModeSupported()) {
   return true;
 } else {
-  if (error_ptr)
-error_ptr->SetErrorString("failed to get reply to handshake packet");
+  std::chrono::steady_clock::time_point end_of_handshake =
+  std::chrono::steady_clock::now();
+  auto handshake_timeout =
+  std::chrono::duration(end_of_handshake - start_of_handshake)
+  .count();
+  if (error_ptr) {
+if (packet_result == PacketResult::ErrorDisconnected)
+  error_ptr->SetErrorString("Connection shut down by remote side "
+"while waiting for reply to initial "
+"handshake packet");
+else if (packet_result == PacketResult::ErrorReplyTimeout)
+  error_ptr->SetErrorStringWithFormat(
+  "failed to get reply to handshake packet within timeout of "
+  "%.1f seconds",
+  handshake_timeout);
+else
+  error_ptr->SetErrorString("failed to get reply to handshake packet");
+  }
 }
   } else {
 if (error_ptr)


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -82,6 +82,8 @@
 
   // Start the read thread after we send the handshake ack since if we fail to
   // send the handshake ack, there is no reason to continue...
+  std::chrono::steady_clock::time_point start_of_handshake =
+  std::chrono::steady_clock::now();
   if (SendAck()) {
 // Wait for any responses that might have been queued up in the remote
 // GDB server and flush them all
@@ -97,8 +99,24 @@
 if (QueryNoAckModeSupported()) {
   return true;
 } else {
-  if (error_ptr)
-error_ptr->SetErrorString("failed to get reply to handshake packet");
+  std::chrono::steady_clock::time_point end_of_handshake =
+  std::chrono::steady_clock::now();
+  auto handshake_timeout =
+  std::chrono::duration(end_of_handshake - start_of_handshake)
+  .count();
+  if (error_ptr) {
+if (packet_result == PacketResult::ErrorDisconnected)
+  error_ptr->SetErrorString("Connection shut down by remote side "
+"while waiting for reply to initial "
+"handshake packet");
+else if (packet_result == PacketResult::ErrorReplyTimeout)
+  error_ptr->SetErrorStringWithFormat(
+  "failed to get reply to handshake packet within timeout of "
+  "%.1f seconds",
+  handshake_timeout);
+else
+  error_ptr->SetErrorString("failed to get reply to handshake packet");
+  }
 }
   } else {
 if (error_ptr)