[Lldb-commits] [lldb] r306134 - Fix typo: using && instead of & when evaluating a mask

2017-06-26 Thread Mehdi Amini via lldb-commits
Author: mehdi_amini
Date: Fri Jun 23 13:20:13 2017
New Revision: 306134

URL: http://llvm.org/viewvc/llvm-project?rev=306134=rev
Log:
Fix typo: using && instead of & when evaluating a mask

Summary: Reported by coverity, I don't know how to provide a test.

Reviewers: zturner

Subscribers: lldb-commits, emaste

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

Modified:
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=306134=306133=306134=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Fri Jun 23 
13:20:13 2017
@@ -1693,7 +1693,7 @@ size_t ObjectFileELF::GetSectionHeaderIn
 // ABI Mask doesn't cover N32 and N64 ABI.
 if (header.e_ident[EI_CLASS] == llvm::ELF::ELFCLASS64)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N64;
-else if (header.e_flags && llvm::ELF::EF_MIPS_ABI2)
+else if (header.e_flags & llvm::ELF::EF_MIPS_ABI2)
   arch_flags |= lldb_private::ArchSpec::eMIPSABI_N32;
 break;
   }


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


[Lldb-commits] [PATCH] D34613: Add debug_frame section support

2017-06-26 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.

If the CIEs can change per CIE,l then this patch is fine.




Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:38
   DWARFCallFrameInfo(ObjectFile , lldb::SectionSP ,
  lldb::RegisterKind reg_kind, bool is_eh_frame);
 

labath wrote:
> clayborg wrote:
> > Remove "reg_kind" and "is_eh_frame" and replace with 
> > DWARFCallFrameInfo::Type. See inlined comment below by the CFIVersion enum.
> I think the enum could be used more prominently internally (I haven't checked 
> that more closely yet). However, I think that using it here is wrong. The 
> reason is that the decision which debug_frame version we are parsing does not 
> happen at this level. This is a per-CIE property -- the same debug_frame 
> section can contain CIE's with different version numbers (e.g. if they were 
> compiled with different compilers or flags -- in fact, that's how I built my 
> test module). At this level, all we need to know is whether we are parsing an 
> eh_frame or debug_frame section, which is a still boolean value.
> 
> Theoretically, we could have a separate two-valued (eh, dwarf) enum here, and 
> then, internally, when speaking about a specific CIE, use the 4-valued enum 
> you proposed.
> 
> The register kind argument could probably be removed though.
Ah, interesting. Might be nice to make a DWARF/EHFrame enum later to make the 
code more readable when DWARFCallFrameInfo are created. No need to do that now 
if you need to get this in. 


https://reviews.llvm.org/D34613



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


[Lldb-commits] [PATCH] D34400: Move Connection from Core to Host

2017-06-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

Nice, yea this will be a big help I think.


https://reviews.llvm.org/D34400



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


[Lldb-commits] [PATCH] D34625: Move StructuredData from Core to Utility

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath updated this revision to Diff 103976.
labath added a comment.
Herald added a subscriber: emaste.

Well, PluginProcessPOSIX does not seem to be using anything from Core directly,
although I doubt that is thanks to this patch. It will be a long time before
anything stops depending on Core (my goal here is to do it for Host, but even
that is a long way off).


https://reviews.llvm.org/D34625

Files:
  include/lldb/Breakpoint/Breakpoint.h
  include/lldb/Breakpoint/BreakpointOptions.h
  include/lldb/Core/Event.h
  include/lldb/Core/SearchFilter.h
  include/lldb/Core/StructuredData.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Core/TraceOptions.h
  include/lldb/DataFormatters/TypeSummary.h
  include/lldb/DataFormatters/TypeSynthetic.h
  include/lldb/Host/XML.h
  include/lldb/Interpreter/ScriptInterpreter.h
  include/lldb/Target/InstrumentationRuntime.h
  include/lldb/Target/InstrumentationRuntimeStopInfo.h
  include/lldb/Target/Process.h
  include/lldb/Target/StopInfo.h
  include/lldb/Target/StructuredDataPlugin.h
  include/lldb/Target/SystemRuntime.h
  include/lldb/Target/Thread.h
  include/lldb/Target/ThreadPlanPython.h
  include/lldb/Utility/JSON.h
  include/lldb/Utility/StructuredData.h
  source/API/SBStructuredData.cpp
  source/API/SBThread.cpp
  source/API/SBThreadPlan.cpp
  source/Core/CMakeLists.txt
  source/Core/FormatEntity.cpp
  source/Core/StructuredData.cpp
  source/Host/macosx/Host.mm
  source/Host/windows/Host.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.h
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h
  source/Plugins/InstrumentationRuntime/ASan/ASanRuntime.h
  
source/Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h
  source/Plugins/InstrumentationRuntime/TSan/TSanRuntime.h
  source/Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.h
  source/Plugins/Process/POSIX/CMakeLists.txt
  source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
  source/Plugins/Process/Utility/DynamicRegisterInfo.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Plugins/Process/gdb-remote/ThreadGDBRemote.h
  source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.h
  source/Target/Platform.cpp
  source/Target/ThreadSpec.cpp
  source/Utility/CMakeLists.txt
  source/Utility/JSON.cpp
  source/Utility/StructuredData.cpp
  unittests/Core/CMakeLists.txt
  unittests/Core/StructuredDataTest.cpp
  unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
  unittests/Utility/CMakeLists.txt
  unittests/Utility/Inputs/StructuredData-basic.json
  unittests/Utility/StructuredDataTest.cpp
  unittests/tools/lldb-server/tests/MessageObjects.cpp

Index: unittests/tools/lldb-server/tests/MessageObjects.cpp
===
--- unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -8,7 +8,7 @@
 //===--===//
 
 #include "MessageObjects.h"
-#include "lldb/Core/StructuredData.h"
+#include "lldb/Utility/StructuredData.h"
 #include "llvm/ADT/StringExtras.h"
 #include "gtest/gtest.h"
 
