Re: [Lldb-commits] [PATCH] D23977: Support of lldb on Kfreebsd

[Sylvestre Ledru via lldb-commits]

sylvestre.ledru updated this revision to Diff 72758.
sylvestre.ledru added a comment.
Herald added subscribers: mgorny, beanz.

Updated by Pino!


https://reviews.llvm.org/D23977

Files:
  cmake/LLDBDependencies.cmake
  cmake/modules/LLDBConfig.cmake
  scripts/Python/modules/CMakeLists.txt
  scripts/utilsOsType.py

Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -410,3 +410,5 @@
 list(APPEND system_libs ${CURSES_LIBRARIES})
 include_directories(${CURSES_INCLUDE_DIR})
 endif ()
+
+find_package(Backtrace REQUIRED)
Index: scripts/Python/modules/CMakeLists.txt
===
--- scripts/Python/modules/CMakeLists.txt
+++ scripts/Python/modules/CMakeLists.txt
@@ -5,7 +5,7 @@
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-macro-redefined")
 endif ()
 
-# build the Python readline suppression module only on Linux
-if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND NOT __ANDROID_NDK__)
+# build the Python readline suppression module only on Linux or GNU systems
+if ((CMAKE_SYSTEM_NAME MATCHES "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "GNU" OR 
CMAKE_SYSTEM_NAME STREQUAL "kFreeBSD") AND NOT __ANDROID_NDK__)
add_subdirectory(readline)
 endif()
Index: scripts/utilsOsType.py
===
--- scripts/utilsOsType.py
+++ scripts/utilsOsType.py
@@ -35,6 +35,7 @@
 Linux = 3
 NetBSD = 4
 Windows = 5
+kFreeBSD = 6
 else:
 class EnumOsType(object):
 values = ["Unknown",
@@ -42,7 +43,8 @@
   "FreeBSD",
   "Linux",
   "NetBSD",
-  "Windows"]
+  "Windows",
+  "kFreeBSD"]
 class __metaclass__(type):
 #++---
 # Details:  Fn acts as an enumeration.
@@ -86,5 +88,7 @@
 eOSType = EnumOsType.NetBSD
 elif strOS == "win32":
 eOSType = EnumOsType.Windows
+elif strOS.startswith("gnukfreebsd"):
+eOSType = EnumOsType.kFreeBSD
 
 return eOSType
Index: cmake/LLDBDependencies.cmake
===
--- cmake/LLDBDependencies.cmake
+++ cmake/LLDBDependencies.cmake
@@ -152,10 +152,7 @@
 endif()
   endif()
 endif()
-# On FreeBSD/NetBSD backtrace() is provided by libexecinfo, not libc.
-if (CMAKE_SYSTEM_NAME MATCHES "FreeBSD" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD")
-  list(APPEND LLDB_SYSTEM_LIBS execinfo)
-endif()
+list(APPEND LLDB_SYSTEM_LIBS ${Backtrace_LIBRARY})
 
 if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
   list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})


Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -410,3 +410,5 @@
 list(APPEND system_libs ${CURSES_LIBRARIES})
 include_directories(${CURSES_INCLUDE_DIR})
 endif ()
+
+find_package(Backtrace REQUIRED)
Index: scripts/Python/modules/CMakeLists.txt
===
--- scripts/Python/modules/CMakeLists.txt
+++ scripts/Python/modules/CMakeLists.txt
@@ -5,7 +5,7 @@
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-macro-redefined")
 endif ()
 
-# build the Python readline suppression module only on Linux
-if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND NOT __ANDROID_NDK__)
+# build the Python readline suppression module only on Linux or GNU systems
+if ((CMAKE_SYSTEM_NAME MATCHES "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "GNU" OR CMAKE_SYSTEM_NAME STREQUAL "kFreeBSD") AND NOT __ANDROID_NDK__)
add_subdirectory(readline)
 endif()
Index: scripts/utilsOsType.py
===
--- scripts/utilsOsType.py
+++ scripts/utilsOsType.py
@@ -35,6 +35,7 @@
 Linux = 3
 NetBSD = 4
 Windows = 5
+kFreeBSD = 6
 else:
 class EnumOsType(object):
 values = ["Unknown",
@@ -42,7 +43,8 @@
   "FreeBSD",
   "Linux",
   "NetBSD",
-  "Windows"]
+  "Windows",
+  "kFreeBSD"]
 class __metaclass__(type):
 #++---
 # Details:  Fn acts as an enumeration.
@@ -86,5 +88,7 @@
 eOSType = EnumOsType.NetBSD
 elif strOS == "win32":
 eOSType = EnumOsType.Windows
+elif strOS.startswith("gnukfreebsd"):
+eOSType = EnumOsType.kFreeBSD
 
 return eOSType
Index: cmake/LLDBDependencies.cmake
===
--- cmake/LLDBDependencies.cmake
+++ cmake/LLDBDependencies.cmake
@@ -152,10 +152,7 @@
 endif()
   endif()
 endif()
-# On FreeBSD/NetBSD backtrace() is 

Re: [Lldb-commits] Use explicit delete in DISALLOW_COPY_AND_ASSIGN

[Zachary Turner via lldb-commits]

Not sure why operator= is returning a const&. It doesn't matter in practice
but it's a bit strange. Anyway lgtm
On Tue, Sep 27, 2016 at 7:11 PM Daniel Austin Noland via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> Explicit delete is preferable to declaring a method private when
> your intention is to prevent that method from ever being used.
>
> * better compiler error messages
> * can't be bypassed by friendship
> * clearer intent
>
> This was discussed here:
> http://lists.llvm.org/pipermail/lldb-dev/2016-September/011394.html
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] Use explicit delete in DISALLOW_COPY_AND_ASSIGN

[Daniel Austin Noland via lldb-commits]

Explicit delete is preferable to declaring a method private when
your intention is to prevent that method from ever being used.
   
* better compiler error messages
* can't be bypassed by friendship
* clearer intent
   
This was discussed here:
http://lists.llvm.org/pipermail/lldb-dev/2016-September/011394.html

diff --git a/include/lldb/lldb-defines.h b/include/lldb/lldb-defines.h
index a8e6776..a1318f4 100644
--- a/include/lldb/lldb-defines.h
+++ b/include/lldb/lldb-defines.h
@@ -157,8 +157,8 @@
 /// assignment operators in C++ classes.
 //--
 #define DISALLOW_COPY_AND_ASSIGN(TypeName) 
\
-  TypeName(const TypeName &);  
\
-  const TypeName =(const TypeName &)
+  TypeName(const TypeName &) = delete; 
\
+  const TypeName =(const TypeName &) = delete
 
 #endif // #if defined(__cplusplus)
 


signature.asc
Description: OpenPGP digital signature
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [Project] LLDB

[Tim Hammerquist via lldb-commits]

penryu added a watcher: penryu.

PROJECT DETAIL
  https://reviews.llvm.org/project/profile/39/



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

Re: [Lldb-commits] [PATCH] D10167: Fix TestJoinAfterBreak test on Windows

[Eugene Zelenko via lldb-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko closed this revision.
Eugene.Zelenko added a comment.

Committed in https://reviews.llvm.org/rL238787.


https://reviews.llvm.org/D10167



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

Re: [Lldb-commits] [PATCH] D10216: Initial diff for FreeBSD kernel debugging support

[Eugene Zelenko via lldb-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

Looks like patch was not committed. Need to be rebased and format with 
Clang-format.


https://reviews.llvm.org/D10216



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

Re: [Lldb-commits] [PATCH] D24610: LLDB Arm Watchpoints: Use single hardware watchpoint slot to watch multiple bytes where possible

[Muhammad Omair Javaid via lldb-commits]

omjavaid updated this revision to Diff 72723.
omjavaid added a comment.

Give this approach a rethink I dont see a lot of problems with this final 
implementation unless it fails on other architectures.
We are already hacking our way to have these byte selection watchpoints working 
in existing code. New code seems to be improving the hack in my opinion.

Let me explain what I am doing and may be you can provide your suggestion and 
feedback.

Watchpoint Install => Register watchpoint into hardware watchpoint register 
cache
Watchpoint Enable => Enable in cache and write registers using ptrace interface.

Ideally we should be able to install/uninstall watchpoints in cache and then 
enable them all on a resume.
In case of arm If a watchpoint is hit we should be able to disable that 
watchpoint. 
Step over watchpoint instruction and then re-enable the watchpoint.
Our existing implementation will require a lot of changes to do that so thats 
why here is what i am doing.

SetHardwareWatchpoint => Performs Install and Enable

- If a new watchpoint slot is going to be used we Install and enable.
- For new watchpoint we should be able to complete both Install and or we 
report an error.
- If a duplicate slot is going to be used we Install and enable if required.
- Install means updating size if needed plus updating ref count.
- Enable means updating registers if size was updated.

ClearHardwareWatchpoint

- Disable and uinstall watchpoint means
- Decrement ref count and clear hardware watchpoint regsiters.
- Advantage of keeping ref counts is:
- If refcount is greater than zero then SetHardwareWatchpoint cannot use this 
slot for a new watchpoint (new address).
- But SetHardwareWatchpoint can be use this slot to install duplicate 
watchpoints (same address but different byte or word)

ClearAllHardwareWatchpoint -- Just clear the whole watchpoint cache and call 
SetHardwareWatchpoint for all available watchpoints.

NativeThreadLinux:

  On Watchpoint Remove -> Invalidate watchpoint cache
  On Resume - > Re-validate watchpoints by creating a new cache and re-enabling 
all watchpoints

So this fixes our step-over issue and also preserves watchpoint slot if it is 
being used by multiple watchpoints.

Can you think of any scenarios which might fail for this approach?


https://reviews.llvm.org/D24610

Files:
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/Makefile
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multi_watchpoint_slots/main.c
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
  source/Plugins/Process/Linux/NativeThreadLinux.cpp
  source/Plugins/Process/Linux/NativeThreadLinux.h

Index: source/Plugins/Process/Linux/NativeThreadLinux.h
===
--- source/Plugins/Process/Linux/NativeThreadLinux.h
+++ source/Plugins/Process/Linux/NativeThreadLinux.h
@@ -108,6 +108,7 @@
   using WatchpointIndexMap = std::map;
   WatchpointIndexMap m_watchpoint_index_map;
   cpu_set_t m_original_cpu_set; // For single-step workaround.
+  bool m_invalidate_watchpoints;
 };
 
 typedef std::shared_ptr NativeThreadLinuxSP;
Index: source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -87,7 +87,8 @@
 NativeThreadLinux::NativeThreadLinux(NativeProcessLinux *process,
  lldb::tid_t tid)
 : NativeThreadProtocol(process, tid), m_state(StateType::eStateInvalid),
-  m_stop_info(), m_reg_context_sp(), m_stop_description() {}
+  m_stop_info(), m_reg_context_sp(), m_stop_description(),
+  m_invalidate_watchpoints(false) {}
 
 std::string NativeThreadLinux::GetName() {
   NativeProcessProtocolSP process_sp = m_process_wp.lock();
@@ -185,8 +186,10 @@
 return Error();
   uint32_t wp_index = wp->second;
   m_watchpoint_index_map.erase(wp);
-  if (GetRegisterContext()->ClearHardwareWatchpoint(wp_index))
+  if (GetRegisterContext()->ClearHardwareWatchpoint(wp_index)) {
+m_invalidate_watchpoints = true;
 return Error();
+  }
   return Error("Clearing hardware watchpoint failed.");
 }
 
@@ -198,8 +201,13 @@
   m_stop_info.reason = StopReason::eStopReasonNone;
   m_stop_description.clear();
 
-  // If watchpoints have been set, but none on this thread,
-  // then this is a new thread. So set all existing watchpoints.
+  // Invalidate watchpoint index map for a re-sync
+  if (m_invalidate_watchpoints) {
+m_invalidate_watchpoints = false;
+m_watchpoint_index_map.clear();
+  }
+
+  // Re-sync all available watchpoints.
   if (m_watchpoint_index_map.empty()) {
 NativeProcessLinux  = GetProcess();
 
Index: 

[Lldb-commits] [PATCH] D24988: Improvements to testing blacklist

[Francis Ricci via lldb-commits]

fjricci created this revision.
fjricci added reviewers: zturner, labath, tfiala, jingham.
fjricci added subscribers: sas, lldb-commits.

This patch is necessary because individual test cases are not required
to have unique names. Therefore, test cases must now
be specified explicitly in the form ..
Because it works by regex matching, passing just  will
still disable an entire file.

This also allows for multiple exclusion files to be specified.

https://reviews.llvm.org/D24988

Files:
  packages/Python/lldbsuite/test/configuration.py
  packages/Python/lldbsuite/test/dotest.py
  packages/Python/lldbsuite/test/dotest_args.py
  packages/Python/lldbsuite/test/test_result.py

Index: packages/Python/lldbsuite/test/test_result.py
===
--- packages/Python/lldbsuite/test/test_result.py
+++ packages/Python/lldbsuite/test/test_result.py
@@ -139,8 +139,8 @@
 self.getCategoriesForTest(test)):
 self.hardMarkAsSkipped(test)
 if self.checkExclusion(
-configuration.skip_methods,
-test._testMethodName):
+configuration.skip_tests,
+strclass(test.__class__) + '.' + test._testMethodName):
 self.hardMarkAsSkipped(test)
 
 configuration.setCrashInfoHook(
@@ -161,11 +161,8 @@
 
 def addSuccess(self, test):
 if self.checkExclusion(
-configuration.xfail_files,
-strclass(
-test.__class__)) or self.checkExclusion(
-configuration.xfail_methods,
-test._testMethodName):
+configuration.xfail_tests,
+strclass(test.__class__) + '.' + test._testMethodName):
 self.addUnexpectedSuccess(test, None)
 return
 
@@ -239,11 +236,8 @@
 
 def addFailure(self, test, err):
 if self.checkExclusion(
-configuration.xfail_files,
-strclass(
-test.__class__)) or self.checkExclusion(
-configuration.xfail_methods,
-test._testMethodName):
+configuration.xfail_tests,
+strclass(test.__class__) + '.' + test._testMethodName):
 self.addExpectedFailure(test, err, None)
 return
 
Index: packages/Python/lldbsuite/test/dotest_args.py
===
--- packages/Python/lldbsuite/test/dotest_args.py
+++ packages/Python/lldbsuite/test/dotest_args.py
@@ -96,7 +96,7 @@
 '-p',
 metavar='pattern',
 help='Specify a regexp filename pattern for inclusion in the test suite')
-group.add_argument('--excluded', metavar='exclusion-file', help=textwrap.dedent(
+group.add_argument('--excluded', metavar='exclusion-file', action='append', help=textwrap.dedent(
 '''Specify a file for tests to exclude. File should contain lists of regular expressions for test files or methods,
 with each list under a matching header (xfail files, xfail methods, skip files, skip methods)'''))
 group.add_argument(
Index: packages/Python/lldbsuite/test/dotest.py
===
--- packages/Python/lldbsuite/test/dotest.py
+++ packages/Python/lldbsuite/test/dotest.py
@@ -216,33 +216,24 @@

 """
 excl_type = None
-case_type = None
 
 with open(exclusion_file) as f:
 for line in f:
+line = line.strip()
 if not excl_type:
-[excl_type, case_type] = line.split()
+excl_type = line
 continue
 
-line = line.strip()
 if not line:
 excl_type = None
-elif excl_type == 'skip' and case_type == 'files':
-if not configuration.skip_files:
-configuration.skip_files = []
-configuration.skip_files.append(line)
-elif excl_type == 'skip' and case_type == 'methods':
-if not configuration.skip_methods:
-configuration.skip_methods = []
-configuration.skip_methods.append(line)
-elif excl_type == 'xfail' and case_type == 'files':
-if not configuration.xfail_files:
-configuration.xfail_files = []
-configuration.xfail_files.append(line)
-elif excl_type == 'xfail' and case_type == 'methods':
-if not configuration.xfail_methods:
-configuration.xfail_methods = []
-configuration.xfail_methods.append(line)
+elif excl_type == 'skip':
+if not configuration.skip_tests:
+configuration.skip_tests = []
+configuration.skip_tests.append(line)
+elif excl_type == 'xfail':
+if not configuration.xfail_tests:
+

Re: [Lldb-commits] [PATCH] D24936: Make FileSpec use StringRef.

[Zachary Turner via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL282537: Update FileSpec's interface to use StringRefs. 
(authored by zturner).

Changed prior to commit:
  https://reviews.llvm.org/D24936?vs=72555=72705#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24936

Files:
  lldb/trunk/include/lldb/Host/FileSpec.h
  lldb/trunk/source/Host/common/FileSpec.cpp

Index: lldb/trunk/include/lldb/Host/FileSpec.h
===
--- lldb/trunk/include/lldb/Host/FileSpec.h
+++ lldb/trunk/include/lldb/Host/FileSpec.h
@@ -80,16 +80,10 @@
   ///
   /// @see FileSpec::SetFile (const char *path, bool resolve)
   //--
-  // TODO: Convert this constructor to use a StringRef.
-  explicit FileSpec(const char *path, bool resolve_path,
+  explicit FileSpec(llvm::StringRef path, bool resolve_path,
 PathSyntax syntax = ePathSyntaxHostNative);
 
-  explicit FileSpec(const char *path, bool resolve_path, ArchSpec arch);
-
-  explicit FileSpec(const std::string , bool resolve_path,
-PathSyntax syntax = ePathSyntaxHostNative);
-
-  explicit FileSpec(const std::string , bool resolve_path, ArchSpec arch);
+  explicit FileSpec(llvm::StringRef path, bool resolve_path, ArchSpec arch);
 
   //--
   /// Copy constructor
@@ -657,15 +651,10 @@
   /// If \b true, then we will try to resolve links the path using
   /// the static FileSpec::Resolve.
   //--
-  void SetFile(const char *path, bool resolve_path,
-   PathSyntax syntax = ePathSyntaxHostNative);
-
-  void SetFile(const char *path, bool resolve_path, ArchSpec arch);
-
-  void SetFile(const std::string , bool resolve_path,
+  void SetFile(llvm::StringRef path, bool resolve_path,
PathSyntax syntax = ePathSyntaxHostNative);
 
-  void SetFile(const std::string , bool resolve_path, ArchSpec arch);
+  void SetFile(llvm::StringRef path, bool resolve_path, ArchSpec arch);
 
   bool IsResolved() const { return m_is_resolved; }
 
@@ -709,20 +698,13 @@
   //--
   static void Resolve(llvm::SmallVectorImpl );
 
-  FileSpec CopyByAppendingPathComponent(const char *new_path) const;
-
+  FileSpec CopyByAppendingPathComponent(llvm::StringRef component) const;
   FileSpec CopyByRemovingLastPathComponent() const;
 
-  void PrependPathComponent(const char *new_path);
-
-  void PrependPathComponent(const std::string _path);
-
+  void PrependPathComponent(llvm::StringRef component);
   void PrependPathComponent(const FileSpec _path);
 
-  void AppendPathComponent(const char *new_path);
-
-  void AppendPathComponent(const std::string _path);
-
+  void AppendPathComponent(llvm::StringRef component);
   void AppendPathComponent(const FileSpec _path);
 
   void RemoveLastPathComponent();
@@ -746,7 +728,7 @@
   //--
   static void ResolveUsername(llvm::SmallVectorImpl );
 
-  static size_t ResolvePartialUsername(const char *partial_name,
+  static size_t ResolvePartialUsername(llvm::StringRef partial_name,
StringList );
 
   enum EnumerateDirectoryResult {
@@ -763,7 +745,7 @@
   void *baton, FileType file_type, const FileSpec );
 
   static EnumerateDirectoryResult
-  EnumerateDirectory(const char *dir_path, bool find_directories,
+  EnumerateDirectory(llvm::StringRef dir_path, bool find_directories,
  bool find_files, bool find_other,
  EnumerateDirectoryCallbackType callback,
  void *callback_baton);
@@ -773,7 +755,7 @@
   DirectoryCallback;
 
   static EnumerateDirectoryResult
-  ForEachItemInDirectory(const char *dir_path,
+  ForEachItemInDirectory(llvm::StringRef dir_path,
  DirectoryCallback const );
 
 protected:
Index: lldb/trunk/source/Host/common/FileSpec.cpp
===
--- lldb/trunk/source/Host/common/FileSpec.cpp
+++ lldb/trunk/source/Host/common/FileSpec.cpp
@@ -37,6 +37,7 @@
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/CleanUp.h"
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
@@ -58,7 +59,7 @@
   return PathSyntaxIsPosix(syntax) ? "/" : "\\/";
 }
 
-char GetPrefferedPathSeparator(FileSpec::PathSyntax syntax) {
+char GetPreferredPathSeparator(FileSpec::PathSyntax syntax) {
   return GetPathSeparators(syntax)[0];
 }
 
@@ -228,27 +229,27 @@
 #endif
 }
 
-size_t FileSpec::ResolvePartialUsername(const char *partial_name,
+size_t FileSpec::ResolvePartialUsername(llvm::StringRef partial_name,

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL282529: Adding a RegisterContextMinidump_x86_64 converter 
(authored by dvlahovski).

Changed prior to commit:
  https://reviews.llvm.org/D24919?vs=72688=72689#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24919

Files:
  lldb/trunk/include/lldb/lldb-private-types.h
  lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
  lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
  lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -8,16 +8,19 @@
 //===--===//
 
 // Project includes
+#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
 #include "Plugins/Process/minidump/MinidumpParser.h"
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h"
 
 // Other libraries and framework includes
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -50,7 +53,7 @@
 MinidumpParser::Create(data_sp);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
-ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_GT(parser->GetData().size(), 0UL);
   }
 
   llvm::SmallString<128> inputs_folder;
@@ -167,3 +170,61 @@
   ASSERT_TRUE(pid.hasValue());
   ASSERT_EQ(4440UL, pid.getValue());
 }
+
+// Register stuff
+// TODO probably split register stuff tests into different file?
+#define REG_VAL(x) *(reinterpret_cast(x))
+
+TEST_F(MinidumpParserTest, ConvertRegisterContext) {
+  SetUpData("linux-x86_64.dmp");
+  llvm::ArrayRef thread_list = parser->GetThreads();
+  const MinidumpThread thread = thread_list[0];
+  llvm::ArrayRef registers(parser->GetData().data() +
+thread.thread_context.rva,
+thread.thread_context.data_size);
+
+  ArchSpec arch = parser->GetArchitecture();
+  RegisterInfoInterface *reg_interface = new RegisterContextLinux_x86_64(arch);
+  lldb::DataBufferSP buf =
+  ConvertMinidumpContextToRegIface(registers, reg_interface);
+  ASSERT_EQ(reg_interface->GetGPRSize(), buf->GetByteSize());
+
+  const RegisterInfo *reg_info = reg_interface->GetRegisterInfo();
+
+  std::map reg_values;
+
+  // clang-format off
+  reg_values[lldb_rax_x86_64]=  0x;
+  reg_values[lldb_rbx_x86_64]=  0x;
+  reg_values[lldb_rcx_x86_64]=  0x0010;
+  reg_values[lldb_rdx_x86_64]=  0x;
+  reg_values[lldb_rdi_x86_64]=  0x7ffceb349cf0;
+  reg_values[lldb_rsi_x86_64]=  0x;
+  reg_values[lldb_rbp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_rsp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_r8_x86_64] =  0x7fe9bc1aa9c0;
+  reg_values[lldb_r9_x86_64] =  0x;
+  reg_values[lldb_r10_x86_64]=  0x7fe9bc3f16a0;
+  reg_values[lldb_r11_x86_64]=  0x0246;
+  reg_values[lldb_r12_x86_64]=  0x00401c92;
+  reg_values[lldb_r13_x86_64]=  0x7ffceb34a430;
+  reg_values[lldb_r14_x86_64]=  0x;
+  reg_values[lldb_r15_x86_64]=  0x;
+  reg_values[lldb_rip_x86_64]=  0x00401dc6;
+  reg_values[lldb_rflags_x86_64] =  0x00010206;
+  reg_values[lldb_cs_x86_64] =  0x0033;
+  reg_values[lldb_fs_x86_64] =  0x;
+  reg_values[lldb_gs_x86_64] =  0x;
+  reg_values[lldb_ss_x86_64] =  0x;
+  reg_values[lldb_ds_x86_64] =  0x;
+  reg_values[lldb_es_x86_64] =  0x;
+  // clang-format on
+
+  for (uint32_t reg_index = 0; reg_index < reg_interface->GetRegisterCount();
+   ++reg_index) {
+if (reg_values.find(reg_index) != reg_values.end()) {
+  EXPECT_EQ(reg_values[reg_index],
+REG_VAL(buf->GetBytes() + reg_info[reg_index].byte_offset));
+}
+  }
+}
Index: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
===
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
+++ 

[Lldb-commits] [lldb] r282529 - Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

Author: dvlahovski
Date: Tue Sep 27 14:05:55 2016
New Revision: 282529

URL: http://llvm.org/viewvc/llvm-project?rev=282529=rev
Log:
Adding a RegisterContextMinidump_x86_64 converter

Summary:
This is a register context converter from Minidump to Linux reg context.
This knows the layout of the register context in the Minidump file
(which is the same as in Windows FYI) and as a result emits a binary data
buffer that matches the Linux register context binary layout.
This way we can reuse the existing RegisterContextLinux_x86_64 and
RegisterContextCorePOSIX_x86_64 classes.

Reviewers: labath, zturner

Subscribers: beanz, mgorny, lldb-commits, amccarth

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

Added:

lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
Modified:
lldb/trunk/include/lldb/lldb-private-types.h
lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Modified: lldb/trunk/include/lldb/lldb-private-types.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/lldb-private-types.h?rev=282529=282528=282529=diff
==
--- lldb/trunk/include/lldb/lldb-private-types.h (original)
+++ lldb/trunk/include/lldb/lldb-private-types.h Tue Sep 27 14:05:55 2016
@@ -14,6 +14,8 @@
 
 #include "lldb/lldb-private.h"
 
+#include "llvm/ADT/ArrayRef.h"
+
 namespace llvm {
 namespace sys {
 class DynamicLibrary;
@@ -61,6 +63,15 @@ struct RegisterInfo {
   // the byte size of this register.
   size_t dynamic_size_dwarf_len; // The length of the DWARF expression in bytes
  // in the dynamic_size_dwarf_expr_bytes 
member.
+
+  llvm::ArrayRef data(const uint8_t *context_base) const {
+return llvm::ArrayRef(context_base + byte_offset, byte_size);
+  }
+
+  llvm::MutableArrayRef mutable_data(uint8_t *context_base) const {
+return llvm::MutableArrayRef(context_base + byte_offset,
+  byte_size);
+  }
 };
 
 //--

Modified: lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt?rev=282529=282528=282529=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt (original)
+++ lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt Tue Sep 27 
14:05:55 2016
@@ -3,4 +3,5 @@ include_directories(../Utility)
 add_lldb_library(lldbPluginProcessMinidump
   MinidumpTypes.cpp
   MinidumpParser.cpp
+  RegisterContextMinidump_x86_64.cpp
   )

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=282529=282528=282529=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Tue Sep 27 
14:05:55 2016
@@ -64,8 +64,9 @@ MinidumpParser::MinidumpParser(
 : m_data_sp(data_buf_sp), m_header(header), m_directory_map(directory_map) 
{
 }
 
-lldb::offset_t MinidumpParser::GetByteSize() {
-  return m_data_sp->GetByteSize();
+llvm::ArrayRef MinidumpParser::GetData() {
+  return llvm::ArrayRef(m_data_sp->GetBytes(),
+ m_data_sp->GetByteSize());
 }
 
 llvm::ArrayRef

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h?rev=282529=282528=282529=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h Tue Sep 27 
14:05:55 2016
@@ -39,7 +39,7 @@ public:
   static llvm::Optional
   Create(const lldb::DataBufferSP _buf_sp);
 
-  lldb::offset_t GetByteSize();
+  llvm::ArrayRef GetData();
 
   llvm::ArrayRef GetStream(MinidumpStreamType stream_type);
 
@@ -71,6 +71,6 @@ private:
   llvm::DenseMap &_map);
 };
 
-} // namespace minidump
-} // namespace lldb_private
+} // end namespace minidump
+} // end namespace lldb_private
 #endif // liblldb_MinidumpParser_h_

Added: 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp?rev=282529=auto

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 72688.
dvlahovski added a comment.

I like the combined approach more. So this is the implementation


https://reviews.llvm.org/D24919

Files:
  include/lldb/lldb-private-types.h
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -8,16 +8,19 @@
 //===--===//
 
 // Project includes
+#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
 #include "Plugins/Process/minidump/MinidumpParser.h"
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h"
 
 // Other libraries and framework includes
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -50,7 +53,7 @@
 MinidumpParser::Create(data_sp);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
-ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_GT(parser->GetData().size(), 0UL);
   }
 
   llvm::SmallString<128> inputs_folder;
@@ -167,3 +170,61 @@
   ASSERT_TRUE(pid.hasValue());
   ASSERT_EQ(4440UL, pid.getValue());
 }
+
+// Register stuff
+// TODO probably split register stuff tests into different file?
+#define REG_VAL(x) *(reinterpret_cast(x))
+
+TEST_F(MinidumpParserTest, ConvertRegisterContext) {
+  SetUpData("linux-x86_64.dmp");
+  llvm::ArrayRef thread_list = parser->GetThreads();
+  const MinidumpThread thread = thread_list[0];
+  llvm::ArrayRef registers(parser->GetData().data() +
+thread.thread_context.rva,
+thread.thread_context.data_size);
+
+  ArchSpec arch = parser->GetArchitecture();
+  RegisterInfoInterface *reg_interface = new RegisterContextLinux_x86_64(arch);
+  lldb::DataBufferSP buf =
+  ConvertMinidumpContextToRegIface(registers, reg_interface);
+  ASSERT_EQ(reg_interface->GetGPRSize(), buf->GetByteSize());
+
+  const RegisterInfo *reg_info = reg_interface->GetRegisterInfo();
+
+  std::map reg_values;
+
+  // clang-format off
+  reg_values[lldb_rax_x86_64]=  0x;
+  reg_values[lldb_rbx_x86_64]=  0x;
+  reg_values[lldb_rcx_x86_64]=  0x0010;
+  reg_values[lldb_rdx_x86_64]=  0x;
+  reg_values[lldb_rdi_x86_64]=  0x7ffceb349cf0;
+  reg_values[lldb_rsi_x86_64]=  0x;
+  reg_values[lldb_rbp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_rsp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_r8_x86_64] =  0x7fe9bc1aa9c0;
+  reg_values[lldb_r9_x86_64] =  0x;
+  reg_values[lldb_r10_x86_64]=  0x7fe9bc3f16a0;
+  reg_values[lldb_r11_x86_64]=  0x0246;
+  reg_values[lldb_r12_x86_64]=  0x00401c92;
+  reg_values[lldb_r13_x86_64]=  0x7ffceb34a430;
+  reg_values[lldb_r14_x86_64]=  0x;
+  reg_values[lldb_r15_x86_64]=  0x;
+  reg_values[lldb_rip_x86_64]=  0x00401dc6;
+  reg_values[lldb_rflags_x86_64] =  0x00010206;
+  reg_values[lldb_cs_x86_64] =  0x0033;
+  reg_values[lldb_fs_x86_64] =  0x;
+  reg_values[lldb_gs_x86_64] =  0x;
+  reg_values[lldb_ss_x86_64] =  0x;
+  reg_values[lldb_ds_x86_64] =  0x;
+  reg_values[lldb_es_x86_64] =  0x;
+  // clang-format on
+
+  for (uint32_t reg_index = 0; reg_index < reg_interface->GetRegisterCount();
+   ++reg_index) {
+if (reg_values.find(reg_index) != reg_values.end()) {
+  EXPECT_EQ(reg_values[reg_index],
+REG_VAL(buf->GetBytes() + reg_info[reg_index].byte_offset));
+}
+  }
+}
Index: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
@@ -0,0 +1,119 @@
+//===-- RegisterContextMinidump_x86_64.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See 

[Lldb-commits] [PATCH] D24960: Remove unreachable from apis in posix terminal compat windows and return 0

[Carlo Kok via lldb-commits]

carlokok created this revision.
carlokok added a subscriber: LLDB.
carlokok set the repository for this revision to rL LLVM.
carlokok added a project: LLDB.

These apis are called by lldb process launching but the unreachable causes them 
to fail on Windows. However, they don't have to do anything because Windows has 
implied support for terminals.

Repository:
  rL LLVM

https://reviews.llvm.org/D24960

Files:
  /lldb/trunk/include/lldb/Host/windows/PosixApi.h

Index: /lldb/trunk/include/lldb/Host/windows/PosixApi.h
===
--- /lldb/trunk/include/lldb/Host/windows/PosixApi.h
+++ /lldb/trunk/include/lldb/Host/windows/PosixApi.h
@@ -98,19 +98,19 @@
 int strncasecmp(const char *s1, const char *s2, size_t n);
 
 // empty functions
-inline int posix_openpt(int flag) { LLVM_BUILTIN_UNREACHABLE; }
+inline int posix_openpt(int flag) { return 0; }
 
 inline int strerror_r(int errnum, char *buf, size_t buflen) {
-  LLVM_BUILTIN_UNREACHABLE;
+  return 0;
 }
 
-inline int unlockpt(int fd) { LLVM_BUILTIN_UNREACHABLE; }
-inline int grantpt(int fd) { LLVM_BUILTIN_UNREACHABLE; }
-inline char *ptsname(int fd) { LLVM_BUILTIN_UNREACHABLE; }
 
-inline pid_t fork(void) { LLVM_BUILTIN_UNREACHABLE; }
-inline pid_t setsid(void) { LLVM_BUILTIN_UNREACHABLE; }
+inline int unlockpt(int fd) { return 0; }
+inline int grantpt(int fd) { return 0; }
+inline char *ptsname(int fd) { return 0; }
 
+inline pid_t fork(void) { return 0; }
+inline pid_t setsid(void) { return 0; }
 // vsnprintf and snprintf are provided in MSVC 2015 and higher.
 #if _MSC_VER < 1900
 namespace lldb_private {


Index: /lldb/trunk/include/lldb/Host/windows/PosixApi.h
===
--- /lldb/trunk/include/lldb/Host/windows/PosixApi.h
+++ /lldb/trunk/include/lldb/Host/windows/PosixApi.h
@@ -98,19 +98,19 @@
 int strncasecmp(const char *s1, const char *s2, size_t n);
 
 // empty functions
-inline int posix_openpt(int flag) { LLVM_BUILTIN_UNREACHABLE; }
+inline int posix_openpt(int flag) { return 0; }
 
 inline int strerror_r(int errnum, char *buf, size_t buflen) {
-  LLVM_BUILTIN_UNREACHABLE;
+  return 0;
 }
 
-inline int unlockpt(int fd) { LLVM_BUILTIN_UNREACHABLE; }
-inline int grantpt(int fd) { LLVM_BUILTIN_UNREACHABLE; }
-inline char *ptsname(int fd) { LLVM_BUILTIN_UNREACHABLE; }
 
-inline pid_t fork(void) { LLVM_BUILTIN_UNREACHABLE; }
-inline pid_t setsid(void) { LLVM_BUILTIN_UNREACHABLE; }
+inline int unlockpt(int fd) { return 0; }
+inline int grantpt(int fd) { return 0; }
+inline char *ptsname(int fd) { return 0; }
 
+inline pid_t fork(void) { return 0; }
+inline pid_t setsid(void) { return 0; }
 // vsnprintf and snprintf are provided in MSVC 2015 and higher.
 #if _MSC_VER < 1900
 namespace lldb_private {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-return const_cast(_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() 
to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);

zturner wrote:
> clayborg wrote:
> > zturner wrote:
> > > clayborg wrote:
> > > > Remove this comment. The whole point off this function is to get an 
> > > > argument vector that is NULL terminated. Feel free to rename the 
> > > > function as desired to indicate this, but that is the whole point of 
> > > > this function. Feel free to document it if needed as well.
> > > There are two ways we use these argument vectors.  One is in a call to a 
> > > function which takes an argc and an argv.  In that case the null 
> > > terminator is unnecessary because you're specifying the argc explicitly.  
> > > Another is in a call to a function which you do not specify an argc, and 
> > > in tht case it requires it to be null terminated.
> > > 
> > > The point of this function isn't "return me something that's null 
> > > terminated", it's "return me something I can use like an argv".  That 
> > > doesn't require null termination.  And in fact, if you look at the 
> > > callsites I fixed up, not a single one depends on the null termination.  
> > > Everyone just needs to know the size.  And for that you can call 
> > > `result.size()`.  If you add the null terminator, then you have to write 
> > > `result.size()-1`, which is unnecessarily confusing.  I believe it 
> > > provides the most logical behavior as it is currently written.
> > Remove the comment. Even in "main(int argc, const char **argv)" the "argv" 
> > is null terminated. So all unix variants expect the null termination. The 
> > correct solution is to add a boolean argument:
> > 
> > ```
> > std::vector Args::GetArgumentVector(bool null_terminate = 
> > true) const {
> > ```
> > 
> > Then it is clear.
> Even in main though, the null is not counted in the argc, right?  So as 
> written it will conform to the semantics of main.  `argc` is the number of 
> non-null pointers, but the null is still there anyway.  If you add the 
> boolean argument and pass true, it will give you back something whose `argc` 
> (i.e. `result.size()`) is one too high.
BTW, `llvm::SmallString` uses this same trick to return a null terminated 
string without modifying the size.  Look at the implementation of 
`SmallString::c_str()` in `SmallString.h`


https://reviews.llvm.org/D24952



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-return const_cast(_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() 
to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);

clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > Remove this comment. The whole point off this function is to get an 
> > > argument vector that is NULL terminated. Feel free to rename the function 
> > > as desired to indicate this, but that is the whole point of this 
> > > function. Feel free to document it if needed as well.
> > There are two ways we use these argument vectors.  One is in a call to a 
> > function which takes an argc and an argv.  In that case the null terminator 
> > is unnecessary because you're specifying the argc explicitly.  Another is 
> > in a call to a function which you do not specify an argc, and in tht case 
> > it requires it to be null terminated.
> > 
> > The point of this function isn't "return me something that's null 
> > terminated", it's "return me something I can use like an argv".  That 
> > doesn't require null termination.  And in fact, if you look at the 
> > callsites I fixed up, not a single one depends on the null termination.  
> > Everyone just needs to know the size.  And for that you can call 
> > `result.size()`.  If you add the null terminator, then you have to write 
> > `result.size()-1`, which is unnecessarily confusing.  I believe it provides 
> > the most logical behavior as it is currently written.
> Remove the comment. Even in "main(int argc, const char **argv)" the "argv" is 
> null terminated. So all unix variants expect the null termination. The 
> correct solution is to add a boolean argument:
> 
> ```
> std::vector Args::GetArgumentVector(bool null_terminate = true) 
> const {
> ```
> 
> Then it is clear.
Even in main though, the null is not counted in the argc, right?  So as written 
it will conform to the semantics of main.  `argc` is the number of non-null 
pointers, but the null is still there anyway.  If you add the boolean argument 
and pass true, it will give you back something whose `argc` (i.e. 
`result.size()`) is one too high.


https://reviews.llvm.org/D24952



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Greg Clayton via lldb-commits]

clayborg added a comment.

Ok, so just add the boolean arg to Args::GetArgumentVector and we are good to 
go.



Comment at: source/Interpreter/Args.cpp:270-272
@@ -321,5 +269,5 @@
 const char *Args::GetArgumentAtIndex(size_t idx) const {
-  if (idx < m_argv.size())
-return m_argv[idx];
+  if (idx < m_args.size())
+return m_args[idx].c_str();
   return nullptr;
 }

Ok, sounds good, for a later CL then.


Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-return const_cast(_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() 
to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);

zturner wrote:
> clayborg wrote:
> > Remove this comment. The whole point off this function is to get an 
> > argument vector that is NULL terminated. Feel free to rename the function 
> > as desired to indicate this, but that is the whole point of this function. 
> > Feel free to document it if needed as well.
> There are two ways we use these argument vectors.  One is in a call to a 
> function which takes an argc and an argv.  In that case the null terminator 
> is unnecessary because you're specifying the argc explicitly.  Another is in 
> a call to a function which you do not specify an argc, and in tht case it 
> requires it to be null terminated.
> 
> The point of this function isn't "return me something that's null 
> terminated", it's "return me something I can use like an argv".  That doesn't 
> require null termination.  And in fact, if you look at the callsites I fixed 
> up, not a single one depends on the null termination.  Everyone just needs to 
> know the size.  And for that you can call `result.size()`.  If you add the 
> null terminator, then you have to write `result.size()-1`, which is 
> unnecessarily confusing.  I believe it provides the most logical behavior as 
> it is currently written.
Remove the comment. Even in "main(int argc, const char **argv)" the "argv" is 
null terminated. So all unix variants expect the null termination. The correct 
solution is to add a boolean argument:

```
std::vector Args::GetArgumentVector(bool null_terminate = true) 
const {
```

Then it is clear.


Comment at: source/Interpreter/CommandObject.cpp:119
@@ -118,3 +118,2 @@
 // so we need to push a dummy value into position zero.
-args.Unshift(llvm::StringRef("dummy_string"));
 const bool require_validation = true;

zturner wrote:
> clayborg wrote:
> > Why was this removed?
> Because it's a little weird to do it at this high of a level.  This requires 
> internal knowledge of the workings of `Args::ParseOptions()`, which itself 
> calls `OptionParser::Parse()`, It's strange for a function this high up to 
> require such low-level knowledge of the workings of a function.  So instead, 
> I moved this behavior into `Args::ParseOptions`.  If you check that function, 
> you will see that it injects this argument into the beginning.
> 
> This is nice because now it never modifies the internal state of the `Args` 
> itself, but only the copy that gets passed to `getopt_long_only`.
Sounds good. I hate the fact that we have to do this. We might be able to play 
with the getopt_long globals to work around this and not have to add it, but as 
long as you took it into account I am fine.


https://reviews.llvm.org/D24952



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Zachary Turner via lldb-commits]

zturner added inline comments.


Comment at: source/Interpreter/Args.cpp:269-272
@@ -320,6 +268,6 @@
 
 const char *Args::GetArgumentAtIndex(size_t idx) const {
-  if (idx < m_argv.size())
-return m_argv[idx];
+  if (idx < m_args.size())
+return m_args[idx].c_str();
   return nullptr;
 }

clayborg wrote:
> Convert this over to return StringRef?
Yes, that is the next item on my list.  But this function is used EVERYWHERE, 
so that will be a very large CL and should be done orthogonally.


Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-return const_cast(_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() 
to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);

clayborg wrote:
> Remove this comment. The whole point off this function is to get an argument 
> vector that is NULL terminated. Feel free to rename the function as desired 
> to indicate this, but that is the whole point of this function. Feel free to 
> document it if needed as well.
There are two ways we use these argument vectors.  One is in a call to a 
function which takes an argc and an argv.  In that case the null terminator is 
unnecessary because you're specifying the argc explicitly.  Another is in a 
call to a function which you do not specify an argc, and in tht case it 
requires it to be null terminated.

The point of this function isn't "return me something that's null terminated", 
it's "return me something I can use like an argv".  That doesn't require null 
termination.  And in fact, if you look at the callsites I fixed up, not a 
single one depends on the null termination.  Everyone just needs to know the 
size.  And for that you can call `result.size()`.  If you add the null 
terminator, then you have to write `result.size()-1`, which is unnecessarily 
confusing.  I believe it provides the most logical behavior as it is currently 
written.


Comment at: source/Interpreter/Args.cpp:327
@@ -381,11 +326,3 @@
 char quote_char) {
-  // Since we are using a std::list to hold onto the copied C string and
-  // we don't have direct access to the elements, we have to iterate to
-  // find the value.
-  arg_sstr_collection::iterator pos, end = m_args.end();
-  size_t i = idx;
-  for (pos = m_args.begin(); i > 0 && pos != end; ++pos)
---i;
-
-  pos = m_args.insert(pos, arg_str);
+  m_args.insert(m_args.begin() + idx, arg_str);
 

clayborg wrote:
> Is there a conversion operator that converts StringRef to std::string 
> somewhere? How is this working? Iterators being detected?
Yes, StringRef has an `operator std::string()`.


Comment at: source/Interpreter/Args.cpp:373-376
@@ -460,21 +372,6 @@
 
-void Args::SetArguments(const char **argv) {
-  // m_argv will be rebuilt in UpdateArgvFromArgs() below, so there is
-  // no need to clear it here.
-  m_args.clear();
-  m_args_quote_char.clear();
-
-  if (argv) {
-// First copy each string
-for (size_t i = 0; argv[i]; ++i) {
-  m_args.push_back(argv[i]);
-  if ((argv[i][0] == '\'') || (argv[i][0] == '"') || (argv[i][0] == '`'))
-m_args_quote_char.push_back(argv[i][0]);
-  else
-m_args_quote_char.push_back('\0');
-}
-  }
-
-  UpdateArgvFromArgs();
+void Args::SetArguments(const Args ) {
+  m_args = other.m_args;
+  m_args_quote_char = other.m_args_quote_char;
 }
 

clayborg wrote:
> Should we change this to an assignment operator? If so, rename "other" to 
> "rhs".
I thought about it, but it seems like a minor stylistic issue.  I think it 
could go either way.  I'm fine adding it or not.


Comment at: source/Interpreter/CommandObject.cpp:119
@@ -118,3 +118,2 @@
 // so we need to push a dummy value into position zero.
-args.Unshift(llvm::StringRef("dummy_string"));
 const bool require_validation = true;

clayborg wrote:
> Why was this removed?
Because it's a little weird to do it at this high of a level.  This requires 
internal knowledge of the workings of `Args::ParseOptions()`, which itself 
calls `OptionParser::Parse()`, It's strange for a function this high up to 
require such low-level knowledge of the workings of a function.  So instead, I 
moved this behavior into `Args::ParseOptions`.  If you check that function, you 
will see that it injects this argument into the beginning.

This is nice because now it never modifies the internal state of the `Args` 
itself, but only the copy that gets passed to `getopt_long_only`.


Comment at: source/Target/ProcessInfo.cpp:94
@@ -93,3 +93,3 @@
bool first_arg_is_executable) {
-  m_arguments.SetArguments(argv);
+  

[Lldb-commits] [lldb] r282508 - convert TestFatArchives.py over to no-debug-info test

[Todd Fiala via lldb-commits]

Author: tfiala
Date: Tue Sep 27 12:17:21 2016
New Revision: 282508

URL: http://llvm.org/viewvc/llvm-project?rev=282508=rev
Log:
convert TestFatArchives.py over to no-debug-info test

We only use the .o-style debug info here regardless, so having
it run all three debuginfo styles was a waste.

This also strips out the custom build function and uses the
TestBase.build() method.

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py?rev=282508=282507=282508=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/fat_archives/TestFatArchives.py
 Tue Sep 27 12:17:21 2016
@@ -13,23 +13,16 @@ from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
-def execute_command(command):
-# print('%% %s' % (command))
-(exit_status, output) = seven.get_command_status_output(command)
-# if output:
-# print(output)
-# print('status = %u' % (exit_status))
-return exit_status
-
-
 class FatArchiveTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
+NO_DEBUG_INFO_TESTCASE = True
+
 @skipUnlessDarwin
-def test(self):
+def test_breakpoint_resolution_dwarf(self):
 if self.getArchitecture() == 'x86_64':
-execute_command("make CC='%s'" % (os.environ["CC"]))
+self.build()
 self.main()
 else:
 self.skipTest(


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

Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian

[Greg Clayton via lldb-commits]

clayborg added a comment.

Please comment why you are manually bit twiddling due to the size of the 
register that can change. We don't want anyone else copying this kind of code. 
Another solution would be to update the offset of the register when you change 
the byte size so none of this would need to be done. We are already updating 
the byte size of the register in the RegisterInfo, so why not do the same for 
the offset. You could store the original offset in the 
RegisterInfo.kinds[eRegisterKindProcessPlugin] as this field is only for the 
process plug-in's use.


https://reviews.llvm.org/D24603



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Greg Clayton via lldb-commits]

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

See inlined comments.



Comment at: source/Interpreter/Args.cpp:264-265
@@ -315,3 +263,4 @@
 size_t Args::GetArgumentCount() const {
-  if (m_argv.empty())
+  if (m_args.empty())
 return 0;
+  return m_args.size();

Remove this if, it is no longer needed. Just return m_args.size()


Comment at: source/Interpreter/Args.cpp:269-272
@@ -320,6 +268,6 @@
 
 const char *Args::GetArgumentAtIndex(size_t idx) const {
-  if (idx < m_argv.size())
-return m_argv[idx];
+  if (idx < m_args.size())
+return m_args[idx].c_str();
   return nullptr;
 }

Convert this over to return StringRef?


Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-return const_cast(_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() 
to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);

Remove this comment. The whole point off this function is to get an argument 
vector that is NULL terminated. Feel free to rename the function as desired to 
indicate this, but that is the whole point of this function. Feel free to 
document it if needed as well.


Comment at: source/Interpreter/Args.cpp:327
@@ -381,11 +326,3 @@
 char quote_char) {
-  // Since we are using a std::list to hold onto the copied C string and
-  // we don't have direct access to the elements, we have to iterate to
-  // find the value.
-  arg_sstr_collection::iterator pos, end = m_args.end();
-  size_t i = idx;
-  for (pos = m_args.begin(); i > 0 && pos != end; ++pos)
---i;
-
-  pos = m_args.insert(pos, arg_str);
+  m_args.insert(m_args.begin() + idx, arg_str);
 

Is there a conversion operator that converts StringRef to std::string 
somewhere? How is this working? Iterators being detected?


Comment at: source/Interpreter/Args.cpp:373-376
@@ -460,21 +372,6 @@
 
-void Args::SetArguments(const char **argv) {
-  // m_argv will be rebuilt in UpdateArgvFromArgs() below, so there is
-  // no need to clear it here.
-  m_args.clear();
-  m_args_quote_char.clear();
-
-  if (argv) {
-// First copy each string
-for (size_t i = 0; argv[i]; ++i) {
-  m_args.push_back(argv[i]);
-  if ((argv[i][0] == '\'') || (argv[i][0] == '"') || (argv[i][0] == '`'))
-m_args_quote_char.push_back(argv[i][0]);
-  else
-m_args_quote_char.push_back('\0');
-}
-  }
-
-  UpdateArgvFromArgs();
+void Args::SetArguments(const Args ) {
+  m_args = other.m_args;
+  m_args_quote_char = other.m_args_quote_char;
 }
 

Should we change this to an assignment operator? If so, rename "other" to "rhs".


Comment at: source/Interpreter/CommandInterpreter.cpp:2049-2050
@@ -2048,5 +2048,4 @@
 
 cmd_args.Clear();
-cmd_args.SetArguments(new_args.GetArgumentCount(),
-  new_args.GetConstArgumentVector());
+cmd_args.SetArguments(new_args);
   } else {

If we do the operator = above, then:

```
cmd_args = new_args;
```


Comment at: source/Interpreter/CommandInterpreter.cpp:2059-2060
@@ -2059,5 +2058,4 @@
 if (wants_raw_input) {
   cmd_args.Clear();
-  cmd_args.SetArguments(new_args.GetArgumentCount(),
-new_args.GetConstArgumentVector());
+  cmd_args.SetArguments(new_args);
 }

If we do the operator = above, then:

```
cmd_args = new_args;
```


Comment at: source/Interpreter/CommandObject.cpp:119
@@ -118,3 +118,2 @@
 // so we need to push a dummy value into position zero.
-args.Unshift(llvm::StringRef("dummy_string"));
 const bool require_validation = true;

Why was this removed?


Comment at: source/Interpreter/OptionValueArgs.cpp:33
@@ -32,3 +32,3 @@
   else
-args.SetArguments(argv.size(), [0]);
+args.SetArguments(argv);
   return args.GetArgumentCount();

args = argv;


Comment at: source/Interpreter/OptionValueArray.cpp:151
@@ -150,3 +150,3 @@
   else
-args.SetArguments(argv.size(), [0]);
+args.SetArguments(argv);
   return args.GetArgumentCount();

args = argv;


Comment at: source/Target/ProcessInfo.cpp:94
@@ -93,3 +93,3 @@
bool first_arg_is_executable) {
-  m_arguments.SetArguments(argv);
+  m_arguments.SetArguments(Args::ArgvToArrayRef(argv));
 

What not have a SetArguments that takes a "const char **" and does this 
internally? It is no less safe and makes the API more usable

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Zachary Turner via lldb-commits]

zturner accepted this revision.
This revision is now accepted and ready to land.


Comment at: 
source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:50
@@ +49,3 @@
+size = 8;
+break;
+  }

If they are just smaller (but never bigger), you can call `take_front(N)` on 
the result.  So you could say:

```
writeRegister(source_data, 
reg_info[lldb_cs_x86_64].mutable_data(result_base).take_front(2));
```

Now the line is starting to get long so might look ugly if it wraps, so it's up 
to you.  It is somewhat safer at least since you will assert if you 
accidentally specify an invalid size, and you don't have to worry about getting 
the computation right.

Perhaps you could combine this with the approach you've already taken here.  
Add the functions to `RegisterInfo`, then change `getDestRegister` to look like 
this:

```
llvm::MutableArrayRef getDestRegister(const RegisterInfo ,
   uint8_t *context, uint32_t 
lldb_reg_num) {
  auto bytes = reg.mutable_data(context);

  switch (lldb_reg_num) {
  case lldb_cs_x86_64:
  case lldb_ds_x86_64:
  case lldb_es_x86_64:
  case lldb_fs_x86_64:
  case lldb_gs_x86_64:
  case lldb_ss_x86_64:
return bytes.take_front(2);
break;
  case lldb_rflags_x86_64:
return bytes.take_front(4);
break;
  default:
return bytes.take_front(8);
}
```

then you could call `writeRegister` like this:

```
writeRegister(source_data, getDestRegister(reg_info, result_base, 
lldb_cs_x86_64));
```

At this point though I'm not sure what's better.  So don't consider this a 
requirement, up to you :)


https://reviews.llvm.org/D24919



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

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.


Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+#define REG_VAL(x) *(reinterpret_cast(x))
+
+TEST_F(MinidumpParserTest, ConvertRegisterContext) {

amccarth wrote:
> `EXPECT_xxx` will check the condition and report if it's wrong.  But then the 
> test proceeds.
> 
> `ASSERT_xxx` will check the condition, and, if it's wrong, it will report 
> _and_ terminate the current test.
> 
> So my rule of thumb is:
> 
> 1.  Prefer `EXPECT_xxx` when checking that the code does what it's supposed 
> to do.
> 2.  Use `ASSERT_xxx` when the rest of the test depends on this condition.
Thanks :)


https://reviews.llvm.org/D24919



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

[Lldb-commits] [lldb] r282496 - xfail TestExec.py on macOS

[Todd Fiala via lldb-commits]

Author: tfiala
Date: Tue Sep 27 10:57:12 2016
New Revision: 282496

URL: http://llvm.org/viewvc/llvm-project?rev=282496=rev
Log:
xfail TestExec.py on macOS

Tracked by:
rdar://28476369

Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/exec/TestExec.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/exec/TestExec.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/exec/TestExec.py?rev=282496=282495=282496=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/exec/TestExec.py 
(original)
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/exec/TestExec.py 
Tue Sep 27 10:57:12 2016
@@ -27,6 +27,7 @@ class ExecTestCase(TestBase):
 mydir = TestBase.compute_mydir(__file__)
 
 @skipUnlessDarwin
+@expectedFailureAll(oslist=["macosx"], bugnumber="rdar://28476369")
 def test(self):
 if self.getArchitecture() == 'x86_64':
 source = os.path.join(os.getcwd(), "main.cpp")


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

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Adrian McCarthy via lldb-commits]

amccarth added inline comments.


Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+#define REG_VAL(x) *(reinterpret_cast(x))
+
+TEST_F(MinidumpParserTest, ConvertRegisterContext) {

`EXPECT_xxx` will check the condition and report if it's wrong.  But then the 
test proceeds.

`ASSERT_xxx` will check the condition, and, if it's wrong, it will report _and_ 
terminate the current test.

So my rule of thumb is:

1.  Prefer `EXPECT_xxx` when checking that the code does what it's supposed to 
do.
2.  Use `ASSERT_xxx` when the rest of the test depends on this condition.


https://reviews.llvm.org/D24919



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

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

dvlahovski added a comment.

Thanks for the explanation and examples! :) Will keep this in mind in the 
future.


https://reviews.llvm.org/D24919



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

Re: [Lldb-commits] [PATCH] D13350: [lldb] Fix evaluation of qualified global variables

[Eugene Leviant via lldb-commits]

evgeny777 abandoned this revision.
evgeny777 added a comment.

This patch is longer valid after more than 9 months on review due to changes in 
namespace handing in ClangExpressionDeclMap. It probably should be done in a 
different way, but I don't have time for this now.


https://reviews.llvm.org/D13350



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

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Zachary Turner via lldb-commits]

zturner added a comment.

`ASSERT_EQ` causes the test method to return immediately if the condition 
fails.  `EXPECT_EQ` will continue running the same test body.  I like to use 
`ASSERT_EQ`, for example, if you are checking the value of a pointer returned 
by a function.  First you want to check if it's null, then you want to check 
that its value is the value you expect.  So I would do

  ASSERT_NE(p, nullptr);
  EXPECT_EQ(*p, 42);

Because if it's null, the rest of the statements in the test body are undefined.

Another example is if a method returns some value, say number of items in a 
list, and then you want to call another method to process that many items.  Say:

  int x = foo.GetItemCount();
  const Item *items = foo.GetItems();
  bool Success = ProcessItems(items, x);

If the number of items is not what you expect, it doesn't make sense to run the 
next statement.  So you could write:
int x = foo.GetItemCount();
ASSERT_EQ(42, x);
const Item *items = foo.GetItems();
EXPECT_TRUE(ProcessItems(items, x));

  


https://reviews.llvm.org/D24919



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

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

dvlahovski updated this revision to Diff 72659.
dvlahovski added a comment.

Addressing Zachary's and Adrian's comments


https://reviews.llvm.org/D24919

Files:
  source/Plugins/Process/minidump/CMakeLists.txt
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
  unittests/Process/minidump/MinidumpParserTest.cpp

Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -8,16 +8,19 @@
 //===--===//
 
 // Project includes
+#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
 #include "Plugins/Process/minidump/MinidumpParser.h"
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h"
 
 // Other libraries and framework includes
 #include "gtest/gtest.h"
 
 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Host/FileSpec.h"
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -50,7 +53,7 @@
 MinidumpParser::Create(data_sp);
 ASSERT_TRUE(optional_parser.hasValue());
 parser.reset(new MinidumpParser(optional_parser.getValue()));
-ASSERT_GT(parser->GetByteSize(), 0UL);
+ASSERT_GT(parser->GetData().size(), 0UL);
   }
 
   llvm::SmallString<128> inputs_folder;
@@ -167,3 +170,61 @@
   ASSERT_TRUE(pid.hasValue());
   ASSERT_EQ(4440UL, pid.getValue());
 }
+
+// Register stuff
+// TODO probably split register stuff tests into different file?
+#define REG_VAL(x) *(reinterpret_cast(x))
+
+TEST_F(MinidumpParserTest, ConvertRegisterContext) {
+  SetUpData("linux-x86_64.dmp");
+  llvm::ArrayRef thread_list = parser->GetThreads();
+  const MinidumpThread thread = thread_list[0];
+  llvm::ArrayRef registers(parser->GetData().data() +
+thread.thread_context.rva,
+thread.thread_context.data_size);
+
+  ArchSpec arch = parser->GetArchitecture();
+  RegisterInfoInterface *reg_interface = new RegisterContextLinux_x86_64(arch);
+  lldb::DataBufferSP buf =
+  ConvertMinidumpContextToRegIface(registers, reg_interface);
+  ASSERT_EQ(reg_interface->GetGPRSize(), buf->GetByteSize());
+
+  const RegisterInfo *reg_info = reg_interface->GetRegisterInfo();
+
+  std::map reg_values;
+
+  // clang-format off
+  reg_values[lldb_rax_x86_64]=  0x;
+  reg_values[lldb_rbx_x86_64]=  0x;
+  reg_values[lldb_rcx_x86_64]=  0x0010;
+  reg_values[lldb_rdx_x86_64]=  0x;
+  reg_values[lldb_rdi_x86_64]=  0x7ffceb349cf0;
+  reg_values[lldb_rsi_x86_64]=  0x;
+  reg_values[lldb_rbp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_rsp_x86_64]=  0x7ffceb34a210;
+  reg_values[lldb_r8_x86_64] =  0x7fe9bc1aa9c0;
+  reg_values[lldb_r9_x86_64] =  0x;
+  reg_values[lldb_r10_x86_64]=  0x7fe9bc3f16a0;
+  reg_values[lldb_r11_x86_64]=  0x0246;
+  reg_values[lldb_r12_x86_64]=  0x00401c92;
+  reg_values[lldb_r13_x86_64]=  0x7ffceb34a430;
+  reg_values[lldb_r14_x86_64]=  0x;
+  reg_values[lldb_r15_x86_64]=  0x;
+  reg_values[lldb_rip_x86_64]=  0x00401dc6;
+  reg_values[lldb_rflags_x86_64] =  0x00010206;
+  reg_values[lldb_cs_x86_64] =  0x0033;
+  reg_values[lldb_fs_x86_64] =  0x;
+  reg_values[lldb_gs_x86_64] =  0x;
+  reg_values[lldb_ss_x86_64] =  0x;
+  reg_values[lldb_ds_x86_64] =  0x;
+  reg_values[lldb_es_x86_64] =  0x;
+  // clang-format on
+
+  for (uint32_t reg_index = 0; reg_index < reg_interface->GetRegisterCount();
+   ++reg_index) {
+if (reg_values.find(reg_index) != reg_values.end()) {
+  EXPECT_EQ(reg_values[reg_index],
+REG_VAL(buf->GetBytes() + reg_info[reg_index].byte_offset));
+}
+  }
+}
Index: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
===
--- /dev/null
+++ source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
@@ -0,0 +1,119 @@
+//===-- RegisterContextMinidump_x86_64.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

Re: [Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Dimitar Vlahovski via lldb-commits]

dvlahovski marked 5 inline comments as done.


Comment at: 
source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:49
@@ +48,3 @@
+writeRegister(source_data, result_base, _info[lldb_cs_x86_64], 2);
+  }
+

zturner wrote:
> dvlahovski wrote:
> > sizeof(uint16_t), sizeof(uint32_t), etc ?
> I think my comments got out of line and this is no longer at the position I 
> suggested it.  that said, I actually didn't notice all these constants being 
> passed into `writeRegister`.  I would propose changing this.  Find the 
> `RegisterInfo` class in `lldb-private-types.h` and add the following methods:
> 
> ```
> llvm::ArrayRef data(const uint8_t *context_base) const { 
>   return llvm::ArrayRef(context_base + byte_offset, byte_size);
> }
> 
> llvm::MutableArrayRef mutable_data(uint8_t *context_base) const {
>   return llvm::MutableArrayRef(context_base + byte_offset, 
> byte_size);
> }
> ```
> 
> Then you can write:
> 
> ```writeRegister(source_data, 
> reg_info[lldb_cs_x86_64].mutable_data(result_base));```
This will make the code very clean indeed, but there is a problem.

From the `RegisterInfo` I only get the `byte_offset` and I don't use the 
`byte_size` because some of the registers in the Minidump context are smaller 
(e.g. cs is 16 bit). All of the registers in the Linux context are 64 bits in 
size. So that's why I basically hard-coded the size in each of the funciton 
calls. Now with the next revision I tried to make the code clearer.


Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:38
@@ +37,3 @@
+// - uint64_t p4_home
+// - uint64_t p5_home
+// - uint64_t p6_home

So I had the actual struct for the register context in the previous revision, 
but now I've removed it, because I actually don't use it. So added a comment 
describing it's layout.

For the endianness part -  because this code only permutes the bytes in the 
layout the Linux register context expects them, I don't think that here is the 
place to handle that. I was hoping that the Linux register context will handle 
it, but I don't think that's the case when I look at the code in 
`RegisterContextCorePOSIX_x86_64::ReadRegister`.

Am I right - do the Linux context assume little endian? If so maybe I can also 
fix that.


Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+static void registerEqualToVal(const uint64_t val, uint8_t *reg_val) {
+  ASSERT_EQ(val, *(reinterpret_cast(reg_val)));
+}

amccarth wrote:
> +1 to Zach's idea.
> 
> Also, consider whether `EXPECT_EQ` is more appropriate than `ASSERT_EQ` for 
> these.
Probably `EXPECT_EQ` is better because if there is a mismatch in the reg 
values, one can see all of the values.

I still don't have the sense when to use `EXPECT_EQ` and when `ASSERT_EQ`.


https://reviews.llvm.org/D24919



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Zachary Turner via lldb-commits]

zturner updated this revision to Diff 72657.
zturner added a comment.

Updated


https://reviews.llvm.org/D24952

Files:
  examples/plugins/commands/fooplugin.cpp
  include/lldb/API/SBCommandInterpreter.h
  include/lldb/Host/OptionParser.h
  include/lldb/Interpreter/Args.h
  source/API/SBCommandInterpreter.cpp
  source/API/SBLaunchInfo.cpp
  source/API/SBProcess.cpp
  source/API/SBTarget.cpp
  source/Commands/CommandObjectBreakpoint.cpp
  source/Commands/CommandObjectLog.cpp
  source/Host/common/OptionParser.cpp
  source/Interpreter/Args.cpp
  source/Interpreter/CommandInterpreter.cpp
  source/Interpreter/CommandObject.cpp
  source/Interpreter/OptionValueArgs.cpp
  source/Interpreter/OptionValueArray.cpp
  source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  source/Target/ProcessInfo.cpp
  source/Target/ProcessLaunchInfo.cpp

Index: source/Target/ProcessLaunchInfo.cpp
===
--- source/Target/ProcessLaunchInfo.cpp
+++ source/Target/ProcessLaunchInfo.cpp
@@ -339,8 +339,8 @@
 if (m_shell) {
   std::string shell_executable = m_shell.GetPath();
 
-  const char **argv = GetArguments().GetConstArgumentVector();
-  if (argv == nullptr || argv[0] == nullptr)
+  const auto argv = GetArguments().GetArgumentVector();
+  if (!argv[0])
 return false;
   Args shell_arguments;
   std::string safe_arg;
Index: source/Target/ProcessInfo.cpp
===
--- source/Target/ProcessInfo.cpp
+++ source/Target/ProcessInfo.cpp
@@ -91,7 +91,7 @@
 
 void ProcessInfo::SetArguments(char const **argv,
bool first_arg_is_executable) {
-  m_arguments.SetArguments(argv);
+  m_arguments.SetArguments(Args::ArgvToArrayRef(argv));
 
   // Is the first argument the executable?
   if (first_arg_is_executable) {
Index: source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
===
--- source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -421,15 +421,11 @@
   }
 
   // Send the environment and the program + arguments after we connect
-  const char **envp =
-  launch_info.GetEnvironmentEntries().GetConstArgumentVector();
-
-  if (envp) {
-const char *env_entry;
-for (int i = 0; (env_entry = envp[i]); ++i) {
-  if (m_gdb_client.SendEnvironmentPacket(env_entry) != 0)
-break;
-}
+  const auto argv = launch_info.GetEnvironmentEntries().GetArgumentVector();
+
+  for (auto env_entry : argv) {
+if (m_gdb_client.SendEnvironmentPacket(env_entry) != 0)
+  break;
   }
 
   ArchSpec arch_spec = launch_info.GetArchitecture();
Index: source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1391,6 +1391,7 @@
   if (!shell)
 return 1;
 
+  // CLEANUP: This function could be made nicer / safer with StringRef.
   std::string shell_string = shell.GetPath();
   const char *shell_name = strrchr(shell_string.c_str(), '/');
   if (shell_name == NULL)
@@ -1402,13 +1403,10 @@
 // /bin/sh re-exec's itself as /bin/bash requiring another resume.
 // But it only does this if the COMMAND_MODE environment variable
 // is set to "legacy".
-const char **envp =
-launch_info.GetEnvironmentEntries().GetConstArgumentVector();
-if (envp != NULL) {
-  for (int i = 0; envp[i] != NULL; i++) {
-if (strcmp(envp[i], "COMMAND_MODE=legacy") == 0)
-  return 2;
-  }
+const auto argv = launch_info.GetEnvironmentEntries().GetArgumentVector();
+for (auto env_entry : argv) {
+  if (strcmp(env_entry, "COMMAND_MODE=legacy") == 0)
+return 2;
 }
 return 1;
   } else if (strcmp(shell_name, "csh") == 0 ||
Index: source/Interpreter/OptionValueArray.cpp
===
--- source/Interpreter/OptionValueArray.cpp
+++ source/Interpreter/OptionValueArray.cpp
@@ -148,7 +148,7 @@
   if (argv.empty())
 args.Clear();
   else
-args.SetArguments(argv.size(), [0]);
+args.SetArguments(argv);
   return args.GetArgumentCount();
 }
 
Index: source/Interpreter/OptionValueArgs.cpp
===
--- source/Interpreter/OptionValueArgs.cpp
+++ source/Interpreter/OptionValueArgs.cpp
@@ -30,6 +30,6 @@
   if (argv.empty())
 args.Clear();
   else
-args.SetArguments(argv.size(), [0]);
+args.SetArguments(argv);
   return args.GetArgumentCount();
 }
Index: source/Interpreter/CommandObject.cpp
===
--- source/Interpreter/CommandObject.cpp
+++ 

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Adrian McCarthy via lldb-commits]

amccarth added a subscriber: amccarth.


Comment at: include/lldb/Interpreter/Args.h:154
@@ -167,3 +153,3 @@
   //--
-  const char **GetConstArgumentVector() const;
+  void GetArgumentVector(std::vector ) const;
 

Perhaps this should actually return the vector instead of void, and rely on the 
return value optimization or, at worst, vector move.  It would be easier to use 
and it would let you initialize variables as you declare them, (which means you 
could use it in constructor intializers and you could declare the vector as 
const).  It also would eliminate the ambiguity of what happens if the `args` 
already contains some values (e.g., the caller might expect it to append rather 
than replace).


Comment at: source/API/SBCommandInterpreter.cpp:119
@@ +118,3 @@
+std::vector argv;
+command.GetArgumentVector(argv);
+bool ret = m_backend->DoExecute(debugger_sb, [0], sb_return);

Here's an example where, if GetArgumentVector actually returned the result, you 
could simplify the delcaration as:

const auto argv = command.GetArgumentVector();


Comment at: source/Host/common/FileSpec.cpp:251
@@ +250,3 @@
+
+  for (auto  : name_list) {
+matches.AppendString(name);

Should `name` be `const`?  As in:

for (const auto  : name_list) { ... }


Comment at: source/Interpreter/Args.cpp:46
@@ -44,3 +45,3 @@
 // strings in rhs.m_args. We need to copy the string list and update our
 // own m_argv appropriately.
 //--

I think your change eliminated the need for this comment.  If not, it at least 
needs an update.


Comment at: source/Interpreter/Args.cpp:275
@@ +274,3 @@
+  // new_argv.
+  // So make a copy and then swap it in.
+  std::vector new_args(new_argv.begin(), new_argv.end());

clang-format didn't do a great job wrapping the long lines in this comment.  
Can you fix it up?


Comment at: source/Interpreter/Args.cpp:427
@@ -512,1 +426,3 @@
+  GetArgumentVector(args);
+  args.insert(args.begin(), "dummy");
   while (1) {

This is an example where the ambiguity of whether GetArgumentVector appends or 
not could mislead the caller into doing the wrong this.  This looks plausible 
(and more efficient):

std::vector args;
args.push_back("dummy");
GetArgumentVector(args);

If GetArgumentVector returned the vector, then this ambiguity wouldn't exist.


https://reviews.llvm.org/D24952



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

Re: [Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Pavel Labath via lldb-commits]

labath added a subscriber: labath.


Comment at: include/lldb/Interpreter/Args.h:150
@@ -147,19 +149,3 @@
   ///
   /// @return
   /// An array of NULL terminate C string argument pointers that

I think doxygen will complain about `@return` in a function returning void.

BTW, with the NRVO, move semantics and everything, is there any reason to keep 
using the return-through-reference-argument pattern? It would be a lot more 
natural if this indeed _was_ a return value.


https://reviews.llvm.org/D24952



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

Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian

[Nitesh Jain via lldb-commits]

nitesh.jain added a comment.

In https://reviews.llvm.org/D24603#548902, @clayborg wrote:

> So it seems like this can be fixed by doing a few things:
>  1 - just set the RegisterInfo.offset to the actual offset in the buffer and 
> don't use the offset as the ptrace key used to grab the register
>  2 - modify the RegisterInfo.kinds[eRegisterKindProcessPlugin] to be the 
> offset to you to use for ptrace
>
> There should be no need what so ever to do manual bit twiddling with src and 
> dst.


The floating point register size can be dynamically changes . Hence following 
cases are possible when we read registers from ptrace in FPR_linux_mips buffer 
(In case of MIPS, ptrace always return value in 64 bit container irrespective 
of Arch).

1. Floating point register size is 32 (SR.FR = 0).

  a) In case of big endian system, the FPR_linux_mips.f0 will contains two 
floating point registers . The  $f0 register will be store in upper 32 bit and  
$f1 register in lower 32 bit of FPR_linux_mips.f0. The FPR_linux_mips.f1 will 
contain don't care value and similarly for all other registers. Thus each even 
buffer will contain valid value and represent two registers.

  b) In case of liitle endian , the  $f0 will be store in lower 32 bit and $f1 
in upper 32bit of  FPR_linux_mips.f0. The FPR_linux_mips.f1 will contain don't 
care value and similarly for all other registers.

2. Floating point register size is 64 (SR.FR = 1). In these case all buffer 
will have valid value.

Hence we need bit twiddling with src and dst  (in case of SR.FR = 0) so that 
each buffer will represent value for single register.


https://reviews.llvm.org/D24603



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

Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian

[Nitesh Jain via lldb-commits]

nitesh.jain updated this revision to Diff 72618.
nitesh.jain added a comment.
Herald added a subscriber: ki.stfu.

Updated patch as per suggestion.


https://reviews.llvm.org/D24603

Files:
  source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
  source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h
  source/Plugins/Process/Utility/RegisterInfos_mips.h
  source/Plugins/Process/Utility/RegisterInfos_mips64.h
  source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h

Index: source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h
===
--- source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h
+++ source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h
@@ -282,6 +282,84 @@
 k_num_fpr_registers_mips64 +
 k_num_msa_registers_mips64
 };
+
+// Register no. for RegisterKind = eRegisterKindProcessPlugin
+// The ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER used this number
+enum {
+  ptrace_zero_mips, 
+  ptrace_r1_mips,
+  ptrace_r2_mips,
+  ptrace_r3_mips,
+  ptrace_r4_mips,
+  ptrace_r5_mips,
+  ptrace_r6_mips,
+  ptrace_r7_mips,
+  ptrace_r8_mips,
+  ptrace_r9_mips,
+  ptrace_r10_mips,
+  ptrace_r11_mips,
+  ptrace_r12_mips,
+  ptrace_r13_mips,
+  ptrace_r14_mips,
+  ptrace_r15_mips,
+  ptrace_r16_mips,
+  ptrace_r17_mips,
+  ptrace_r18_mips,
+  ptrace_r19_mips,
+  ptrace_r20_mips,
+  ptrace_r21_mips,
+  ptrace_r22_mips,
+  ptrace_r23_mips,
+  ptrace_r24_mips,
+  ptrace_r25_mips,
+  ptrace_r26_mips,
+  ptrace_r27_mips,
+  ptrace_gp_mips,
+  ptrace_sp_mips,
+  ptrace_r30_mips,
+  ptrace_ra_mips,
+  ptrace_f0_mips,
+  ptrace_f1_mips,
+  ptrace_f2_mips,
+  ptrace_f3_mips,
+  ptrace_f4_mips,
+  ptrace_f5_mips,
+  ptrace_f6_mips,
+  ptrace_f7_mips,
+  ptrace_f8_mips,
+  ptrace_f9_mips,
+  ptrace_f10_mips,
+  ptrace_f11_mips,
+  ptrace_f12_mips,
+  ptrace_f13_mips,
+  ptrace_f14_mips,
+  ptrace_f15_mips,
+  ptrace_f16_mips,
+  ptrace_f17_mips,
+  ptrace_f18_mips,
+  ptrace_f19_mips,
+  ptrace_f20_mips,
+  ptrace_f21_mips,
+  ptrace_f22_mips,
+  ptrace_f23_mips,
+  ptrace_f24_mips,
+  ptrace_f25_mips,
+  ptrace_f26_mips,
+  ptrace_f27_mips,
+  ptrace_f28_mips,
+  ptrace_f29_mips,
+  ptrace_f30_mips,
+  ptrace_f31_mips,
+  ptrace_pc_mips,
+  ptrace_cause_mips,
+  ptrace_badvaddr_mips,
+  ptrace_mulhi_mips,
+  ptrace_mullo_mips,
+  ptrace_fcsr_mips,
+  ptrace_fir_mips,
+  ptrace_sr_mips,
+  ptrace_config5_mips
+};
 }
 
 #endif // #ifndef lldb_mips_linux_register_enums_h
Index: source/Plugins/Process/Utility/RegisterInfos_mips64.h
===
--- source/Plugins/Process/Utility/RegisterInfos_mips64.h
+++ source/Plugins/Process/Utility/RegisterInfos_mips64.h
@@ -42,11 +42,11 @@
 
 // Note that the size and offset will be updated by platform-specific classes.
 #ifdef LINUX_MIPS64
-#define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4)   \
+#define DEFINE_GPR(reg, alt, kind1, kind2, kind3)  \
   {\
 #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg),\
   GPR_OFFSET(reg), eEncodingUint, eFormatHex,  \
- {kind1, kind2, kind3, kind4,  \
+ {kind1, kind2, kind3, ptrace_##reg##_mips,\
   gpr_##reg##_mips64 },\
   NULL, NULL, NULL, 0  \
   }
@@ -61,11 +61,11 @@
   }
 #endif
 
-#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3, kind4)  \
+#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3) \
   {\
 #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg) / 2,\
   GPR_OFFSET(reg), eEncodingUint, eFormatHex,  \
- {kind1, kind2, kind3, kind4,  \
+ {kind1, kind2, kind3, ptrace_##reg##_mips,\
   gpr_##reg##_mips64 },\
   NULL, NULL, NULL, 0  \
   }
@@ -75,21 +75,21 @@
 llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and,
 llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shr};
 
-#define DEFINE_FPR(reg, alt, kind1, kind2, kind3, kind4)   \
+#define DEFINE_FPR(reg, alt, kind1, kind2, kind3)  \
   {\
 #reg, alt, sizeof(((FPR_linux_mips *) 0)->reg),\
   FPR_OFFSET(reg),