[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

[Pavel Labath via Phabricator via lldb-commits]

labath added reviewers: clayborg, jingham.
labath added a comment.

This seems like a good idea to me (but Greg or Jim should review this).

Could you please also add a test case for this. It looks like you already have 
a simple program in the bug you linked, so it should be just a matter of 
turning that into a test. Let me know if you run into problems while doing that.


https://reviews.llvm.org/D40283



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

[Lldb-commits] [PATCH] D40133: elf-core: Convert remaining register context to use register set maps

[Pavel Labath via Phabricator via lldb-commits]

labath planned changes to this revision.
labath added a comment.

I am going to come back to this later, with a slightly different approach, but 
first I need to do another cleanup elsewhere.


https://reviews.llvm.org/D40133



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

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.

We've had a single function responsible for splitting a core segment
into notes, and parsing the notes themselves, bearing in mind variations
between 4 supported OS types. This commit splits that code into 5
pieces:

- (os-independent) code for splitting a segment into individual notes
- per-os function for parsing the notes into thread information


https://reviews.llvm.org/D40311

Files:
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.h
  source/Plugins/Process/elf-core/ThreadElfCore.cpp
  source/Plugins/Process/elf-core/ThreadElfCore.h
  source/Plugins/Process/elf-core/elf-core-enums.h

Index: source/Plugins/Process/elf-core/elf-core-enums.h
===
--- source/Plugins/Process/elf-core/elf-core-enums.h
+++ source/Plugins/Process/elf-core/elf-core-enums.h
@@ -10,6 +10,10 @@
 #ifndef LLDB_ELF_CORE_ENUMS_H
 #define LLDB_ELF_CORE_ENUMS_H
 
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/Utility/DataExtractor.h"
+
+namespace lldb_private {
 /// Core files PT_NOTE segment descriptor types
 
 namespace FREEBSD {
@@ -52,4 +56,11 @@
 };
 }
 
+struct CoreNote {
+  ELFNote info;
+  DataExtractor data;
+};
+
+} // namespace lldb_private
+
 #endif // #ifndef LLDB_ELF_CORE_ENUMS_H
Index: source/Plugins/Process/elf-core/ThreadElfCore.h
===
--- source/Plugins/Process/elf-core/ThreadElfCore.h
+++ source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -58,15 +58,15 @@
 
   ELFLinuxPrStatus();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
- lldb_private::ArchSpec &arch);
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
+ const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
   // 64 bit - just sizeof
   // 32 bit - hardcoded because we are reusing the struct, but some of the
   // members are smaller -
   // so the layout is not the same
-  static size_t GetSize(lldb_private::ArchSpec &arch);
+  static size_t GetSize(const lldb_private::ArchSpec &arch);
 };
 
 static_assert(sizeof(ELFLinuxPrStatus) == 112,
@@ -79,7 +79,7 @@
 
   ELFLinuxSigInfo();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
  const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
@@ -114,15 +114,15 @@
 
   ELFLinuxPrPsInfo();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
- lldb_private::ArchSpec &arch);
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
+ const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
   // 64 bit - just sizeof
   // 32 bit - hardcoded because we are reusing the struct, but some of the
   // members are smaller -
   // so the layout is not the same
-  static size_t GetSize(lldb_private::ArchSpec &arch);
+  static size_t GetSize(const lldb_private::ArchSpec &arch);
 };
 
 static_assert(sizeof(ELFLinuxPrPsInfo) == 136,
Index: source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -259,7 +259,7 @@
   memset(this, 0, sizeof(ELFLinuxPrStatus));
 }
 
