[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

2018-07-17 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor created this revision.
Herald added a subscriber: mgorny.

https://reviews.llvm.org/D49415

Files:
  unittests/Utility/CMakeLists.txt
  unittests/Utility/VMRangeTest.cpp

Index: unittests/Utility/VMRangeTest.cpp
===
--- /dev/null
+++ unittests/Utility/VMRangeTest.cpp
@@ -0,0 +1,125 @@
+//===-- VMRangeTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/VMRange.h"
+using namespace lldb_private;
+
+TEST(VMRange, IsValid) {
+  VMRange range;
+  ASSERT_FALSE(range.IsValid());
+
+  range.Reset(0x1, 0x100);
+  ASSERT_TRUE(range.IsValid());
+
+  range.Reset(0x1, 0x1);
+  ASSERT_FALSE(range.IsValid());
+}
+
+TEST(VMRange, Clear) {
+  VMRange range(0x100, 0x200);
+  ASSERT_FALSE(VMRange() == range);
+  range.Clear();
+  ASSERT_TRUE(VMRange() == range);
+}
+
+TEST(VMRange, Comparison) {
+  VMRange range1(0x100, 0x200);
+  VMRange range2(0x100, 0x200);
+  ASSERT_FALSE(range1 != range2);
+  range2.Clear();
+  ASSERT_TRUE(range1 != range2);
+}
+
+TEST(VMRange, Reset) {
+  VMRange range(0x100, 0x200);
+  ASSERT_FALSE(VMRange(0x200, 0x200) == range);
+  range.Reset(0x200, 0x200);
+  ASSERT_TRUE(VMRange(0x200, 0x200) == range);
+}
+
+TEST(VMRange, SetEndAddress) {
+  VMRange range(0x100, 0x200);
+
+  range.SetEndAddress(0xFF);
+  ASSERT_EQ(0U, range.GetByteSize());
+  ASSERT_FALSE(range.IsValid());
+
+  range.SetEndAddress(0x101);
+  ASSERT_EQ(1U, range.GetByteSize());
+  ASSERT_TRUE(range.IsValid());
+}
+
+TEST(VMRange, ContainsAddr) {
+  VMRange range(0x100, 0x200);
+
+  ASSERT_FALSE(range.Contains(0x00));
+  ASSERT_FALSE(range.Contains(0xFE));
+  ASSERT_FALSE(range.Contains(0xFF));
+  ASSERT_TRUE(range.Contains(0x100));
+  ASSERT_TRUE(range.Contains(0x101));
+  ASSERT_TRUE(range.Contains(0x1FF));
+  ASSERT_FALSE(range.Contains(0x200));
+  ASSERT_FALSE(range.Contains(0x201));
+  ASSERT_FALSE(range.Contains(0xFFF));
+}
+
+TEST(VMRange, ContainsRange) {
+  VMRange range(0x100, 0x200);
+
+  ASSERT_FALSE(range.Contains(VMRange(0x0, 0x0)));
+
+  ASSERT_FALSE(range.Contains(VMRange(0x0, 0x100)));
+  ASSERT_FALSE(range.Contains(VMRange(0x0, 0x101)));
+  ASSERT_TRUE(range.Contains(VMRange(0x100, 0x105)));
+  ASSERT_TRUE(range.Contains(VMRange(0x101, 0x105)));
+  ASSERT_TRUE(range.Contains(VMRange(0x100, 0x1FF)));
+  ASSERT_TRUE(range.Contains(VMRange(0x105, 0x200)));
+  ASSERT_FALSE(range.Contains(VMRange(0x105, 0x208)));
+  ASSERT_FALSE(range.Contains(VMRange(0x200, 0x201)));
+
+  range.Clear();
+  ASSERT_FALSE(range.Contains(VMRange(0x0, 0x0)));
+}
+
+TEST(VMRange, Ordering) {
+  VMRange range1(0x44, 0x200);
+  VMRange range2(0x100, 0x1FF);
+  VMRange range3(0x100, 0x200);
+  VMRange range4(0xFFF, 0x100);
+
+  ASSERT_TRUE(range1 <= range1);
+  ASSERT_TRUE(range1 >= range1);
+
+  ASSERT_TRUE(range1 < range2);
+  ASSERT_TRUE(range2 < range3);
+  ASSERT_TRUE(range3 < range4);
+
+  ASSERT_FALSE(range1 > range2);
+  ASSERT_FALSE(range2 > range3);
+  ASSERT_FALSE(range3 > range4);
+}
+
+TEST(VMRange, CollectionContains) {
+  VMRange::collection collection = {VMRange(0x100, 0x105),
+VMRange(0x108, 0x110)};
+
+  ASSERT_FALSE(VMRange::ContainsValue(collection, 0xFF));
+  ASSERT_TRUE(VMRange::ContainsValue(collection, 0x100));
+  ASSERT_FALSE(VMRange::ContainsValue(collection, 0x105));
+  ASSERT_TRUE(VMRange::ContainsValue(collection, 0x109));
+
+  ASSERT_TRUE(VMRange::ContainsRange(collection, VMRange(0x100, 0x104)));
+  ASSERT_TRUE(VMRange::ContainsRange(collection, VMRange(0x108, 0x100)));
+  ASSERT_FALSE(VMRange::ContainsRange(collection, VMRange(0xFF, 0x100)));
+
+  // TODO: Implement and test ContainsRange with values that span multiple
+  // ranges in the collection.
+}
Index: unittests/Utility/CMakeLists.txt
===
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -20,6 +20,7 @@
   UriParserTest.cpp
   UUIDTest.cpp
   VASprintfTest.cpp
+  VMRangeTest.cpp
 
   LINK_LIBS
   lldbUtility
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49406: Invert dependency between lldb-framework and lldb-suite

2018-07-17 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

lgtm


https://reviews.llvm.org/D49406



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

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

Yes, you are right, extra `location = DW_OP_addr(000140004114)` appeared 
after this commit. But I hadn't noticed that, because the test was already 
broken for me on `0fa537f42f1af238c74bf41998dc1af31195839a`. Now I have checked 
one out and ran the test to reveal the cause of the failure. I think it's 
because of different name mangling. I have attached the output of `lldb-test 
symbols` to the message. For tests I use the last version of clang-cl (which I 
build along with lldb), what version do you use (and what version must be used 
in such cases)?

I think, that updating CHECK-SAME expression to allow `location = {{.*}}` is a 
good fix for this, because this patch actually adds a location information.

F6714367: symbols.txt 


Repository:
  rL LLVM

https://reviews.llvm.org/D49018



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

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

I have no opinion on the implementation, but I like that you wrote a 
non-execution test for this, as this will enable us one day to run it on 
non-windows hosts too.




Comment at: lit/SymbolFile/PDB/class-layout.test:50-53
+CHECK-DAG:_List *array[90];
+CHECK-DAG:int x;
+CHECK-DAG:int a;
+CHECK-DAG:float b;

Should these actually be check-**DAG**s? I am assuming the member order will 
impact the final class layout, so you need to match the source code order here. 
(I don't know how hard would it be, but it might be interesting to change the 
dumping code to produce final element offsets, so you can assert the exact 
structure layout).


Repository:
  rL LLVM

https://reviews.llvm.org/D49410



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

2018-07-17 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

Normally this would be clearly a good thing, but the added complication here is 
that this function is part of a class hierarchy, and so  this way you are 
forcing every implementation to take a std::string, even though only one of 
them cares about null-termination.

In performance-critical code, llvm would use `llvm::Twine` as an argument, 
which is able to avoid copies if the input string happens to be null-terminated 
(`toNullTerminatedStringRef`). However, this code is hardly that critical (and 
ScriptInterpreterPython is the only non-trivial class in the hierarchy), so I 
don't think it really matters what you do here.


https://reviews.llvm.org/D49411



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

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

2018-07-17 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

The tests seem fine, but as a matter of style I would suggest two changes:

- replace `ASSERT_XXX` with `EXPECT_XXX`: EXPECT macros will not terminate the 
test, so you'll be able to see additional failures, which is helpful in 
pinpointing the problem. ASSERT is good for cases where the subsequent checks 
make no sense or will crash if the previous check is not satisfied (but that is 
not the case for your checks AFAICT).
- replace `ASSERT_TRUE(foo == bar)` with `ASSERT_EQ(foo, bar)` (and similar for 
other relational operators). Right not that does not matter much, but if 
someone later implements `operator<<` for this class, you will automatically 
get a helpful error message describing the objects instead of a useless "false 
is not true" message.


https://reviews.llvm.org/D49415



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

[Lldb-commits] [lldb] r337261 - Move pretty stack trace printer into driver.

2018-07-17 Thread Jonas Devlieghere via lldb-commits 
Author: jdevlieghere
Date: Tue Jul 17 03:04:19 2018
New Revision: 337261

URL: http://llvm.org/viewvc/llvm-project?rev=337261&view=rev
Log:
Move pretty stack trace printer into driver.

We used to have a pretty stack trace printer in SystemInitializerCommon.
This was disabled on Apple because we didn't want the library to be
setting signal handlers, as this was causing issues when loaded into
Xcode. However, I think it's useful to have this for the LLDB driver, so
I moved it up to use the PrettyStackTraceProgram in the driver's main.

Differential revision: https://reviews.llvm.org/D49377

Modified:
lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
lldb/trunk/tools/driver/Driver.cpp
lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
lldb/trunk/tools/lldb-server/lldb-server.cpp

Modified: lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Initialization/SystemInitializerCommon.cpp?rev=337261&r1=337260&r2=337261&view=diff
==
--- lldb/trunk/source/Initialization/SystemInitializerCommon.cpp (original)
+++ lldb/trunk/source/Initialization/SystemInitializerCommon.cpp Tue Jul 17 
03:04:19 2018
@@ -29,7 +29,6 @@
 #include "lldb/Host/windows/windows.h"
 #endif
 
-#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/TargetSelect.h"
 
 #include 
@@ -63,9 +62,6 @@ void SystemInitializerCommon::Initialize
   }
 #endif
 
