[Lldb-commits] [PATCH] D91508: [LLDB/Lua] add support for one-liner breakpoint callback

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D91508#2412510 , @tammela wrote:

>> I'm not sure what you mean by that. Can you elaborate?
>
> Sure, it's was just a nod to your previous comment about that running 
> callbacks (C++ lambdas) inside a `pcall` is a dangerous API. Although 
> possible, it requires that the developer be very cautious about implicit 
> throws.

Right. That's why I'd like to have good wrappers, which make it easy to do the 
right thing, and hard to do the wrong one.

I don't think we're quite there yet, but before I comment on the API, I want to 
understand one other thing.

I am puzzled by all the wrapping that's happening inside the `PushSBClass` 
functions. What is that protecting us from? I would hope that pushing a swig 
wrapper on the stack is a safe operation...

> The Lua implementation guarantees at least 20 stack slots when the 
> `lua_State` is created so I've added the stack checks for sanity rather than 
> necessity.

So, IIUC, this can only fail if we are running out of memory? If that's the 
case, then I would remove these checks, as (for better or worse) llvm is not 
robust against memory allocation errors, and they add a fair amount of cruft to 
the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508

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

[Lldb-commits] [PATCH] D91963: [lldb] [test/Register] Initial tests for regsets in core dumps [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I can't tell the core sizes from the diff, but my recollection is that even 
non-freebsd core files are larger than one would like. Even with linux, ones 
has to go to great lengths to get it to create a small one.

I'd recommend manually splicing the core to only include the data relevant for 
the tests. I just tried the process on a random core file and it was pretty 
straight-forward. What I did was:

- change the `e_phnum` to one (the NOTE segment was the first segment in the 
headers)
- delete everything after the end of the NOTE segment (the NOTE segment data 
was the first)

Lldb was perfectly happy to open the resulting core file, but I've reduced its 
size by more than 99%. The file still contains remnants of the discarded 
program headers, but they are harmless.

It looks like a similar approach could be applied to NetBSD core files. It 
seems NetBSD places the NOTE segment last in the program headers (not in the 
file), so one would also need to adjust e_phoff to point to it. If FreeBSD puts 
the NOTE segment last in the file, then one would also need to adjust 
`p_offset` in the program header, and splice data out of the middle of the 
file, which is slightly trickier, but not impossible. (I've also successfully 
spliced a single note (NT_FILE) out of the note segment, and it was relatively 
straight-forward, but probably unnecessary).


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

https://reviews.llvm.org/D91963

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

[Lldb-commits] [PATCH] D91963: [lldb] [test/Register] Initial tests for regsets in core dumps [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Come to think of it, given the number of core files you have amassed, it might 
not be unreasonable to turn this process into a shell script (in python, in 
shell with readelf+grep+dd, or whatever). Then we could check that in somewhere 
and ask people to run it whenever they want to contribute a core file (for 
register reading purposes, at least).


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

https://reviews.llvm.org/D91963

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

[Lldb-commits] [PATCH] D91508: [LLDB/Lua] add support for one-liner breakpoint callback

[Pedro Tammela via Phabricator via lldb-commits]

tammela marked an inline comment as done.
tammela added a comment.

In D91508#2413582 , @labath wrote:

> Right. That's why I'd like to have good wrappers, which make it easy to do 
> the right thing, and hard to do the wrong one.
>
> I don't think we're quite there yet, but before I comment on the API, I want 
> to understand one other thing.
>
> I am puzzled by all the wrapping that's happening inside the `PushSBClass` 
> functions. What is that protecting us from? I would hope that pushing a swig 
> wrapper on the stack is a safe operation...

I thought that too, but internally it's a naked call to `lua_newuserdata()` 
which might throw in case of a memory error.

> So, IIUC, this can only fail if we are running out of memory? If that's the 
> case, then I would remove these checks, as (for better or worse) llvm is not 
> robust against memory allocation errors, and they add a fair amount of cruft 
> to the code.

Fair enough. Will remove those.
Since this seems to be a fact of life for LLVM, perhaps wrapping potential 
memory errors turns out to be just bloat. If that's the case, then the wrapping 
in `PushSBClass` is not needed and the `abort()` call that Lua does is honest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508

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

[Lldb-commits] [PATCH] D91634: [lldb] Error when there are no ports to launch a gdbserver on

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 307356.
DavidSpickett added a comment.
Herald added a subscriber: mgorny.

- Move PortMap into a class we can unittest
- Use llvm::Expected for GetNextAvailablePort


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91634

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
  lldb/tools/lldb-server/lldb-platform.cpp
  lldb/unittests/tools/lldb-server/tests/CMakeLists.txt
  lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp

Index: lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp
===
--- /dev/null
+++ lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp
@@ -0,0 +1,117 @@
+//===-- PortMapTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h"
+
+using namespace lldb_private::process_gdb_remote;
+
+TEST(PortMapTest, Constructors) {
+  // Default construct to empty map
+  GDBRemoteCommunicationServerPlatform::PortMap p1;
+  ASSERT_TRUE(p1.empty());
+
+  // Empty means no restrictions, return 0 and and bind to get a port
+  llvm::Expected available_port = p1.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(0, *available_port);
+
+  // Adding any port makes it not empty
+  p1.AllowPort(1);
+  ASSERT_FALSE(p1.empty());
+
+  // So we will return the added port this time
+  available_port = p1.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(1, *available_port);
+
+  // Construct from a range of ports
+  GDBRemoteCommunicationServerPlatform::PortMap p2(1, 4);
+  ASSERT_FALSE(p2.empty());
+
+  // Use up all the ports
+  for (uint16_t expected = 1; expected < 4; ++expected) {
+available_port = p2.GetNextAvailablePort();
+ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+EXPECT_EQ(expected, *available_port);
+p2.AssociatePortWithProcess(*available_port, 1);
+  }
+
+  // Now we fail since we're not an empty port map but all ports are used
+  available_port = p2.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Failed());
+}
+
+TEST(PortMapTest, FreePort) {
+  GDBRemoteCommunicationServerPlatform::PortMap p(1, 4);
+  // Use up all the ports
+  for (uint16_t port = 1; port < 4; ++port) {
+p.AssociatePortWithProcess(port, 1);
+  }
+
+  llvm::Expected available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Failed());
+
+  // Can't free a port that isn't in the map
+  ASSERT_FALSE(p.FreePort(0));
+  ASSERT_FALSE(p.FreePort(4));
+
+  // After freeing a port it becomes available
+  ASSERT_TRUE(p.FreePort(2));
+  available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(2, *available_port);
+}
+
+TEST(PortMapTest, FreePortForProcess) {
+  GDBRemoteCommunicationServerPlatform::PortMap p;
+  p.AllowPort(1);
+  p.AllowPort(2);
+  ASSERT_TRUE(p.AssociatePortWithProcess(1, 11));
+  ASSERT_TRUE(p.AssociatePortWithProcess(2, 22));
+
+  // All ports have been used
+  llvm::Expected available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Failed());
+
+  // Can't free a port for a process that doesn't have any
+  ASSERT_FALSE(p.FreePortForProcess(33));
+
+  ASSERT_TRUE(p.FreePortForProcess(22));
+  available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(2, *available_port);
+
+  // proces 22 no longer has a port
+  ASSERT_FALSE(p.FreePortForProcess(22));
+}
+
+TEST(PortMapTest, AllowPort) {
+  GDBRemoteCommunicationServerPlatform::PortMap p;
+
+  // Allow port 1 and tie it to process 11
+  p.AllowPort(1);
+  ASSERT_TRUE(p.AssociatePortWithProcess(1, 11));
+
+  // Allowing it a second time shouldn't change existing mapping
+  p.AllowPort(1);
+  llvm::Expected available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Failed());
+
+  // A new port is marked as free when allowed
+  p.AllowPort(2);
+  available_port = p.GetNextAvailablePort();
+  ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded());
+  ASSERT_EQ(2, *available_port);
+
+  // 11 should still be tied to port 1
+  ASSERT_TRUE(p.FreePortForProcess(11));
+}
Index: lldb/un

[Lldb-commits] [PATCH] D92035: [lldb] Use llvm::Optional for port in LaunchGDBServer

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
DavidSpickett requested review of this revision.
Herald added a subscriber: JDevlieghere.

Previously we used UINT16_MAX to mean no port/no specifc
port. This leads to confusion because 65535 is a valid
port number.

Instead use an optional. If you want a specific port call
LaunchGDBServer as normal, otherwise pass an empty optional
and it will be set to the port that gets chosen.
(or left empty in the case where we fail to find a port)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92035

Files:
  
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
  lldb/tools/lldb-server/lldb-platform.cpp

Index: lldb/tools/lldb-server/lldb-platform.cpp
===
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -350,13 +350,13 @@
 if (platform.IsConnected()) {
   if (inferior_arguments.GetArgumentCount() > 0) {
 lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
-uint16_t port = 0;
+llvm::Optional port = 0;
 std::string socket_name;
 Status error = platform.LaunchGDBServer(inferior_arguments,
 "", // hostname
 pid, port, socket_name);
 if (error.Success())
-  platform.SetPendingGdbServer(pid, port, socket_name);
+  platform.SetPendingGdbServer(pid, *port, socket_name);
 else
   fprintf(stderr, "failed to start gdbserver: %s\n", error.AsCString());
   }
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
@@ -17,6 +17,7 @@
 #include "lldb/Host/Socket.h"
 
 #include "llvm/Support/Error.h"
+#include "llvm/ADT/Optional.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
@@ -65,8 +66,10 @@
 
   void SetInferiorArguments(const lldb_private::Args &args);
 
+  // Set port if you want to use a specific port number.
+  // Otherwise port will be set to the port that was chosen for you.
   Status LaunchGDBServer(const lldb_private::Args &args, std::string hostname,
- lldb::pid_t &pid, uint16_t &port,
+ lldb::pid_t &pid, llvm::Optional &port,
  std::string &socket_name);
 
   void SetPendingGdbServer(lldb::pid_t pid, uint16_t port,
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
@@ -157,8 +157,8 @@
 
 Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer(
 const lldb_private::Args &args, std::string hostname, lldb::pid_t &pid,
-uint16_t &port, std::string &socket_name) {
-  if (port == UINT16_MAX) {
+llvm::Optional &port, std::string &socket_name) {
+  if (!port) {
 llvm::Expected available_port = m_port_map.GetNextAvailablePort();
 if (available_port)
   port = *available_port;
@@ -179,7 +179,7 @@
 
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM));
   LLDB_LOGF(log, "Launching debugserver with: %s:%u...", hostname.c_str(),
-port);
+*port);
 
   // Do not run in a new session so that it can not linger after the platform
   // closes.
@@ -194,7 +194,7 @@
 #if !defined(__APPLE__)
   url << m_socket_scheme << "://";
 #endif
-  uint16_t *port_ptr = &port;
+  uint16_t *port_ptr = port.getPointer();
   if (m_socket_protocol == Socket::ProtocolTcp) {
 llvm::StringRef platform_scheme;
 llvm::StringRef platform_ip;
@@ -205,7 +205,7 @@
platform_port, platform_path);
 UNUSED_IF_ASSERT_DISABLED(ok);
 assert(ok);
-url << platform_ip.str() << ":" << port;
+url << platform_ip.str() << ":" << *port;
   } else {
 socket_name = GetDomainSocketPath("gdbserver").GetPath();
 url << socket_name;
@@ -219,11 +219,11 @@
   if (pid != LLDB_INVALID_PROCESS_ID) {
 std::lock_guard guard(m_spawned_pids_mutex);
 m_spawned_pids.insert(pid);
-if (port > 0)
-  m_port_map.AssociatePortWithProcess(port, pid);
+if (*port > 0)
+  m_port_map.AssociatePortWithProcess(*port, pid);
   } else {
-if (port > 0)
-  m_port_map.FreePort(port);
+if (*port > 0)
+  m_port_map.FreePort(*port);
   }
   return error;
 }
@@ -243,12 +243,15 @@
   packet.SetFilePos(::strlen("qLaunchGDBServer;"));
   llv

[Lldb-commits] [PATCH] D92035: [lldb] Use llvm::Optional for port in LaunchGDBServer

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp:197
 #endif
-  uint16_t *port_ptr = &port;
+  uint16_t *port_ptr = port.getPointer();
   if (m_socket_protocol == Socket::ProtocolTcp) {

I know this is a bit unorthodox but things got messy trying to pass an Optional 
into StartDebugServerProcess. So I figured it's better to keep 
StartDebugServerProcess the same for now and limit the scope of any mistakes. 
(the logic gets quite hard to follow)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92035

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

[Lldb-commits] [PATCH] D91634: [lldb] Error when there are no ports to launch a gdbserver on

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:30
 public:
-  using PortMap = std::map;
   using PacketHandler =

Only the one in GDBRemoteCommunicationServerPlatform is/was used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91634

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

[Lldb-commits] [PATCH] D91508: [LLDB/Lua] add support for one-liner breakpoint callback

[Pedro Tammela via Phabricator via lldb-commits]

tammela updated this revision to Diff 307440.
tammela added a comment.

Remove unnecessary wrappers/checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508

Files:
  lldb/bindings/lua/lua-swigsafecast.swig
  lldb/bindings/lua/lua-wrapper.swig
  lldb/bindings/lua/lua.swig
  lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
  lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
  lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
  lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
  lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp

Index: lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
+++ lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
@@ -13,6 +13,30 @@
 
 extern "C" int luaopen_lldb(lua_State *L) { return 0; }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
+
+// Disable warning C4190: 'LLDBSwigPythonBreakpointCallbackFunction' has
+// C-linkage specified, but returns UDT 'llvm::Expected' which is
+// incompatible with C
+#if _MSC_VER
+#pragma warning (push)
+#pragma warning (disable : 4190)
+#endif
+
+extern "C" llvm::Expected
+LLDBSwigLuaBreakpointCallbackFunction(lua_State *L, void *baton,
+  lldb::StackFrameSP &stop_frame_sp,
+  lldb::BreakpointLocationSP &bp_loc_sp) {
+  return false;
+}
+
+#if _MSC_VER
+#pragma warning (pop)
+#endif
+
+#pragma clang diagnostic pop
+
 TEST(LuaTest, RunValid) {
   Lua lua;
   llvm::Error error = lua.Run("foo = 1");
Index: lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
@@ -0,0 +1,7 @@
+# REQUIRES: lua
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
+# RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
+b main
+breakpoint command add -s lua -o 'print(frame)'
+run
+# CHECK: frame #0
Index: lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
+++ lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h
@@ -10,11 +10,20 @@
 #define liblldb_ScriptInterpreterLua_h_
 
 #include "lldb/Interpreter/ScriptInterpreter.h"
+#include "lldb/Utility/Status.h"
+#include "lldb/lldb-enumerations.h"
 
 namespace lldb_private {
 class Lua;
 class ScriptInterpreterLua : public ScriptInterpreter {
 public:
+  class CommandDataLua : public BreakpointOptions::CommandData {
+  public:
+CommandDataLua() : BreakpointOptions::CommandData() {
+  interpreter = lldb::eScriptLanguageLua;
+}
+  };
+
   ScriptInterpreterLua(Debugger &debugger);
 
   ~ScriptInterpreterLua() override;
@@ -41,6 +50,11 @@
 
   static const char *GetPluginDescriptionStatic();
 
+  static bool BreakpointCallbackFunction(void *baton,
+ StoppointCallbackContext *context,
+ lldb::user_id_t break_id,
+ lldb::user_id_t break_loc_id);
+
   // PluginInterface protocol
   lldb_private::ConstString GetPluginName() override;
 
@@ -51,6 +65,9 @@
   llvm::Error EnterSession(lldb::user_id_t debugger_id);
   llvm::Error LeaveSession();
 
+  Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
+  const char *command_body_text) override;
+
 private:
   std::unique_ptr m_lua;
   bool m_session_is_active = false;
Index: lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
@@ -8,14 +8,17 @@
 
 #include "ScriptInterpreterLua.h"
 #include "Lua.h"
+#include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Target/ExecutionContext.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/FormatAdapters.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -174,6 +177,49 @@
   return m_lua->Run(str);
 }
 
+bool ScriptInterpreterLua::BreakpointCallbackFunction(
+void *baton, StoppointCallbackContext *context, user_id_t break_id,
+user_id_t break_loc_id) {
+  assert(context);
+
+  ExecutionContext exe_ctx

[Lldb-commits] [PATCH] D92063: [LLDB] RegisterInfoPOSIX_arm64 remove unused bytes from g/G packet

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid created this revision.
omjavaid added a reviewer: labath.
Herald added subscribers: kristof.beyls, emaste.
omjavaid requested review of this revision.

This came up while putting together our new strategy to create g/G packets 
where register offsets are calculated in increasing order of register numbers 
without any unused spacing. RegisterInfoPOSIX_arm64::GPR size was being 
calculated after alignment correction to 8 bytes which meant there 4 bytes 
unused space between last gpr (cpsr) and first vector register V. To remove any 
ambiguity I have placed a 4 byte pad at the end of RegisterInfoPOSIX_arm64::GPR 
and also subtracted the same from any offset calculation to avoid any unused 
fragment in g/G packet which will eventually break our offset calculation 
algorithm.


https://reviews.llvm.org/D92063

Files:
  
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h

Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
@@ -36,6 +36,7 @@
 uint64_t sp;// x31
 uint64_t pc;// pc
 uint32_t cpsr;  // cpsr
+uint32_t pad;   // 4-byte pad for 8-byte alignment
   };
 
   // based on RegisterContextDarwin_arm64.h
Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
@@ -15,15 +15,20 @@
 
 #include "RegisterInfoPOSIX_arm64.h"
 
+#define GPR_PAD_BYTES_ARM64 sizeof(uint32_t)
+#define GPR_REGS_SIZE_ARM64\
+  (sizeof(RegisterInfoPOSIX_arm64::GPR) - GPR_PAD_BYTES_ARM64)
+#define FPU_REGS_SIZE_ARM64 sizeof(RegisterInfoPOSIX_arm64::FPU)
+#define EXC_REGS_SIZE_ARM64 sizeof(RegisterInfoPOSIX_arm64::EXC)
 // Based on RegisterContextDarwin_arm64.cpp
 #define GPR_OFFSET(idx) ((idx)*8)
 #define GPR_OFFSET_NAME(reg)   \
   (LLVM_EXTENSION offsetof(RegisterInfoPOSIX_arm64::GPR, reg))
 
-#define FPU_OFFSET(idx) ((idx)*16 + sizeof(RegisterInfoPOSIX_arm64::GPR))
+#define FPU_OFFSET(idx) ((idx)*16 + GPR_REGS_SIZE_ARM64)
 #define FPU_OFFSET_NAME(reg)   \
   (LLVM_EXTENSION offsetof(RegisterInfoPOSIX_arm64::FPU, reg) +\
-   sizeof(RegisterInfoPOSIX_arm64::GPR))
+   GPR_REGS_SIZE_ARM64)
 
 // This information is based on AArch64 with SVE architecture reference manual.
 // AArch64 with SVE has 32 Z and 16 P vector registers. There is also an FFR
@@ -39,19 +44,16 @@
 // vector length requires register context to update sizes of SVE Z, P and FFR.
 // Also register context needs to update byte offsets of all registers affected
 // by the change in vector length.
-#define SVE_REGS_DEFAULT_OFFSET_LINUX sizeof(RegisterInfoPOSIX_arm64::GPR)
+#define SVE_REGS_DEFAULT_OFFSET_LINUX GPR_REGS_SIZE_ARM64
 
 #define SVE_OFFSET_VG SVE_REGS_DEFAULT_OFFSET_LINUX
 
 #define EXC_OFFSET_NAME(reg)   \
   (LLVM_EXTENSION offsetof(RegisterInfoPOSIX_arm64::EXC, reg) +\
-   sizeof(RegisterInfoPOSIX_arm64::GPR) +  \
-   sizeof(RegisterInfoPOSIX_arm64::FPU))
+   GPR_REGS_SIZE_ARM64 + FPU_REGS_SIZE_ARM64)
 #define DBG_OFFSET_NAME(reg)   \
   (LLVM_EXTENSION offsetof(RegisterInfoPOSIX_arm64::DBG, reg) +\
-   sizeof(RegisterInfoPOSIX_arm64::GPR) +  \
-   sizeof(RegisterInfoPOSIX_arm64::FPU) +  \
-   sizeof(RegisterInfoPOSIX_arm64::EXC))
+   GPR_REGS_SIZE_ARM64 + FPU_REGS_SIZE_ARM64 + EXC_REGS_SIZE_ARM64)
 
 #define DEFINE_DBG(reg, i) \
   #reg, NULL,  \
@@ -62,9 +64,7 @@
dbg_##reg##i }, \
NULL, NULL, NULL, 0
 #define REG_CONTEXT_SIZE   \
-  (sizeof(RegisterInfoPOSIX_arm64::GPR) +  \
-   sizeof(RegisterInfoPOSIX_arm64::FPU) +  \
-   sizeof(RegisterInfoPOSIX_arm64::EXC))
+  (GPR_REGS_SIZE_ARM64 + FPU_REGS_SIZE_ARM64 + EXC_REGS_SIZE_ARM64)
 
 // Include RegisterInfos_arm64 to declare our g_reg