-size_t ELFLinuxPrStatus::GetSize(lldb_private::ArchSpec &arch) {
+size_t ELFLinuxPrStatus::GetSize(const lldb_private::ArchSpec &arch) {
   constexpr size_t mips_linux_pr_status_size_o32 = 96;
   constexpr size_t mips_linux_pr_status_size_n32 = 72;
   if (arch.IsMIPS()) {
@@ -285,7 +285,8 @@
   }
 }
 
-Status ELFLinuxPrStatus::Parse(DataExtractor &data, ArchSpec &arch) {
+Status ELFLinuxPrStatus::Parse(const DataExtractor &data,
+   const ArchSpec &arch) {
   Status error;
   if (GetSize(arch) > data.GetByteSize()) {
 error.SetErrorStringWithFormat(
@@ -334,7 +335,7 @@
   memset(this, 0, sizeof(ELFLinuxPrPsInfo));
 }
 
-size_t ELFLinuxPrPsInfo::GetSize(lldb_private::ArchSpec &arch) {
+size_t ELFLinuxPrPsInfo::GetSize(const lldb_private::ArchSpec &arch) {
   constexpr size_t mips_linux_pr_psinfo_size_o32_n32 = 128;
   if (arch.IsMIPS()) {
 uint8_t address_byte_size = arch.GetAddressByteSize();
@@ -355,7 +356,8 @@
   }
 }
 
-Status ELFLinuxPrPsInfo::Parse(DataExtractor &data, ArchSpec &arch) {
+Status ELFLinuxPrPsInfo::Parse(const DataExtractor &data,
+   const ArchSpec &arch) {
   Status error;
   ByteOrder byteorder = data.GetByteOrder();
   if (GetSize(arch) > data.GetByteSize()) {
@@ -424,7 +426,7 @@
   }
 }
 
-Status ELFLinuxSigInfo::Parse(DataExtractor &data, const ArchSpec &arch) {
+Status ELFLinuxSigInfo::Parse(const DataExtractor &data, c

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:488
+ELFNote note = ELFNote();
+note.Parse(segment, &offset);
+

Do we need to check anything after parsing a note here to ensure it parsed? Can 
offset end up not changing and could we get into an infinite loop here? Seems 
like we should do:
```
if (!note.Parse(segment, &offset))
  break;
```



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:763-765
+return llvm::make_error(
+"Don't know how to parse core file. Unsupported OS.",
+llvm::inconvertibleErrorCode());

This won't cause a crash right?



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.h:32
 #include "Plugins/ObjectFile/ELF/ELFHeader.h"
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/Process/elf-core/elf-core-enums.h"

Why is this needed here? Doesn't seem to be. Can you include only in .cpp file?


https://reviews.llvm.org/D40311



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

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 123816.
labath added a comment.

Address review comments.


https://reviews.llvm.org/D40311

Files:
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.h
  source/Plugins/Process/elf-core/ThreadElfCore.cpp
  source/Plugins/Process/elf-core/ThreadElfCore.h
  source/Plugins/Process/elf-core/elf-core-enums.h

Index: source/Plugins/Process/elf-core/elf-core-enums.h
===
--- source/Plugins/Process/elf-core/elf-core-enums.h
+++ source/Plugins/Process/elf-core/elf-core-enums.h
@@ -10,6 +10,10 @@
 #ifndef LLDB_ELF_CORE_ENUMS_H
 #define LLDB_ELF_CORE_ENUMS_H
 
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "lldb/Utility/DataExtractor.h"
+
+namespace lldb_private {
 /// Core files PT_NOTE segment descriptor types
 
 namespace FREEBSD {
@@ -52,4 +56,11 @@
 };
 }
 
+struct CoreNote {
+  ELFNote info;
+  DataExtractor data;
+};
+
+} // namespace lldb_private
+
 #endif // #ifndef LLDB_ELF_CORE_ENUMS_H
Index: source/Plugins/Process/elf-core/ThreadElfCore.h
===
--- source/Plugins/Process/elf-core/ThreadElfCore.h
+++ source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -58,15 +58,15 @@
 
   ELFLinuxPrStatus();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
- lldb_private::ArchSpec &arch);
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
+ const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
   // 64 bit - just sizeof
   // 32 bit - hardcoded because we are reusing the struct, but some of the
   // members are smaller -
   // so the layout is not the same
-  static size_t GetSize(lldb_private::ArchSpec &arch);
+  static size_t GetSize(const lldb_private::ArchSpec &arch);
 };
 
 static_assert(sizeof(ELFLinuxPrStatus) == 112,
@@ -79,7 +79,7 @@
 
   ELFLinuxSigInfo();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
  const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
@@ -114,15 +114,15 @@
 
   ELFLinuxPrPsInfo();
 
-  lldb_private::Status Parse(lldb_private::DataExtractor &data,
- lldb_private::ArchSpec &arch);
+  lldb_private::Status Parse(const lldb_private::DataExtractor &data,
+ const lldb_private::ArchSpec &arch);
 
   // Return the bytesize of the structure
   // 64 bit - just sizeof
   // 32 bit - hardcoded because we are reusing the struct, but some of the
   // members are smaller -
   // so the layout is not the same
-  static size_t GetSize(lldb_private::ArchSpec &arch);
+  static size_t GetSize(const lldb_private::ArchSpec &arch);
 };
 
 static_assert(sizeof(ELFLinuxPrPsInfo) == 136,
Index: source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -259,7 +259,7 @@
   memset(this, 0, sizeof(ELFLinuxPrStatus));
 }
 
-size_t ELFLinuxPrStatus::GetSize(lldb_private::ArchSpec &arch) {
+size_t ELFLinuxPrStatus::GetSize(const lldb_private::ArchSpec &arch) {
   constexpr size_t mips_linux_pr_status_size_o32 = 96;
   constexpr size_t mips_linux_pr_status_size_n32 = 72;
   if (arch.IsMIPS()) {
@@ -285,7 +285,8 @@
   }
 }
 
-Status ELFLinuxPrStatus::Parse(DataExtractor &data, ArchSpec &arch) {
+Status ELFLinuxPrStatus::Parse(const DataExtractor &data,
+   const ArchSpec &arch) {
   Status error;
   if (GetSize(arch) > data.GetByteSize()) {
 error.SetErrorStringWithFormat(
@@ -334,7 +335,7 @@
   memset(this, 0, sizeof(ELFLinuxPrPsInfo));
 }
 
-size_t ELFLinuxPrPsInfo::GetSize(lldb_private::ArchSpec &arch) {
+size_t ELFLinuxPrPsInfo::GetSize(const lldb_private::ArchSpec &arch) {
   constexpr size_t mips_linux_pr_psinfo_size_o32_n32 = 128;
   if (arch.IsMIPS()) {
 uint8_t address_byte_size = arch.GetAddressByteSize();
@@ -355,7 +356,8 @@
   }
 }
 
-Status ELFLinuxPrPsInfo::Parse(DataExtractor &data, ArchSpec &arch) {
+Status ELFLinuxPrPsInfo::Parse(const DataExtractor &data,
+   const ArchSpec &arch) {
   Status error;
   ByteOrder byteorder = data.GetByteOrder();
   if (GetSize(arch) > data.GetByteSize()) {
@@ -424,7 +426,7 @@
   }
 }
 
-Status ELFLinuxSigInfo::Parse(DataExtractor &data, const ArchSpec &arch) {
+Status ELFLinuxSigInfo::Parse(const DataExtractor &data, const ArchSpec &arch) {
   Status error;
   if (GetSize(arch) > data.GetByteSize()) {
 error.SetErrorStringWithFormat(
Index: source/Plugins/Process/elf-core/ProcessElfCore.h
===
--- source/Plugins/Process/elf-core/Proce

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:488
+ELFNote note = ELFNote();
+note.Parse(segment, &offset);
+

clayborg wrote:
> Do we need to check anything after parsing a note here to ensure it parsed? 
> Can offset end up not changing and could we get into an infinite loop here? 
> Seems like we should do:
> ```
> if (!note.Parse(segment, &offset))
>   break;
> ```
Good point. There was no error checking in the original code, but it looks like 
there should be...



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:763-765
+return llvm::make_error(
+"Don't know how to parse core file. Unsupported OS.",
+llvm::inconvertibleErrorCode());

clayborg wrote:
> This won't cause a crash right?
Only if the caller does not do anything with the result function (and it that 
case, it will assert regardless of whether the function returned an error or 
not).


https://reviews.llvm.org/D40311



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

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:735
 ///   (see ELFNote structure)
 /// 3) A Thread Context in a core file usually described by 3 NOTE entries.
 ///a) NT_PRSTATUS - Register context

Can we label it as SVR4-style (Linux, FreeBSD, Solaris)? Alternatively move it 
to other place in order to describe Linux and FreeBSD separately (NetBSD an 
OpenBSD can be skipped now).


https://reviews.llvm.org/D40311



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