-#if not defined(__APPLE__)
-  llvm::EnablePrettyStackTrace();
-#endif
   Log::Initialize();
   HostInfo::Initialize();
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);

Modified: lldb/trunk/tools/driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/Driver.cpp?rev=337261&r1=337260&r2=337261&view=diff
==
--- lldb/trunk/tools/driver/Driver.cpp (original)
+++ lldb/trunk/tools/driver/Driver.cpp Tue Jul 17 03:04:19 2018
@@ -41,7 +41,10 @@
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 #include 
 
 #if !defined(__APPLE__)
@@ -1231,6 +1234,10 @@ main(int argc, char const *argv[])
   const char **argv = argvPointers.data();
 #endif
 
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   SBDebugger::Initialize();
 
   SBHostOS::ThreadCreated("");

Modified: lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MIDriverMain.cpp?rev=337261&r1=337260&r2=337261&view=diff
==
--- lldb/trunk/tools/lldb-mi/MIDriverMain.cpp (original)
+++ lldb/trunk/tools/lldb-mi/MIDriverMain.cpp Tue Jul 17 03:04:19 2018
@@ -28,6 +28,9 @@
 
 // Third party headers:
 #include "lldb/API/SBHostOS.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 #include 
 #include 
 #include 
@@ -169,6 +172,10 @@ int main(int argc, char const *argv[]) {
 #endif //  _WIN32
 #endif // MICONFIG_DEBUG_SHOW_ATTACH_DBG_DLG
 
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   // *** Order is important here ***
   bool bOk = DriverSystemInit();
   if (!bOk) {

Modified: lldb/trunk/tools/lldb-server/lldb-server.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-server/lldb-server.cpp?rev=337261&r1=337260&r2=337261&view=diff
==
--- lldb/trunk/tools/lldb-server/lldb-server.cpp (original)
+++ lldb/trunk/tools/lldb-server/lldb-server.cpp Tue Jul 17 03:04:19 2018
@@ -12,7 +12,10 @@
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 
 #include 
 #include 
@@ -45,6 +48,10 @@ static void terminate() { g_debugger_lif
 // main
 //--
 int main(int argc, char *argv[]) {
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   int option_error = 0;
   const char *progname = argv[0];
   if (argc < 2) {


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

[Lldb-commits] [PATCH] D49377: Move pretty stack trace printer into driver.

2018-07-17 Thread Jonas Devlieghere via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337261: Move pretty stack trace printer into driver. 
(authored by JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49377?vs=155684&id=155834#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49377

Files:
  lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
  lldb/trunk/tools/driver/Driver.cpp
  lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
  lldb/trunk/tools/lldb-server/lldb-server.cpp


Index: lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
@@ -29,7 +29,6 @@
 #include "lldb/Host/windows/windows.h"
 #endif
 
-#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/TargetSelect.h"
 
 #include 
@@ -63,9 +62,6 @@
   }
 #endif
 
-#if not defined(__APPLE__)
-  llvm::EnablePrettyStackTrace();
-#endif
   Log::Initialize();
   HostInfo::Initialize();
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
Index: lldb/trunk/tools/driver/Driver.cpp
===
--- lldb/trunk/tools/driver/Driver.cpp
+++ lldb/trunk/tools/driver/Driver.cpp
@@ -41,7 +41,10 @@
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 #include 
 
 #if !defined(__APPLE__)
@@ -1231,6 +1234,10 @@
   const char **argv = argvPointers.data();
 #endif
 
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   SBDebugger::Initialize();
 
   SBHostOS::ThreadCreated("");
Index: lldb/trunk/tools/lldb-server/lldb-server.cpp
===
--- lldb/trunk/tools/lldb-server/lldb-server.cpp
+++ lldb/trunk/tools/lldb-server/lldb-server.cpp
@@ -12,7 +12,10 @@
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 
 #include 
 #include 
@@ -45,6 +48,10 @@
 // main
 //--
 int main(int argc, char *argv[]) {
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   int option_error = 0;
   const char *progname = argv[0];
   if (argc < 2) {
Index: lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
===
--- lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
+++ lldb/trunk/tools/lldb-mi/MIDriverMain.cpp
@@ -28,6 +28,9 @@
 
 // Third party headers:
 #include "lldb/API/SBHostOS.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 #include 
 #include 
 #include 
@@ -169,6 +172,10 @@
 #endif //  _WIN32
 #endif // MICONFIG_DEBUG_SHOW_ATTACH_DBG_DLG
 
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   // *** Order is important here ***
   bool bOk = DriverSystemInit();
   if (!bOk) {


Index: lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
@@ -29,7 +29,6 @@
 #include "lldb/Host/windows/windows.h"
 #endif
 
-#include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/TargetSelect.h"
 
 #include 
@@ -63,9 +62,6 @@
   }
 #endif
 
-#if not defined(__APPLE__)
-  llvm::EnablePrettyStackTrace();
-#endif
   Log::Initialize();
   HostInfo::Initialize();
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
Index: lldb/trunk/tools/driver/Driver.cpp
===
--- lldb/trunk/tools/driver/Driver.cpp
+++ lldb/trunk/tools/driver/Driver.cpp
@@ -41,7 +41,10 @@
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/API/SBThread.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
 #include 
 
 #if !defined(__APPLE__)
@@ -1231,6 +1234,10 @@
   const char **argv = argvPointers.data();
 #endif
 
+  llvm::StringRef ToolName = argv[0];
+  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
+  llvm::PrettyStackTraceProgram X(argc, argv);
+
   SBDebugger::Initialize();
 
   SBHostOS::ThreadCreated("");
Index: lldb/trunk/tools/lldb-server/

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

2018-07-17 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

I suppose we can add an off-by-default DWP mode so that it can be used for 
integration testing.

However, I wouldn't consider any future DWP changes "tested" if the code is 
only exercised via this mode. As I said above, I think the majority of DWP code 
can be exercised  without even running a process via `lldb-test`. If there are 
any odd corner casees that can be only reached by running the full pipeline, 
then we add a couple of specialized tests which explicitly set MAKE_DWP=YES, 
and are run unconditionally (similar to how we have two debug_names tests run 
via dotest because the functionality was not accessible via lldb-test).

After some thought, I don't think the script idea I came up with initially is 
necessary here, though the situation may be different for DWZ, as the dwz 
utility seems to be quite temperamental -- IIRC it will error out if the input 
file happens to not contain a .debug_info section. For the purposes of 
integration testing I think it would be better to just silently accept these so 
that all tests work "out of the box" with dwz. It might be nice to hide these 
details in a wrapper script.


https://reviews.llvm.org/D48782



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

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

2018-07-17 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM, Thanks!


https://reviews.llvm.org/D49271



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

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

Ah, we need some way to synchronize our work :)

I think in this case it will be better to appreciate the pros and cons of our 
patches, choose the better variant and improve that with the features available 
in another variant. I will investigate your patch to understand the 
differences, can you look at my patch too, please?

I agree with Pavel, the test in your patch looks better, than in my one. I 
think there must be non-execute test in the result patch too.


Repository:
  rL LLVM

https://reviews.llvm.org/D49410



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

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

It seems that you forgot to add the testing `*.cpp` files from the `Input` 
directory to the patch...


Repository:
  rL LLVM

https://reviews.llvm.org/D49410



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

2018-07-17 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added a comment.

In https://reviews.llvm.org/D49411#1164680, @labath wrote:

> Normally this would be clearly a good thing, but the added complication here 
> is that this function is part of a class hierarchy, and so  this way you are 
> forcing every implementation to take a std::string, even though only one of 
> them cares about null-termination.
>
> In performance-critical code, llvm would use `llvm::Twine` as an argument, 
> which is able to avoid copies if the input string happens to be 
> null-terminated (`toNullTerminatedStringRef`). However, this code is hardly 
> that critical (and ScriptInterpreterPython is the only non-trivial class in 
> the hierarchy), so I don't think it really matters what you do here.


Fair points all, Pavel - thanks for chiming in!


https://reviews.llvm.org/D49411



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

[Lldb-commits] [lldb] r337291 - [CMake] Have check-lldb-unit use CMAKE_CURRENT_BINARY_DIR

2018-07-17 Thread Jonas Devlieghere via lldb-commits 
Author: jdevlieghere
Date: Tue Jul 17 08:11:15 2018
New Revision: 337291

URL: http://llvm.org/viewvc/llvm-project?rev=337291&view=rev
Log:
[CMake] Have check-lldb-unit use CMAKE_CURRENT_BINARY_DIR

llvm-lit uses `map_config` directives (generated at configuration-time) to
make it possible to pass a test path relative to the source instead of
the build folder (CMAKE_CURRENT_BINARY_DIR).

However, this doesn't work in the case of swift where the build
directory layout prevents llvm-lit from knowing about lldb and its test
paths. This caused check-lldb-unit to fail in this configuration, while
everything was working as expected upstream. This patch fixes the issue
by passing a path relative to the build directory. This was already the
case for check-lldb-lit.

Modified:
lldb/trunk/lit/CMakeLists.txt

Modified: lldb/trunk/lit/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/CMakeLists.txt?rev=337291&r1=337290&r2=337291&view=diff
==
--- lldb/trunk/lit/CMakeLists.txt (original)
+++ lldb/trunk/lit/CMakeLists.txt Tue Jul 17 08:11:15 2018
@@ -73,7 +73,7 @@ set(LLDB_TEST_PARAMS
 add_lit_testsuite(check-lldb-lit "Running lldb lit test suite"
   ${CMAKE_CURRENT_BINARY_DIR}
   PARAMS lldb_site_config=${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
-   lldb_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
+ lldb_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
   DEPENDS ${LLDB_TEST_DEPS}
   )
 
@@ -85,7 +85,8 @@ if (TARGET clang)
   add_dependencies(check-lldb-lit clang)
 endif()
 
-add_lit_testsuites(LLDB ${CMAKE_CURRENT_SOURCE_DIR}
+add_lit_testsuites(LLDB
+  ${CMAKE_CURRENT_BINARY_DIR}
   PARAMS lldb_site_config=${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
  lldb_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg
   DEPENDS ${LLDB_TEST_DEPS}


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

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

2018-07-17 Thread Stella Stamenova via Phabricator via lldb-commits 
stella.stamenova added a comment.

When you send a review for the fix, please add me to it. Could you also take 
care of the extra space in the output? Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D49018



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

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

2018-07-17 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor updated this revision to Diff 155899.
teemperor added a comment.

- ASSERT_ -> EXPECT_
- Now using gtest's comparison macros where possible.
- Added a PrintTo function as otherwise the gtest comparison macros won't 
compile.


https://reviews.llvm.org/D49415

Files:
  unittests/Utility/CMakeLists.txt
  unittests/Utility/VMRangeTest.cpp

Index: unittests/Utility/VMRangeTest.cpp
===
--- /dev/null
+++ unittests/Utility/VMRangeTest.cpp
@@ -0,0 +1,143 @@
+//===-- VMRangeTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/VMRange.h"
+
+using namespace lldb_private;
+
+namespace lldb_private {
+void PrintTo(const VMRange &v, std::ostream *os) {
+  (*os) << "VMRange(" << v.GetBaseAddress() << ", " << v.GetEndAddress() << ")";
+}
+} // namespace lldb_private
+
+TEST(VMRange, IsValid) {
+  VMRange range;
+  EXPECT_FALSE(range.IsValid());
+
+  range.Reset(0x1, 0x100);
+  EXPECT_TRUE(range.IsValid());
+
+  range.Reset(0x1, 0x1);
+  EXPECT_FALSE(range.IsValid());
+}
+
+TEST(VMRange, Clear) {
+  VMRange range(0x100, 0x200);
+  EXPECT_NE(VMRange(), range);
+  range.Clear();
+  EXPECT_EQ(VMRange(), range);
+}
+
+TEST(VMRange, Comparison) {
+  VMRange range1(0x100, 0x200);
+  VMRange range2(0x100, 0x200);
+  EXPECT_EQ(range1, range2);
+  range2.Clear();
+  EXPECT_NE(range1, range2);
+}
+
+TEST(VMRange, Reset) {
+  VMRange range(0x100, 0x200);
+  EXPECT_FALSE(VMRange(0x200, 0x200) == range);
+  range.Reset(0x200, 0x200);
+  EXPECT_TRUE(VMRange(0x200, 0x200) == range);
+}
+
+TEST(VMRange, SetEndAddress) {
+  VMRange range(0x100, 0x200);
+
+  range.SetEndAddress(0xFF);
+  EXPECT_EQ(0U, range.GetByteSize());
+  EXPECT_FALSE(range.IsValid());
+
+  range.SetEndAddress(0x101);
+  EXPECT_EQ(1U, range.GetByteSize());
+  EXPECT_TRUE(range.IsValid());
+}
+
+TEST(VMRange, ContainsAddr) {
+  VMRange range(0x100, 0x200);
+
+  EXPECT_FALSE(range.Contains(0x00));
+  EXPECT_FALSE(range.Contains(0xFE));
+  EXPECT_FALSE(range.Contains(0xFF));
+  EXPECT_TRUE(range.Contains(0x100));
+  EXPECT_TRUE(range.Contains(0x101));
+  EXPECT_TRUE(range.Contains(0x1FF));
+  EXPECT_FALSE(range.Contains(0x200));
+  EXPECT_FALSE(range.Contains(0x201));
+  EXPECT_FALSE(range.Contains(0xFFF));
+}
+
+TEST(VMRange, ContainsRange) {
+  VMRange range(0x100, 0x200);
+
+  EXPECT_FALSE(range.Contains(VMRange(0x0, 0x0)));
+
+  EXPECT_FALSE(range.Contains(VMRange(0x0, 0x100)));
+  EXPECT_FALSE(range.Contains(VMRange(0x0, 0x101)));
+  EXPECT_TRUE(range.Contains(VMRange(0x100, 0x105)));
+  EXPECT_TRUE(range.Contains(VMRange(0x101, 0x105)));
+  EXPECT_TRUE(range.Contains(VMRange(0x100, 0x1FF)));
+  EXPECT_TRUE(range.Contains(VMRange(0x105, 0x200)));
+  EXPECT_FALSE(range.Contains(VMRange(0x105, 0x208)));
+  EXPECT_FALSE(range.Contains(VMRange(0x200, 0x201)));
+
+  range.Clear();
+  EXPECT_FALSE(range.Contains(VMRange(0x0, 0x0)));
+}
+
+TEST(VMRange, Ordering) {
+  VMRange range1(0x44, 0x200);
+  VMRange range2(0x100, 0x1FF);
+  VMRange range3(0x100, 0x200);
+  VMRange range4(0xFFF, 0x1000);
+
+  EXPECT_LE(range1, range1);
+  EXPECT_GE(range1, range1);
+
+  EXPECT_LT(range1, range2);
+  EXPECT_LT(range2, range3);
+  EXPECT_LT(range3, range4);
+
+  EXPECT_GT(range2, range1);
+  EXPECT_GT(range3, range2);
+  EXPECT_GT(range4, range3);
+
+  // Ensure that < and > are always false when comparing ranges with themselves.
+  EXPECT_FALSE(range1 < range1);
+  EXPECT_FALSE(range2 < range2);
+  EXPECT_FALSE(range3 < range3);
+  EXPECT_FALSE(range4 < range4);
+
+  EXPECT_FALSE(range1 > range1);
+  EXPECT_FALSE(range2 > range2);
+  EXPECT_FALSE(range3 > range3);
+  EXPECT_FALSE(range4 > range4);
+}
+
+TEST(VMRange, CollectionContains) {
+  VMRange::collection collection = {VMRange(0x100, 0x105),
+VMRange(0x108, 0x110)};
+
+  EXPECT_FALSE(VMRange::ContainsValue(collection, 0xFF));
+  EXPECT_TRUE(VMRange::ContainsValue(collection, 0x100));
+  EXPECT_FALSE(VMRange::ContainsValue(collection, 0x105));
+  EXPECT_TRUE(VMRange::ContainsValue(collection, 0x109));
+
+  EXPECT_TRUE(VMRange::ContainsRange(collection, VMRange(0x100, 0x104)));
+  EXPECT_TRUE(VMRange::ContainsRange(collection, VMRange(0x108, 0x100)));
+  EXPECT_FALSE(VMRange::ContainsRange(collection, VMRange(0xFF, 0x100)));
+
+  // TODO: Implement and test ContainsRange with values that span multiple
+  // ranges in the collection.
+}
Index: unittests/Utility/CMakeLists.txt
===
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -20,6 +20,7 @@
   UriParserTest.cpp
   UUIDTest.cpp
   VASprintfTest.cpp
+  VMRa

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

The expression `location = {{.*}},` should process that space correctly. It is 
possible to remove that space in `DWARFExpression::DumpLocation` function, but 
I don't know how necessary is it.

But what about a compiler version? I want to fix the issue if something wrong 
with my testing environment...

I've already added you to https://reviews.llvm.org/D49368. But it's not clear 
enough yet what patch will be applied, https://reviews.llvm.org/D49368 or 
https://reviews.llvm.org/D49410 :) I'll add you to 
https://reviews.llvm.org/D49410 too.


Repository:
  rL LLVM

https://reviews.llvm.org/D49018



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

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

2018-07-17 Thread Stella Stamenova via Phabricator via lldb-commits 
stella.stamenova added a comment.

Please fix the space. The test will process it fine, but that doesn't mean we 
shouldn't fix it. I'll email you with the setup we use for tests.


Repository:
  rL LLVM

https://reviews.llvm.org/D49018



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

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

I think that this patch is more solid and covers more cases, than 
https://reviews.llvm.org/D49368, especially in `CreateLLDBTypeFromPDBType` 
part. But https://reviews.llvm.org/D49368 has also following:

- Filling of a layout info. It allows use of packed types, bitfield structs 
with unnamed fields (unnamed fields themselves are not processed, but named 
fields are located correctly) etc., you can look at 
https://reviews.llvm.org/D49368 test case;
- Adding of method overrides for CXXRecordType.

In my opinion, it's not so difficult to add these features to this patch. What 
do you think about it?




Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:647-648
+auto member_ast_type = type_sp->GetLayoutCompilerType();
+if (!member_ast_type.IsCompleteType())
+  member_ast_type.GetCompleteType();
+// CXX class type member must be parsed and complete ahead.

It's not so important, but I think that these lines can be deleted if arguments 
of subsequent `if` will be flipped.



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:696
+  base_ast_type.GetOpaqueQualType(), access,
+  pdb_base_class->isVirtualBaseClass(), /*base_of_class*/ true);
+  base_classes.push_back(base_spec);

If I understand correctly, `base_of_class` must be `false` for structs. May be 
check the kind of `pdb_udt` here?



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:587
+// only do this once.
+if (result->GetID() == type_uid) {
+  pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result);

I don't fully understand, please, explain me, when does this can be `false`?



Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:588
+if (result->GetID() == type_uid) {
+  pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result);
+}

What are the advantages and disadvantages of immediate (here) and deferred (at 
the `CompleteType` function) completions? I thought that deferred completion is 
a kind of lazy optimization, so we lost its advantages?


Repository:
  rL LLVM

https://reviews.llvm.org/D49410



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

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Aaron Smith via Phabricator via lldb-commits 
asmith updated this revision to Diff 155904.
asmith added a comment.

Adding missing inputs


https://reviews.llvm.org/D49410

Files:
  lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
  lit/SymbolFile/PDB/Inputs/PointerTypeTest.cpp
  lit/SymbolFile/PDB/class-layout.test
  lit/SymbolFile/PDB/pointers.test
  source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  source/Plugins/SymbolFile/PDB/PDBASTParser.h
  source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

Index: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -45,7 +45,7 @@
 #include "llvm/DebugInfo/PDB/PDBSymbolTypeTypedef.h"
 #include "llvm/DebugInfo/PDB/PDBSymbolTypeUDT.h"
 
-#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
+#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h" // For IsCPPMangledName
 #include "Plugins/SymbolFile/PDB/PDBASTParser.h"
 #include "Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h"
 
@@ -557,9 +557,37 @@
   if (pdb_type == nullptr)
 return nullptr;
 
+  // Parse base classes and nested UDTs first.
+  switch (pdb_type->getSymTag()) {
+  case PDB_SymType::UDT:
+  case PDB_SymType::BaseClass: {
+if (auto base_classes =
+pdb_type->findAllChildren()) {
+  while (auto base_class = base_classes->getNext())
+ResolveTypeUID(base_class->getSymIndexId());
+}
+if (pdb_type->getRawSymbol().hasNestedTypes()) {
+  if (auto nested_udts = pdb_type->findAllChildren()) {
+while (auto nested = nested_udts->getNext())
+  ResolveTypeUID(nested->getSymIndexId());
+  }
+}
+  } break;
+  default:
+break;
+  }
+
   lldb::TypeSP result = pdb->CreateLLDBTypeFromPDBType(*pdb_type);
   if (result) {
 m_types.insert(std::make_pair(type_uid, result));
+
+// Complete the type for UDT & BaseClass symbol immediately. BaseClass is
+// parsed by its type which might have been completed before. Make sure we
+// only do this once.
+if (result->GetID() == type_uid) {
+  pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result);
+}
+
 auto type_list = GetTypeList();
 if (type_list)
   type_list->Insert(result);
Index: source/Plugins/SymbolFile/PDB/PDBASTParser.h
===
--- source/Plugins/SymbolFile/PDB/PDBASTParser.h
+++ source/Plugins/SymbolFile/PDB/PDBASTParser.h
@@ -31,6 +31,7 @@
 class PDBSymbol;
 class PDBSymbolData;
 class PDBSymbolTypeBuiltin;
+class PDBSymbolTypeUDT;
 } // namespace pdb
 } // namespace llvm
 
@@ -41,10 +42,17 @@
 
   lldb::TypeSP CreateLLDBTypeFromPDBType(const llvm::pdb::PDBSymbol &type);
 
+  bool CompleteRecordTypeForPDBSymbol(const llvm::pdb::PDBSymbol &pdb_symbol,
+  lldb::TypeSP type_sp);
+
 private:
   bool AddEnumValue(lldb_private::CompilerType enum_type,
 const llvm::pdb::PDBSymbolData &data) const;
 
+  bool
+  AddMembersToRecordType(const lldb_private::CompilerType &udt_compiler_type,
+ const llvm::pdb::PDBSymbolTypeUDT &pdb_udt);
+
   lldb_private::ClangASTContext &m_ast;
   lldb_private::ClangASTImporter m_ast_importer;
 };
Index: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/DeclCXX.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol/ClangExternalASTSourceCommon.h" // For ClangASTMetadata
 #include "lldb/Symbol/ClangUtil.h"
 #include "lldb/Symbol/Declaration.h"
 #include "lldb/Symbol/SymbolFile.h"
@@ -175,6 +176,28 @@
   return compiler_type.GetTypeName();
 }
 
+AccessType TranslateMemberAccess(PDB_MemberAccess access) {
+  if (access == PDB_MemberAccess::Public)
+return lldb::eAccessPublic;
+  if (access == PDB_MemberAccess::Private)
+return lldb::eAccessPrivate;
+  if (access == PDB_MemberAccess::Protected)
+return lldb::eAccessProtected;
+  return lldb::eAccessNone;
+}
+
+AccessType GetDefaultAccessibilityForUdtKind(PDB_UdtType udt_kind) {
+  if (udt_kind == PDB_UdtType::Class)
+return lldb::eAccessPrivate;
+  if (udt_kind == PDB_UdtType::Struct)
+return lldb::eAccessPublic;
+  if (udt_kind == PDB_UdtType::Union)
+return lldb::eAccessPublic;
+  if (udt_kind == PDB_UdtType::Interface)
+return lldb::eAccessPrivate;
+  return lldb::eAccessNone;
+}
+
 bool GetDeclarationForSymbol(const PDBSymbol &symbol, Declaration &decl) {
   auto &raw_sym = symbol.getRawSymbol();
   auto first_line_up = raw_sym.getSrcLineOnTypeDefn();
@@ -216,29 +239,84 @@
   Declaration decl;
 
   switch (type.getSymTag()) {
+  case PDB_SymType::BaseClass: {
+auto ty =
+m_ast.GetSymbolFile()->ResolveTypeUID(type.getRawSymbol().getTypeId());
+return ty ? ty->shared

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

2018-07-17 Thread Zachary Turner via Phabricator via lldb-commits 
zturner added inline comments.



Comment at: lit/SymbolFile/PDB/class-layout.test:3
+RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/ClassLayoutTest.cpp /o 
%T/ClassLayoutTest.cpp.obj
+RUN: link %T/ClassLayoutTest.cpp.obj /DEBUG /nodefaultlib /ENTRY:main 
/OUT:%T/ClassLayoutTest.cpp.exe
+RUN: lldb-test symbols %T/ClassLayoutTest.cpp.exe | FileCheck %s

If you change this to `lld-link` this test may be able to run on non windows 
today.



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:186
+return lldb::eAccessProtected;
+  return lldb::eAccessNone;
+}

I wonder if this would be better as an `llvm_unreachable`.  Being able to 
assume this function returns a sane value would simplify calling code.



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:198
+return lldb::eAccessPrivate;
+  return lldb::eAccessNone;
+}

And here.



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:246
+return ty ? ty->shared_from_this() : nullptr;
+  } break;
   case PDB_SymType::UDT: {

This looks unusual.  Did you clang-format it?



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:271-272
+AccessType access = TranslateMemberAccess(udt->getAccess());
+if (access == lldb::eAccessNone && udt->isNested() &&
+udt->getClassParent()) {
+  access = GetDefaultAccessibilityForUdtKind(udt_kind);

So is this `access == None` a thing that can actually happen?  AFAICT it's 
impossible for `getAccess()` to return anything other than public, protected, 
or private, in which case this code path will never get executed, so we can 
just delete it.



Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:291-295
+if (udt->isConstType())
+  clang_type = clang_type.AddConstModifier();
+
+if (udt->isVolatileType())
+  clang_type = clang_type.AddVolatileModifier();

Can you remind me what this means?  How would an entire type be marked const or 
volatile?  There's no instance variable in play here, these are just types as 
they are declared in the source code, if I'm not mistaken.  Something like 
`const class Foo {int x; };` doesn't make any sense.  So I'm curious under what 
circumstances these 2 if statements would evaluate to true.


https://reviews.llvm.org/D49410



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

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

2018-07-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added inline comments.



Comment at: unittests/Utility/VMRangeTest.cpp:42
+  VMRange range1(0x100, 0x200);
+  VMRange range2(0x100, 0x200);
+  EXPECT_EQ(range1, range2);

Seems like a few more edge cases might be good:

```
EXPECT_NE(range1, VMRange(0x100, 0x1ff));
EXPECT_NE(range1, VMRange(0x100, 0x201));
EXPECT_NE(range1, VMRange(0x0ff, 0x200));
EXPECT_NE(range1, VMRange(0x101 0x200));
```



Comment at: unittests/Utility/VMRangeTest.cpp:71
+  EXPECT_FALSE(range.Contains(0x00));
+  EXPECT_FALSE(range.Contains(0xFE));
+  EXPECT_FALSE(range.Contains(0xFF));

Probably don't need the 0xFE as 0xFF will test the condition of an address 
before 0x100



Comment at: unittests/Utility/VMRangeTest.cpp:78
+  EXPECT_FALSE(range.Contains(0x201));
+  EXPECT_FALSE(range.Contains(0xFFF));
+}

Should probably test the UINT64_MAX address instead of just 0xfff as the last 
edge case



Comment at: unittests/Utility/VMRangeTest.cpp:88
+  EXPECT_FALSE(range.Contains(VMRange(0x0, 0x101)));
+  EXPECT_TRUE(range.Contains(VMRange(0x100, 0x105)));
+  EXPECT_TRUE(range.Contains(VMRange(0x101, 0x105)));

add a zero sized range check where the address is inside the other:

```
  EXPECT_FALSE(range.Contains(VMRange(0x100, 0x100)));
```



Comment at: unittests/Utility/VMRangeTest.cpp:91
+  EXPECT_TRUE(range.Contains(VMRange(0x100, 0x1FF)));
+  EXPECT_TRUE(range.Contains(VMRange(0x105, 0x200)));
+  EXPECT_FALSE(range.Contains(VMRange(0x105, 0x208)));

Add a test to see that an equal range is also contained:
```
  EXPECT_TRUE(range.Contains(VMRange(0x100, 0x200));
```



Comment at: unittests/Utility/VMRangeTest.cpp:92
+  EXPECT_TRUE(range.Contains(VMRange(0x105, 0x200)));
+  EXPECT_FALSE(range.Contains(VMRange(0x105, 0x208)));
+  EXPECT_FALSE(range.Contains(VMRange(0x200, 0x201)));

change 0x208 to 0x201 so it is just outside the range



Comment at: unittests/Utility/VMRangeTest.cpp:92
+  EXPECT_TRUE(range.Contains(VMRange(0x105, 0x200)));
+  EXPECT_FALSE(range.Contains(VMRange(0x105, 0x208)));
+  EXPECT_FALSE(range.Contains(VMRange(0x200, 0x201)));

clayborg wrote:
> change 0x208 to 0x201 so it is just outside the range
Add check for MAX address:

EXPECT_FALSE(range.Contains(VMRange(0x105, UINT64_MAX)));



Comment at: unittests/Utility/VMRangeTest.cpp:103
+  VMRange range3(0x100, 0x200);
+  VMRange range4(0xFFF, 0x1000);
+

Test ordering of empty ranges? Not sure if that makes sense


https://reviews.llvm.org/D49415



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

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

2018-07-17 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov added a comment.

I've removed spaces in such places, but i'm not sure if this won't break 
anything (may be something relies on that spaces). @clayborg, are these spaces 
have some special purpose? I think it's a decor and not really 
important.F6716378: patch.diff 


Repository:
  rL LLVM

https://reviews.llvm.org/D49018



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

[Lldb-commits] [PATCH] D49309: No longer pass a StringRef to the Python API

2018-07-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added inline comments.



Comment at: 
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:860
 PyRefType::Owned,
-Py_BuildValue("(Os)", session_dict.get(), command));
+Py_BuildValue("(Os)", session_dict.get(), 
command_str.c_str()));
 if (pargs.IsValid()) {

Of course this could have been fixed by using "s#" which allows the length to 
be supplied:

```
Py_BuildValue("(Os#)", session_dict.get(), command.data(), 
(int)command.size()));
```


Repository:
  rL LLVM

https://reviews.llvm.org/D49309



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

2018-07-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

Much simpler solution is inlined. Let me know what you think?




Comment at: 
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:859
 PyRefType::Owned,
-Py_BuildValue("(Os)", session_dict.get(), 
command_str.c_str()));
+Py_BuildValue("(Os)", session_dict.get(), command.c_str()));
 if (pargs.IsValid()) {

Remove everything in this patch and just do:

```
Py_BuildValue("(Os#)", session_dict.get(), command.data(), 
(int)command.size()));
```


https://reviews.llvm.org/D49411



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

[Lldb-commits] [PATCH] D49435: Added unit tests for Flags

2018-07-17 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor created this revision.
Herald added a subscriber: mgorny.

https://reviews.llvm.org/D49435

Files:
  unittests/Utility/CMakeLists.txt
  unittests/Utility/FlagsTest.cpp

Index: unittests/Utility/FlagsTest.cpp
===
--- /dev/null
+++ unittests/Utility/FlagsTest.cpp
@@ -0,0 +1,199 @@
+//===-- FlagsTest.cpp ---===-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Flags.h"
+
+using namespace lldb_private;
+
+enum DummyFlags {
+  eFlag0 = 1 << 0,
+  eFlag1 = 1 << 1,
+  eFlag2 = 1 << 2,
+  eAllFlags  = (eFlag0 | eFlag1 | eFlag2)
+};
+
+TEST(Flags, GetBitSize) {
+  Flags f;
+  // Methods like ClearCount depend on this specific value, so we test
+  // against it here.
+  EXPECT_EQ(32U, f.GetBitSize());
+}
+
+TEST(Flags, Reset) {
+  Flags f;
+  f.Reset(0x3);
+  EXPECT_EQ(0x3U, f.Get());
+  EXPECT_EQ(2U, f.SetCount());
+}
+
+TEST(Flags, Clear) {
+  Flags f;
+  f.Reset(0x3);
+  EXPECT_EQ(2U, f.SetCount());
+
+  f.Clear(0x5);
+  EXPECT_EQ(1U, f.SetCount());
+
+  f.Clear();
+  EXPECT_EQ(0U, f.SetCount());
+}
+
+TEST(Flags, AllSet) {
+  Flags f;
+
+  EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+  f.Set(eFlag0);
+  EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+  f.Set(eFlag1);
+  EXPECT_TRUE(f.AllSet(eFlag0 | eFlag1));
+
+  f.Clear(eFlag1);
+  EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+  f.Clear(eFlag0);
+  EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnySet) {
+  Flags f;
+
+  EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+
+  f.Set(eFlag0);
+  EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+  f.Set(eFlag1);
+  EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+  f.Clear(eFlag1);
+  EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+  f.Clear(eFlag0);
+  EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, Test) {
+  Flags f;
+
+  EXPECT_FALSE(f.Test(eFlag0));
+  EXPECT_FALSE(f.Test(eFlag1));
+  EXPECT_FALSE(f.Test(eFlag2));
+
+  f.Set(eFlag0);
+  EXPECT_TRUE(f.Test(eFlag0));
+  EXPECT_FALSE(f.Test(eFlag1));
+  EXPECT_FALSE(f.Test(eFlag2));
+
+  f.Set(eFlag1);
+  EXPECT_TRUE(f.Test(eFlag0));
+  EXPECT_TRUE(f.Test(eFlag1));
+  EXPECT_FALSE(f.Test(eFlag2));
+
+  f.Clear(eFlag0);
+  EXPECT_FALSE(f.Test(eFlag0));
+  EXPECT_TRUE(f.Test(eFlag1));
+  EXPECT_FALSE(f.Test(eFlag2));
+
+  // FIXME: Should Flags assert on Test(eFlag0 | eFlag1) (more than one bit)?
+}
+
+TEST(Flags, AllClear) {
+  Flags f;
+
+  EXPECT_TRUE(f.AllClear(eFlag0 | eFlag1));
+
+  f.Set(eFlag0);
+  EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+  f.Set(eFlag1);
+  f.Clear(eFlag0);
+  EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+  f.Clear(eFlag1);
+  EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnyClear) {
+  Flags f;
+  EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+  f.Set(eFlag0);
+  EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+  f.Set(eFlag1);
+  f.Set(eFlag0);
+  EXPECT_FALSE(f.AnyClear(eFlag0 | eFlag1));
+
+  f.Clear(eFlag1);
+  EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+  f.Clear(eFlag0);
+  EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, IsClear) {
+  Flags f;
+
+  EXPECT_TRUE(f.IsClear(eFlag0));
+  EXPECT_TRUE(f.IsClear(eFlag1));
+
+  f.Set(eFlag0);
+  EXPECT_FALSE(f.IsClear(eFlag0));
+  EXPECT_TRUE(f.IsClear(eFlag1));
+
+  f.Set(eFlag1);
+  EXPECT_FALSE(f.IsClear(eFlag0));
+  EXPECT_FALSE(f.IsClear(eFlag1));
+
+  f.Clear(eFlag0);
+  EXPECT_TRUE(f.IsClear(eFlag0));
+  EXPECT_FALSE(f.IsClear(eFlag1));
+
+  f.Clear(eFlag1);
+  EXPECT_TRUE(f.IsClear(eFlag0));
+  EXPECT_TRUE(f.IsClear(eFlag1));
+}
+
+TEST(Flags, ClearCount) {
+  Flags f;
+  EXPECT_EQ(32U, f.ClearCount());
+
+  f.Set(eFlag0);
+  EXPECT_EQ(31U, f.ClearCount());
+
+  f.Set(eFlag0);
+  EXPECT_EQ(31U, f.ClearCount());
+
+  f.Set(eFlag1);
+  EXPECT_EQ(30U, f.ClearCount());
+
+  f.Set(eAllFlags);
+  EXPECT_EQ(29U, f.ClearCount());
+}
+
+TEST(Flags, SetCount) {
+  Flags f;
+  EXPECT_EQ(0U, f.SetCount());
+
+  f.Set(eFlag0);
+  EXPECT_EQ(1U, f.SetCount());
+
+  f.Set(eFlag0);
+  EXPECT_EQ(1U, f.SetCount());
+
+  f.Set(eFlag1);
+  EXPECT_EQ(2U, f.SetCount());
+
+  f.Set(eAllFlags);
+  EXPECT_EQ(3U, f.SetCount());
+}
Index: unittests/Utility/CMakeLists.txt
===
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -7,6 +7,7 @@
   ConstStringTest.cpp
   CompletionRequestTest.cpp
   EnvironmentTest.cpp
+  FlagsTest.cpp
   FileSpecTest.cpp
   JSONTest.cpp
   LogTest.cpp
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

I have an upcoming patch for adding ARM and ARM64 support to the minidump 
parser and i was curious how you created the invalid minidump files that are 
part of this patch?


Repository:
  rL LLVM

https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

2018-07-17 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

@clayborg  Seems reasonable, even though the Python docs say that we should use 
Py_ssize_t instead of int. I'll update the patch.


https://reviews.llvm.org/D49411



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

Re: [Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-17 Thread Leonard Mosescu via lldb-commits 
Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Regarding the invalid minidumps, I used my favorite hex editor
 to manually corrupt the minidump
streams directory.

On Tue, Jul 17, 2018 at 10:36 AM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> I have an upcoming patch for adding ARM and ARM64 support to the minidump
> parser and i was curious how you created the invalid minidump files that
> are part of this patch?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49202
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-17 Thread Leonard Mosescu via Phabricator via lldb-commits 
lemo added a comment.

Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Regarding the invalid minidumps, I used my favorite hex editor
https://www.sweetscape.com/010editor/ to manually corrupt the minidump
streams directory.


Repository:
  rL LLVM

https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

2018-07-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

In https://reviews.llvm.org/D49202#1165455, @lemo wrote:

> Great timing! ARM support would be most welcome. Are you planning to
>  support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
>  Mentovai just reminded me that the ARM support was added independently and
>  some of the structures are different)


Currently I am adding support for the Breakpad ARM and ARM64 architectures.

> Regarding the invalid minidumps, I used my favorite hex editor
>  https://www.sweetscape.com/010editor/ to manually corrupt the minidump
>  streams directory.

Gotcha. I have a minidump generator I am writing as part of a python module 
that can save minidumps from any live process so I will continue to use that.


Repository:
  rL LLVM

https://reviews.llvm.org/D49202



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

[Lldb-commits] [PATCH] D49406: Invert dependency between lldb-framework and lldb-suite

2018-07-17 Thread Alex Langford via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337311: Invert dependency between lldb-framework and 
lldb-suite (authored by xiaobai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49406

Files:
  lldb/trunk/CMakeLists.txt
  lldb/trunk/cmake/modules/LLDBFramework.cmake
  lldb/trunk/source/API/CMakeLists.txt
  lldb/trunk/tools/driver/CMakeLists.txt


Index: lldb/trunk/cmake/modules/LLDBFramework.cmake
===
--- lldb/trunk/cmake/modules/LLDBFramework.cmake
+++ lldb/trunk/cmake/modules/LLDBFramework.cmake
@@ -41,5 +41,5 @@
   PUBLIC_HEADER "${framework_headers}")
 
 add_dependencies(lldb-framework
-  liblldb
-  lldb-framework-headers)
+  lldb-framework-headers
+  lldb-suite)
Index: lldb/trunk/CMakeLists.txt
===
--- lldb/trunk/CMakeLists.txt
+++ lldb/trunk/CMakeLists.txt
@@ -40,6 +40,7 @@
 # lldb-suite is a dummy target that encompasses all the necessary tools and
 # libraries for building a fully-functioning liblldb.
 add_custom_target(lldb-suite)
+set(LLDB_SUITE_TARGET lldb-suite)
 
 option(LLDB_BUILD_FRAMEWORK "Build the Darwin LLDB.framework" Off)
 if(LLDB_BUILD_FRAMEWORK)
@@ -55,6 +56,7 @@
   set(PRODUCT_NAME "LLDB")
   set(EXECUTABLE_NAME "LLDB")
   set(CURRENT_PROJECT_VERSION "360.99.0")
+  set(LLDB_SUITE_TARGET lldb-framework)
 
   set(LLDB_FRAMEWORK_DIR
 ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_FRAMEWORK_INSTALL_DIR})
@@ -163,9 +165,7 @@
 if (LLDB_BUILD_FRAMEWORK)
   add_custom_target(lldb-framework)
   include(LLDBFramework)
-  add_dependencies(lldb-suite lldb-framework)
 endif()
-add_dependencies(lldb-suite liblldb)
 
 if (NOT LLDB_DISABLE_PYTHON)
 # Add a Post-Build Event to copy over Python files and create the symlink
@@ -187,7 +187,7 @@
 COMMENT "Python script sym-linking LLDB Python API")
 
 # We depend on liblldb and lldb-argdumper being built before we can do 
this step.
-add_dependencies(finish_swig lldb-suite)
+add_dependencies(finish_swig ${LLDB_SUITE_TARGET})
 
 # If we build the readline module, we depend on that happening
 # first.
Index: lldb/trunk/source/API/CMakeLists.txt
===
--- lldb/trunk/source/API/CMakeLists.txt
+++ lldb/trunk/source/API/CMakeLists.txt
@@ -91,6 +91,8 @@
 Support
   )
 
+add_dependencies(lldb-suite liblldb)
+
 if (MSVC)
   set_property(SOURCE ${LLDB_WRAP_PYTHON} APPEND_STRING PROPERTY COMPILE_FLAGS 
" /W0")
 else()
@@ -111,7 +113,7 @@
 set_target_properties(liblldb
   PROPERTIES
   VERSION ${LLDB_VERSION}
-  )
+)
 
 if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
   if (NOT LLDB_EXPORT_ALL_SYMBOLS)
@@ -138,7 +140,7 @@
   set_target_properties(liblldb
 PROPERTIES
 OUTPUT_NAME lldb
-)
+  )
 endif()
 
 if (LLDB_WRAP_PYTHON)
Index: lldb/trunk/tools/driver/CMakeLists.txt
===
--- lldb/trunk/tools/driver/CMakeLists.txt
+++ lldb/trunk/tools/driver/CMakeLists.txt
@@ -24,4 +24,4 @@
   add_definitions( -DIMPORT_LIBLLDB )
 endif()
 
-add_dependencies(lldb lldb-suite)
+add_dependencies(lldb ${LLDB_SUITE_TARGET})


Index: lldb/trunk/cmake/modules/LLDBFramework.cmake
===
--- lldb/trunk/cmake/modules/LLDBFramework.cmake
+++ lldb/trunk/cmake/modules/LLDBFramework.cmake
@@ -41,5 +41,5 @@
   PUBLIC_HEADER "${framework_headers}")
 
 add_dependencies(lldb-framework
-  liblldb
-  lldb-framework-headers)
+  lldb-framework-headers
+  lldb-suite)
Index: lldb/trunk/CMakeLists.txt
===
--- lldb/trunk/CMakeLists.txt
+++ lldb/trunk/CMakeLists.txt
@@ -40,6 +40,7 @@
 # lldb-suite is a dummy target that encompasses all the necessary tools and
 # libraries for building a fully-functioning liblldb.
 add_custom_target(lldb-suite)
+set(LLDB_SUITE_TARGET lldb-suite)
 
 option(LLDB_BUILD_FRAMEWORK "Build the Darwin LLDB.framework" Off)
 if(LLDB_BUILD_FRAMEWORK)
@@ -55,6 +56,7 @@
   set(PRODUCT_NAME "LLDB")
   set(EXECUTABLE_NAME "LLDB")
   set(CURRENT_PROJECT_VERSION "360.99.0")
+  set(LLDB_SUITE_TARGET lldb-framework)
 
   set(LLDB_FRAMEWORK_DIR
 ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_FRAMEWORK_INSTALL_DIR})
@@ -163,9 +165,7 @@
 if (LLDB_BUILD_FRAMEWORK)
   add_custom_target(lldb-framework)
   include(LLDBFramework)
-  add_dependencies(lldb-suite lldb-framework)
 endif()
-add_dependencies(lldb-suite liblldb)
 
 if (NOT LLDB_DISABLE_PYTHON)
 # Add a Post-Build Event to copy over Python files and create the symlink
@@ -187,7 +187,7 @@
 COMMENT "Python script sym-linking LLDB Python API")
 
 # We depend on liblldb and lldb-argdumper being built before we can do this step.
-add_dependencies(finish_swig lldb-suite)
+add_dependencies(finis

[Lldb-commits] [lldb] r337311 - Invert dependency between lldb-framework and lldb-suite

2018-07-17 Thread Alex Langford via lldb-commits 
Author: xiaobai
Date: Tue Jul 17 11:28:51 2018
New Revision: 337311

URL: http://llvm.org/viewvc/llvm-project?rev=337311&view=rev
Log:
Invert dependency between lldb-framework and lldb-suite

Summary:
Currently, if you build lldb-framework the entire framework doesn't
actually build. In order to build the entire framework, you need to actually
build lldb-suite. This abstraction doesn't feel quite right because
lldb-framework truly does depend on lldb-suite (liblldb + related tools).

In this change I want to invert their dependency. This will mean that lldb and
finish_swig will depend on lldb-framework in a framework build, and lldb-suite
otherwise. Instead of adding conditional logic everywhere to handle this, I
introduce LLDB_SUITE_TARGET to handle it.

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

Modified:
lldb/trunk/CMakeLists.txt
lldb/trunk/cmake/modules/LLDBFramework.cmake
lldb/trunk/source/API/CMakeLists.txt
lldb/trunk/tools/driver/CMakeLists.txt

Modified: lldb/trunk/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/CMakeLists.txt?rev=337311&r1=337310&r2=337311&view=diff
==
--- lldb/trunk/CMakeLists.txt (original)
+++ lldb/trunk/CMakeLists.txt Tue Jul 17 11:28:51 2018
@@ -40,6 +40,7 @@ endif()
 # lldb-suite is a dummy target that encompasses all the necessary tools and
 # libraries for building a fully-functioning liblldb.
 add_custom_target(lldb-suite)
+set(LLDB_SUITE_TARGET lldb-suite)
 
 option(LLDB_BUILD_FRAMEWORK "Build the Darwin LLDB.framework" Off)
 if(LLDB_BUILD_FRAMEWORK)
@@ -55,6 +56,7 @@ if(LLDB_BUILD_FRAMEWORK)
   set(PRODUCT_NAME "LLDB")
   set(EXECUTABLE_NAME "LLDB")
   set(CURRENT_PROJECT_VERSION "360.99.0")
+  set(LLDB_SUITE_TARGET lldb-framework)
 
   set(LLDB_FRAMEWORK_DIR
 ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_FRAMEWORK_INSTALL_DIR})
@@ -163,9 +165,7 @@ endif()
 if (LLDB_BUILD_FRAMEWORK)
   add_custom_target(lldb-framework)
   include(LLDBFramework)
-  add_dependencies(lldb-suite lldb-framework)
 endif()
-add_dependencies(lldb-suite liblldb)
 
 if (NOT LLDB_DISABLE_PYTHON)
 # Add a Post-Build Event to copy over Python files and create the symlink
@@ -187,7 +187,7 @@ if (NOT LLDB_DISABLE_PYTHON)
 COMMENT "Python script sym-linking LLDB Python API")
 
 # We depend on liblldb and lldb-argdumper being built before we can do 
this step.
-add_dependencies(finish_swig lldb-suite)
+add_dependencies(finish_swig ${LLDB_SUITE_TARGET})
 
 # If we build the readline module, we depend on that happening
 # first.

Modified: lldb/trunk/cmake/modules/LLDBFramework.cmake
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/cmake/modules/LLDBFramework.cmake?rev=337311&r1=337310&r2=337311&view=diff
==
--- lldb/trunk/cmake/modules/LLDBFramework.cmake (original)
+++ lldb/trunk/cmake/modules/LLDBFramework.cmake Tue Jul 17 11:28:51 2018
@@ -41,5 +41,5 @@ set_target_properties(liblldb PROPERTIES
   PUBLIC_HEADER "${framework_headers}")
 
 add_dependencies(lldb-framework
-  liblldb
-  lldb-framework-headers)
+  lldb-framework-headers
+  lldb-suite)

