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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yes, standalone patches are the way to go. To help you, I've tried to annotate 
the various assertions, what kind of problems they could cause, and possible 
testing strategies.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:71
 
-  if (m_header.addr_size != 4 && m_header.addr_size != 8)
 return llvm::make_error(

I don't believe this will cause any failures, but it will stop lldb from using 
the debug_aranges section (and fall back to other, potentially slower, 
alternatives).



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:812
   bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
-  bool addr_size_OK = (header.m_addr_size == 4) || (header.m_addr_size == 8);
   bool type_offset_OK =

This will definitely mean you won't be able to parse any (DWARF) debug info. 
For a test inspiration, you could look at the existing .s files in 
test/Shell/SymbolFile/DWARF.



Comment at: lldb/source/Symbol/DWARFCallFrameInfo.cpp:42
   const uint32_t addr_size = DE.GetAddressByteSize();
-  assert(addr_size == 4 || addr_size == 8);
 

This will probably cause some problems when unwinding via eh/debug_frame. 
Testing will be somewhat tricky, as I don't think we'll parse this without a 
running process. I'd leave this one for the end.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:134
   for (uint32_t I = 0; I < AddrCount; ++I)
-if (HeaderData.AddrSize == 4)
   Addrs.push_back(Data.getU32(OffsetPtr));

This one is also pretty critical for DWARF debug info. You could probably test 
it together with the DWARFUnit thingy. To trigger, it should be enough to try 
to display a global variable ("target variable my_global").


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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D73961#1860270 , @aykevl wrote:

> > 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.


yaml2obj can certainly be useful, the yaml files are not that readable when you 
need to create more complicated inputs (e.g. with DWARF). Writing assembly 
could be more useful there.

But anyway, we can evaluate that on a case-by-case basis.


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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

You should be able to test most (if not all) of this stuff without an actual 
process. lldb can display global variables, set breakpoints, resolve addresses, 
etc. without being connected to a debug stub.

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 
?


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 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 ||