[Lldb-commits] [PATCH] D40587: [lldb] Minor fixes for in TaskPool

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap updated this revision to Diff 124661.
alexshap added a comment.

fix the tests


Repository:
  rL LLVM

https://reviews.llvm.org/D40587

Files:
  include/lldb/Host/TaskPool.h
  source/Host/common/TaskPool.cpp
  unittests/Host/TaskPoolTest.cpp


Index: unittests/Host/TaskPoolTest.cpp
===
--- unittests/Host/TaskPoolTest.cpp
+++ unittests/Host/TaskPoolTest.cpp
@@ -2,6 +2,8 @@
 
 #include "lldb/Host/TaskPool.h"
 
+using namespace lldb_private;
+
 TEST(TaskPoolTest, AddTask) {
   auto fn = [](int x) { return x * x + 1; };
 
Index: source/Host/common/TaskPool.cpp
===
--- source/Host/common/TaskPool.cpp
+++ source/Host/common/TaskPool.cpp
@@ -14,6 +14,8 @@
 #include// for queue
 #include   // for thread
 
+namespace lldb_private {
+
 namespace {
 class TaskPoolImpl {
 public:
@@ -46,13 +48,20 @@
 
 TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
 
+unsigned GetHardwareConcurrencyHint() {
+  // std::thread::hardware_concurrency may return 0
+  // if the value is not well defined or not computable.
+  static const unsigned g_hardware_concurrency = 
+std::max(1u, std::thread::hardware_concurrency());
+  return g_hardware_concurrency;
+}
+
 void TaskPoolImpl::AddTask(std::function &&task_fn) {
-  static const uint32_t max_threads = std::thread::hardware_concurrency();
   const size_t min_stack_size = 8 * 1024 * 1024;
 
   std::unique_lock lock(m_tasks_mutex);
   m_tasks.emplace(std::move(task_fn));
-  if (m_thread_count < max_threads) {
+  if (m_thread_count < GetHardwareConcurrencyHint()) {
 m_thread_count++;
 // Note that this detach call needs to happen with the m_tasks_mutex held.
 // This prevents the thread
@@ -77,7 +86,7 @@
   break;
 }
 
-std::function f = pool->m_tasks.front();
+std::function f = std::move(pool->m_tasks.front());
 pool->m_tasks.pop();
 lock.unlock();
 
@@ -87,10 +96,9 @@
 
 void TaskMapOverInt(size_t begin, size_t end,
 const llvm::function_ref &func) {
+  const size_t num_workers = std::min(end, 
GetHardwareConcurrencyHint());
   std::atomic idx{begin};
-  size_t num_workers =
-  std::min(end, std::thread::hardware_concurrency());
-
+  
   auto wrapper = [&idx, end, &func]() {
 while (true) {
   size_t i = idx.fetch_add(1);
@@ -107,3 +115,6 @@
   for (size_t i = 0; i < num_workers; i++)
 futures[i].wait();
 }
+
+} // namespace lldb_private
+
Index: include/lldb/Host/TaskPool.h
===
--- include/lldb/Host/TaskPool.h
+++ include/lldb/Host/TaskPool.h
@@ -18,6 +18,8 @@
 #include// for mutex, unique_lock, condition_variable
 #include  // for forward, result_of, move
 
+namespace lldb_private {
+
 // Global TaskPool class for running tasks in parallel on a set of worker 
thread
 // created the first
 // time the task pool is used. The TaskPool provide no guarantee about the 
order
@@ -89,4 +91,8 @@
 void TaskMapOverInt(size_t begin, size_t end,
 const llvm::function_ref &func);
 
+unsigned GetHardwareConcurrencyHint();
+
+} // namespace lldb_private
+
 #endif // #ifndef utility_TaskPool_h_


Index: unittests/Host/TaskPoolTest.cpp
===
--- unittests/Host/TaskPoolTest.cpp
+++ unittests/Host/TaskPoolTest.cpp
@@ -2,6 +2,8 @@
 
 #include "lldb/Host/TaskPool.h"
 
+using namespace lldb_private;
+
 TEST(TaskPoolTest, AddTask) {
   auto fn = [](int x) { return x * x + 1; };
 
Index: source/Host/common/TaskPool.cpp
===
--- source/Host/common/TaskPool.cpp
+++ source/Host/common/TaskPool.cpp
@@ -14,6 +14,8 @@
 #include// for queue
 #include   // for thread
 
+namespace lldb_private {
+
 namespace {
 class TaskPoolImpl {
 public:
@@ -46,13 +48,20 @@
 
 TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
 
+unsigned GetHardwareConcurrencyHint() {
+  // std::thread::hardware_concurrency may return 0
+  // if the value is not well defined or not computable.
+  static const unsigned g_hardware_concurrency = 
+std::max(1u, std::thread::hardware_concurrency());
+  return g_hardware_concurrency;
+}
+
 void TaskPoolImpl::AddTask(std::function &&task_fn) {
-  static const uint32_t max_threads = std::thread::hardware_concurrency();
   const size_t min_stack_size = 8 * 1024 * 1024;
 
   std::unique_lock lock(m_tasks_mutex);
   m_tasks.emplace(std::move(task_fn));
-  if (m_thread_count < max_threads) {
+  if (m_thread_count < GetHardwareConcurrencyHint()) {
 m_thread_count++;
 // Note that this detach call needs to happen with the m_tasks_mutex held.
 // This prevents the thread
@@ -77,7 +86,7 @@
   break;
 }
 
-std::function f = pool->m_tasks.front();
+std::function f = std::move(pool->m_tasks.front());
 pool->m_tasks.pop();
 lock.unlock();
 
@@ -

[Lldb-commits] [PATCH] D40587: [lldb] Minor fixes for in TaskPool

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap created this revision.

1. Move everything into the namespace lldb_private
2. Add missing std::move in TaskPoolImpl::Worker
3. std::thread::hardware_concurrency may return 0, handle this case correctly


Repository:
  rL LLVM

https://reviews.llvm.org/D40587

Files:
  include/lldb/Host/TaskPool.h
  source/Host/common/TaskPool.cpp


Index: source/Host/common/TaskPool.cpp
===
--- source/Host/common/TaskPool.cpp
+++ source/Host/common/TaskPool.cpp
@@ -14,6 +14,8 @@
 #include// for queue
 #include   // for thread
 
+namespace lldb_private {
+
 namespace {
 class TaskPoolImpl {
 public:
@@ -46,13 +48,20 @@
 
 TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
 
+unsigned GetHardwareConcurrencyHint() {
+  // std::thread::hardware_concurrency may return 0
+  // if the value is not well defined or not computable.
+  static const unsigned g_hardware_concurrency = 
+std::max(1u, std::thread::hardware_concurrency());
+  return g_hardware_concurrency;
+}
+
 void TaskPoolImpl::AddTask(std::function &&task_fn) {
-  static const uint32_t max_threads = std::thread::hardware_concurrency();
   const size_t min_stack_size = 8 * 1024 * 1024;
 
   std::unique_lock lock(m_tasks_mutex);
   m_tasks.emplace(std::move(task_fn));
-  if (m_thread_count < max_threads) {
+  if (m_thread_count < GetHardwareConcurrencyHint()) {
 m_thread_count++;
 // Note that this detach call needs to happen with the m_tasks_mutex held.
 // This prevents the thread
@@ -77,7 +86,7 @@
   break;
 }
 
-std::function f = pool->m_tasks.front();
+std::function f = std::move(pool->m_tasks.front());
 pool->m_tasks.pop();
 lock.unlock();
 
@@ -87,10 +96,9 @@
 
 void TaskMapOverInt(size_t begin, size_t end,
 const llvm::function_ref &func) {
+  const size_t num_workers = std::min(end, 
GetHardwareConcurrencyHint());
   std::atomic idx{begin};
-  size_t num_workers =
-  std::min(end, std::thread::hardware_concurrency());
-
+  
   auto wrapper = [&idx, end, &func]() {
 while (true) {
   size_t i = idx.fetch_add(1);
@@ -107,3 +115,6 @@
   for (size_t i = 0; i < num_workers; i++)
 futures[i].wait();
 }
+
+} // namespace lldb_private
+
Index: include/lldb/Host/TaskPool.h
===
--- include/lldb/Host/TaskPool.h
+++ include/lldb/Host/TaskPool.h
@@ -18,6 +18,8 @@
 #include// for mutex, unique_lock, condition_variable
 #include  // for forward, result_of, move
 
+namespace lldb_private {
+
 // Global TaskPool class for running tasks in parallel on a set of worker 
thread
 // created the first
 // time the task pool is used. The TaskPool provide no guarantee about the 
order
@@ -89,4 +91,8 @@
 void TaskMapOverInt(size_t begin, size_t end,
 const llvm::function_ref &func);
 
+unsigned GetHardwareConcurrencyHint();
+
+} // namespace lldb_private
+
 #endif // #ifndef utility_TaskPool_h_


Index: source/Host/common/TaskPool.cpp
===
--- source/Host/common/TaskPool.cpp
+++ source/Host/common/TaskPool.cpp
@@ -14,6 +14,8 @@
 #include// for queue
 #include   // for thread
 
+namespace lldb_private {
+
 namespace {
 class TaskPoolImpl {
 public:
@@ -46,13 +48,20 @@
 
 TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
 
+unsigned GetHardwareConcurrencyHint() {
+  // std::thread::hardware_concurrency may return 0
+  // if the value is not well defined or not computable.
+  static const unsigned g_hardware_concurrency = 
+std::max(1u, std::thread::hardware_concurrency());
+  return g_hardware_concurrency;
+}
+
 void TaskPoolImpl::AddTask(std::function &&task_fn) {
-  static const uint32_t max_threads = std::thread::hardware_concurrency();
   const size_t min_stack_size = 8 * 1024 * 1024;
 
   std::unique_lock lock(m_tasks_mutex);
   m_tasks.emplace(std::move(task_fn));
-  if (m_thread_count < max_threads) {
+  if (m_thread_count < GetHardwareConcurrencyHint()) {
 m_thread_count++;
 // Note that this detach call needs to happen with the m_tasks_mutex held.
 // This prevents the thread
@@ -77,7 +86,7 @@
   break;
 }
 
-std::function f = pool->m_tasks.front();
+std::function f = std::move(pool->m_tasks.front());
 pool->m_tasks.pop();
 lock.unlock();
 
@@ -87,10 +96,9 @@
 
 void TaskMapOverInt(size_t begin, size_t end,
 const llvm::function_ref &func) {
+  const size_t num_workers = std::min(end, GetHardwareConcurrencyHint());
   std::atomic idx{begin};
-  size_t num_workers =
-  std::min(end, std::thread::hardware_concurrency());
-
+  
   auto wrapper = [&idx, end, &func]() {
 while (true) {
   size_t i = idx.fetch_add(1);
@@ -107,3 +115,6 @@
   for (size_t i = 0; i < num_workers; i++)
 futures[i].wait();
 }
+
+} // namespace lldb_private
+
Index: include/lldb/Host/TaskPool.h

[Lldb-commits] [lldb] r319226 - Add elf-core/RegisterUtilities.{cpp, h} to the project file.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Tue Nov 28 13:11:15 2017
New Revision: 319226

URL: http://llvm.org/viewvc/llvm-project?rev=319226&view=rev
Log:
Add elf-core/RegisterUtilities.{cpp,h} to the project file.

Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj

Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=319226&r1=319225&r2=319226&view=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Tue Nov 28 13:11:15 2017
@@ -728,6 +728,7 @@
4C877B391F30EF990068FCFB /* SBProcessInfo.h in Headers */ = 
{isa = PBXBuildFile; fileRef = 4987FB201F30EC9900E5C17D /* SBProcessInfo.h */; 
settings = {ATTRIBUTES = (Public, ); }; };
4C88BC2A1BA3722B00AA0964 /* Expression.cpp in Sources */ = {isa 
= PBXBuildFile; fileRef = 4C88BC291BA3722B00AA0964 /* Expression.cpp */; };
4C88BC2D1BA391B000AA0964 /* UserExpression.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 4C0083331B9A5DE200D5CF24 /* UserExpression.cpp 
*/; };
+   4CA9D1401FCE07CD00300E18 /* RegisterUtilities.cpp in Sources */ 
= {isa = PBXBuildFile; fileRef = 4CA9D13C1FCE07AF00300E18 /* 
RegisterUtilities.cpp */; };
4CAA19E61F5A40040099E692 /* BreakpointName.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 4C7D48281F509CCD005314B4 /* BreakpointName.cpp 
*/; };
4CABA9E0134A8BCD00539BDD /* ValueObjectMemory.cpp in Sources */ 
= {isa = PBXBuildFile; fileRef = 4CABA9DF134A8BCD00539BDD /* 
ValueObjectMemory.cpp */; };
4CC7C6501D5298F30076FF94 /* OCamlLanguage.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 4CC7C64D1D5298E20076FF94 /* OCamlLanguage.cpp 
*/; };
@@ -2583,6 +2584,8 @@
4C98D3E1118FB98F00E575D0 /* IRExecutionUnit.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = 
IRExecutionUnit.h; path = include/lldb/Expression/IRExecutionUnit.h; sourceTree 
= ""; };
4CA9637911B6E99A00780E28 /* CommandObjectApropos.cpp */ = {isa 
= PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
name = CommandObjectApropos.cpp; path = 
source/Commands/CommandObjectApropos.cpp; sourceTree = ""; };
4CA9637A11B6E99A00780E28 /* CommandObjectApropos.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = 
CommandObjectApropos.h; path = source/Commands/CommandObjectApropos.h; 
sourceTree = ""; };
+   4CA9D13C1FCE07AF00300E18 /* RegisterUtilities.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
path = RegisterUtilities.cpp; sourceTree = ""; };
+   4CA9D13D1FCE07AF00300E18 /* RegisterUtilities.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = 
RegisterUtilities.h; sourceTree = ""; };
4CAA56121422D96A001FFA01 /* BreakpointResolverFileRegex.h */ = 
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; 
name = BreakpointResolverFileRegex.h; path = 
include/lldb/Breakpoint/BreakpointResolverFileRegex.h; sourceTree = ""; 
};
4CAA56141422D986001FFA01 /* BreakpointResolverFileRegex.cpp */ 
= {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.cpp.cpp; name = BreakpointResolverFileRegex.cpp; path = 
source/Breakpoint/BreakpointResolverFileRegex.cpp; sourceTree = ""; };
4CAB257C18EC9DB800BAD33E /* SafeMachO.h */ = {isa = 
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SafeMachO.h; path 
= include/lldb/Utility/SafeMachO.h; sourceTree = ""; };
@@ -4847,6 +4850,8 @@
267F684E1CC02E270086832B /* 
RegisterContextPOSIXCore_s390x.h */,
26BC17A618C7F4CB00D2196D /* 
RegisterContextPOSIXCore_x86_64.cpp */,
26BC17A718C7F4CB00D2196D /* 
RegisterContextPOSIXCore_x86_64.h */,
+   4CA9D13C1FCE07AF00300E18 /* 
RegisterUtilities.cpp */,
+   4CA9D13D1FCE07AF00300E18 /* RegisterUtilities.h 
*/,
26BC17A818C7F4CB00D2196D /* ThreadElfCore.cpp 
*/,
26BC17A918C7F4CB00D2196D /* ThreadElfCore.h */,
);
@@ -7818,6 +7823,7 @@
2689010413353E6F00698AC0 /* 
ThreadPlanStepInRange.cpp in Sources */,
2689010513353E6F00698AC0 /* 
ThreadPlanStepOverRange.cpp in Sources */,
3FDFE56D19AF9C44009756A7 /* HostThreadPosix.cpp 
in Sources */,
+   4CA9D1401FCE07CD00300E18 /* 
RegisterUtilities.cpp in Sources */,
AF235EB51FBE7858009C5541 /* 
RegisterInfoPOSIX_ppc64

[Lldb-commits] [lldb] r319213 - Update remote debugging page with many more details.

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Tue Nov 28 12:04:43 2017
New Revision: 319213

URL: http://llvm.org/viewvc/llvm-project?rev=319213&view=rev
Log:
Update remote debugging page with many more details.


Modified:
lldb/trunk/www/remote.html

Modified: lldb/trunk/www/remote.html
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/www/remote.html?rev=319213&r1=319212&r2=319213&view=diff
==
--- lldb/trunk/www/remote.html (original)
+++ lldb/trunk/www/remote.html Tue Nov 28 12:04:43 2017
@@ -86,7 +86,9 @@
   Once the binaries are in place, you just need to run the 
lldb-server
   in platform mode and specify the port it should listen on. For 
example, the command
 
-lldb-server platform --listen *:1234
+
+  remote% lldb-server platform --listen "*:1234" --server
+
 
   will start the LLDB platform and wait for incoming connections 
from any address to
   port 1234. Specifying an address instead of * will 
only allow
@@ -104,14 +106,35 @@
   your remote system. A list of available plug-ins can be obtained 
through
   platform list.
 
-
+
+  local% lldb
+  (lldb) platform list
+  Available platforms:
+  host: Local Mac OS X user platform plug-in.
+  remote-freebsd: Remote FreeBSD user platform plug-in.
+  remote-linux: Remote Linux user platform plug-in.
+  remote-netbsd: Remote NetBSD user platform plug-in.
+  remote-windows: Remote Windows user platform plug-in.
+  remote-android: Remote Android user platform plug-in.
+  remote-ios: Remote iOS platform plug-in.
+  remote-macosx: Remote Mac OS X user platform plug-in.
+  ios-simulator: iOS simulator platform plug-in.
+  darwin-kernel: Darwin Kernel platform plug-in.
+  tvos-simulator: Apple TV simulator platform plug-in.
+  watchos-simulator: Apple Watch simulator platform plug-in.
+  remote-tvos: Remote Apple TV platform plug-in.
+  remote-watchos: Remote Apple Watch platform plug-in.
+  remote-gdb-server: A platform that uses the GDB remote 
protocol as the communication transport.
+
 
   The default platform is the platform host which is 
used for local
   debugging. Apart from this, the list should contain a number of 
plug-ins, for
   debugging different kinds of systems. The remote plug-ins are 
prefixed with
-  "remote-". For example, to debug a remote Linux 
application, you should
-  run platform select remote-linux.
-
+  "remote-". For example, to debug a remote Linux 
application:
+
+
+  (lldb) patform select remote-linux
+
 
 
   After selecting the platform plug-in, you should receive a 
prompt which confirms
@@ -121,9 +144,19 @@
   command takes a number of arguments (as always, use the 
help command
   to find out more), but normally you only need to specify the 
address to connect to,
   e.g.:
-
-platform connect connect://host:port
-
+
+
+  (lldb) platform connect connect://remote:1234
+    Platform: remote-linux
+  Triple: x86_64-gnu-linux
+    Hostname: remote
+   Connected: yes
+  WorkingDir: /tmp
+
+
+  Note that the platform has a working directory of 
/tmp. This directory
+  will be used as the directory that executables will be uploaded 
to by default when
+  launching a process from local.
 
   After this, you should be able to debug normally. You can use the
   process attach to attach to an existing remote 
process or
@@ -134,7 +167,98 @@
   put-file, mkdir, etc. The environment 
can be prepared
   further using the platform shell command.
 
+Launching a locally built process on the remote machine
+Install and run in the platform working directory
+
+  To launch a locally built process on the remote system in the
+  platform working directory:
+
+
+  (lldb) file a.out
+  (lldb) run
+
+
+  This will cause LLDB to create a target with the "a.out"
+  executable that you cross built. The "run" command will cause
+  LLDB to upload "a.out" to the platform's current working
+  directory only if the file has changed.
+  The platform connection allo

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

For that you still need something like what we have, but it would be nice
if the internal code that knew more about the contents could use that extra
information for better clarity and safety.

I’d honestly probably make the interfaces deal in ArrayRef and assert in
the API boundaries that that the size makes sense in the given context.
On Tue, Nov 28, 2017 at 11:48 AM Jim Ingham  wrote:

> How would the ObjectFile API's that take or return UUID's work in this
> case?
>
> Jim
>
>
> > On Nov 28, 2017, at 11:44 AM, Zachary Turner via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Also worth pointing out that when you write things this way, this
> UUID class can be part of a larger structure that matches the record
> layout of a header or section in a binary file, and then you can just
> memcpy over the class and your'e good to go.  For example you could have
> >
> > ```
> > struct MachOHeader {
> >...
> >...
> >UUID<16> uuid;
> >...
> > };
> >
> > MachOHeader H;
> > ::memcpy(&H, File, sizeof(H));
> > File += sizeof(H);
> > ```
> >
> > and you could do the same for some ELF structure.
> >
> > ```
> > struct ElfHeader {
> >...
> >...
> >   union {
> > UUID<20> BuildId;
> > UUID<4> DebugCrc;
> >   }
> >...
> > };
> >
> > ElfHeader H;
> > ::memcpy(&H, File, sizeof(H));
> > File += sizeof(H);
> > ```
> >
> > And everything just works.
> >
> > On Tue, Nov 28, 2017 at 11:36 AM Zachary Turner 
> wrote:
> > Eh, that actually just makes me think the compiler *can* check it.  For
> example, right now you can have mach-o files with 20 byte UUIDs.  But just
> in the code, not in practice.  You could have a bug in your code that
> accidentally wrote the wrong number of bytes from a dynamic buffer.
> >
> > You could enforce this at the compiler level by saying:
> >
> > class ObjectFileMachO {
> >   UUID<16> uuid;
> > };
> >
> > Not only is this more correct, but it is less error prone and is also
> nice documentation to the reader who may be just learning about MachO that
> this UUID is always 16 bytes.
> >
> > For the case of ELF, it sounds like you either have a 20 byte UUID or a
> 4 byte UUID, but never both, and never any other size.  That makes me think
> of:
> >
> > class ObjectFileELF {
> >   union {
> > UUID<20> BuildId;
> > UUID<4> DebugCrc;
> >   }
> > };
> >
> > And now the person reading this code can immediately tell that there
> will either be one or the other, and depending on which one it is, he/she
> already knows something about it, like how many bytes it is and what it
> represents.
> >
> > To me this is much more clear than:
> >
> > class ObjectFileELF {
> >   // This might not actually be a build id, and it could be a variable
> size, and you also have to be careful
> >   // not to put some strange number of bytes in it that we don't
> recognize, but it's up to the user
> >   // to know under what circumstances it should be a certain number of
> bytes, and you should also always
> >   // be careful ensure that there's no buffer overruns since you'll be
> working with dynamically sized buffers
> >   // and the compiler can't warn you when you're doing something wrong.
> >   UUID BuildIdOrDebugCrc;
> > };
> >
> > On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton 
> wrote:
> >
> >> On Nov 28, 2017, at 10:24 AM, Zachary Turner 
> wrote:
> >>
> >>
> >>
> >> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton 
> wrote:
> >>> On Nov 27, 2017, at 10:11 PM, Zachary Turner 
> wrote:
> >>>
> >>> As an aside, I don't really like this class.  For example, You can
> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
> sense to me.
> >>
> >> What about an invalid UUID[0] being assigned with a valid UUID[16] or
> UUID[20]? Why doesn't this make sense? I don't follow.
> >>
> >> Nothing is invalid, I just think it's better and expresses the intent
> more clearly if you can only assign between UUIDs of the same size.  For
> example, If the UUID class were templated on size, then there would not
> even be such thing as a UUID[0] or a "universally invalid UUID".  There
> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
> would be different things.
> >>
> >>
> >>>
> >>> As a future cleanup, I think this class should probably be a template
> such as UUID, and then internally it can store a std::array N>.  And we can static_assert that N is of a known size if we desire.
> >>
> >> UUID values are objects contained as members inside of other objects.
> They all default to start with no preconceived notion of what the UUID
> should be. IMHO the UUID class is just fine and needs to be able to
> represent any UUID, from empty uninitialized ones, and be able to be
> assigned and changed at will.
> >>
> >>
> >> Is there ever a use case for changing the number of bytes in a UUID?
> If you're working with 16-byte UUIDs, does it ever actually happen that now
> you have a 20-byte UUID?  Can you imagine a use case currently where an
> N-byte UU

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Jim Ingham via lldb-commits]

How would the ObjectFile API's that take or return UUID's work in this case?

Jim


> On Nov 28, 2017, at 11:44 AM, Zachary Turner via lldb-commits 
>  wrote:
> 
> Also worth pointing out that when you write things this way, this UUID 
> class can be part of a larger structure that matches the record layout of a 
> header or section in a binary file, and then you can just memcpy over the 
> class and your'e good to go.  For example you could have
> 
> ```
> struct MachOHeader {
>...
>...
>UUID<16> uuid;
>...
> };
> 
> MachOHeader H;
> ::memcpy(&H, File, sizeof(H));
> File += sizeof(H);
> ```
> 
> and you could do the same for some ELF structure.
> 
> ```
> struct ElfHeader {
>...
>...
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
>...
> }; 
> 
> ElfHeader H;
> ::memcpy(&H, File, sizeof(H));
> File += sizeof(H);
> ```
> 
> And everything just works.
> 
> On Tue, Nov 28, 2017 at 11:36 AM Zachary Turner  wrote:
> Eh, that actually just makes me think the compiler *can* check it.  For 
> example, right now you can have mach-o files with 20 byte UUIDs.  But just in 
> the code, not in practice.  You could have a bug in your code that 
> accidentally wrote the wrong number of bytes from a dynamic buffer.  
> 
> You could enforce this at the compiler level by saying:
> 
> class ObjectFileMachO {
>   UUID<16> uuid;
> };
> 
> Not only is this more correct, but it is less error prone and is also nice 
> documentation to the reader who may be just learning about MachO that this 
> UUID is always 16 bytes.
> 
> For the case of ELF, it sounds like you either have a 20 byte UUID or a 4 
> byte UUID, but never both, and never any other size.  That makes me think of:
> 
> class ObjectFileELF {
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
> };
> 
> And now the person reading this code can immediately tell that there will 
> either be one or the other, and depending on which one it is, he/she already 
> knows something about it, like how many bytes it is and what it represents.
> 
> To me this is much more clear than:
> 
> class ObjectFileELF {
>   // This might not actually be a build id, and it could be a variable size, 
> and you also have to be careful
>   // not to put some strange number of bytes in it that we don't recognize, 
> but it's up to the user
>   // to know under what circumstances it should be a certain number of bytes, 
> and you should also always
>   // be careful ensure that there's no buffer overruns since you'll be 
> working with dynamically sized buffers
>   // and the compiler can't warn you when you're doing something wrong.
>   UUID BuildIdOrDebugCrc;
> };
> 
> On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:
> 
>> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>> 
>> 
>> 
>> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>> 
>>> As an aside, I don't really like this class.  For example, You can 
>>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of 
>>> sense to me.
>> 
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or 
>> UUID[20]? Why doesn't this make sense? I don't follow.
>> 
>> Nothing is invalid, I just think it's better and expresses the intent more 
>> clearly if you can only assign between UUIDs of the same size.  For example, 
>> If the UUID class were templated on size, then there would not even be such 
>> thing as a UUID[0] or a "universally invalid UUID".  There would be an 
>> "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those would be 
>> different things.
>>  
>> 
>>> 
>>> As a future cleanup, I think this class should probably be a template such 
>>> as UUID, and then internally it can store a std::array.  And 
>>> we can static_assert that N is of a known size if we desire.
>> 
>> UUID values are objects contained as members inside of other objects. They 
>> all default to start with no preconceived notion of what the UUID should be. 
>> IMHO the UUID class is just fine and needs to be able to represent any UUID, 
>> from empty uninitialized ones, and be able to be assigned and changed at 
>> will.
>> 
>> 
>> Is there ever a use case for changing the number of bytes in a UUID?  If 
>> you're working with 16-byte UUIDs, does it ever actually happen that now you 
>> have a 20-byte UUID?  Can you imagine a use case currently where an N-byte 
>> UUID is being compared against an M-byte UUID in a real-world scenario?  If 
>> the answer is no, then it may as well be enforced by the compiler. 
> 
> The ObjectFile class has a "UUID m_uuid;" member that any object file can 
> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20 
> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is 
> around, and these are current represented as 20 byte UUIDs with just the 
> first 4 bytes filled in. So no, we can't enforce this using the compiler. I 
> don't s

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

Also worth pointing out that when you write things this way, this UUID
class can be part of a larger structure that matches the record layout of a
header or section in a binary file, and then you can just memcpy over the
class and your'e good to go.  For example you could have

```
struct MachOHeader {
   ...
   ...
   UUID<16> uuid;
   ...
};

MachOHeader H;
::memcpy(&H, File, sizeof(H));
File += sizeof(H);
```

and you could do the same for some ELF structure.

```
struct ElfHeader {
   ...
   ...
  union {
UUID<20> BuildId;
UUID<4> DebugCrc;
  }
   ...
};

ElfHeader H;
::memcpy(&H, File, sizeof(H));
File += sizeof(H);
```

And everything just works.

On Tue, Nov 28, 2017 at 11:36 AM Zachary Turner  wrote:

> Eh, that actually just makes me think the compiler *can* check it.  For
> example, right now you can have mach-o files with 20 byte UUIDs.  But just
> in the code, not in practice.  You could have a bug in your code that
> accidentally wrote the wrong number of bytes from a dynamic buffer.
>
> You could enforce this at the compiler level by saying:
>
> class ObjectFileMachO {
>   UUID<16> uuid;
> };
>
> Not only is this more correct, but it is less error prone and is also nice
> documentation to the reader who may be just learning about MachO that this
> UUID is always 16 bytes.
>
> For the case of ELF, it sounds like you either have a 20 byte UUID or a 4
> byte UUID, but never both, and never any other size.  That makes me think
> of:
>
> class ObjectFileELF {
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
> };
>
> And now the person reading this code can immediately tell that there will
> either be one or the other, and depending on which one it is, he/she
> already knows something about it, like how many bytes it is and what it
> represents.
>
> To me this is much more clear than:
>
> class ObjectFileELF {
>   // This might not actually be a build id, and it could be a variable
> size, and you also have to be careful
>   // not to put some strange number of bytes in it that we don't
> recognize, but it's up to the user
>   // to know under what circumstances it should be a certain number of
> bytes, and you should also always
>   // be careful ensure that there's no buffer overruns since you'll be
> working with dynamically sized buffers
>   // and the compiler can't warn you when you're doing something wrong.
>   UUID BuildIdOrDebugCrc;
> };
>
> On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:
>
>>
>> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>>
>>
>>
>> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>>
>>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>>
>>> As an aside, I don't really like this class.  For example, You can
>>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>>> sense to me.
>>>
>>>
>>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>>> UUID[20]? Why doesn't this make sense? I don't follow.
>>>
>>
>> Nothing is invalid, I just think it's better and expresses the intent
>> more clearly if you can only assign between UUIDs of the same size.  For
>> example, If the UUID class were templated on size, then there would not
>> even be such thing as a UUID[0] or a "universally invalid UUID".  There
>> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
>> would be different things.
>>
>>
>>>
>>>
>>> As a future cleanup, I think this class should probably be a template
>>> such as UUID, and then internally it can store a std::array>> N>.  And we can static_assert that N is of a known size if we desire.
>>>
>>>
>>> UUID values are objects contained as members inside of other objects.
>>> They all default to start with no preconceived notion of what the UUID
>>> should be. IMHO the UUID class is just fine and needs to be able to
>>> represent any UUID, from empty uninitialized ones, and be able to be
>>> assigned and changed at will.
>>>
>>>
>> Is there ever a use case for changing the number of bytes in a UUID?  If
>> you're working with 16-byte UUIDs, does it ever actually happen that now
>> you have a 20-byte UUID?  Can you imagine a use case currently where an
>> N-byte UUID is being compared against an M-byte UUID in a real-world
>> scenario?  If the answer is no, then it may as well be enforced by the
>> compiler.
>>
>>
>> The ObjectFile class has a "UUID m_uuid;" member that any object file can
>> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20
>> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is
>> around, and these are current represented as 20 byte UUIDs with just the
>> first 4 bytes filled in. So no, we can't enforce this using the compiler. I
>> don't see a need to change way from a byte buffer that has the max number
>> of bytes needed for any currently supported UUID (20 right now).
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

Eh, that actually just makes me think the compiler *can* check it.  For
example, right now you can have mach-o files with 20 byte UUIDs.  But just
in the code, not in practice.  You could have a bug in your code that
accidentally wrote the wrong number of bytes from a dynamic buffer.

You could enforce this at the compiler level by saying:

class ObjectFileMachO {
  UUID<16> uuid;
};

Not only is this more correct, but it is less error prone and is also nice
documentation to the reader who may be just learning about MachO that this
UUID is always 16 bytes.

For the case of ELF, it sounds like you either have a 20 byte UUID or a 4
byte UUID, but never both, and never any other size.  That makes me think
of:

class ObjectFileELF {
  union {
UUID<20> BuildId;
UUID<4> DebugCrc;
  }
};

And now the person reading this code can immediately tell that there will
either be one or the other, and depending on which one it is, he/she
already knows something about it, like how many bytes it is and what it
represents.

To me this is much more clear than:

class ObjectFileELF {
  // This might not actually be a build id, and it could be a variable
size, and you also have to be careful
  // not to put some strange number of bytes in it that we don't recognize,
but it's up to the user
  // to know under what circumstances it should be a certain number of
bytes, and you should also always
  // be careful ensure that there's no buffer overruns since you'll be
working with dynamically sized buffers
  // and the compiler can't warn you when you're doing something wrong.
  UUID BuildIdOrDebugCrc;
};

On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:

>
> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>
>
>
> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>
>> As an aside, I don't really like this class.  For example, You can
>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>> sense to me.
>>
>>
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>> UUID[20]? Why doesn't this make sense? I don't follow.
>>
>
> Nothing is invalid, I just think it's better and expresses the intent more
> clearly if you can only assign between UUIDs of the same size.  For
> example, If the UUID class were templated on size, then there would not
> even be such thing as a UUID[0] or a "universally invalid UUID".  There
> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
> would be different things.
>
>
>>
>>
>> As a future cleanup, I think this class should probably be a template
>> such as UUID, and then internally it can store a std::array> N>.  And we can static_assert that N is of a known size if we desire.
>>
>>
>> UUID values are objects contained as members inside of other objects.
>> They all default to start with no preconceived notion of what the UUID
>> should be. IMHO the UUID class is just fine and needs to be able to
>> represent any UUID, from empty uninitialized ones, and be able to be
>> assigned and changed at will.
>>
>>
> Is there ever a use case for changing the number of bytes in a UUID?  If
> you're working with 16-byte UUIDs, does it ever actually happen that now
> you have a 20-byte UUID?  Can you imagine a use case currently where an
> N-byte UUID is being compared against an M-byte UUID in a real-world
> scenario?  If the answer is no, then it may as well be enforced by the
> compiler.
>
>
> The ObjectFile class has a "UUID m_uuid;" member that any object file can
> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20
> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is
> around, and these are current represented as 20 byte UUIDs with just the
> first 4 bytes filled in. So no, we can't enforce this using the compiler. I
> don't see a need to change way from a byte buffer that has the max number
> of bytes needed for any currently supported UUID (20 right now).
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via lldb-commits]

> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
> 
> 
> 
> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  > wrote:
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner > > wrote:
>> 
>> As an aside, I don't really like this class.  For example, You can currently 
>> assign a UUID[16] to a UUID[20].  That doesn't make a lot of sense to me.
> 
> What about an invalid UUID[0] being assigned with a valid UUID[16] or 
> UUID[20]? Why doesn't this make sense? I don't follow.
> 
> Nothing is invalid, I just think it's better and expresses the intent more 
> clearly if you can only assign between UUIDs of the same size.  For example, 
> If the UUID class were templated on size, then there would not even be such 
> thing as a UUID[0] or a "universally invalid UUID".  There would be an 
> "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those would be 
> different things.
>  
> 
>> 
>> As a future cleanup, I think this class should probably be a template such 
>> as UUID, and then internally it can store a std::array.  And 
>> we can static_assert that N is of a known size if we desire.
> 
> UUID values are objects contained as members inside of other objects. They 
> all default to start with no preconceived notion of what the UUID should be. 
> IMHO the UUID class is just fine and needs to be able to represent any UUID, 
> from empty uninitialized ones, and be able to be assigned and changed at will.
> 
> 
> Is there ever a use case for changing the number of bytes in a UUID?  If 
> you're working with 16-byte UUIDs, does it ever actually happen that now you 
> have a 20-byte UUID?  Can you imagine a use case currently where an N-byte 
> UUID is being compared against an M-byte UUID in a real-world scenario?  If 
> the answer is no, then it may as well be enforced by the compiler. 

The ObjectFile class has a "UUID m_uuid;" member that any object file can fill 
in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20 bytes 
UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is around, and 
these are current represented as 20 byte UUIDs with just the first 4 bytes 
filled in. So no, we can't enforce this using the compiler. I don't see a need 
to change way from a byte buffer that has the max number of bytes needed for 
any currently supported UUID (20 right now). ___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

ok, this is fine then. Just need to test somehow.


https://reviews.llvm.org/D40539



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

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:18 AM Stephane Sezer via Phabricator <
revi...@reviews.llvm.org> wrote:

>
>
> The other alternative seems a bit less explicit to me but I don't mind it
> too much. What's the issue with using `std::any_of` exactly?
>
>
In the sense of correctness, nothing is wrong with using std::any_of.  But
if there's more than one way to do it, and one is easier to read /
understand, I just prefer it.  I'm not blocking the CL or anything, it was
just a suggestion in the interest of readability.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:24 AM Zachary Turner  wrote:

> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>
>> As an aside, I don't really like this class.  For example, You can
>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>> sense to me.
>>
>>
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>> UUID[20]? Why doesn't this make sense? I don't follow.
>>
>
> Nothing is invalid, I just think it's better and expresses the intent more
> clearly if you can only assign between UUIDs of the same size.  For
> example, If the UUID class were templated on size, then there would not
> even be such thing as a UUID[0] or a "universally invalid UUID".  There
> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
> would be different things.
>
>
>>
>>
>> As a future cleanup, I think this class should probably be a template
>> such as UUID, and then internally it can store a std::array> N>.  And we can static_assert that N is of a known size if we desire.
>>
>>
>> UUID values are objects contained as members inside of other objects.
>> They all default to start with no preconceived notion of what the UUID
>> should be. IMHO the UUID class is just fine and needs to be able to
>> represent any UUID, from empty uninitialized ones, and be able to be
>> assigned and changed at will.
>>
>>
> Is there ever a use case for changing the number of bytes in a UUID?  If
> you're working with 16-byte UUIDs, does it ever actually happen that now
> you have a 20-byte UUID?  Can you imagine a use case currently where an
> N-byte UUID is being compared against an M-byte UUID in a real-world
> scenario?  If the answer is no, then it may as well be enforced by the
> compiler.
>

Also, if the data is just a `std::array`, and that size can never change,
then memcmps and finds are replaced with equality operators, which is a win
IMO.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Stephane Sezer via Phabricator via lldb-commits]

sas added a comment.

In https://reviews.llvm.org/D40539#937900, @clayborg wrote:

> In https://reviews.llvm.org/D40539#937854, @sas wrote:
>
> > Basically, if you have a `.debug` directory in the same directory where the 
> > original object file is, you can have debug symbols there. For instance, 
> > you can have:
> >
> >   /my/project/myElf.exe
> >   /my/project/.debug/myElf.exe
> >
> >
> > with the first file being a standard stripped elf file, and the second one 
> > being the associated debug symbols.
>
>
> Do adding a FileSpec that contains just the basename will cause us to look 
> for this file in the same directory as the original ELF file in the .debug 
> folder?


Yes.

> Does this occur elsewhere? Why not just construct the correct path in 
> FileSpec to begin with?

It occurs in some other class where we try to locate the debug symbols for a 
given object file. There, we construct the list of paths to look for because we 
use multiple sources to get the paths. We use the setting I mentioned earlier, 
the `.debug` folder, and some order path scheme with `.build-id` folders.


https://reviews.llvm.org/D40539



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

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:

> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>
> As an aside, I don't really like this class.  For example, You can
> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
> sense to me.
>
>
> What about an invalid UUID[0] being assigned with a valid UUID[16] or
> UUID[20]? Why doesn't this make sense? I don't follow.
>

Nothing is invalid, I just think it's better and expresses the intent more
clearly if you can only assign between UUIDs of the same size.  For
example, If the UUID class were templated on size, then there would not
even be such thing as a UUID[0] or a "universally invalid UUID".  There
would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
would be different things.


>
>
> As a future cleanup, I think this class should probably be a template such
> as UUID, and then internally it can store a std::array.  And
> we can static_assert that N is of a known size if we desire.
>
>
> UUID values are objects contained as members inside of other objects. They
> all default to start with no preconceived notion of what the UUID should
> be. IMHO the UUID class is just fine and needs to be able to represent any
> UUID, from empty uninitialized ones, and be able to be assigned and changed
> at will.
>
>
Is there ever a use case for changing the number of bytes in a UUID?  If
you're working with 16-byte UUIDs, does it ever actually happen that now
you have a 20-byte UUID?  Can you imagine a use case currently where an
N-byte UUID is being compared against an M-byte UUID in a real-world
scenario?  If the answer is no, then it may as well be enforced by the
compiler.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

No one is relying on the 16 bytes of zeroes that I know of. UUID::IsValid() is 
the test that everyone uses to tell if the UUID is valid or not. I still prefer 
to just set the length to zero as this does allow us to have a UUID of all 
zeroes if needed.


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D40539#937854, @sas wrote:

> Basically, if you have a `.debug` directory in the same directory where the 
> original object file is, you can have debug symbols there. For instance, you 
> can have:
>
>   /my/project/myElf.exe
>   /my/project/.debug/myElf.exe
>
>
> with the first file being a standard stripped elf file, and the second one 
> being the associated debug symbols.


Do adding a FileSpec that contains just the basename will cause us to look for 
this file in the same directory as the original ELF file in the .debug folder? 
Does this occur elsewhere? Why not just construct the correct path in FileSpec 
to begin with?


https://reviews.llvm.org/D40539



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

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via lldb-commits]

> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
> 
> As an aside, I don't really like this class.  For example, You can currently 
> assign a UUID[16] to a UUID[20].  That doesn't make a lot of sense to me.

What about an invalid UUID[0] being assigned with a valid UUID[16] or UUID[20]? 
Why doesn't this make sense? I don't follow.

> 
> As a future cleanup, I think this class should probably be a template such as 
> UUID, and then internally it can store a std::array.  And we 
> can static_assert that N is of a known size if we desire.

UUID values are objects contained as members inside of other objects. They all 
default to start with no preconceived notion of what the UUID should be. IMHO 
the UUID class is just fine and needs to be able to represent any UUID, from 
empty uninitialized ones, and be able to be assigned and changed at will.

Greg

> 
> On Mon, Nov 27, 2017 at 9:38 PM Davide Italiano via Phabricator 
> mailto:revi...@reviews.llvm.org>> wrote:
> davide added a comment.
> 
> Yes, what Zachary said. Thanks for the cleanup. There's a decent amount of 
> code in lldb that can be written in a very compact way but it's manually 
> unrolled. I think in this case the code produced should be equivalent (and if 
> not, I'd be curious to see the codegen).
> More generally, whenever a function is not in a hot loop, we might consider 
> preferring readability over performances anyway.
> 
> 
> https://reviews.llvm.org/D40537 
> 
> 
> 

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

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Stephane Sezer via Phabricator via lldb-commits]

sas added a comment.

In https://reviews.llvm.org/D40537#937880, @zturner wrote:

> In https://reviews.llvm.org/D40537#937866, @sas wrote:
>
> > In https://reviews.llvm.org/D40537#937196, @zturner wrote:
> >
> > > You could use llvm's range adapters to make this slightly better.
> > >
> > >   auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes);
> > >   return llvm::find(Bytes, 0) != Bytes.end();
> > >   
> > >
> > > Another option would just be `return *this != UUID(m_num_uuid_bytes);`
> >
> >
> > We want at least one **non-zero** element, we don't want a zero-element, so 
> > the proposed code wouldn't work. I'm not sure if there's an llvm utility 
> > that allows doing that directly without having to pass a lambda.
>
>
> Wouldn't the other alternative work, where you just use `operator==` against 
> a default constructed instance?


The other alternative seems a bit less explicit to me but I don't mind it too 
much. What's the issue with using `std::any_of` exactly?


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D40537#937866, @sas wrote:

> In https://reviews.llvm.org/D40537#937196, @zturner wrote:
>
> > You could use llvm's range adapters to make this slightly better.
> >
> >   auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes);
> >   return llvm::find(Bytes, 0) != Bytes.end();
> >   
> >
> > Another option would just be `return *this != UUID(m_num_uuid_bytes);`
>
>
> We want at least one **non-zero** element, we don't want a zero-element, so 
> the proposed code wouldn't work. I'm not sure if there's an llvm utility that 
> allows doing that directly without having to pass a lambda.


Wouldn't the other alternative work, where you just use `operator=` against a 
default constructed instance?


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Stephane Sezer via Phabricator via lldb-commits]

sas added a comment.

In https://reviews.llvm.org/D40537#937862, @clayborg wrote:

> A better solution would be to initialize UUID::m_num_uuid_bytes with zero and 
> only set it to a valid value if we set bytes into it. Then UUID::IsValid() 
> becomes easy:
>
>   bool UUID::IsValid() const { return m_num_uuid_bytes > 0; }
>
>
> This would allows us to actually have a UUID value that is valid and all 
> zeroes. A few comments would need to be fixed as it currently assumes length 
> is 16 or 20.


Yes but the current default constructor of the `UUID` class creates a 16-bytes 
all-zeroes UUID. I'm not sure I want to be changing the default behavior that 
the rest of lldb might be depending on currently.


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Stephane Sezer via Phabricator via lldb-commits]

sas added a comment.

In https://reviews.llvm.org/D40537#937196, @zturner wrote:

> You could use llvm's range adapters to make this slightly better.
>
>   auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes);
>   return llvm::find(Bytes, 0) != Bytes.end();
>   
>
> Another option would just be `return *this != UUID(m_num_uuid_bytes);`


We want at least one **non-zero** element, we don't want a zero-element, so the 
proposed code wouldn't work. I'm not sure if there's an llvm utility that 
allows doing that directly without having to pass a lambda.


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

A better solution would be to initialize UUID::m_num_uuid_bytes with zero and 
only set it to a valid value if we set bytes into it. Then UUID::IsValid() 
becomes easy:

  bool UUID::IsValid() const { return m_num_uuid_bytes > 0; }

This would allows us to actually have a UUID value that is valid and all 
zeroes. A few comments would need to be fixed as it currently assumes length is 
16 or 20.


https://reviews.llvm.org/D40537



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

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Stephane Sezer via Phabricator via lldb-commits]

sas added a comment.

Basically, if you have a `.debug` directory in the same directory where the 
original object file is, you can have debug symbols there. For instance, you 
can have:
/my/project/myElf.exe
/my/project/.debug/myElf.exe

with the first file being a standard stripped elf file, and the second one 
being the associated debug symbols.

One other place where these names can be used is with the 
`target.debug-file-search-paths` setting.


https://reviews.llvm.org/D40539



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

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I am not sure I follow this patch. We are adding a FileSpec whose path is just 
the basename of the current ELF file? What do we do with that? Do we look in 
certain directories to try and find this file? How this this basename added to 
the list end up getting found in the end?


https://reviews.llvm.org/D40539



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

[Lldb-commits] [PATCH] D40536: Simplify UUID constructors

[Stephane Sezer via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL319191: Simplify UUID constructors (authored by sas).

Repository:
  rL LLVM

https://reviews.llvm.org/D40536

Files:
  lldb/trunk/source/Utility/UUID.cpp


Index: lldb/trunk/source/Utility/UUID.cpp
===
--- lldb/trunk/source/Utility/UUID.cpp
+++ lldb/trunk/source/Utility/UUID.cpp
@@ -21,11 +21,10 @@
 
 namespace lldb_private {
 
-UUID::UUID() : m_num_uuid_bytes(16) { ::memset(m_uuid, 0, sizeof(m_uuid)); }
+UUID::UUID() { Clear(); }
 
 UUID::UUID(const UUID &rhs) {
-  m_num_uuid_bytes = rhs.m_num_uuid_bytes;
-  ::memcpy(m_uuid, rhs.m_uuid, sizeof(m_uuid));
+  SetBytes(rhs.m_uuid, rhs.m_num_uuid_bytes);
 }
 
 UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes) {


Index: lldb/trunk/source/Utility/UUID.cpp
===
--- lldb/trunk/source/Utility/UUID.cpp
+++ lldb/trunk/source/Utility/UUID.cpp
@@ -21,11 +21,10 @@
 
 namespace lldb_private {
 
-UUID::UUID() : m_num_uuid_bytes(16) { ::memset(m_uuid, 0, sizeof(m_uuid)); }
+UUID::UUID() { Clear(); }
 
 UUID::UUID(const UUID &rhs) {
-  m_num_uuid_bytes = rhs.m_num_uuid_bytes;
-  ::memcpy(m_uuid, rhs.m_uuid, sizeof(m_uuid));
+  SetBytes(rhs.m_uuid, rhs.m_num_uuid_bytes);
 }
 
 UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r319191 - Simplify UUID constructors

[Stephane Sezer via lldb-commits]

Author: sas
Date: Tue Nov 28 09:50:31 2017
New Revision: 319191

URL: http://llvm.org/viewvc/llvm-project?rev=319191&view=rev
Log:
Simplify UUID constructors

Summary: This remove a small amount of duplicated code.

Reviewers: clayborg, zturner, davide

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D40536

Modified:
lldb/trunk/source/Utility/UUID.cpp

Modified: lldb/trunk/source/Utility/UUID.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/UUID.cpp?rev=319191&r1=319190&r2=319191&view=diff
==
--- lldb/trunk/source/Utility/UUID.cpp (original)
+++ lldb/trunk/source/Utility/UUID.cpp Tue Nov 28 09:50:31 2017
@@ -21,11 +21,10 @@
 
 namespace lldb_private {
 
-UUID::UUID() : m_num_uuid_bytes(16) { ::memset(m_uuid, 0, sizeof(m_uuid)); }
+UUID::UUID() { Clear(); }
 
 UUID::UUID(const UUID &rhs) {
-  m_num_uuid_bytes = rhs.m_num_uuid_bytes;
-  ::memcpy(m_uuid, rhs.m_uuid, sizeof(m_uuid));
+  SetBytes(rhs.m_uuid, rhs.m_num_uuid_bytes);
 }
 
 UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes) {


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

Re: [Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 2:48 AM Pavel Labath via Phabricator via
lldb-commits  wrote:

> labath added a comment.
>
> On 28 November 2017 at 06:12, Zachary Turner via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
>
> > yaml2core would be an excellent idea for a tool.
>
> An (elf) core file is an elf file, with metadata in the elf program
> headers, so I think this would naturally fit into yaml2obj. Unfortunately,
> yaml2obj currently does not support program headers (but there is a TODO to
> add that).  If that TODO is fixed, we would have a yaml representation of
> both core files and elf executables (which we also can't do because they
> have program headers, and yaml2obj strips them).
>

yes but a windows core file (i.e. minidump) is not a coff file.  So I think
a tool like yaml2core would be useful, and for the ELF case it can simply
link against the same code that yaml2obj is using for elv.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

yaml2obj sounds generally like a good solution. I should note that right now 
yaml2obj is only used/tested for the (narrow) use-case of testing the linker. 
For creating debug-info-related testcases, I found it both to high and too low 
an abstraction. Too low, because it currently forces you to manually specify 
details such as Mach-O load commands, and cannot calculate offsets itself (you 
really want some kind of label support), too high, because it can for example 
only synthesize DWARF4 headers in debug info sections. For this reason we often 
went with assembler instead of yaml for llvm-dwarfdump tests. None of the 
problems I mentioned should be unsurmountable, but the tool needs some love in 
order to make it useful for each specific use-case.


https://reviews.llvm.org/D40539



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

[Lldb-commits] [PATCH] D40557: Variable: Fix usage of uninitialised value

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
Herald added a subscriber: mehdi_amini.

Variable::GetValuesForVariableExpressionPath was passing an
uninitialised value for the final_task_on_target argument. On my
compiler/optimization level combo, the final_task_on_target happened to
contain "dereference" in some circumstances, which produced hilarious
results. The same is true for other arguments to the
GetValueForExpressionPath call.

The correct behavior here seems to be to just omit the arguments
altogether and let the default behavior take place.


https://reviews.llvm.org/D40557

Files:
  source/Symbol/Variable.cpp


Index: source/Symbol/Variable.cpp
===
--- source/Symbol/Variable.cpp
+++ source/Symbol/Variable.cpp
@@ -425,14 +425,8 @@
   llvm::StringRef variable_sub_expr_path =
   variable_expr_path.drop_front(variable_name.size());
   if (!variable_sub_expr_path.empty()) {
-ValueObject::ExpressionPathScanEndReason reason_to_stop;
-ValueObject::ExpressionPathEndResultType final_value_type;
-ValueObject::GetValueForExpressionPathOptions options;
-ValueObject::ExpressionPathAftermath final_task_on_target;
-
 valobj_sp = variable_valobj_sp->GetValueForExpressionPath(
-variable_sub_expr_path, &reason_to_stop, &final_value_type, 
options,
-&final_task_on_target);
+variable_sub_expr_path);
 if (!valobj_sp) {
   error.SetErrorStringWithFormat(
   "invalid expression path '%s' for variable '%s'",


Index: source/Symbol/Variable.cpp
===
--- source/Symbol/Variable.cpp
+++ source/Symbol/Variable.cpp
@@ -425,14 +425,8 @@
   llvm::StringRef variable_sub_expr_path =
   variable_expr_path.drop_front(variable_name.size());
   if (!variable_sub_expr_path.empty()) {
-ValueObject::ExpressionPathScanEndReason reason_to_stop;
-ValueObject::ExpressionPathEndResultType final_value_type;
-ValueObject::GetValueForExpressionPathOptions options;
-ValueObject::ExpressionPathAftermath final_task_on_target;
-
 valobj_sp = variable_valobj_sp->GetValueForExpressionPath(
-variable_sub_expr_path, &reason_to_stop, &final_value_type, options,
-&final_task_on_target);
+variable_sub_expr_path);
 if (!valobj_sp) {
   error.SetErrorStringWithFormat(
   "invalid expression path '%s' for variable '%s'",
___
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]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL319162: elf-core: Convert remaining register context to use 
register set maps (authored by labath).

Repository:
  rL LLVM

https://reviews.llvm.org/D40133

Files:
  lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt
  lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h
  
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h
  
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_ppc64le.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_ppc64le.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h
  lldb/trunk/source/Plugins/Process/elf-core/RegisterUtilities.cpp
  lldb/trunk/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.cpp
  lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.h
  lldb/trunk/source/Plugins/Process/elf-core/elf-core-enums.h
  lldb/trunk/source/Plugins/Process/minidump/ThreadMinidump.cpp

Index: lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.h
===
--- lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.h
+++ lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -10,15 +10,11 @@
 #ifndef liblldb_ThreadElfCore_h_
 #define liblldb_ThreadElfCore_h_
 
-// C Includes
-// C++ Includes
-#include 
-
-// Other libraries and framework includes
-// Project includes
+#include "Plugins/Process/elf-core/RegisterUtilities.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "llvm/ADT/DenseMap.h"
+#include 
 
 struct compat_timeval {
   alignas(8) uint64_t tv_sec;
@@ -130,9 +126,7 @@
 
 struct ThreadData {
   lldb_private::DataExtractor gpregset;
-  lldb_private::DataExtractor fpregset;
-  lldb_private::DataExtractor vregset;
-  llvm::DenseMap regsets;
+  std::vector notes;
   lldb::tid_t tid;
   int signo = 0;
   int prstatus_sig = 0;
@@ -179,9 +173,7 @@
   int m_signo;
 
   lldb_private::DataExtractor m_gpregset_data;
-  lldb_private::DataExtractor m_fpregset_data;
-  lldb_private::DataExtractor m_vregset_data;
-  llvm::DenseMap m_regsets_data;
+  std::vector m_notes;
 
   bool CalculateStopInfo() override;
 };
Index: lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h
===
--- lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h
+++ lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h
@@ -10,19 +10,16 @@
 #ifndef liblldb_RegisterContextCorePOSIX_x86_64_h_
 #define liblldb_RegisterContextCorePOSIX_x86_64_h_
 
-// C Includes
-// C++ Includes
-// Other libraries and framework includes
-// Project includes
 #include "Plugins/Process/Utility/RegisterContextPOSIX_x86.h"
+#include "Plugins/Process/elf-core/RegisterUtilities.h"
 
 class RegisterContextCorePOSIX_x86_64 : public RegisterContextPOSIX_x86 {
 public:
   RegisterContextCorePOSIX_x86_64(
   lldb_private::Thread &thread,
   lldb_private::RegisterInfoInterface *register_info,
   const lldb_private::DataExtractor &gpregset,
-  const lldb_private::DataExtractor &fpregset);
+  llvm::ArrayRef notes);
 
   bool ReadRegister(const lldb_private::RegisterInfo *reg_info,
 lldb_private::RegisterValue &value) override;
Index: lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -47,9 +47,7 @@
 //--
 ThreadElfCore::ThreadElfCore(Process &process, const ThreadData &td)
 : Thread(process, td.tid), m_thread_name(td.name), m_thread_reg_ctx_sp(),
-  m_signo(td.signo), m_gpregset_data(td.gpregset),
-  m_fpregset_data(td.fpregset), m_vregset_data(td.vregset),
-  m_regsets_data(td.regsets) 

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

[Pavel Labath via lldb-commits]

Author: labath
Date: Tue Nov 28 03:10:23 2017
New Revision: 319162

URL: http://llvm.org/viewvc/llvm-project?rev=319162&view=rev
Log:
elf-core: Convert remaining register context to use register set maps

In https://reviews.llvm.org/D39681, we started using a map instead
passing a long list of register sets to the ppc64le register context.
However, existing register contexts were still using the old method.

This converts the remaining register contexts to use this approach.
While doing that, I've had to modify the approach a bit:
- the general purpose register set is still kept as a separate field,
because this one is always present, and it's parsing is somewhat
different than that of other register sets.
- since the same register sets have different IDs on different operating
systems, but we use the same register context class to represent
different register sets, I've needed to add a layer of indirection to
translate os-specific constants (e.g. NETBSD::NT_AMD64_FPREGS) into more
generic terms (e.g. floating point register set).

While slightly more complicated, this setup allows for better separation
of concerns. The parsing code in ProcessElfCore can focus on parsing
OS-specific core file notes, and can completely ignore
architecture-specific register sets (by just storing any unrecognised
notes in a map). These notes will then be passed on to the
architecture-specific register context, which can just deal with
architecture specifics, because the OS-specific note types are hidden in
a register set description map.

This way, adding an register set, which is already supported on other
OSes, to a new OS, should in most cases be as simple as adding a new
entry into the register set description map.

Differential Revision: https://reviews.llvm.org/D40133

Added:
lldb/trunk/source/Plugins/Process/elf-core/RegisterUtilities.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterUtilities.h
Removed:
lldb/trunk/source/Plugins/Process/elf-core/elf-core-enums.h
Modified:
lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt
lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.h
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.cpp

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_powerpc.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_ppc64le.cpp

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_ppc64le.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_s390x.h

lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp
lldb/trunk/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.h
lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.cpp
lldb/trunk/source/Plugins/Process/elf-core/ThreadElfCore.h
lldb/trunk/source/Plugins/Process/minidump/ThreadMinidump.cpp

Modified: lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt?rev=319162&r1=319161&r2=319162&view=diff
==
--- lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt (original)
+++ lldb/trunk/source/Plugins/Process/elf-core/CMakeLists.txt Tue Nov 28 
03:10:23 2017
@@ -10,6 +10,7 @@ add_lldb_library(lldbPluginProcessElfCor
   RegisterContextPOSIXCore_ppc64le.cpp
   RegisterContextPOSIXCore_s390x.cpp
   RegisterContextPOSIXCore_x86_64.cpp
+  RegisterUtilities.cpp
 
   LINK_LIBS
 lldbCore

Modified: lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp?rev=319162&r1=319161&r2=319162&view=diff
==
--- lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp (original)
+++ lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp Tue Nov 28 
03:10:23 2017
@@ -32,11 +32,9 @@
 
 #include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
-
-// Project includes
+#include "Plugins/Process/elf-core/RegisterUtilities.h"
 #include "ProcessElfCore.h"
 #include "ThreadElfCore.h"
-#include "elf-core-enums.

[Lldb-commits] [PATCH] D40434: Fix floating point register write on new x86 linux kernels

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL319161: Fix floating point register write on new x86 linux 
kernels (authored by labath).

Repository:
  rL LLVM

https://reviews.llvm.org/D40434

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/trunk/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h
  lldb/trunk/source/Plugins/Process/Utility/RegisterContext_x86.h

Index: lldb/trunk/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h
===
--- lldb/trunk/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h
+++ lldb/trunk/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h
@@ -149,9 +149,10 @@
   RegInfo m_reg_info;
   FPRType
   m_fpr_type; // determines the type of data stored by union FPR, if any.
-  FPR m_fpr;  // floating-point registers including extended register sets.
-  IOVEC m_iovec;  // wrapper for xsave.
-  YMM m_ymm_set;  // copy of ymmh and xmm register halves.
+  lldb_private::FPR m_fpr; // floating-point registers including extended
+   // register sets.
+  lldb_private::IOVEC m_iovec; // wrapper for xsave.
+  lldb_private::YMM m_ymm_set; // copy of ymmh and xmm register halves.
   std::unique_ptr
   m_register_info_ap; // Register Info Interface (FreeBSD or Linux)
 
Index: lldb/trunk/source/Plugins/Process/Utility/RegisterContext_x86.h
===
--- lldb/trunk/source/Plugins/Process/Utility/RegisterContext_x86.h
+++ lldb/trunk/source/Plugins/Process/Utility/RegisterContext_x86.h
@@ -13,8 +13,10 @@
 #include 
 #include 
 
+#include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/Support/Compiler.h"
 
+namespace lldb_private {
 //---
 // i386 ehframe, dwarf regnums
 //---
@@ -313,13 +315,28 @@
 
 LLVM_PACKED_START
 struct XSAVE_HDR {
-  uint64_t xstate_bv; // OS enabled xstate mask to determine the extended states
+  enum class XFeature : uint64_t {
+FP = 1,
+SSE = FP << 1,
+YMM = SSE << 1,
+BNDREGS = YMM << 1,
+BNDCSR = BNDREGS << 1,
+OPMASK = BNDCSR << 1,
+ZMM_Hi256 = OPMASK << 1,
+Hi16_ZMM = ZMM_Hi256 << 1,
+PT = Hi16_ZMM << 1,
+PKRU = PT << 1,
+LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue*/ PKRU)
+  };
+
+  XFeature xstate_bv; // OS enabled xstate mask to determine the extended states
   // supported by the processor
-  uint64_t xcomp_bv;  // Mask to indicate the format of the XSAVE area and of
+  XFeature xcomp_bv;  // Mask to indicate the format of the XSAVE area and of
   // the XRSTOR instruction
   uint64_t reserved1[1];
   uint64_t reserved2[5];
 };
+static_assert(sizeof(XSAVE_HDR) == 64, "XSAVE_HDR layout incorrect");
 LLVM_PACKED_END
 
 // x86 extensions to FXSAVE (i.e. for AVX and MPX processors)
@@ -355,4 +372,8 @@
   size_t iov_len; // sizeof(XSAVE)
 };
 
+LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+
+} // namespace lldb_private
+
 #endif
Index: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
+++ lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
@@ -528,6 +528,22 @@
   return error;
 }
 
+void NativeRegisterContextLinux_x86_64::UpdateXSTATEforWrite(
+uint32_t reg_index) {
+  XSAVE_HDR::XFeature &xstate_bv = m_fpr.xstate.xsave.header.xstate_bv;
+  if (IsFPR(reg_index)) {
+// IsFPR considers both %st and %xmm registers as floating point, but these
+// map to two features. Set both flags, just in case.
+xstate_bv |= XSAVE_HDR::XFeature::FP | XSAVE_HDR::XFeature::SSE;
+  } else if (IsAVX(reg_index)) {
+// Lower bytes of some %ymm registers are shared with %xmm registers.
+xstate_bv |= XSAVE_HDR::XFeature::YMM | XSAVE_HDR::XFeature::SSE;
+  } else if (IsMPX(reg_index)) {
+// MPX registers map to two XSAVE features.
+xstate_bv |= XSAVE_HDR::XFeature::BNDREGS | XSAVE_HDR::XFeature::BNDCSR;
+  }
+}
+
 Status NativeRegisterContextLinux_x86_64::WriteRegister(
 const RegisterInfo *reg_info, const RegisterValue ®_value) {
   assert(reg_info && "reg_info is null");
@@ -538,6 +554,8 @@
? reg_info->name
: "");
 
+  UpdateXSTATEforWrite(reg_index);
+
   if (IsGPR(reg_index))
 return WriteRegisterRaw(reg_index, reg_value);
 
Index: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLi

[Lldb-commits] [lldb] r319161 - Fix floating point register write on new x86 linux kernels

[Pavel Labath via lldb-commits]

Author: labath
Date: Tue Nov 28 02:56:54 2017
New Revision: 319161

URL: http://llvm.org/viewvc/llvm-project?rev=319161&view=rev
Log:
Fix floating point register write on new x86 linux kernels

Summary:
New linux kernels (on systems that support the XSAVES instruction) will
not update the inferior registers unless the corresponding flag in the
XSAVE header is set. Normally this flag will be set in our image of the
XSAVE area (since we obtained it from the kernel), but if the inferior
has never used the corresponding register set, the respective flag can
be clear.

This fixes the issue by making sure we explicitly set the flags
corresponding to the registers we modify. I don't try to precisely match
the flags to set on each write, as the rules could get quite complicated
-- I use a simpler over-approximation instead.

This was already caught by test_fp_register_write, but that was only
because the code that ran before main() did not use some of the register
sets. Since nothing in this test relies on being stopped in main(), I
modify the test to stop at the entry point instead, so we can be sure
the inferior did not have a chance to access these registers.

Reviewers: clayborg, valentinagiusti

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D40434

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
lldb/trunk/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h
lldb/trunk/source/Plugins/Process/Utility/RegisterContext_x86.h

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py?rev=319161&r1=319160&r2=319161&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/register/register_command/TestRegisters.py
 Tue Nov 28 02:56:54 2017
@@ -272,14 +272,18 @@ class RegisterCommandsTestCase(TestBase)
 target = self.dbg.CreateTarget(exe)
 self.assertTrue(target, VALID_TARGET)
 
-lldbutil.run_break_set_by_symbol(
-self, "main", num_expected_locations=-1)
+# Launch the process, stop at the entry point.
+error = lldb.SBError()
+process = target.Launch(
+lldb.SBListener(),
+None, None, # argv, envp
+None, None, None, # stdin/out/err
+self.get_process_working_directory(),
+0, # launch flags
+True, # stop at entry
+error)
+self.assertTrue(error.Success(), "Launch succeeds. Error is :" + 
str(error))
 
-# Launch the process, and do not stop at the entry point.
-process = target.LaunchSimple(
-None, None, self.get_process_working_directory())
-
-process = target.GetProcess()
 self.assertTrue(
 process.GetState() == lldb.eStateStopped,
 PROCESS_STOPPED)

Modified: 
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp?rev=319161&r1=319160&r2=319161&view=diff
==
--- 
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp 
(original)
+++ 
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp 
Tue Nov 28 02:56:54 2017
@@ -528,6 +528,22 @@ NativeRegisterContextLinux_x86_64::ReadR
   return error;
 }
 
+void NativeRegisterContextLinux_x86_64::UpdateXSTATEforWrite(
+uint32_t reg_index) {
+  XSAVE_HDR::XFeature &xstate_bv = m_fpr.xstate.xsave.header.xstate_bv;
+  if (IsFPR(reg_index)) {
+// IsFPR considers both %st and %xmm registers as floating point, but these
+// map to two features. Set both flags, just in case.
+xstate_bv |= XSAVE_HDR::XFeature::FP | XSAVE_HDR::XFeature::SSE;
+  } else if (IsAVX(reg_index)) {
+// Lower bytes of some %ymm registers are shared with %xmm registers.
+xstate_bv |= XSAVE_HDR::XFeature::YMM | XSAVE_HDR::XFeature::SSE;
+  } else if (IsMPX(reg_index)) {
+// MPX registers map to two XSAVE features.
+xstate_bv |= XSAVE_HDR::XFeature::BNDREGS | XSAVE_HDR::XFeature::BNDCSR;
+  }
+}
+
 Status NativeRegisterContextLinux_x86_64::WriteRegister(
 const RegisterInfo *reg_info, const RegisterValue ®_value) {
   assert(reg_info && "reg_info is null");
@@ -538,6 +554,8 @@ Status NativeRegisterContextLinux

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

On 28 November 2017 at 06:12, Zachary Turner via lldb-commits 
 wrote:

> yaml2core would be an excellent idea for a tool.

An (elf) core file is an elf file, with metadata in the elf program headers, so 
I think this would naturally fit into yaml2obj. Unfortunately, yaml2obj 
currently does not support program headers (but there is a TODO to add that).  
If that TODO is fixed, we would have a yaml representation of both core files 
and elf executables (which we also can't do because they have program headers, 
and yaml2obj strips them).

The tricky part there is the specifying the interaction between program headers 
and section headers, as they describe overlapping views of the same data.


https://reviews.llvm.org/D40539



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