Modified: lldb/trunk/source/API/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/CMakeLists.txt?rev=337311&r1=337310&r2=337311&view=diff
==
--- lldb/trunk/source/API/CMakeLists.txt (original)
+++ lldb/trunk/source/API/CMakeLists.txt Tue Jul 17 11:28:51 2018
@@ -91,6 +91,8 @@ add_lldb_library(liblldb SHARED
 Support
   )
 
+add_dependencies(lldb-suite liblldb)
+
 if (MSVC)
   set_property(SOURCE ${LLDB_WRAP_PYTHON} APPEND_STRING PROPERTY COMPILE_FLAGS 
" /W0")
 else()
@@ -111,7 +113,7 @@ endif ()
 set_target_properties(liblldb
   PROPERTIES
   VERSION ${LLDB_VERSION}
-  )
+)
 
 if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
   if (NOT LLDB_EXPORT_ALL_SYMBOLS)
@@ -138,7 +140,7 @@ else()
   set_target_properties(liblldb
 PROPERTIES
 OUTPUT_NAME lldb
-)
+  )
 endif()
 
 if (LLDB_WRAP_PYTHON)

Modified: lldb/trunk/tools/driver/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/CMakeLists.txt?rev=337311&r1=337310&r2=337311&view=diff
==
--- lldb/trunk/tools/driver/CMakeLists.txt (original)
+++ lldb/trunk/tools/driver/CMakeLists.txt Tue Jul 17 11:28:51 2018
@@ -24,4 +24,4 @@ if ( CMAKE_SYSTEM_NAME MATCHES "Windows"
   add_definitions( -DIMPORT_LIBLLDB )
 endif()
 
-add_dependencies(lldb lldb-suite)
+add_dependencies(lldb ${LLDB_SUITE_TARGET})


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

[Lldb-commits] [lldb] r337335 - Link the lldb driver ("lldb") against the llvm static

2018-07-17 Thread Jason Molenda via lldb-commits 
Author: jmolenda
Date: Tue Jul 17 16:44:09 2018
New Revision: 337335

URL: http://llvm.org/viewvc/llvm-project?rev=337335&view=rev
Log:
Link the lldb driver ("lldb") against the llvm static
libraries because of the new prettystackprinter dependency.

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=337335&r1=337334&r2=337335&view=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Tue Jul 17 16:44:09 2018
@@ -7285,7 +7285,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
-   shellScript = "cd 
\"${TARGET_BUILD_DIR}/${PUBLIC_HEADERS_FOLDER_PATH}\"\nfor file in 
*.h\ndo\n\t/usr/bin/sed -i '' 's/\\(#include\\)[ 
]*\"lldb\\/\\(API\\/\\)\\{0,1\\}\\(.*\\)\"/\\1 /1' 
\"$file\"\n\t/usr/bin/sed -i '' 's|/1' \"$file\"\n
/usr/bin/sed -i '' 's|http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

2018-07-17 Thread Davide Italiano via Phabricator via lldb-commits 
davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Probably last round of comments. Thanks for your patience!




Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile:7
+include $(LEVEL)/Makefile.rules
+CXXFLAGS += -O0
+CXXFLAGS += -std=c++17

you don't need this, it's implicit (`-O0`)



Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:8
+
+lldbinline.MakeInlineTest(__file__, globals(), [no_debug_info_test])

I do wonder if you need a decorator to indicate that this is a libcxx only test 
(and skip everywhere the library isn't supported).



Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:496
+  "libc++ std::optional synthetic children",
+  ConstString("^std::__(ndk)?1::optional<.+>(( )?&)?$"),
+  stl_synth_flags, true);

I'm a little nervous about this regex, but I think it's fine and I'll let Jim 
take another look in case I missed something.



Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:42-51
+  ValueObjectSP engaged_sp(
+  valobj_sp->GetChildMemberWithName(ConstString("__engaged_"), true));
+
+  if (!engaged_sp)
+return false;
+
+  llvm::StringRef engaged_as_cstring(

This is really obscure to somebody who doesn't understand the type internally. 
Can you add a comment explaining the structure? (here or in the synthetic)


https://reviews.llvm.org/D49271



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

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

2018-07-17 Thread Jim Ingham via Phabricator via lldb-commits 
jingham accepted this revision.
jingham added a comment.

This looks fine.  I agree with Davide that putting some description of the type 
you are formatting in comments somewhere would be make things easier for 
somebody else who might have to fix this (or to you when you've totally 
forgotten how this worked a year from now...)

I'm okay with this if you fix the things Davide asked about (except the regex, 
that looks right to me.)




Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:8
+
+lldbinline.MakeInlineTest(__file__, globals(), [no_debug_info_test])

davide wrote:
> I do wonder if you need a decorator to indicate that this is a libcxx only 
> test (and skip everywhere the library isn't supported).
Yes, you do need to mark this or you will get spurious failures at least on 
Windows, which turns off the libcxx category.



Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:496
+  "libc++ std::optional synthetic children",
+  ConstString("^std::__(ndk)?1::optional<.+>(( )?&)?$"),
+  stl_synth_flags, true);

davide wrote:
> I'm a little nervous about this regex, but I think it's fine and I'll let Jim 
> take another look in case I missed something.
That looks right to me, it's catching the type or a reference to it.


https://reviews.llvm.org/D49271



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