[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Ayke via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0818e6cf1d30: [LLDB] Add support for AVR breakpoints 
(authored by aykevl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255

Files:
  lldb/source/Target/Platform.cpp


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1865,6 +1865,12 @@
 }
   } break;
 
+  case llvm::Triple::avr: {
+static const uint8_t g_hex_opcode[] = {0x98, 0x95};
+trap_opcode = g_hex_opcode;
+trap_opcode_size = sizeof(g_hex_opcode);
+  } break;
+
   case llvm::Triple::mips:
   case llvm::Triple::mips64: {
 static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d};


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1865,6 +1865,12 @@
 }
   } break;
 
+  case llvm::Triple::avr: {
+static const uint8_t g_hex_opcode[] = {0x98, 0x95};
+trap_opcode = g_hex_opcode;
+trap_opcode_size = sizeof(g_hex_opcode);
+  } break;
+
   case llvm::Triple::mips:
   case llvm::Triple::mips64: {
 static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d};
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Pavel Labath via Phabricator via lldb-commits]

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

> As Dylan already mentioned, the opcode is indeed for the BREAK instruction.

Ok, let's go with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

In D74255#1871958 , @labath wrote:

> However, if we look at this locally, if the AVR architecture has a trap 
> opcode (maybe to implement `__builtin_debugbreak()` -- I am assuming that's 
> what 0x98 0x95 is), then I don't see a problem with this function returning 
> it.


As Dylan already mentioned, the opcode is indeed for the BREAK instruction. 
Here it is:
https://github.com/llvm/llvm-project/blob/release/9.x/llvm/test/MC/AVR/inst-break.s
https://www.microchip.com/webdoc/avrassembler/avrassembler.wb_BREAK.html

The documentation even states:

> The BREAK instruction is used by the On-Chip Debug system, and is normally 
> not used in the application software.

I would argue that even though the direct need for this patch may be to work 
around a limitation elsewhere, it is a useful change on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Dylan McKay via Phabricator via lldb-commits]

dylanmckay added a comment.

> However, if we look at this locally, if the AVR architecture has a trap 
> opcode (maybe to implement __builtin_debugbreak() -- I am assuming that's 
> what 0x98 0x95 is), then I don't see a problem with this function returning 
> it.

Chiming in from an AVR perspective, the AVR does have a trap opcode (`BREAK`) 
and this is encoded by 0x9895

> The BREAK instruction is used by the On-chip Debug system, and is normally 
> not used in the application
> software. When the BREAK instruction is executed, the AVR CPU is set in the 
> Stopped Mode. This gives the
>  On-chip Debugger access to internal resources.
> 
> If any Lock bits are set, or either the JTAGEN or OCDEN Fuses are 
> unprogrammed, the CPU will treat the
>  BREAK instruction as a NOP and will not enter the Stopped mode.
> 
> This instruction is not available in all devices. Refer to the device 
> specific instruction set summary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The whole flow here is pretty nonsensical -- the only reason we ask for the 
opcode is to get its size so we can put it into the Z0 packet -- however this 
is only needed for targets like arm, which have mutiple ISAs/opcodes, and we've 
already gotten complaints about sending that unconditionally.

However, if we look at this locally, if the AVR architecture has a trap opcode 
(maybe to implement `__builtin_debugbreak()` -- I am assuming that's what 0x98 
0x95 is), then I don't see a problem with this function returning it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Might also be good to fix the crash.  If you don't support software 
breakpoints, you shouldn't get asked what your breakpoint opcode is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

We have infrastructure to "mock" a gdb-remote server (see 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/) to test 
lldb's interaction with it.

For a simple patch like this, I don't think a test is really required 
(especially as they are not the easiest tests to write), but OTOH, you are 
going to need this sooner or later, so now may be a good time to try it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74255



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

[Lldb-commits] [PATCH] D74255: [LLDB] Add support for AVR breakpoints

[Ayke via Phabricator via lldb-commits]

aykevl created this revision.
aykevl added reviewers: labath, clayborg.
Herald added subscribers: lldb-commits, Jim, dylanmckay.
Herald added a project: LLDB.

I believe the actual opcode does not matter because the AVR architecture is a 
Harvard architecture that does not support writing to program memory. 
Therefore, debuggers and emulators provide hardware breakpoints. But for some 
reason, this opcode must be defined or else LLDB will crash with an assertion 
error.

---

I would like to add a test case for this, but I'm not quite sure how to do 
that. It appears that to trigger the crash (fixed by this patch), there needs 
to be a `gdb-remote`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74255

Files:
  lldb/source/Target/Platform.cpp


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1855,6 +1855,12 @@
 }
   } break;
 
+  case llvm::Triple::avr: {
+static const uint8_t g_hex_opcode[] = {0x98, 0x95};
+trap_opcode = g_hex_opcode;
+trap_opcode_size = sizeof(g_hex_opcode);
+  } break;
+
   case llvm::Triple::mips:
   case llvm::Triple::mips64: {
 static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d};


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1855,6 +1855,12 @@
 }
   } break;
 
+  case llvm::Triple::avr: {
+static const uint8_t g_hex_opcode[] = {0x98, 0x95};
+trap_opcode = g_hex_opcode;
+trap_opcode_size = sizeof(g_hex_opcode);
+  } break;
+
   case llvm::Triple::mips:
   case llvm::Triple::mips64: {
 static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d};
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits