[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

[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] D73961: [LLDB] Addresses can be two bytes in size

[Ayke via Phabricator via lldb-commits]

aykevl abandoned this revision.
aykevl added a comment.

Closing this one. Maybe there is something useful in this patch but if there 
is, I'll submit that in a standalone (more focused) patch. The majority of it 
has been merged in D73969 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73961



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

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969



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

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGec1efe71130f: [LLDB] Let DataExtractor deal with two-byte 
addresses (authored by aykevl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -112,6 +112,39 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
+  DataExtractor E2(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+  DataExtractor E5(buffer, sizeof buffer, lldb::eByteOrderLittle, 5);
+  DataExtractor E7(buffer, sizeof buffer, lldb::eByteOrderLittle, 7);
+
+  lldb::offset_t offset;
+
+  // Test 2-byte addresses (for AVR).
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetAddress());
+  EXPECT_EQ(2U, offset);
+
+  // Test 5-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x030201U, E5.GetMaxU64(, 3));
+  EXPECT_EQ(3U, offset);
+  offset = 3;
+  EXPECT_EQ(0x0807060504U, E5.GetAddress());
+  EXPECT_EQ(8U, offset);
+
+  // Test 7-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x0504030201U, E7.GetMaxU64(, 5));
+  EXPECT_EQ(5U, offset);
+  offset = 0;
+  EXPECT_EQ(0x07060504030201U, E7.GetAddress());
+  EXPECT_EQ(7U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = data.m_addr_size;
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   // If "data" contains shared pointer to data, then we can use that
   if (data.m_data_sp) {
 m_byte_order = data.m_byte_order;
@@ -680,12 +680,12 @@
 //
 // RETURNS the address that was extracted, or zero on failure.
 uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 
 uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64_unchecked(offset_ptr, m_addr_size);
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

In D73969#1860696 , @labath wrote:

> I agree only one of those two places should be enough. My idea was to make 
> the constructors delegate to one another (if necessary by creating a private 
> uber-constructor that everybody can delegate to). Then we could have only one 
> assert.
>
> But if that's hard for some reason, your proposal seems fine too.


Honestly I don't feel very comfortable changing the constructors. I'm not very 
experienced with C++ and I'm afraid I mess something up. I could remove them 
from all constructors, reducing the number of asserts from 7 to 3?


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

https://reviews.llvm.org/D73969



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

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl updated this revision to Diff 246443.
aykevl added a comment.

Rebase on master (after `GetPointer` was removed).


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

https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -112,6 +112,39 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
+  DataExtractor E2(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+  DataExtractor E5(buffer, sizeof buffer, lldb::eByteOrderLittle, 5);
+  DataExtractor E7(buffer, sizeof buffer, lldb::eByteOrderLittle, 7);
+
+  lldb::offset_t offset;
+
+  // Test 2-byte addresses (for AVR).
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetAddress());
+  EXPECT_EQ(2U, offset);
+
+  // Test 5-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x030201U, E5.GetMaxU64(, 3));
+  EXPECT_EQ(3U, offset);
+  offset = 3;
+  EXPECT_EQ(0x0807060504U, E5.GetAddress());
+  EXPECT_EQ(8U, offset);
+
+  // Test 7-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x0504030201U, E7.GetMaxU64(, 5));
+  EXPECT_EQ(5U, offset);
+  offset = 0;
+  EXPECT_EQ(0x07060504030201U, E7.GetAddress());
+  EXPECT_EQ(7U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = data.m_addr_size;
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   // If "data" contains shared pointer to data, then we can use that
   if (data.m_data_sp) {
 m_byte_order = data.m_byte_order;
@@ -680,12 +680,12 @@
 //
 // RETURNS the address that was extracted, or zero on failure.
 uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 
 uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64_unchecked(offset_ptr, m_addr_size);
 }
 
___
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

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

>> Right now there are asserts both when constructing/copying(?) the object (5 
>> asserts) and at the place where `m_addr_size` is used (3 asserts). I would 
>> propose picking one place, such as where it is used. That would reduce the 
>> number of asserts to 3 and keep them close together. What do you think?
> 
> I agree only one of those two places should be enough. My idea was to make 
> the constructors delegate to one another (if necessary by creating a private 
> uber-constructor that everybody can delegate to). Then we could have only one 
> assert.

Should I do that in the same patch, or in a different one? Before or after this 
patch? It seems like doing the constructor merging in this patch would make it 
more complicated than necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969



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

[Lldb-commits] [PATCH] D73961: [LLDB] Addresses can be two bytes in size

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

> How do you normally generate the binaries with these kinds of addresses? Can 
> they be produced with clang? Can they be produced with llvm-mc? Linked with 
> lld ?

With `avr-gcc`. I think it's easiest to just generate a minimal binary using 
`yaml2obj`.

For the rest I'm focusing on D73969  now, this 
patch should probably be abandoned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73961



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

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl updated this revision to Diff 242694.
aykevl set the repository for this revision to rG LLVM Github Monorepo.
aykevl added a comment.

> I do have one question though. Will the DataExtractor actually do something 
> reasonable for non-power-of-2 sizes (5,6,7) ? If yes, then great -- if not, 
> we should keep the assert as `2 || 4 || 8`.

I checked, and the only places it is actually used is in `GetAddress`, 
`GetAddress_unchecked`, and `GetPointer`. All those end up at `GetMaxU64` (or 
the unchecked variant) which seems to take care of odd pointer lengths. I have 
added a test case to make sure this will keep working.

> Since we are doing the same test all over `m_addr_size >= 1 && m_addr_size <= 
> 8` can we just make it a function and avoid the repetition and potential 
> erroneous updating later on that does not fix them all?

Right now there are asserts both when constructing/copying(?) the object (5 
asserts) and at the place where `m_addr_size` is used (3 asserts). I would 
propose picking one place, such as where it is used. That would reduce the 
number of asserts to 3 and keep them close together. What do you think?

Sidenote: `GetAddress` and `GetPointer` are the same function but with a 
different name. Even the doc comment right before is almost identical. I tried 
tracing back where they came from and they can both be traced back to the 
initial LLDB checin from Apple 
.
 Maybe this is worth deduplicating eventually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -112,6 +112,39 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
+  DataExtractor E2(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+  DataExtractor E5(buffer, sizeof buffer, lldb::eByteOrderLittle, 5);
+  DataExtractor E7(buffer, sizeof buffer, lldb::eByteOrderLittle, 7);
+
+  lldb::offset_t offset;
+
+  // Test 2-byte addresses (for AVR).
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E2.GetAddress());
+  EXPECT_EQ(2U, offset);
+
+  // Test 5-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x030201U, E5.GetMaxU64(, 3));
+  EXPECT_EQ(3U, offset);
+  offset = 3;
+  EXPECT_EQ(0x0807060504U, E5.GetAddress());
+  EXPECT_EQ(8U, offset);
+
+  // Test 7-byte addresses.
+  offset = 0;
+  EXPECT_EQ(0x0504030201U, E7.GetMaxU64(, 5));
+  EXPECT_EQ(5U, offset);
+  offset = 0;
+  EXPECT_EQ(0x07060504030201U, E7.GetAddress());
+  EXPECT_EQ(7U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = 

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl updated this revision to Diff 242340.
aykevl added a comment.

Added a unit test.

I think it would also be a good idea to test this by loading an AVR binary and 
connecting to a debugger (thus triggering this code), but I don't know how to 
add such a test. Suggestions would be appreciated.


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

https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp


Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -103,6 +103,20 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+
+  lldb::offset_t offset;
+
+  offset = 0;
+  EXPECT_EQ(0x01U, E.GetMaxU64(, 1));
+  EXPECT_EQ(1U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + 
length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = data.m_addr_size;
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   // If "data" contains shared pointer to data, then we can use that
   if (data.m_data_sp) {
 m_byte_order = data.m_byte_order;
@@ -679,12 +679,12 @@
 //
 // RETURNS the address that was extracted, or zero on failure.
 uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 
 uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64_unchecked(offset_ptr, m_addr_size);
 }
 
@@ -695,7 +695,7 @@
 //
 // RETURNS the pointer that was extracted, or zero on failure.
 uint64_t DataExtractor::GetPointer(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 


Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -103,6 +103,20 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+
+  lldb::offset_t offset;
+
+  offset = 0;
+  EXPECT_EQ(0x01U, E.GetMaxU64(, 1));
+  EXPECT_EQ(1U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: 

[Lldb-commits] [PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

[Ayke via Phabricator via lldb-commits]

aykevl created this revision.
aykevl added reviewers: labath, dylanmckay, Andrzej.
aykevl added projects: LLVM, LLDB.
Herald added subscribers: lldb-commits, aprantl.
aykevl updated this revision to Diff 242340.
aykevl added a comment.

Added a unit test.

I think it would also be a good idea to test this by loading an AVR binary and 
connecting to a debugger (thus triggering this code), but I don't know how to 
add such a test. Suggestions would be appreciated.


AVR usually uses two byte addresses. By making DataExtractor deal with this, it 
is possible to load AVR binaries that don't have debug info associated with 
them.

---

This is a more focused version of D73961 . 
With this, I can load an avr-gcc compiled binary and connect to a gdb-remote as 
long as it doesn't include any instructions that cannot yet be disassembled 
(see D73911  and D73958 
).


https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp


Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -103,6 +103,20 @@
   EXPECT_EQ(4U, offset);
 }
 
+TEST(DataExtractorTest, UncommonAddressSize) {
+  uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 2);
+
+  lldb::offset_t offset;
+
+  offset = 0;
+  EXPECT_EQ(0x01U, E.GetMaxU64(, 1));
+  EXPECT_EQ(1U, offset);
+  offset = 0;
+  EXPECT_EQ(0x0201U, E.GetMaxU64(, 2));
+  EXPECT_EQ(2U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + 
length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size >= 1 && addr_size <= 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = data.m_addr_size;
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   // If "data" contains shared pointer to data, then we can use that
   if (data.m_data_sp) {
 m_byte_order = data.m_byte_order;
@@ -679,12 +679,12 @@
 //
 // RETURNS the address that was extracted, or zero on failure.
 uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 
 uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64_unchecked(offset_ptr, m_addr_size);
 }
 
@@ -695,7 +695,7 @@
 //
 // RETURNS the pointer that was extracted, or zero on failure.
 uint64_t DataExtractor::GetPointer(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size >= 1 && m_addr_size <= 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 


Index: lldb/unittests/Utility/DataExtractorTest.cpp
===
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ 

[Lldb-commits] [PATCH] D73961: [LLDB] Addresses can be two bytes in size

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

See D73969 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73961



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

[Lldb-commits] [PATCH] D73961: [LLDB] Addresses can be two bytes in size

[Ayke via Phabricator via lldb-commits]

aykevl created this revision.
aykevl added reviewers: granata.enrico, labath, dylanmckay, Andrzej.
Herald added subscribers: llvm-commits, lldb-commits, hiraditya.
Herald added projects: LLDB, LLVM.

Addresses are usually two bytes in size on AVR, so make sure LLDB deals with 
that.

---

I'm not sure I caught all cases. In particular, 
lldb/include/lldb/DataFormatters/FormattersHelpers.h might be related but I've 
left it alone as it didn't seem to be necessary.

Honestly I think LLDB would become a lot more forgiving to uncommon 
architectures when these asserts are removed and avoids assumptions based on 
the address size. When the address size is relevant (for example, to read a 
value from memory), it should have an exhaustive test with a default case that 
does an assert (for easy debugging). For example:

  switch (addr_size) {
  case 4:
  // do one thing
  case 8:
  // do something else
  default:
  assert(false && "unknown addr_size");
  }

This way, it's easy to find the places that make these assumptions just by 
following the asserts.

I'm not sure how to add a test for this. I can test it locally by connecting to 
a remote debugger (simavr ). However, I 
don't know how to do something like that in the LLDB testing framework. 
Additionally, for that to work I only needed the change in 
DumpDataExtractor.cpp so I'm not sure how to exhaustively test this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73961

Files:
  lldb/source/Core/DumpDataExtractor.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp
  lldb/source/Utility/DataExtractor.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

Index: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
===
--- llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
+++ llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
@@ -131,7 +131,9 @@
   Data.setAddressSize(HeaderData.AddrSize);
   uint32_t AddrCount = DataSize / HeaderData.AddrSize;
   for (uint32_t I = 0; I < AddrCount; ++I)
-if (HeaderData.AddrSize == 4)
+if (HeaderData.AddrSize == 2)
+  Addrs.push_back(Data.getU16(OffsetPtr));
+else if (HeaderData.AddrSize == 4)
   Addrs.push_back(Data.getU32(OffsetPtr));
 else
   Addrs.push_back(Data.getU64(OffsetPtr));
Index: lldb/source/Utility/DataExtractor.cpp
===
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -133,7 +133,7 @@
   m_end(const_cast(static_cast(data)) + length),
   m_byte_order(endian), m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size == 2 || addr_size == 4 || addr_size == 8);
 }
 
 // Make a shared pointer reference to the shared data in "data_sp" and set the
@@ -146,7 +146,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(endian),
   m_addr_size(addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(addr_size == 4 || addr_size == 8);
+  assert(addr_size == 2 || addr_size == 4 || addr_size == 8);
   SetData(data_sp);
 }
 
@@ -160,7 +160,7 @@
 : m_start(nullptr), m_end(nullptr), m_byte_order(data.m_byte_order),
   m_addr_size(data.m_addr_size), m_data_sp(),
   m_target_byte_size(target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size == 2 || m_addr_size == 4 || m_addr_size == 8);
   if (data.ValidOffset(offset)) {
 offset_t bytes_available = data.GetByteSize() - offset;
 if (length > bytes_available)
@@ -173,7 +173,7 @@
 : m_start(rhs.m_start), m_end(rhs.m_end), m_byte_order(rhs.m_byte_order),
   m_addr_size(rhs.m_addr_size), m_data_sp(rhs.m_data_sp),
   m_target_byte_size(rhs.m_target_byte_size) {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size == 2 || m_addr_size == 4 || m_addr_size == 8);
 }
 
 // Assignment operator
@@ -251,7 +251,7 @@
   offset_t data_offset,
   offset_t data_length) {
   m_addr_size = data.m_addr_size;
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size == 2 || m_addr_size == 4 || m_addr_size == 8);
   // If "data" contains shared pointer to data, then we can use that
   if (data.m_data_sp) {
 m_byte_order = data.m_byte_order;
@@ -679,12 +679,12 @@
 //
 // RETURNS the address that was extracted, or zero on failure.
 uint64_t DataExtractor::GetAddress(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || m_addr_size == 8);
+  assert(m_addr_size == 2 || m_addr_size == 4 || m_addr_size == 8);
   return GetMaxU64(offset_ptr, m_addr_size);
 }
 
 uint64_t DataExtractor::GetAddress_unchecked(offset_t *offset_ptr) const {
-  assert(m_addr_size == 4 || 

[Lldb-commits] [PATCH] D73539: [AVR] Recognize the AVR architecture in lldb

[Ayke via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG727ed11b24c0: [AVR] Recognize the AVR architecture in lldb 
(authored by aykevl).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73539

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/source/Utility/ArchSpec.cpp
  lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml


Index: lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
@@ -0,0 +1,30 @@
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+# CHECK: Plugin name: elf
+# CHECK: Architecture: avr--
+# CHECK: Executable: true
+# CHECK: Stripped: false
+# CHECK: Type: executable
+# CHECK: Strata: user
+# CHECK:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: r-x
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x0
+# CHECK-NEXT:   VM size: 4
+# CHECK-NEXT:   File size: 4
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_AVR
+  Flags:   [ EF_AVR_ARCH_AVR1, EF_AVR_ARCH_AVR4 ]
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x0001
+Content: 'FECF'
+...
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian ARC
 
+eCore_avr,
+
 eCore_wasm32,
 
 kNumCores,


Index: lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
@@ -0,0 +1,30 @@
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+# CHECK: Plugin name: elf
+# CHECK: Architecture: avr--
+# CHECK: Executable: true
+# CHECK: Stripped: false
+# CHECK: Type: executable
+# CHECK: Strata: user
+# CHECK:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: r-x
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x0
+# CHECK-NEXT:   VM size: 4
+# CHECK-NEXT:   File size: 4
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_AVR
+  Flags:   [ EF_AVR_ARCH_AVR1, EF_AVR_ARCH_AVR4 ]
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x0001
+Content: 'FECF'
+...
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian ARC
 
+eCore_avr,
+
 eCore_wasm32,
 
 kNumCores,

[Lldb-commits] [PATCH] D73539: [AVR] Recognize the AVR architecture in lldb

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

Ok, I've updated the title and the commit message (text until the separator). 
Does that look good?

I have commit access so I can merge this myself.


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

https://reviews.llvm.org/D73539



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

[Lldb-commits] [PATCH] D73539: [AVR] Basic support for remote debugging

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

It's worth noting that I haven't managed to get a line table from a binary 
produced by LLVM (with my own compiler frontend) and linked by avr-gcc. But I 
believe that is a bug in the AVR backend of LLVM as a binary built entirely by 
`avr-gcc` works just fine.


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

https://reviews.llvm.org/D73539



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

[Lldb-commits] [PATCH] D73539: [AVR] Basic support for remote debugging

[Ayke via Phabricator via lldb-commits]

aykevl updated this revision to Diff 240857.
aykevl added a comment.
Herald added subscribers: MaskRay, emaste.
Herald added a reviewer: espindola.

@labath Thank you for the quick review!

I have added a test per your suggestions. I'm not very familiar with lldb but 
this passes the tests.


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

https://reviews.llvm.org/D73539

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/source/Utility/ArchSpec.cpp
  lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml


Index: lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
@@ -0,0 +1,30 @@
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+# CHECK: Plugin name: elf
+# CHECK: Architecture: avr--
+# CHECK: Executable: true
+# CHECK: Stripped: false
+# CHECK: Type: executable
+# CHECK: Strata: user
+# CHECK:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: r-x
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x0
+# CHECK-NEXT:   VM size: 4
+# CHECK-NEXT:   File size: 4
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_AVR
+  Flags:   [ EF_AVR_ARCH_AVR1, EF_AVR_ARCH_AVR4 ]
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x0001
+Content: 'FECF'
+...
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian ARC
 
+eCore_avr,
+
 eCore_wasm32,
 
 kNumCores,


Index: lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/avr-basic-info.yaml
@@ -0,0 +1,30 @@
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+# CHECK: Plugin name: elf
+# CHECK: Architecture: avr--
+# CHECK: Executable: true
+# CHECK: Stripped: false
+# CHECK: Type: executable
+# CHECK: Strata: user
+# CHECK:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: r-x
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x0
+# CHECK-NEXT:   VM size: 4
+# CHECK-NEXT:   File size: 4
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_AVR
+  Flags:   [ EF_AVR_ARCH_AVR1, EF_AVR_ARCH_AVR4 ]
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x0001
+Content: 'FECF'
+...
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian 

[Lldb-commits] [PATCH] D73539: [AVR] Basic support for remote debugging

[Ayke via Phabricator via lldb-commits]

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

Add bare-metal AVR support to lldb. Loading a binary works, but little else.

Things that work:

  $ ./llvm-build.master/bin/lldb tmp/avr.elf
  (lldb) target create "tmp/avr.elf"
  Current executable set to 
'/home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.elf' (avr).
  (lldb) image lookup -F main
  1 match found in /home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.elf:
  Address: avr.elf[0x0080] (avr.elf.PT_LOAD[0]..text + 128)
  Summary: avr.elf`main at avr.c:10
  (lldb) image dump line-table avr.c
  Line table for /home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.c in 
`avr.elf
  0x0080: /home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.c:10
  0x0084: /home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.c:13
  0x0088: /usr/lib/avr/include/util/delay.h:163
  0x009a: /home/ayke/src/github.com/tinygo-org/tinygo/tmp/avr.c:15
  0x009c: /usr/lib/avr/include/util/delay.h:163
  0x00b0: /usr/lib/avr/include/util/delay.h:163

Source code (copied from the internet somewhere):

  #ifndef F_CPU
  #define F_CPU 1600UL // 16 MHz clock speed
  #endif
  
  #include 
  #include 
  
  int main(void)
  {
DDRC = 0xFF; //Nakes PORTC as Output
while(1) //infinite loop
{
  PORTC = 0xFF; //Turns ON All LEDs
  _delay_ms(1000); //1 second delay
  PORTC= 0x00; //Turns OFF All LEDs
  _delay_ms(1000); //1 second delay
}
  }

Compiled using avr-gcc (with `-gdwarf-4` as it defaults to the stabs debug 
format in Debian):

  avr-gcc -Og -gdwarf-4 -mmcu=atmega328p -o avr.elf avr.c

Things like disassembling a binary don't work yet, but I think that can be done 
independently by getting llvm-objdump to work with the AVR target (it currently 
results in an assertion failure).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73539

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/source/Utility/ArchSpec.cpp


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian ARC
 
+eCore_avr,
+
 eCore_wasm32,
 
 kNumCores,


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -218,6 +218,8 @@
  ArchSpec::eCore_uknownMach64, "unknown-mach-64"},
 {eByteOrderLittle, 4, 2, 4, llvm::Triple::arc, ArchSpec::eCore_arc, "arc"},
 
+{eByteOrderLittle, 2, 2, 4, llvm::Triple::avr, ArchSpec::eCore_avr, "avr"},
+
 {eByteOrderLittle, 4, 1, 4, llvm::Triple::wasm32, ArchSpec::eCore_wasm32,
  "wasm32"},
 };
@@ -448,6 +450,8 @@
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
 {ArchSpec::eCore_arc, llvm::ELF::EM_ARC_COMPACT2, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // ARC
+{ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // AVR
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/include/lldb/Utility/ArchSpec.h
===
--- lldb/include/lldb/Utility/ArchSpec.h
+++ lldb/include/lldb/Utility/ArchSpec.h
@@ -188,6 +188,8 @@
 
 eCore_arc, // little endian ARC
 
+eCore_avr,
+
 eCore_wasm32,
 
 kNumCores,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits