[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

2017-10-09 Thread Tamas Berghammer via Phabricator via lldb-commits 
tberghammer added inline comments.



Comment at: source/Host/common/Symbols.cpp:288-294
+  // FIXME: at the moment llvm-dwp doesn't output build ids,
+  // nor does binutils dwp. Thus in the case of DWPs
+  // we skip uuids check. This needs to be fixed
+  // to avoid consistency issues as soon as
+  // llvm-dwp and binutils dwp gain support for build ids.
+  if (file_spec.GetFileNameExtension().GetStringRef() == "dwp" ||
+  mspec.GetUUID() == module_uuid)

What do you think about adding a new argument to 
Symbols::LocateExecutableSymbolFile (with a potential default value) to specify 
if we want to check the UUID and then move this logic to the place where we are 
looking for the dwp file? I think that would make dwp specific logic more 
concise in one place.


Repository:
  rL LLVM

https://reviews.llvm.org/D38568



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

[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

2017-10-09 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the 
unchecked version as noted in inline comments, or remove the function if no one 
is using this function. Just a few quick fixes and this will be good to go.




Comment at: source/Utility/DataExtractor.cpp:566
   size_t byte_size) const {
-  switch (byte_size) {
-  case 1:
-return GetU8(offset_ptr);
-break;
-  case 2:
-return GetU16(offset_ptr);
-break;
-  case 4:
-return GetU32(offset_ptr);
-break;
-  default:
-assert(false && "GetMaxU32 unhandled case!");
-break;
-  }
-  return 0;
+  assert(byte_size <= 4 && "GetMaxU32 unhandled case!");
+  return GetMaxU64(offset_ptr, byte_size);

petpav01 wrote:
> zturner wrote:
> > jingham wrote:
> > > petpav01 wrote:
> > > > jingham wrote:
> > > > > This is trivial, and you didn't change what was there, but this 
> > > > > message makes it sound like this is just something we haven't gotten 
> > > > > to yet.  It's really "You passed in an illegal byte size"...  Might 
> > > > > be clearer if the message said that.
> > > > I was not sure what is the expected behaviour when the input 
> > > > `byte_size` exceeds the size of the return type of each of these 
> > > > `GetMax...()` methods. The current behaviour is to assert this 
> > > > situation but comments describing the methods (in both 
> > > > `DataExtractor.cpp` and `DataExtractor.h`) say that nothing should get 
> > > > extracted in these cases and zero is returned.
> > > > 
> > > > Maybe the patch should go a bit further and clean this up as follows:
> > > > * Remove duplicated comments in `DataExtractor.cpp` for 
> > > > `DataExtractor::GetMaxU32()` and `GetMaxU64()` and keep only their 
> > > > Doxygen versions in `DataExtractor.h`.
> > > > * Update comments in `DataExtractor.h` for 
> > > > `DataExtractor::GetMaxU32()`, `GetMaxU64()`, `GetMaxS64()`, 
> > > > `GetMaxU64Bitfield()` and `GetMaxS64Bitfield()` to match the current 
> > > > implementation.
> > > > * Change assertion text in `DataExtractor::GetMaxU32()` and 
> > > > `GetMaxU64()` from "unhandled case" to "invalid byte size".
> > > > 
> > > > Does this sound reasonable?
> > > The released versions of lldb - at least the ones Apple releases - have 
> > > asserts disabled.   This isn't unique to lldb, clang does the same thing. 
> > >  
> > > 
> > > I do my day-to-day debugging using a TOT build with asserts enabled, and 
> > > we run the testsuite that way so the asserts catch errors at this stage.  
> > > But for the general public, the function will behave as described.  It 
> > > would be great to remove the duplicated docs - that's just begging for 
> > > one or the other to get out of date.  But the descriptions are 
> > > functionally correct.  And  then changing the text to "invalid byte size" 
> > > also seems good to me.
> > Being pedantic, this *is* a functionality change.  Previously, we would 
> > assert on a size of 3 or 0, with this change we will allow those cases 
> > through.
> To explain myself better, what I was thinking about is that e.g. 
> `GetMaxU64()` should have part:
> 
> "\a byte_size should have a value greater than or equal to one and less than 
> or equal to eight since the return value is 64 bits wide. Any \a byte_size 
> values less than 1 or greater than 8 will result in nothing being extracted, 
> and zero being returned."
> 
> changed to:
> 
> "\a byte_size must have a value greater than or equal to one and less than or 
> equal to eight since the return value is 64 bits wide. The behaviour is 
> undefined for any \a byte_size values less than 1 or greater than 8."
> 
> This way the comment provides information that does not depend on whether 
> assertions are enabled or not. The behaviour for `byte_size > 8` is said to 
> be undefined in the updated description because it either results in an 
> assertion failure or some undefined behaviour if asserts are disabled.
> 
> If the behaviour for `byte_size > 4/8` with assertions disabled should 
> actually be that these methods still return 0 and do not advance the offset 
> then the patch has two bugs:
> * The general case added in `GetMaxU64()` is not correct. It returns an 
> unexpected value for `byte_size > 8` and advances the offset.
> * `GetMaxU32()` needs to have `if (byte_size > 4) return 0;` added before it 
> calls `GetMaxU64()` to avoid the same problem for any `byte_size > 4`.
> 
> An additional thing is that the patch causes that `byte_size == 0` is now 
> fully valid and does not assert. This might not be the best idea given that 
> the current descriptions say that `byte_size` values should be in interval 
> [1, 4/8]. I will add the assertion for `byte_size == 0` back in the updated 
> patch so the changes affect/enable only `byte_size` in range [1, 4/8] (which 
> are clear to be valid) and the zero corner case has its behaviour unchanged.
use lldbassert i

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

2017-10-09 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added inline comments.



Comment at: source/Host/common/Symbols.cpp:288-294
+  // FIXME: at the moment llvm-dwp doesn't output build ids,
+  // nor does binutils dwp. Thus in the case of DWPs
+  // we skip uuids check. This needs to be fixed
+  // to avoid consistency issues as soon as
+  // llvm-dwp and binutils dwp gain support for build ids.
+  if (file_spec.GetFileNameExtension().GetStringRef() == "dwp" ||
+  mspec.GetUUID() == module_uuid)

tberghammer wrote:
> What do you think about adding a new argument to 
> Symbols::LocateExecutableSymbolFile (with a potential default value) to 
> specify if we want to check the UUID and then move this logic to the place 
> where we are looking for the dwp file? I think that would make dwp specific 
> logic more concise in one place.
An easier fix is to clear the UUID field from in "module_spec" _before_ you 
call this function. The module spec can be filled in as much as is needed, so 
either clear the UUID in module_spec if it is set, or just don't set it. This 
function should ignore the UUID comparison if the UUID field is not valid in 
module_spec, so if it isn't doing this, then it should


Repository:
  rL LLVM

https://reviews.llvm.org/D38568



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

[Lldb-commits] [PATCH] D38323: Enable breakpoints and read/write GPRs for ppc64le

2017-10-09 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Sorry for the delay, I was out on vacation for a week.


Repository:
  rL LLVM

https://reviews.llvm.org/D38323



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

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

2017-10-09 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

(sorry for the delay, I was out on vacation for a week)


Repository:
  rL LLVM

https://reviews.llvm.org/D38568



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

[Lldb-commits] [lldb] r315221 - Update ABISysV_arm64::RegisterIsVolatile to accept registers prefixed with r

2017-10-09 Thread Stephane Sezer via lldb-commits 
Author: sas
Date: Mon Oct  9 10:49:32 2017
New Revision: 315221

URL: http://llvm.org/viewvc/llvm-project?rev=315221&view=rev
Log:
Update ABISysV_arm64::RegisterIsVolatile to accept registers prefixed with r

Summary:
While the specification says that the 64bit registers are prefixed with
`x`, it seems that many people still use `r`. Until recently, we had been using
the `r` prefix instead of the `x` prefix in ds2. This caused lldb to fail during
unwinding. I think it's reasonable to check for a register prefixed with `r`,
since some people still choose to use `r`.

Reviewers: sas, fjricci, clayborg

Reviewed By: sas, clayborg

Subscribers: aemerson, javed.absar, kristof.beyls

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

Change by Alex Langford 

Modified:
lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp

Modified: lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp?rev=315221&r1=315220&r2=315221&view=diff
==
--- lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp Mon Oct  9 
10:49:32 2017
@@ -2018,7 +2018,7 @@ bool ABISysV_arm64::RegisterIsVolatile(c
 if (name[0] == 'l' && name[1] == 'r') // lr
   return false;
 
-if (name[0] == 'x') {
+if (name[0] == 'x' || name[0] == 'r') {
   // Volatile registers: x0-x18
   // Although documentation says only x19-28 + sp are callee saved
   // We ll also have to treat x30 as non-volatile.


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

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

2017-10-09 Thread Alexander Shaposhnikov via Phabricator via lldb-commits 
alexshap updated this revision to Diff 118250.
alexshap added a comment.

Ignore the UUID comparison if the UUID field is not valid (in this case the 
interface seems to be a bit cleaner (module_spec is the single source of truth 
(without extra parameters))


Repository:
  rL LLVM

https://reviews.llvm.org/D38568

Files:
  source/Host/common/Symbols.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -30,6 +30,7 @@
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/Symbols.h"
 
 #include "lldb/Interpreter/OptionValueFileSpecList.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
@@ -4352,7 +4353,11 @@
 
 SymbolFileDWARFDwp *SymbolFileDWARF::GetDwpSymbolFile() {
   llvm::call_once(m_dwp_symfile_once_flag, [this]() {
-FileSpec dwp_filespec(m_obj_file->GetFileSpec().GetPath() + ".dwp", false);
+ModuleSpec module_spec;
+module_spec.GetFileSpec() = m_obj_file->GetFileSpec();
+module_spec.GetSymbolFileSpec() =
+FileSpec(m_obj_file->GetFileSpec().GetPath() + ".dwp", false);
+FileSpec dwp_filespec = Symbols::LocateExecutableSymbolFile(module_spec);
 if (dwp_filespec.Exists()) {
   m_dwp_symfile = SymbolFileDWARFDwp::Create(GetObjectFile()->GetModule(),
  dwp_filespec);
Index: source/Host/common/Symbols.cpp
===
--- source/Host/common/Symbols.cpp
+++ source/Host/common/Symbols.cpp
@@ -285,7 +285,12 @@
   if (num_specs == 1) {
 ModuleSpec mspec;
 if (specs.GetModuleSpecAtIndex(0, mspec)) {
-  if (mspec.GetUUID() == module_uuid)
+  // Skip the uuids check if module_uuid is invalid.
+  // For example, this happens for *.dwp files since
+  // at the moment llvm-dwp doesn't output build ids,
+  // nor does binutils dwp.
+  if (!module_uuid.IsValid() ||
+  module_uuid == mspec.GetUUID())
 return file_spec;
 }
   }


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -30,6 +30,7 @@
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/Symbols.h"
 
 #include "lldb/Interpreter/OptionValueFileSpecList.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
@@ -4352,7 +4353,11 @@
 
 SymbolFileDWARFDwp *SymbolFileDWARF::GetDwpSymbolFile() {
   llvm::call_once(m_dwp_symfile_once_flag, [this]() {
-FileSpec dwp_filespec(m_obj_file->GetFileSpec().GetPath() + ".dwp", false);
+ModuleSpec module_spec;
+module_spec.GetFileSpec() = m_obj_file->GetFileSpec();
+module_spec.GetSymbolFileSpec() =
+FileSpec(m_obj_file->GetFileSpec().GetPath() + ".dwp", false);
+FileSpec dwp_filespec = Symbols::LocateExecutableSymbolFile(module_spec);
 if (dwp_filespec.Exists()) {
   m_dwp_symfile = SymbolFileDWARFDwp::Create(GetObjectFile()->GetModule(),
  dwp_filespec);
Index: source/Host/common/Symbols.cpp
===
--- source/Host/common/Symbols.cpp
+++ source/Host/common/Symbols.cpp
@@ -285,7 +285,12 @@
   if (num_specs == 1) {
 ModuleSpec mspec;
 if (specs.GetModuleSpecAtIndex(0, mspec)) {
-  if (mspec.GetUUID() == module_uuid)
+  // Skip the uuids check if module_uuid is invalid.
+  // For example, this happens for *.dwp files since
+  // at the moment llvm-dwp doesn't output build ids,
+  // nor does binutils dwp.
+  if (!module_uuid.IsValid() ||
+  module_uuid == mspec.GetUUID())
 return file_spec;
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

2017-10-09 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rL LLVM

https://reviews.llvm.org/D38568



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