Index: unittests/Utility/StructuredDataTest.cpp
===
--- /dev/null
+++ unittests/Utility/StructuredDataTest.cpp
@@ -0,0 +1,68 @@
+//===-- StructuredDataTest.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/Status.h"
+#include "lldb/Utility/StreamString.h"
+#include "lldb/Utility/StructuredData.h"
+#include "llvm/Support/Path.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+class StructuredDataTest : public testing::Test {
+public:
+  static void SetUpTestCase() {
+s_inputs_folder = llvm::sys::path::parent_path(TestMainArgv0);
+llvm::sys::path::append(s_inputs_folder, "Inputs");
+  }
+
+protected:
+  static llvm::SmallString<128> s_inputs_folder;
+};
+}
+
+llvm::SmallString<128> StructuredDataTest::s_inputs_folder;
+
+TEST_F(StructuredDataTest, StringDump) {
+  std::pair TestCases[] = {
+{ R"(asdfg)", R"("asdfg")" },
+{ R"(as"df)", R"("as\"df")" },
+{ R"(as\df)", R"("as\\df")" },
+  };
+  

[Lldb-commits] [PATCH] D34613: Add debug_frame section support

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In https://reviews.llvm.org/D34613#790745, @clayborg wrote:

> Glad this is happening. Does this mean we won't see the "bad eh frame" 
> warnings anymore on linux? See inlined comments.


Unfortunately, I doubt it. This does nothing about eh_frame parsing, it just 
adds debug_frame support. So, you may start seeing more of those, as we will be 
parsing more stuff. :)

That said, I haven't seen that warning happen, at least not on x86. If you have 
a simple repro case, I could take a look.




Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:38
   DWARFCallFrameInfo(ObjectFile , lldb::SectionSP ,
  lldb::RegisterKind reg_kind, bool is_eh_frame);
 

clayborg wrote:
> Remove "reg_kind" and "is_eh_frame" and replace with 
> DWARFCallFrameInfo::Type. See inlined comment below by the CFIVersion enum.
I think the enum could be used more prominently internally (I haven't checked 
that more closely yet). However, I think that using it here is wrong. The 
reason is that the decision which debug_frame version we are parsing does not 
happen at this level. This is a per-CIE property -- the same debug_frame 
section can contain CIE's with different version numbers (e.g. if they were 
compiled with different compilers or flags -- in fact, that's how I built my 
test module). At this level, all we need to know is whether we are parsing an 
eh_frame or debug_frame section, which is a still boolean value.

Theoretically, we could have a separate two-valued (eh, dwarf) enum here, and 
then, internally, when speaking about a specific CIE, use the 4-valued enum you 
proposed.

The register kind argument could probably be removed though.



Comment at: source/Symbol/FuncUnwinders.cpp:219-221
+  // Only supported on x86 architectures where we get debug_frame from the
+  // compiler that describes the prologue instructions perfectly, and sometimes
+  // the epilogue instructions too.

clayborg wrote:
> What compiler suddenly started including complete unwind info in 
> .debug_frame? They are all supposed, but none ever have. MacOSX x86 and 
> x86_64 has never done this right. I think this if statement is too inclusive. 
> Can you narrow it down? I would love to see an example of this in action. I 
> have never seen the x86 PIC bump code be properly described in any compiler. 
> Are we trying to use this for unwinding frame zero? I would hope not unless 
> we really and carefully know the exact compiler that produced the info. It 
> would be preferable marked with an augmentation code that says "yes, I am 
> really complete everywhere".
This is just copied from the equivalent eh_frame function, line 147 (which 
seems to have been added in r224689). I think what's that trying to say is that 
eh_frame/debug_frame augmentation is only supported on x86.


https://reviews.llvm.org/D34613



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


[Lldb-commits] [PATCH] D34400: Move Connection from Core to Host

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath updated this revision to Diff 103971.
labath added a comment.

This is how things would look like if we move Connection to Utility instead. I
needed to do two things to make this happen:

- Keep Connection::CreateDefaultConnection in Host (now known as 
Host::CreateDefaultConnection), because this actually creates concrete 
instances).
- Move IOObject to Utility as well. Connection interface was depending on this 
class. Although theoretically a forward declaration could suffice, we still 
shouldn't depend on stuff forward-declared in another module. The same 
rationale can apply to this class as well -- it's an abstract interface, which 
can live in Utility, and all concrete instances can (hopefully) stay in Host.


https://reviews.llvm.org/D34400

Files:
  include/lldb/Core/Connection.h
  include/lldb/Host/File.h
  include/lldb/Host/Host.h
  include/lldb/Host/IOObject.h
  include/lldb/Host/MainLoopBase.h
  include/lldb/Host/Socket.h
  include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
  include/lldb/Host/windows/ConnectionGenericFileWindows.h
  include/lldb/Utility/Connection.h
  include/lldb/Utility/IOObject.h
  source/API/SBCommunication.cpp
  source/Core/CMakeLists.txt
  source/Core/Communication.cpp
  source/Core/Connection.cpp
  source/Host/CMakeLists.txt
  source/Host/common/Host.cpp
  source/Host/common/IOObject.cpp
  source/Host/posix/ConnectionFileDescriptorPosix.cpp
  source/Utility/CMakeLists.txt
  source/Utility/Connection.cpp
  source/Utility/IOObject.cpp
  tools/lldb-server/Acceptor.h

Index: tools/lldb-server/Acceptor.h
===
--- tools/lldb-server/Acceptor.h
+++ tools/lldb-server/Acceptor.h
@@ -9,7 +9,7 @@
 #ifndef lldb_server_Acceptor_h_
 #define lldb_server_Acceptor_h_
 
-#include "lldb/Core/Connection.h"
+#include "lldb/Utility/Connection.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Utility/Status.h"
 
Index: source/Utility/IOObject.cpp
===
--- source/Utility/IOObject.cpp
+++ source/Utility/IOObject.cpp
@@ -7,8 +7,9 @@
 //
 //===--===//
 
-#include "lldb/Host/IOObject.h"
+#include "lldb/Utility/IOObject.h"
 
 using namespace lldb_private;
 
 const IOObject::WaitableHandle IOObject::kInvalidHandleValue = -1;
+IOObject::~IOObject() = default;
Index: source/Utility/Connection.cpp
===
--- source/Utility/Connection.cpp
+++ source/Utility/Connection.cpp
@@ -1,14 +1,14 @@
-//===-- IOObject.cpp *- C++ -*-===//
+//===-- Connection.cpp --*- C++ -*-===//
 //
 // The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===--===//
 
-#include "lldb/Host/IOObject.h"
+#include "lldb/Utility/Connection.h"
 
 using namespace lldb_private;
 
-const IOObject::WaitableHandle IOObject::kInvalidHandleValue = -1;
+Connection::~Connection() = default;
Index: source/Utility/CMakeLists.txt
===
--- source/Utility/CMakeLists.txt
+++ source/Utility/CMakeLists.txt
@@ -1,13 +1,15 @@
 add_lldb_library(lldbUtility
   Baton.cpp
+  Connection.cpp
   ConstString.cpp
   DataBufferHeap.cpp
   DataBufferLLVM.cpp
   DataEncoder.cpp
   DataExtractor.cpp
   FastDemangle.cpp
   FileSpec.cpp
   History.cpp
+  IOObject.cpp
   JSON.cpp
   LLDBAssert.cpp
   Log.cpp
Index: source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -16,10 +16,10 @@
 
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Host/Config.h"
-#include "lldb/Host/IOObject.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/SocketAddress.h"
 #include "lldb/Utility/SelectHelper.h"
+#include "lldb/Utility/Timeout.h"
 
 // C Includes
 #include 
@@ -42,7 +42,6 @@
 #include "llvm/ADT/SmallVector.h"
 #endif
 // Project includes
-#include "lldb/Core/Communication.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Socket.h"
Index: source/Host/common/Host.cpp
===
--- source/Host/common/Host.cpp
+++ source/Host/common/Host.cpp
@@ -58,6 +58,7 @@
 #include "lldb/Host/Predicate.h"
 #include "lldb/Host/ProcessLauncher.h"
 #include "lldb/Host/ThreadLauncher.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Target/FileAction.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
 #include "lldb/Target/UnixSignals.h"
@@ -74,6 +75,7 @@
 
 #if defined(_WIN32)
 

[Lldb-commits] [PATCH] D34613: Add debug_frame section support

2017-06-26 Thread Greg Clayton via Phabricator via lldb-commits
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Glad this is happening. Does this mean we won't see the "bad eh frame" warnings 
anymore on linux? See inlined comments.




Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:38
   DWARFCallFrameInfo(ObjectFile , lldb::SectionSP ,
  lldb::RegisterKind reg_kind, bool is_eh_frame);
 

Remove "reg_kind" and "is_eh_frame" and replace with DWARFCallFrameInfo::Type. 
See inlined comment below by the CFIVersion enum.



Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:77
   enum { CFI_AUG_MAX_SIZE = 8, CFI_HEADER_SIZE = 8 };
+  enum CFIVersion {
+CFI_VERSION1 = 1, // DWARF v.2

How about we remove "is_eh_frame" from constructor and make CFIVersion into 
CFIType and make it public. We went from 2 flavors (DWARF and EH frame) to now 
4, so we should just use an enum to define the exact flavor.

```
enum Type {
  EHFrame,
  DWARFv2,
  DWARFv3,
  DWARFv4
};
```

The enum is already in DWARFCallFrameInfo so no need for a prefix.



Comment at: source/Symbol/DWARFCallFrameInfo.cpp:306
+// may ignore these fields?
+if (!m_is_eh_frame && cie_sp->version >= CFI_VERSION4) {
+  cie_sp->address_size = m_cfi_data.GetU8();

Update to use DWARFCallFrameInfo::Type



Comment at: source/Symbol/DWARFCallFrameInfo.cpp:315
+cie_sp->return_addr_reg_num =
+!m_is_eh_frame && cie_sp->version >= CFI_VERSION3
+? static_cast(m_cfi_data.GetULEB128())

Update to use DWARFCallFrameInfo::Type



Comment at: source/Symbol/DWARFCallFrameInfo.cpp:485
+// FDE entries with cie_id == 0 shouldn't be ignored for it.
+if ((cie_id == 0 && m_is_eh_frame) || cie_id == UINT32_MAX || len == 0) {
+  auto cie_sp = ParseCIE(current_entry);

Update to use DWARFCallFrameInfo::Type



Comment at: source/Symbol/DWARFCallFrameInfo.cpp:499
+
+if (!m_is_eh_frame)
+  cie_offset = cie_id;

Update to use DWARFCallFrameInfo::Type



Comment at: source/Symbol/DWARFCallFrameInfo.cpp:567
+  // FDE entries with zeroth cie_offset may occur for debug_frame.
+  assert(!(m_is_eh_frame && 0 == cie_offset) && cie_offset != UINT32_MAX);
 

Update to use DWARFCallFrameInfo::Type



Comment at: source/Symbol/FuncUnwinders.cpp:219-221
+  // Only supported on x86 architectures where we get debug_frame from the
+  // compiler that describes the prologue instructions perfectly, and sometimes
+  // the epilogue instructions too.

What compiler suddenly started including complete unwind info in .debug_frame? 
They are all supposed, but none ever have. MacOSX x86 and x86_64 has never done 
this right. I think this if statement is too inclusive. Can you narrow it down? 
I would love to see an example of this in action. I have never seen the x86 PIC 
bump code be properly described in any compiler. Are we trying to use this for 
unwinding frame zero? I would hope not unless we really and carefully know the 
exact compiler that produced the info. It would be preferable marked with an 
augmentation code that says "yes, I am really complete everywhere".



Comment at: source/Symbol/UnwindTable.cpp:62
+m_debug_frame_up.reset(
+new DWARFCallFrameInfo(m_object_file, sect, eRegisterKindDWARF, 
false));
+  }

Update to use DWARFCallFrameInfo::Type



https://reviews.llvm.org/D34613



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


Re: [Lldb-commits] [PATCH] D34625: Move StructuredData from Core to Utility

2017-06-26 Thread Zachary Turner via lldb-commits
Can you run the analyze deps script before and after this patch, and update
the cmake files if any deps are no longer necessary?
On Mon, Jun 26, 2017 at 8:24 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath created this revision.
> Herald added subscribers: mgorny, kubamracek.
>
> It had a dependency on StringConvert and file reading code, which is not
> in Utility. I've replaced that code by equivalent llvm operations.
>
> I've added a unit test to demonstrate that parsing a file still works.
>
>
> https://reviews.llvm.org/D34625
>
> Files:
>   include/lldb/Breakpoint/Breakpoint.h
>   include/lldb/Breakpoint/BreakpointOptions.h
>   include/lldb/Core/Event.h
>   include/lldb/Core/SearchFilter.h
>   include/lldb/Core/StructuredData.h
>   include/lldb/Core/StructuredDataImpl.h
>   include/lldb/Core/TraceOptions.h
>   include/lldb/DataFormatters/TypeSummary.h
>   include/lldb/DataFormatters/TypeSynthetic.h
>   include/lldb/Host/XML.h
>   include/lldb/Interpreter/ScriptInterpreter.h
>   include/lldb/Target/InstrumentationRuntime.h
>   include/lldb/Target/InstrumentationRuntimeStopInfo.h
>   include/lldb/Target/Process.h
>   include/lldb/Target/StopInfo.h
>   include/lldb/Target/StructuredDataPlugin.h
>   include/lldb/Target/SystemRuntime.h
>   include/lldb/Target/Thread.h
>   include/lldb/Target/ThreadPlanPython.h
>   include/lldb/Utility/JSON.h
>   include/lldb/Utility/StructuredData.h
>   source/API/SBStructuredData.cpp
>   source/API/SBThread.cpp
>   source/API/SBThreadPlan.cpp
>   source/Core/CMakeLists.txt
>   source/Core/FormatEntity.cpp
>   source/Core/StructuredData.cpp
>   source/Host/macosx/Host.mm
>   source/Host/windows/Host.cpp
>   source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.h
>   source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
>   source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h
>   source/Plugins/InstrumentationRuntime/ASan/ASanRuntime.h
>
> source/Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h
>   source/Plugins/InstrumentationRuntime/TSan/TSanRuntime.h
>   source/Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h
>   source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
>   source/Plugins/OperatingSystem/Python/OperatingSystemPython.h
>   source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
>   source/Plugins/Process/Utility/DynamicRegisterInfo.h
>   source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
>
> source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
>   source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
>   source/Plugins/Process/gdb-remote/ThreadGDBRemote.h
>   source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
>   source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.h
>   source/Target/Platform.cpp
>   source/Target/ThreadSpec.cpp
>   source/Utility/CMakeLists.txt
>   source/Utility/JSON.cpp
>   source/Utility/StructuredData.cpp
>   unittests/Core/CMakeLists.txt
>   unittests/Core/StructuredDataTest.cpp
>   unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
>   unittests/Utility/CMakeLists.txt
>   unittests/Utility/Inputs/StructuredData-basic.json
>   unittests/Utility/StructuredDataTest.cpp
>   unittests/tools/lldb-server/tests/MessageObjects.cpp
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits
ravitheja added a comment.

In https://reviews.llvm.org/D33674#790653, @labath wrote:

> In https://reviews.llvm.org/D33674#790595, @ravitheja wrote:
>
> > Yes you can start looking at it. The thing with munmap stuff is the 
> > following, you basically don't want to give the user an opportunity to have 
> > an uninitialized or instances which have been destroyed but not deleted.
> >  So I removed the Destroy function from the public space. Now the user need 
> > not worry about Destroy function.
>
>
> Ok, so that was one of my reasons for doing this. The other one is internal 
> code cleanlyness -- it's much easier to verify that the code is healthy by 
> just looking at it when the initialization and deinitialization is close 
> together. unique_ptr allows you to do that. In this code
>
>   if ((result = mmap(..., size)) != MAP_FAILED)
> ptr_up = std::unique_ptr(result, mmap_deleter(size));
>
>
> the init and deinit code is on adjecant line, and it's guaranteed than memory 
> will be freed. Here:
>
>   first_function() {
>   ptr = mmap(..., size);
>   ref=ArrayRef(ptr, size-1);
>   ...
>   }
>  
>   ...
>  
>   second_function() {
> ...
> // Remember to increment size by one
> munmap(ref.data(), ref.size()+1);
> ... 
>   }
>
>
> it's not so obvious because the code is far apart and you need to carry state 
> around. To verify correctness I need to look at the two pieces of code, and 
> then find all possible code paths between them.


Ok I will write it like that. Please tell me if u want more changes, I will do 
them all together for the next patch.




Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

labath wrote:
> ravitheja wrote:
> > labath wrote:
> > > Isn't the drop-back expression equivalent to 
> > > `src.take_front(src_cyc_index)`?
> > The problem with that is I don't see the documentation of some functions 
> > and I have to dig in the code.
> > Which is why I did not used it.
> That's fine, there are a lot of these APIs and it's not possible to remember 
> all of them.  However, now that you know about the function, wouldn't you 
> agree that it's cleaner to use it?
Ok I will use that.


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits
ravitheja added a comment.

>> Regarding the testing strategy, we have tests at two levels, one at the SB 
>> API level and the other at the tool level.
> 
> Cool, are you going to put some of those tests in this patch?

The problem with uploading tests is that they need minimum Broadwell machine 
and a newer Linux kernel (> 4.2 something )


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D34625: Move StructuredData from Core to Utility

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath created this revision.
Herald added subscribers: mgorny, kubamracek.

It had a dependency on StringConvert and file reading code, which is not
in Utility. I've replaced that code by equivalent llvm operations.

I've added a unit test to demonstrate that parsing a file still works.


https://reviews.llvm.org/D34625

Files:
  include/lldb/Breakpoint/Breakpoint.h
  include/lldb/Breakpoint/BreakpointOptions.h
  include/lldb/Core/Event.h
  include/lldb/Core/SearchFilter.h
  include/lldb/Core/StructuredData.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/Core/TraceOptions.h
  include/lldb/DataFormatters/TypeSummary.h
  include/lldb/DataFormatters/TypeSynthetic.h
  include/lldb/Host/XML.h
  include/lldb/Interpreter/ScriptInterpreter.h
  include/lldb/Target/InstrumentationRuntime.h
  include/lldb/Target/InstrumentationRuntimeStopInfo.h
  include/lldb/Target/Process.h
  include/lldb/Target/StopInfo.h
  include/lldb/Target/StructuredDataPlugin.h
  include/lldb/Target/SystemRuntime.h
  include/lldb/Target/Thread.h
  include/lldb/Target/ThreadPlanPython.h
  include/lldb/Utility/JSON.h
  include/lldb/Utility/StructuredData.h
  source/API/SBStructuredData.cpp
  source/API/SBThread.cpp
  source/API/SBThreadPlan.cpp
  source/Core/CMakeLists.txt
  source/Core/FormatEntity.cpp
  source/Core/StructuredData.cpp
  source/Host/macosx/Host.mm
  source/Host/windows/Host.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.h
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h
  source/Plugins/InstrumentationRuntime/ASan/ASanRuntime.h
  
source/Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h
  source/Plugins/InstrumentationRuntime/TSan/TSanRuntime.h
  source/Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.h
  source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
  source/Plugins/Process/Utility/DynamicRegisterInfo.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Plugins/Process/gdb-remote/ThreadGDBRemote.h
  source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.h
  source/Target/Platform.cpp
  source/Target/ThreadSpec.cpp
  source/Utility/CMakeLists.txt
  source/Utility/JSON.cpp
  source/Utility/StructuredData.cpp
  unittests/Core/CMakeLists.txt
  unittests/Core/StructuredDataTest.cpp
  unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
  unittests/Utility/CMakeLists.txt
  unittests/Utility/Inputs/StructuredData-basic.json
  unittests/Utility/StructuredDataTest.cpp
  unittests/tools/lldb-server/tests/MessageObjects.cpp

Index: unittests/tools/lldb-server/tests/MessageObjects.cpp
===
--- unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -8,7 +8,7 @@
 //===--===//
 
 #include "MessageObjects.h"
-#include "lldb/Core/StructuredData.h"
+#include "lldb/Utility/StructuredData.h"
 #include "llvm/ADT/StringExtras.h"
 #include "gtest/gtest.h"
 
Index: unittests/Utility/StructuredDataTest.cpp
===
--- /dev/null
+++ unittests/Utility/StructuredDataTest.cpp
@@ -0,0 +1,68 @@
+//===-- StructuredDataTest.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/Status.h"
+#include "lldb/Utility/StreamString.h"
+#include "lldb/Utility/StructuredData.h"
+#include "llvm/Support/Path.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+class StructuredDataTest : public testing::Test {
+public:
+  static void SetUpTestCase() {
+s_inputs_folder = llvm::sys::path::parent_path(TestMainArgv0);
+llvm::sys::path::append(s_inputs_folder, "Inputs");
+  }
+
+protected:
+  static llvm::SmallString<128> s_inputs_folder;
+};
+}
+
+llvm::SmallString<128> StructuredDataTest::s_inputs_folder;
+
+TEST_F(StructuredDataTest, StringDump) {
+  std::pair TestCases[] = {
+{ R"(asdfg)", R"("asdfg")" },
+{ R"(as"df)", R"("as\"df")" },
+{ R"(as\df)", R"("as\\df")" },
+  };
+  for(auto P : TestCases) {
+StreamString S;
+const bool pretty_print = false;
+

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In https://reviews.llvm.org/D33674#790595, @ravitheja wrote:

> Yes you can start looking at it. The thing with munmap stuff is the 
> following, you basically don't want to give the user an opportunity to have 
> an uninitialized or instances which have been destroyed but not deleted.
>  So I removed the Destroy function from the public space. Now the user need 
> not worry about Destroy function.


Ok, so that was one of my reasons for doing this. The other one is internal 
code cleanlyness -- it's much easier to verify that the code is healthy by just 
looking at it when the initialization and deinitialization is close together. 
unique_ptr allows you to do that. In this code

  if ((result = mmap(..., size)) != MAP_FAILED)
ptr_up = std::unique_ptr(result, mmap_deleter(size));

the init and deinit code is on adjecant line, and it's guaranteed than memory 
will be freed. Here:

  first_function() {
  ptr = mmap(..., size);
  ref=ArrayRef(ptr, size-1);
  ...
  }
  
  ...
  
  second_function() {
...
// Remember to increment size by one
munmap(ref.data(), ref.size()+1);
... 
  }

it's not so obvious because the code is far apart and you need to carry state 
around. To verify correctness I need to look at the two pieces of code, and 
then find all possible code paths between them.

> Regarding the testing strategy, we have tests at two levels, one at the SB 
> API level and the other at the tool level.

Cool, are you going to put some of those tests in this patch?




Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

ravitheja wrote:
> labath wrote:
> > Isn't the drop-back expression equivalent to 
> > `src.take_front(src_cyc_index)`?
> The problem with that is I don't see the documentation of some functions and 
> I have to dig in the code.
> Which is why I did not used it.
That's fine, there are a lot of these APIs and it's not possible to remember 
all of them.  However, now that you know about the function, wouldn't you agree 
that it's cleaner to use it?


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits
ravitheja added inline comments.



Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

labath wrote:
> Isn't the drop-back expression equivalent to `src.take_front(src_cyc_index)`?
The problem with that is I don't see the documentation of some functions and I 
have to dig in the code.
Which is why I did not used it.


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits
ravitheja added a comment.

Yes you can start looking at it. The thing with munmap stuff is the following, 
you basically don't want to give the user an opportunity to have an 
uninitialized or instances which have been destroyed but not deleted.
So I removed the Destroy function from the public space. Now the user need not 
worry about Destroy function.

Regarding the testing strategy, we have tests at two levels, one at the SB API 
level and the other at the tool level.


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

Should I start looking at this or you have more changes planned? I still see 
manual memory management in the Destroy function...

BTW, what's your testing strategy for this?




Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector parts = {
+  src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+

Isn't the drop-back expression equivalent to `src.take_front(src_cyc_index)`?


https://reviews.llvm.org/D33674



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


[Lldb-commits] [PATCH] D34613: Add debug_frame section support

2017-06-26 Thread Tatyana Krasnukha via Phabricator via lldb-commits
tatyana-krasnukha added a comment.

Works well for me. Thank you a lot for bringing it up to scratch!


https://reviews.llvm.org/D34613



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


[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

2017-06-26 Thread Ravitheja Addepally via Phabricator via lldb-commits
ravitheja updated this revision to Diff 103943.
ravitheja marked 6 inline comments as done.
ravitheja added a comment.

More changes from past feedback.


https://reviews.llvm.org/D33674

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/linux/Support.h
  source/Host/linux/Support.cpp
  source/Plugins/Process/Linux/CMakeLists.txt
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/ProcessorTrace.cpp
  source/Plugins/Process/Linux/ProcessorTrace.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  unittests/Process/CMakeLists.txt
  unittests/Process/Linux/
  unittests/Process/Linux/CMakeLists.txt
  unittests/Process/Linux/ProcessorTraceTest.cpp

Index: unittests/Process/Linux/ProcessorTraceTest.cpp
===
--- /dev/null
+++ unittests/Process/Linux/ProcessorTraceTest.cpp
@@ -0,0 +1,152 @@
+//===-- ProcessorTraceMonitorTest.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 "ProcessorTrace.h"
+#include "llvm/ADT/ArrayRef.h"
+// C Includes
+
+// C++ Includes
+
+using namespace lldb_private;
+using namespace process_linux;
+
+size_t ReadCylicBufferWrapper(void *buf, size_t buf_size, void *cyc_buf,
+  size_t cyc_buf_size, size_t cyc_start,
+  size_t offset) {
+  llvm::MutableArrayRef dst(reinterpret_cast(buf),
+ buf_size);
+  llvm::MutableArrayRef src(reinterpret_cast(cyc_buf),
+ cyc_buf_size);
+  ProcessorTraceMonitor::ReadCyclicBuffer(dst, src, cyc_start, offset);
+  return dst.size();
+}
+
+TEST(CyclicBuffer, EdgeCases) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char bigger_buffer[10] = {};
+  char equal_size_buffer[7] = {};
+  char smaller_buffer[4] = {};
+
+  // empty buffer to read into
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, 0, cyclic_buffer,
+  sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // empty cyclic buffer
+  bytes_read = ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+  cyclic_buffer, 0, 3, 0);
+  ASSERT_EQ(0, bytes_read);
+
+  // bigger offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 6);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong offset
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+
+  // wrong start
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, sizeof(smaller_buffer),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 7);
+  ASSERT_EQ(0, bytes_read);
+}
+
+TEST(CyclicBuffer, EqualSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  char cyclic[] = "cyclic";
+  for (int i = 0; i < sizeof(cyclic); i++) {
+// We will always leave the last bytes untouched
+// so that string comparisions work.
+char equal_size_buffer[7] = {};
+bytes_read =
+ReadCylicBufferWrapper(equal_size_buffer, sizeof(cyclic_buffer),
+   cyclic_buffer, sizeof(cyclic_buffer), 3, i);
+ASSERT_EQ((sizeof(cyclic) - i - 1), bytes_read);
+ASSERT_STREQ(equal_size_buffer, (cyclic + i));
+  }
+}
+
+TEST(CyclicBuffer, SmallerSizeBuffer) {
+  size_t bytes_read = 0;
+  uint8_t cyclic_buffer[6] = {'l', 'i', 'c', 'c', 'y', 'c'};
+
+  // We will always leave the last bytes untouched
+  // so that string comparisions work.
+  char smaller_buffer[4] = {};
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 0);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cyc");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 1);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "ycl");
+
+  bytes_read =
+  ReadCylicBufferWrapper(smaller_buffer, (sizeof(smaller_buffer) - 1),
+ cyclic_buffer, sizeof(cyclic_buffer), 3, 2);
+  ASSERT_EQ(3, bytes_read);
+  ASSERT_STREQ(smaller_buffer, "cli");
+
+  

[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

Hi Abhishek,

Thank you for incorporating the changes. I still see some room for improvement 
in the cmake files. I realize that this is something most people are not 
familiar with, and is not exactly glamorous work, but it's still part of our 
codebase and we should make sure it follows best practices.




Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)

Could we avoid building the shared library altogether if both features are OFF?



Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+  PTPluginInitialize(debugger);
+  MPXPluginInitialize(debugger);

You will need some ifdef magic to make sure these still compile when the 
feature is off.



Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:9
+
+set(MPX_DEPS ${MPX_DEPS} LLVMSupport PARENT_SCOPE)

What you want here is to define an INTERFACE dependency on the MPX library 
instead.
vanilla cmake way would be `target_link_libraries(lldbIntelMPX INTERFACE 
LLVMSupport)`. **However**, we should use the llvm function instead, as that 
also handles other llvm-specific magic (for example, this code will break if 
someone does a LLVM_LINK_LLVM_DYLIB build).

So, I am asking for the third time:
Have you tried using add_lldb_library instead?

The correct invocation should be `add_lldb_library(foo.cpp LINK_LIBS Support)` 
and the rest of this file can just go away.



Comment at: tools/intel-features/scripts/lldb-intel-features.swig:9
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when

There appear to be no typedefs here.



Comment at: tools/intel-features/scripts/lldb-intel-features.swig:14
+   as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"

You are already defining this as a part of the swig invocation in cmake.



Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+

abhishek.aggarwal wrote:
> labath wrote:
> > Could we just use standard lldb typemaps? I don't see anything ipt specific 
> > here...
> Unfortunately, we can't because that file uses lldb_private::PythonString, 
> lldb_private::PythonList etc and for that I will have to link to 
> liblldbPluginScripInterpreterPython.
Ok, let's leave this as-is then. We could try factoring out common part of 
those typemaps, but I'm not sure that's such a good idea at the moment.


https://reviews.llvm.org/D33035



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


[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature

2017-06-26 Thread Abhishek via Phabricator via lldb-commits
abhishek.aggarwal updated this revision to Diff 103924.
abhishek.aggarwal added a comment.

Changes after feedback

- Created static libs for each hardware feature to combine it to generate 
single shared library
- Removed error codes decoding logic and directly using error strings


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,18 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when
+ processing glibc's stdint.h. */
+/* The ISO C99 standard specifies that in C++ implementations limit macros such
+   as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: 

[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature

2017-06-26 Thread Abhishek via Phabricator via lldb-commits
abhishek.aggarwal added a comment.

Hi Pavel .. My comments are inlined. Please let me know if you have more 
concerns. I am submitting the patch with all the proposed changes.




Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+  LIBRARY DESTINATION bin)

labath wrote:
> "bin" sounds wrong here. Shouldn't this go ti lib${LLVM_LIBDIR_SUFFIX}?
> To properly handle DLL targets (i don't know whether ipt support windows) 
> you'd need something like (see function add_lldb_library):
> ```
> install(TARGETS ${name}
>   COMPONENT ${name}
>   RUNTIME DESTINATION bin
>   LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
>   ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX})
> ```
> Although it may be than you can just call that function yourself.
Fixing it.



Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:5
+
+set(SOURCES ${SOURCES} "intel-mpx/cli-wrapper-mpxtable.cpp" PARENT_SCOPE)
+

labath wrote:
> Normally we build a single .a file for each source folder, which are then 
> linked into other targets as appropriate, and I don't see a reason to deviate 
> from this here.
> 
> Same goes for the ipt subfolder.
Agreed. I am submitting the changes.



Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+

labath wrote:
> Could we just use standard lldb typemaps? I don't see anything ipt specific 
> here...
Unfortunately, we can't because that file uses lldb_private::PythonString, 
lldb_private::PythonList etc and for that I will have to link to 
liblldbPluginScripInterpreterPython.



Comment at: tools/intel-pt/Decoder.cpp:27
+  std::string ) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {

labath wrote:
> abhishek.aggarwal wrote:
> > abhishek.aggarwal wrote:
> > > clayborg wrote:
> > > > We really shouldn't be interpreting integer error codes here. The 
> > > > string in the "sberror" should be what you use. Modify the code that 
> > > > creates these errors in the first place to also put a string values you 
> > > > have here into the lldb_private::Error to begin with and remove this 
> > > > function.
> > > Removing this function.
> > Currently, the codes are generated by lldb server and remote packets don't 
> > support sending error strings from lldb server to lldb client.
> > Now, I can think of 2 ways:
> > 
> > 1.  modify remote packets to send error strings as well (in addition to 
> > error codes).
> > 2.  or decode error codes and generate error strings at lldb client side
> > 
> > Which one do you prefer? @labath prefers 1st one (in discussions of 
> > https://reviews.llvm.org/D33674). If you also prefers 1st one then I (or 
> > @ravitheja) can work on modifying packets accordingly and submit a separate 
> > patch for that.
> > 
> > My idea is to keep error codes and its decoding logic here for now. Once we 
> > submit patch of modified packets and that patch gets approved, I can remove 
> > this decoding function from here all together. Please let me know what you 
> > prefer.
> How about we just avoid interpreting the error codes for now and print a 
> generic "error 47" message instead. This avoids the awkward state, where we 
> have two enums that need to be kept in sync, which is a really bad idea.
> 
> I think having more generic error code will be very useful - there are a lot 
> of packets that would benefit from more helpful error messages.
I am removing this function as @ravitheja is working on enabling lldb remote 
packets to send error strings directly. So, we don't even need "error 47" 
message.


https://reviews.llvm.org/D33035



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


[Lldb-commits] [PATCH] D34613: Add debug_frame section support

2017-06-26 Thread Pavel Labath via Phabricator via lldb-commits
labath created this revision.
Herald added subscribers: aprantl, mgorny.

This is a beefed-up version of https://reviews.llvm.org/D33504, which adds 
support for dwarf 4
debug_frame section format.

The main difference here is that the decision whether to use eh_frame or
debug_frame is done on a per-function basis instead of per-object file.
This is necessary because one module can contain both sections (for
example, the start files added by the linker will typically pull in
eh_frame), but we want to be able to access both, for maximum
information.

I also add unit test for parsing various CFI formats (eh_frame,
debug_frame v3 and debug_frame v4).


https://reviews.llvm.org/D34613

Files:
  include/lldb/Symbol/DWARFCallFrameInfo.h
  include/lldb/Symbol/FuncUnwinders.h
  include/lldb/Symbol/UnwindTable.h
  source/Commands/CommandObjectTarget.cpp
  source/Symbol/DWARFCallFrameInfo.cpp
  source/Symbol/FuncUnwinders.cpp
  source/Symbol/UnwindTable.cpp
  unittests/Symbol/CMakeLists.txt
  unittests/Symbol/Inputs/basic-call-frame-info.yaml
  unittests/Symbol/TestDWARFCallFrameInfo.cpp

Index: unittests/Symbol/TestDWARFCallFrameInfo.cpp
===
--- /dev/null
+++ unittests/Symbol/TestDWARFCallFrameInfo.cpp
@@ -0,0 +1,149 @@
+//===-- TestDWARFCallFrameInfo.cpp --*- C++ -*-===//
+//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/Process/Utility/RegisterContext_x86.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleSpec.h"
+#include "lldb/Core/Section.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Symbol/DWARFCallFrameInfo.h"
+#include "lldb/Utility/StreamString.h"
+#include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+extern const char *TestMainArgv0;
+
+using namespace lldb_private;
+using namespace lldb;
+
+class DWARFCallFrameInfoTest : public testing::Test {
+public:
+  void SetUp() override {
+HostInfo::Initialize();
+ObjectFileELF::Initialize();
+
+m_inputs_folder = llvm::sys::path::parent_path(TestMainArgv0);
+llvm::sys::path::append(m_inputs_folder, "Inputs");
+llvm::sys::fs::make_absolute(m_inputs_folder);
+  }
+
+  void TearDown() override {
+ObjectFileELF::Terminate();
+HostInfo::Terminate();
+  }
+
+protected:
+  llvm::SmallString<128> m_inputs_folder;
+
+  void TestBasic(bool eh_frame, llvm::StringRef symbol);
+};
+
+#define ASSERT_NO_ERROR(x) \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {\
+llvm::SmallString<128> MessageStorage; \
+llvm::raw_svector_ostream Message(MessageStorage); \
+Message << #x ": did not return errc::success.\n"  \
+<< "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"  \
+<< "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";  \
+GTEST_FATAL_FAILURE_(MessageStorage.c_str());  \
+  } else { \
+  }
+
+namespace lldb_private {
+static std::ostream <<(std::ostream , const UnwindPlan::Row ) {
+  StreamString SS;
+  row.Dump(SS, nullptr, nullptr, 0);
+  return OS << SS.GetData();
+}
+}
+
+static UnwindPlan::Row GetExpectedRow0() {
+  UnwindPlan::Row row;
+  row.SetOffset(0);
+  row.GetCFAValue().SetIsRegisterPlusOffset(dwarf_rsp_x86_64, 8);
+  row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rip_x86_64, -8, false);
+  return row;
+}
+
+static UnwindPlan::Row GetExpectedRow1() {
+  UnwindPlan::Row row;
+  row.SetOffset(1);
+  row.GetCFAValue().SetIsRegisterPlusOffset(dwarf_rsp_x86_64, 16);
+  row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rip_x86_64, -8, false);
+  row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rbp_x86_64, -16, false);
+  return row;
+}
+
+static UnwindPlan::Row GetExpectedRow2() {
+  UnwindPlan::Row row;
+  row.SetOffset(4);
+  row.GetCFAValue().SetIsRegisterPlusOffset(dwarf_rbp_x86_64, 16);
+  row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rip_x86_64, -8, false);
+  row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rbp_x86_64, -16, false);
+  return row;
+}
+
+void DWARFCallFrameInfoTest::TestBasic(bool eh_frame, llvm::StringRef symbol) {
+  llvm::SmallString<128> yaml = m_inputs_folder;
+  llvm::sys::path::append(yaml, "basic-call-frame-info.yaml");
+  llvm::SmallString<128> obj = m_inputs_folder;
+
+  ASSERT_NO_ERROR(llvm::sys::fs::createTemporaryFile(
+  "basic-call-frame-info-%%", "obj", obj));
+  

[Lldb-commits] [lldb] r306278 - Shorten sanitizer plugin names

2017-06-26 Thread Pavel Labath via lldb-commits
Author: labath
Date: Mon Jun 26 01:13:22 2017
New Revision: 306278

URL: http://llvm.org/viewvc/llvm-project?rev=306278=rev
Log:
Shorten sanitizer plugin names

Summary:
The new UndefinedBehaviorSanitizer plugin was breaking file path length
limits, because it's (fairly long name) appears multiple times in the
path. Cmake ends up putting the object file at path
tools/lldb/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/CMakeFiles/lldbPluginInstrumentationRuntimeUndefinedBehaviorSanitizer.dir/UndefinedBehaviorSanitizerRuntime.cpp.obj
which is 191 characters long and very dangerously close to the 260
character path limit on windows systems (also, just the include line for
that file was breaking the 80 character line limit).

This renames the sanitizer plugins to use shorter names (asan, ubsan,
tsan). I think this will still be quite understandable to everyone as
those are the names everyone uses to refer to them anyway.

Reviewers: zturner, kubamracek, jingham

Subscribers: lldb-commits, mgorny

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

Added:
lldb/trunk/source/Plugins/InstrumentationRuntime/ASan/
lldb/trunk/source/Plugins/InstrumentationRuntime/ASan/ASanRuntime.cpp
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp
lldb/trunk/source/Plugins/InstrumentationRuntime/ASan/ASanRuntime.h
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.h
lldb/trunk/source/Plugins/InstrumentationRuntime/ASan/CMakeLists.txt
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/CMakeLists.txt
lldb/trunk/source/Plugins/InstrumentationRuntime/TSan/
lldb/trunk/source/Plugins/InstrumentationRuntime/TSan/CMakeLists.txt
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/CMakeLists.txt
lldb/trunk/source/Plugins/InstrumentationRuntime/TSan/TSanRuntime.cpp
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
lldb/trunk/source/Plugins/InstrumentationRuntime/TSan/TSanRuntime.h
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.h
lldb/trunk/source/Plugins/InstrumentationRuntime/UBSan/
lldb/trunk/source/Plugins/InstrumentationRuntime/UBSan/CMakeLists.txt
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/CMakeLists.txt
lldb/trunk/source/Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.cpp
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/UndefinedBehaviorSanitizerRuntime.cpp
lldb/trunk/source/Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h
  - copied, changed from r306186, 
lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/UndefinedBehaviorSanitizerRuntime.h
Removed:

lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.cpp

lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.h

lldb/trunk/source/Plugins/InstrumentationRuntime/AddressSanitizer/CMakeLists.txt

lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/CMakeLists.txt

lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp

lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.h

lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/CMakeLists.txt

lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/UndefinedBehaviorSanitizerRuntime.cpp

lldb/trunk/source/Plugins/InstrumentationRuntime/UndefinedBehaviorSanitizer/UndefinedBehaviorSanitizerRuntime.h
Modified:
lldb/trunk/source/API/SystemInitializerFull.cpp
lldb/trunk/source/Plugins/InstrumentationRuntime/CMakeLists.txt

Modified: lldb/trunk/source/API/SystemInitializerFull.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SystemInitializerFull.cpp?rev=306278=306277=306278=diff
==
--- lldb/trunk/source/API/SystemInitializerFull.cpp (original)
+++ lldb/trunk/source/API/SystemInitializerFull.cpp Mon Jun 26 01:13:22 2017
@@ -49,9 +49,9 @@
 #include "Plugins/DynamicLoader/Static/DynamicLoaderStatic.h"
 #include "Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h"
 #include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
-#include 
"Plugins/InstrumentationRuntime/AddressSanitizer/AddressSanitizerRuntime.h"
-#include 
"Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.h"
-#include