[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.


It sounds like that could be an artefact from how we parse function types from 
dwarf. I haven't checked if that's what happens in this particular case, but I 
know that we parse function types by just taking the function DIE (which 
describes the whole function, not just its type) and let the generic 
ParseTypeFromDWARF function loose on it. It could be that this function picks 
up the DW_AT_name attribute from there (which is the function name in this 
case, but for other kinds of types it would actually be the type name). We 
could probably change that function to ignore the DW_AT_name for functions, 
though it's not clear to me whether the current behavior has any adverse 
effects (the final type name seems to come out alright...)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88483/new/

https://reviews.llvm.org/D88483

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

[Lldb-commits] [PATCH] D89807: Fix "Unknown arguments specified" to if in lldb

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Sorry about the breakage.




Comment at: lldb/cmake/modules/FindPythonAndSwig.cmake:51
   "${SWIG_VERSION}" VERSION_LESS "4.0" AND WIN32 AND (
-  ${MULTI_CONFIG} OR (${uppercase_CMAKE_BUILD_TYPE} STREQUAL "DEBUG")))
+  ${MULTI_CONFIG} OR (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")))
 # Technically this can happen with non-Windows builds too, but we are not

Maybe add quotes here instead of dropping the `${}`. I'm not really sure which 
one is more idiomatic, but that's what I did for the other variables 
(SWIG_VERSION, etc.) -- it seems I just forgot about this one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89807/new/

https://reviews.llvm.org/D89807

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

[Lldb-commits] [PATCH] D89859: Remove .svn from exclude list as we moved to git

[Sylvestre Ledru via Phabricator via lldb-commits]

sylvestre.ledru created this revision.
sylvestre.ledru added a reviewer: serge-sans-paille.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, mgorny.
Herald added a reviewer: bollu.
Herald added projects: clang, LLDB, LLVM.
sylvestre.ledru requested review of this revision.
Herald added a subscriber: JDevlieghere.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89859

Files:
  clang/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  lld/CMakeLists.txt
  lldb/cmake/modules/LLDBConfig.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/CMakeLists.txt
  polly/CMakeLists.txt
  polly/lib/External/CMakeLists.txt

Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -279,7 +279,6 @@
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
-  PATTERN ".svn" EXCLUDE
   )
   endif()
 
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -125,7 +125,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${POLLY_BINARY_DIR}/include/
@@ -133,7 +132,6 @@
 FILES_MATCHING
 PATTERN "*.h"
 PATTERN "CMakeFiles" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 endif()
 
Index: llvm/cmake/modules/CMakeLists.txt
===
--- llvm/cmake/modules/CMakeLists.txt
+++ llvm/cmake/modules/CMakeLists.txt
@@ -93,7 +93,6 @@
 file(COPY .
   DESTINATION ${llvm_cmake_builddir}
   FILES_MATCHING PATTERN *.cmake
-  PATTERN .svn EXCLUDE
   PATTERN CMakeFiles EXCLUDE
   )
 
@@ -152,7 +151,6 @@
 DESTINATION ${LLVM_INSTALL_PACKAGE_DIR}
 COMPONENT cmake-exports
 FILES_MATCHING PATTERN *.cmake
-PATTERN .svn EXCLUDE
 PATTERN LLVMConfig.cmake EXCLUDE
 PATTERN LLVMConfigExtensions.cmake EXCLUDE
 PATTERN LLVMConfigVersion.cmake EXCLUDE
Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -1104,7 +1104,6 @@
 PATTERN "*.td"
 PATTERN "*.inc"
 PATTERN "LICENSE.TXT"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${LLVM_INCLUDE_DIR}/llvm ${LLVM_INCLUDE_DIR}/llvm-c
@@ -1118,7 +1117,6 @@
 # Exclude include/llvm/CMakeFiles/intrinsics_gen.dir, matched by "*.def"
 PATTERN "CMakeFiles" EXCLUDE
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   if (LLVM_INSTALL_MODULEMAPS)
Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -228,7 +228,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 
@@ -237,7 +236,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 
Index: lld/CMakeLists.txt
===
--- lld/CMakeLists.txt
+++ lld/CMakeLists.txt
@@ -195,7 +195,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 endif()
 
Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -174,7 +174,6 @@
   DESTINATION "${LIBCLANG_HEADERS_INSTALL_DESTINATION}"
   FILES_MATCHING
   PATTERN "*.h"
-  PATTERN ".svn" EXCLUDE
   )
 
 # LLVM_DISTRIBUTION_COMPONENTS requires that each component have both a
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -446,7 +446,6 @@
 PATTERN "*.def"
 PATTERN "*.h"
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/include/clang
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is a good start. See inline comments for details. In addition to 
the test for the "target symbols add" flow it would be good to have a separate 
test for the ObjectFilePDB functionality. You can look at existing tests in 
`test/Shell/ObjectFile` for inspiration.




Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:10
+#include "ObjectFilePDB.h"
+
+#include "lldb/Core/Module.h"

You should be able to delete this line too clang-format should know to put this 
file first.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:45
+static UUID GetPDBUUID(InfoStream &IS) {
+  // This part is similar with what has done in ObjectFilePECOFF.
+  using llvm::support::endian::read16be;

There's also a third copy of this code in ProcessMinidump (MinidumpParser.cpp). 
I think it's time to factor this out somehow. We could either create a new file 
in the Utility module, or put this functionality in directly inside the UUID 
class -- I'm not currently sure what's better.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:172
+  lldb_private::UUID &uuid = module_spec.GetUUID();
+  if (!uuid.IsValid())
+uuid = GetPDBUUID(*info_stream);

I'm pretty sure this check is bogus and can never be false.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:176-197
+  switch (dbi_stream->getMachineType()) {
+  case PDB_Machine::Amd64:
+spec.SetTriple("x86_64-pc-windows");
+specs.Append(module_spec);
+break;
+  case PDB_Machine::x86:
+spec.SetTriple("i386-pc-windows");

Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}

Leave this unimplemented. PDBs don't have sections and are not loaded into 
memory, so the function does not apply.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:215-216
+  m_sections_up = std::make_unique();
+  for (auto s : unified_section_list)
+m_sections_up->AddSection(s);
+}

This shouldn't be necessary. What "normally" happens here is that an object 
file is adding own sections *into* the "unified" section list -- not the other 
way around. Since we're pretending that the pdb file has no sections (and 
that's kind of true) I think you should just return an empty section list here, 
and not touch the unified list at all.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:145
 return nullptr;
-
   return objfile_up.release();

revert spurious changes.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
 std::unique_ptr file_up =
 loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), 
m_allocator);
 

Instead of changing `loadMatchingPDBFile` I think we ought to change this to 
something like:
```
if (auto *pdb = llvm::dyn_cast(m_objfile_sp)) {
  file_up = pdb->giveMeAPDBFile(); 
  // PS: giveMeAPDBFile is a new pdb-specific public method on ObjectFilePDB
  // PS: possibly this should not return a PDBFile, but a PDBIndex directly -- 
I don't know the details but the general idea is that it could/should reuse the 
pdb parsing state that the objectfile class has already created
} else 
  file_up = do_the_loadMatchingPDBFile_fallback();
```

The reason for that is that it avoids opening the pdb file twice (once in the 
object file and once here). Another reason is that I believe the entire 
`loadMatchingPDBFile` function should disappear. Finding the pdb file should 
not be the job of the symbol file class -- we have symbol vendors for that. 
However, we should keep the fallback for now to keep the patch small...



Comment at: lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp:14
+// Create lldbinit
+// RUN: echo -e "target symbols add %t/executable/bar.pdb\nquit" > 
%t/load-pdb.lldbinit
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t/executable/foo.exe -s \

I'd just pass commands to lldb directly via `-o`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89812/new/

https://reviews.llvm.org/D89812

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

[Lldb-commits] [PATCH] D85241: [intel-pt] Disable/Enable tracing to guarantee the trace is correct

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Sounds reasonable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85241/new/

https://reviews.llvm.org/D85241

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

[Lldb-commits] [PATCH] D89156: [lldb] GetSharedModule: Collect old modules in SmallVector

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89156/new/

https://reviews.llvm.org/D89156

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added a reviewer: labath.
Herald added subscribers: pengfei, krytarowski.
mgorny requested review of this revision.

This is a WIP on unifying the x86 watchpoint implementation that is used on 
Linux and NetBSD. For the initial version, I've copied the code from NetBSD and 
tested it on Linux. Both my minimal testing worked and I didn't see any change 
in `check-lldb`.

The NetBSD implementation differs from Linux:

1. Uses `ReadRegister()` instead of `ReadRegisterRaw()`. I don't think that's a 
problem, might be a little less optimized but shouldn't matter.
2. Uses global watchpoint bit rather than local (local don't work correctly on 
NetBSD). If you prefer staying with global bit, I can trivially abstract that 
out.
3. Clears DR6 on adding new watchpoint with a different address, rather than on 
clearing it. This is necessary since LLDB clears and readds watchpoint on every 
hit, on all threads. This prevented the NetBSD plugin from correctly detecting 
serialized watchpoint hits (since DR6 was cleared on other threads that were 
still pending).


https://reviews.llvm.org/D89874

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h

Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
@@ -0,0 +1,53 @@
+//===-- WatchpointMixin_x86.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#ifndef lldb_WatchpointMixin_x86_h
+#define lldb_WatchpointMixin_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class WatchpointMixin_x86 : virtual public NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  // TODO: do we need to call it explicitly like on netbsd?
+  Status ClearWatchpointHit(uint32_t wp_index);
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+uint32_t watch_flags,
+uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  int GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_WatchpointMixin_x86_h
+
+#endif // defined(__i386__) || defined(__x86_64__)
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
@@ -0,0 +1,261 @@
+//===-- WatchpointMixin_x86.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#include "WatchpointMixin_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+
+using namespace lldb_private;
+
+int WatchpointMixin_x86::GetDR(int num) const {
+  assert(num >= 0 && num <= 7);
+  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
+  case llvm::Triple::x86:
+return lldb_dr0_i386 + num;
+  case llvm::Triple::x86_64:
+return lldb_dr0_x86_64 + num;
+  default:
+llvm_unreachable("Unhandled target architecture.");
+  }
+}
+
+Status WatchpointMixin_x86::IsWatchpointHit(uint32_t wp_index,
+   bool &is_hit) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())

[Lldb-commits] [PATCH] D89875: [nfc] [lldb] Fix harmless slicing of DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: labath.
jankratochvil added a project: LLDB.
Herald added subscribers: JDevlieghere, kristof.beyls.
jankratochvil requested review of this revision.

Is `return {...};` implicit ctor OK for LLDB? I have found only one line 
(`return {sdk};`) in rG5935227e11f5 
 using 
that syntax and I would normally use it everywhere.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89875

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -166,7 +166,7 @@
 
   void SetBaseAddress(dw_addr_t base_addr);
 
-  DWARFBaseDIE GetUnitDIEOnly() { return DWARFDIE(this, GetUnitDIEPtrOnly()); }
+  DWARFBaseDIE GetUnitDIEOnly() { return {this, GetUnitDIEPtrOnly()}; }
 
   DWARFDIE DIE() { return DWARFDIE(this, DIEPtr()); }
 


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -166,7 +166,7 @@
 
   void SetBaseAddress(dw_addr_t base_addr);
 
-  DWARFBaseDIE GetUnitDIEOnly() { return DWARFDIE(this, GetUnitDIEPtrOnly()); }
+  DWARFBaseDIE GetUnitDIEOnly() { return {this, GetUnitDIEPtrOnly()}; }
 
   DWARFDIE DIE() { return DWARFDIE(this, DIEPtr()); }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:296
 const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
-: NativeRegisterContextLinux(native_thread,
+: NativeRegisterContextRegisterInfo(native_thread, 
CreateRegisterInfoInterface(target_arch)),
+  NativeRegisterContextLinux(native_thread,

I'm wondering if we can eliminate the init inside 
`NativeRegisterContextRegisterInfo`



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:146-155
+  // clear dr6 if address or bits changed (i.e. we're not reenabling the same
+  // watchpoint)
+  if (drN_value.GetAsUInt64() != addr ||
+  (dr7_value.GetAsUInt64() & bit_mask) != (rw_bits | size_bits)) {
+ClearWatchpointHit(wp_index);
+
+error = WriteRegister(reg_info_drN, RegisterValue(addr));

This is the DR6 cleanup I was talking about.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())

On NetBSD, we have to call this explicitly when processing watchpoint hit (in 
SIGTRAP handling). Not sure if Linux would need that too (in which case we'd 
have to add a virtual method to the base class, I guess), or if the kernel 
takes care of it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h:110
 
-  bool IsGPR(uint32_t reg_index) const;
+  bool IsGPROrDR(uint32_t reg_index) const;
 

I have changed this to facilitate `WriteRegister()`. Alternatively, I suppose 
we could reverse the loop to match `ReadRegister()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89875: [nfc] [lldb] Fix harmless slicing of DWARFDIE

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

> Is return {...}; implicit ctor OK for LLDB? I have found only one line 
> (return {sdk};) in rG5935227e11f5 
>  using 
> that syntax and I would normally use it everywhere.

We don't have a policy on that. I personally wouldn't recommend using it 
literally _everywhere_ (this can get messy in very long function and 
particularly if you start nesting the `{}`s), but here it's perfectly fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89875/new/

https://reviews.llvm.org/D89875

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

[Lldb-commits] [PATCH] D89875: [nfc] [lldb] Fix harmless slicing of DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7bf066a20f4b: [nfc] [lldb] Fix harmless slicing of DWARFDIE 
(authored by jankratochvil).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89875/new/

https://reviews.llvm.org/D89875

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -166,7 +166,7 @@
 
   void SetBaseAddress(dw_addr_t base_addr);
 
-  DWARFBaseDIE GetUnitDIEOnly() { return DWARFDIE(this, GetUnitDIEPtrOnly()); }
+  DWARFBaseDIE GetUnitDIEOnly() { return {this, GetUnitDIEPtrOnly()}; }
 
   DWARFDIE DIE() { return DWARFDIE(this, DIEPtr()); }
 


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -166,7 +166,7 @@
 
   void SetBaseAddress(dw_addr_t base_addr);
 
-  DWARFBaseDIE GetUnitDIEOnly() { return DWARFDIE(this, GetUnitDIEPtrOnly()); }
+  DWARFBaseDIE GetUnitDIEOnly() { return {this, GetUnitDIEPtrOnly()}; }
 
   DWARFDIE DIE() { return DWARFDIE(this, DIEPtr()); }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 7bf066a - [nfc] [lldb] Fix harmless slicing of DWARFDIE

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2020-10-21T15:49:53+02:00
New Revision: 7bf066a20f4bfd52a79ae7650632bb3925171104

URL: 
https://github.com/llvm/llvm-project/commit/7bf066a20f4bfd52a79ae7650632bb3925171104
DIFF: 
https://github.com/llvm/llvm-project/commit/7bf066a20f4bfd52a79ae7650632bb3925171104.diff

LOG: [nfc] [lldb] Fix harmless slicing of DWARFDIE

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
index abe16182ef62..d6009518da12 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -166,7 +166,7 @@ class DWARFUnit : public lldb_private::UserID {
 
   void SetBaseAddress(dw_addr_t base_addr);
 
-  DWARFBaseDIE GetUnitDIEOnly() { return DWARFDIE(this, GetUnitDIEPtrOnly()); }
+  DWARFBaseDIE GetUnitDIEOnly() { return {this, GetUnitDIEPtrOnly()}; }
 
   DWARFDIE DIE() { return DWARFDIE(this, DIEPtr()); }
 



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

[Lldb-commits] [PATCH] D88483: Add possibility to get module from SBType

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum added a comment.

In D88483#2343694 , @labath wrote:

> 
> It sounds like that could be an artefact from how we parse function types 
> from dwarf. I haven't checked if that's what happens in this particular case, 
> but I know that we parse function types by just taking the function DIE 
> (which describes the whole function, not just its type) and let the generic 
> ParseTypeFromDWARF function loose on it. It could be that this function picks 
> up the DW_AT_name attribute from there (which is the function name in this 
> case, but for other kinds of types it would actually be the type name). We 
> could probably change that function to ignore the DW_AT_name for functions, 
> though it's not clear to me whether the current behavior has any adverse 
> effects (the final type name seems to come out alright...)
> 

Thanks for reply, it seems it works the way you tell. Added to my above example 
one more function definition, so the source file function_type.c looks like:

  int main1 (int argc, const char * argv[]) {
  return 0;
  }
  
  int main (int argc, const char * argv[]) {
  return 0;
  }

At the end of dwarf parsing content of m_die_to_type is:

  (lldb_private::ConstString) $27 = (m_string = "main1")
  (lldb_private::ConstString) $28 = (m_string = "int (int, const char **)")
  
  (lldb_private::ConstString) $29 = (m_string = "main")
  (lldb_private::ConstString) $30 = (m_string = "int (int, const char **)")
  
  (lldb_private::ConstString) $31 = (m_string = "int")
  (lldb_private::ConstString) $32 = (m_string = "int")
  
  (lldb_private::ConstString) $33 = (m_string = 0x)
  (lldb_private::ConstString) $34 = (m_string = "const char **")
  
  (lldb_private::ConstString) $35 = (m_string = 0x)
  (lldb_private::ConstString) $36 = (m_string = "const char *")
  
  (lldb_private::ConstString) $37 = (m_string = "char")
  (lldb_private::ConstString) $38 = (m_string = "char")

Where first and second m_string in each block are m_name and 
m_compiler_type->GetTypeName() of Type correspondingly. Later on in 
SymbolFileDWARF::GetTypes type duplicates are filtered out:

  std::set compiler_type_set;
for (Type *type : type_set) {
  CompilerType compiler_type = type->GetForwardCompilerType();
  if (compiler_type_set.find(compiler_type) == compiler_type_set.end()) {
compiler_type_set.insert(compiler_type);
type_list.Insert(type->shared_from_this());
  }
}

Ending up with resulting type list as follows:

  (lldb_private::ConstString) $27 = (m_string = "main1")
  (lldb_private::ConstString) $28 = (m_string = "int (int, const char **)")
  
  (lldb_private::ConstString) $31 = (m_string = "int")
  (lldb_private::ConstString) $32 = (m_string = "int")
  
  (lldb_private::ConstString) $33 = (m_string = 0x)
  (lldb_private::ConstString) $34 = (m_string = "const char **")
  
  (lldb_private::ConstString) $35 = (m_string = 0x)
  (lldb_private::ConstString) $36 = (m_string = "const char *")
  
  (lldb_private::ConstString) $37 = (m_string = "char")
  (lldb_private::ConstString) $38 = (m_string = "char")

Maybe of course that's not a problem but for me it was at least confusing, 
Type's m_name actually not a type's name but identifier in a source file. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88483/new/

https://reviews.llvm.org/D88483

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

[Lldb-commits] [PATCH] D89859: Remove .svn from exclude list as we moved to git

[Ed Maste via Phabricator via lldb-commits]

emaste added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89859/new/

https://reviews.llvm.org/D89859

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

[Lldb-commits] [PATCH] D89859: Remove .svn from exclude list as we moved to git

[Sylvestre Ledru via Phabricator via lldb-commits]

sylvestre.ledru added a comment.

ed if you want to approve it ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89859/new/

https://reviews.llvm.org/D89859

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

[Lldb-commits] [lldb] 0784e17 - Remove .svn from exclude list as we moved to git

[Sylvestre Ledru via lldb-commits]

Author: Sylvestre Ledru
Date: 2020-10-21T16:09:21+02:00
New Revision: 0784e17f1b4ac6b613ebf1fb1fb9b0dc9d0776ec

URL: 
https://github.com/llvm/llvm-project/commit/0784e17f1b4ac6b613ebf1fb1fb9b0dc9d0776ec
DIFF: 
https://github.com/llvm/llvm-project/commit/0784e17f1b4ac6b613ebf1fb1fb9b0dc9d0776ec.diff

LOG: Remove .svn from exclude list as we moved to git

Reviewed By: emaste

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

Added: 


Modified: 
clang/CMakeLists.txt
clang/tools/libclang/CMakeLists.txt
lld/CMakeLists.txt
lldb/cmake/modules/LLDBConfig.cmake
llvm/CMakeLists.txt
llvm/cmake/modules/CMakeLists.txt
polly/CMakeLists.txt
polly/lib/External/CMakeLists.txt

Removed: 




diff  --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index 900ef0a4d737..a2b99e2e37e9 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -446,7 +446,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 PATTERN "*.def"
 PATTERN "*.h"
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/include/clang

diff  --git a/clang/tools/libclang/CMakeLists.txt 
b/clang/tools/libclang/CMakeLists.txt
index 15f7ff94dfea..51ff2e7e1565 100644
--- a/clang/tools/libclang/CMakeLists.txt
+++ b/clang/tools/libclang/CMakeLists.txt
@@ -174,7 +174,6 @@ install(DIRECTORY ../../include/clang-c
   DESTINATION "${LIBCLANG_HEADERS_INSTALL_DESTINATION}"
   FILES_MATCHING
   PATTERN "*.h"
-  PATTERN ".svn" EXCLUDE
   )
 
 # LLVM_DISTRIBUTION_COMPONENTS requires that each component have both a

diff  --git a/lld/CMakeLists.txt b/lld/CMakeLists.txt
index 8b8c7178c616..82b4b9b9b198 100644
--- a/lld/CMakeLists.txt
+++ b/lld/CMakeLists.txt
@@ -195,7 +195,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 endif()
 

diff  --git a/lldb/cmake/modules/LLDBConfig.cmake 
b/lldb/cmake/modules/LLDBConfig.cmake
index 5fbc89892c73..2fdf1502d055 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -228,7 +228,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 
@@ -237,7 +236,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 

diff  --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index a7ac346ae364..344ccb6fda2f 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -1104,7 +1104,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 PATTERN "*.td"
 PATTERN "*.inc"
 PATTERN "LICENSE.TXT"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${LLVM_INCLUDE_DIR}/llvm ${LLVM_INCLUDE_DIR}/llvm-c
@@ -1118,7 +1117,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 # Exclude include/llvm/CMakeFiles/intrinsics_gen.dir, matched by "*.def"
 PATTERN "CMakeFiles" EXCLUDE
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   if (LLVM_INSTALL_MODULEMAPS)

diff  --git a/llvm/cmake/modules/CMakeLists.txt 
b/llvm/cmake/modules/CMakeLists.txt
index 4b8879f65fe4..505dc9a29d70 100644
--- a/llvm/cmake/modules/CMakeLists.txt
+++ b/llvm/cmake/modules/CMakeLists.txt
@@ -93,7 +93,6 @@ set(llvm_config_include_buildtree_only_exports)
 file(COPY .
   DESTINATION ${llvm_cmake_builddir}
   FILES_MATCHING PATTERN *.cmake
-  PATTERN .svn EXCLUDE
   PATTERN CMakeFiles EXCLUDE
   )
 
@@ -152,7 +151,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 DESTINATION ${LLVM_INSTALL_PACKAGE_DIR}
 COMPONENT cmake-exports
 FILES_MATCHING PATTERN *.cmake
-PATTERN .svn EXCLUDE
 PATTERN LLVMConfig.cmake EXCLUDE
 PATTERN LLVMConfigExtensions.cmake EXCLUDE
 PATTERN LLVMConfigVersion.cmake EXCLUDE

diff  --git a/polly/CMakeLists.txt b/polly/CMakeLists.txt
index fe7f6b78b479..ca7c04c565bb 100644
--- a/polly/CMakeLists.txt
+++ b/polly/CMakeLists.txt
@@ -125,7 +125,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${POLLY_BINARY_DIR}/include/
@@ -133,7 +132,6 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
 FILES_MATCHING
 PATTERN "*.h"
 PATTERN "CMakeFiles" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 endif()
 

diff  --git a/polly/lib/External/CMakeLists.txt 
b/polly/lib/External/CMakeLists.txt
index c953ea48475d..8991094d92c7 100644
--- a/polly/lib/External/CMakeLists.txt
+++ b/polly/lib/External/CMakeLists.txt
@@ -279,7 +279,6 @@ if (POLLY_BUNDLED_ISL)
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
-  PATTERN ".svn" EXCLUDE
   )
   endif()
 



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
htt

[Lldb-commits] [PATCH] D89859: Remove .svn from exclude list as we moved to git

[Sylvestre Ledru via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0784e17f1b4a: Remove .svn from exclude list as we moved to 
git (authored by sylvestre.ledru).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89859/new/

https://reviews.llvm.org/D89859

Files:
  clang/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  lld/CMakeLists.txt
  lldb/cmake/modules/LLDBConfig.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/CMakeLists.txt
  polly/CMakeLists.txt
  polly/lib/External/CMakeLists.txt

Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -279,7 +279,6 @@
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
-  PATTERN ".svn" EXCLUDE
   )
   endif()
 
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -125,7 +125,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${POLLY_BINARY_DIR}/include/
@@ -133,7 +132,6 @@
 FILES_MATCHING
 PATTERN "*.h"
 PATTERN "CMakeFiles" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 endif()
 
Index: llvm/cmake/modules/CMakeLists.txt
===
--- llvm/cmake/modules/CMakeLists.txt
+++ llvm/cmake/modules/CMakeLists.txt
@@ -93,7 +93,6 @@
 file(COPY .
   DESTINATION ${llvm_cmake_builddir}
   FILES_MATCHING PATTERN *.cmake
-  PATTERN .svn EXCLUDE
   PATTERN CMakeFiles EXCLUDE
   )
 
@@ -152,7 +151,6 @@
 DESTINATION ${LLVM_INSTALL_PACKAGE_DIR}
 COMPONENT cmake-exports
 FILES_MATCHING PATTERN *.cmake
-PATTERN .svn EXCLUDE
 PATTERN LLVMConfig.cmake EXCLUDE
 PATTERN LLVMConfigExtensions.cmake EXCLUDE
 PATTERN LLVMConfigVersion.cmake EXCLUDE
Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -1104,7 +1104,6 @@
 PATTERN "*.td"
 PATTERN "*.inc"
 PATTERN "LICENSE.TXT"
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${LLVM_INCLUDE_DIR}/llvm ${LLVM_INCLUDE_DIR}/llvm-c
@@ -1118,7 +1117,6 @@
 # Exclude include/llvm/CMakeFiles/intrinsics_gen.dir, matched by "*.def"
 PATTERN "CMakeFiles" EXCLUDE
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   if (LLVM_INSTALL_MODULEMAPS)
Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -228,7 +228,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 
@@ -237,7 +236,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 PATTERN ".cmake" EXCLUDE
 )
 
Index: lld/CMakeLists.txt
===
--- lld/CMakeLists.txt
+++ lld/CMakeLists.txt
@@ -195,7 +195,6 @@
 DESTINATION include
 FILES_MATCHING
 PATTERN "*.h"
-PATTERN ".svn" EXCLUDE
 )
 endif()
 
Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -174,7 +174,6 @@
   DESTINATION "${LIBCLANG_HEADERS_INSTALL_DESTINATION}"
   FILES_MATCHING
   PATTERN "*.h"
-  PATTERN ".svn" EXCLUDE
   )
 
 # LLVM_DISTRIBUTION_COMPONENTS requires that each component have both a
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -446,7 +446,6 @@
 PATTERN "*.def"
 PATTERN "*.h"
 PATTERN "config.h" EXCLUDE
-PATTERN ".svn" EXCLUDE
 )
 
   install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/include/clang
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this, and I think this is in a pretty good shape as it stands. The thing 
I'm not sure of is the naming of the class. I'd probably name it something like 
NativeRegisterContext_x86 (and hope that it can include non-watchpoint 
functionality in the future). As it stands now, this is not really a mix-in but 
a full register context class and in theory one can imagine that some target 
which only supports a single architecture will not bother with the 
NativeRegisterContextLinux stuff, but inherit straight from 
`NativeRegisterContext_x86`. (It's comment can allude to the fact that it is 
indented to be mixed with subclasses implementing os-specific functionality, 
which will also help explaining the virtual inheritance.)




Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:18
 
-class NativeRegisterContextLinux : public NativeRegisterContextRegisterInfo {
+class NativeRegisterContextLinux : virtual public 
NativeRegisterContextRegisterInfo {
 public:

I believe the more common spelling is `public virtual`



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:296
 const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
-: NativeRegisterContextLinux(native_thread,
+: NativeRegisterContextRegisterInfo(native_thread, 
CreateRegisterInfoInterface(target_arch)),
+  NativeRegisterContextLinux(native_thread,

mgorny wrote:
> I'm wondering if we can eliminate the init inside 
> `NativeRegisterContextRegisterInfo`
Actually I'd delete the constructor of `NativeRegisterContextLinux`. 

C++ standard says:
```
An abstract base class is never a most derived class, this its constructors 
never initialize virtual base classes, therefore the corresponding 
mem-initializers may be omitted.
```
Since the only thing that constructor does (used to do) is call 
NativeRegisterContextRegisterInfo's ctor, we can just delete it.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h:110
 
-  bool IsGPR(uint32_t reg_index) const;
+  bool IsGPROrDR(uint32_t reg_index) const;
 

mgorny wrote:
> I have changed this to facilitate `WriteRegister()`. Alternatively, I suppose 
> we could reverse the loop to match `ReadRegister()`.
I think this is fine.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+

This class won't be using any os- or arch-specific features, so we can just 
drop these.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:146-155
+  // clear dr6 if address or bits changed (i.e. we're not reenabling the same
+  // watchpoint)
+  if (drN_value.GetAsUInt64() != addr ||
+  (dr7_value.GetAsUInt64() & bit_mask) != (rw_bits | size_bits)) {
+ClearWatchpointHit(wp_index);
+
+error = WriteRegister(reg_info_drN, RegisterValue(addr));

mgorny wrote:
> This is the DR6 cleanup I was talking about.
This seems fine. Maybe worth mentioning that this is here to avoid confusing 
the NetBSD kernel.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())

mgorny wrote:
> On NetBSD, we have to call this explicitly when processing watchpoint hit (in 
> SIGTRAP handling). Not sure if Linux would need that too (in which case we'd 
> have to add a virtual method to the base class, I guess), or if the kernel 
> takes care of it.
No clue. If it's not done currently, then I guess it's not needed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

[Pavel Labath via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa5fa63fd140: [lldb] Port lldb gdb-server to libOption 
(authored by labath).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89477/new/

https://reviews.llvm.org/D89477

Files:
  lldb/include/lldb/Utility/Args.h
  lldb/source/Utility/Args.cpp
  lldb/test/Shell/lldb-server/TestErrorMessages.test
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-server/LLGSOptions.td
  lldb/tools/lldb-server/lldb-gdbserver.cpp

Index: lldb/tools/lldb-server/lldb-gdbserver.cpp
===
--- lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -17,7 +17,6 @@
 #include 
 #endif
 
-
 #include "Acceptor.h"
 #include "LLDBServerUtilities.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
@@ -25,8 +24,6 @@
 #include "lldb/Host/Config.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
-#include "lldb/Host/HostGetOpt.h"
-#include "lldb/Host/OptionParser.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/StringConvert.h"
@@ -34,7 +31,11 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/WithColor.h"
 
 #if defined(__linux__)
 #include "Plugins/Process/Linux/NativeProcessLinux.h"
@@ -88,31 +89,6 @@
 #endif
 }
 
-// option descriptors for getopt_long_only()
-
-static int g_debug = 0;
-static int g_verbose = 0;
-
-static struct option g_long_options[] = {
-{"debug", no_argument, &g_debug, 1},
-{"verbose", no_argument, &g_verbose, 1},
-{"log-file", required_argument, nullptr, 'l'},
-{"log-channels", required_argument, nullptr, 'c'},
-{"attach", required_argument, nullptr, 'a'},
-{"named-pipe", required_argument, nullptr, 'N'},
-{"pipe", required_argument, nullptr, 'U'},
-{"native-regs", no_argument, nullptr,
- 'r'}, // Specify to use the native registers instead of the gdb defaults
-   // for the architecture.  NOTE: this is a do-nothing arg as it's
-   // behavior is default now.  FIXME remove call from lldb-platform.
-{"reverse-connect", no_argument, nullptr,
- 'R'}, // Specifies that llgs attaches to the client address:port rather
-   // than llgs listening for a connection from address on port.
-{"setsid", no_argument, nullptr,
- 'S'}, // Call setsid() to make llgs run in its own session.
-{"fd", required_argument, nullptr, 'F'},
-{nullptr, 0, nullptr, 0}};
-
 #ifndef _WIN32
 // Watch for signals
 static int g_sighup_received_count = 0;
@@ -129,20 +105,6 @@
 }
 #endif // #ifndef _WIN32
 
-static void display_usage(const char *progname, const char *subcommand) {
-  fprintf(stderr, "Usage:\n  %s %s "
-  "[--log-file log-file-name] "
-  "[--log-channels log-channel-list] "
-  "[--setsid] "
-  "[--fd file-descriptor]"
-  "[--named-pipe named-pipe-path] "
-  "[--native-regs] "
-  "[--attach pid] "
-  "[[HOST]:PORT] "
-  "[-- PROGRAM ARG1 ARG2 ...]\n",
-  progname, subcommand);
-}
-
 void handle_attach_to_pid(GDBRemoteCommunicationServerLLGS &gdb_server,
   lldb::pid_t pid) {
   Status error = gdb_server.AttachToProcess(pid);
@@ -176,12 +138,12 @@
 handle_attach_to_process_name(gdb_server, attach_target);
 }
 
-void handle_launch(GDBRemoteCommunicationServerLLGS &gdb_server, int argc,
-   const char *const argv[]) {
+void handle_launch(GDBRemoteCommunicationServerLLGS &gdb_server,
+   llvm::ArrayRef Arguments) {
   ProcessLaunchInfo info;
   info.GetFlags().Set(eLaunchFlagStopAtEntry | eLaunchFlagDebug |
   eLaunchFlagDisableASLR);
-  info.SetArguments(const_cast(argv), true);
+  info.SetArguments(Args(Arguments), true);
 
   llvm::SmallString<64> cwd;
   if (std::error_code ec = llvm::sys::fs::current_path(cwd)) {
@@ -198,7 +160,7 @@
   Status error = gdb_server.LaunchProcess();
   if (error.Fail()) {
 llvm::errs() << llvm::formatv("error: failed to launch '{0}': {1}\n",
-  argv[0], error);
+  Arguments[0], error);
 exit(1);
   }
 }
@@ -229,7 +191,7 @@
 
 void ConnectToRemote(MainLoop &mainloop,
  GDBRemoteCommunicationServerLLGS &gdb_server,
- bool reverse_connect, const char *const host_and_port,
+ bool reverse_connect, llvm::StringRef host_and_port,
  const char *const progname, con

[Lldb-commits] [lldb] fa5fa63 - [lldb] Port lldb gdb-server to libOption

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-10-21T16:16:18+02:00
New Revision: fa5fa63fd140f2d4bad0357839378606a583b32c

URL: 
https://github.com/llvm/llvm-project/commit/fa5fa63fd140f2d4bad0357839378606a583b32c
DIFF: 
https://github.com/llvm/llvm-project/commit/fa5fa63fd140f2d4bad0357839378606a583b32c.diff

LOG: [lldb] Port lldb gdb-server to libOption

The existing help text was very terse and was missing several important
options. In the new version, I add a short description of each option
and a slightly longer description of the tool as a whole.

The new option list does not include undocumented no-op options:
--debug and --verbose. It also does not include undocumented short
aliases for long options, with two exceptions: -h, because it's
well-known; and -S (--setsid), as it's used in one test. Using these
options will now produce an error. I believe that is acceptable as users
aren't generally invoking lldb-server directly, and the only way to
learn about the short aliases was by looking at the source.

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

Added: 
lldb/test/Shell/lldb-server/TestErrorMessages.test
lldb/tools/lldb-server/LLGSOptions.td

Modified: 
lldb/include/lldb/Utility/Args.h
lldb/source/Utility/Args.cpp
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/lldb-gdbserver.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Args.h 
b/lldb/include/lldb/Utility/Args.h
index 2cce7d0c697c..82e6d147ae56 100644
--- a/lldb/include/lldb/Utility/Args.h
+++ b/lldb/include/lldb/Utility/Args.h
@@ -66,6 +66,7 @@ class Args {
 
   Args(const Args &rhs);
   explicit Args(const StringList &list);
+  explicit Args(llvm::ArrayRef args);
 
   Args &operator=(const Args &rhs);
 

diff  --git a/lldb/source/Utility/Args.cpp b/lldb/source/Utility/Args.cpp
index 4f3285404b6d..2cbe727ed240 100644
--- a/lldb/source/Utility/Args.cpp
+++ b/lldb/source/Utility/Args.cpp
@@ -175,6 +175,11 @@ Args::Args(const StringList &list) : Args() {
 AppendArgument(arg);
 }
 
+Args::Args(llvm::ArrayRef args) : Args() {
+  for (llvm::StringRef arg : args)
+AppendArgument(arg);
+}
+
 Args &Args::operator=(const Args &rhs) {
   Clear();
 

diff  --git a/lldb/test/Shell/lldb-server/TestErrorMessages.test 
b/lldb/test/Shell/lldb-server/TestErrorMessages.test
new file mode 100644
index ..ef64ec6e5aba
--- /dev/null
+++ b/lldb/test/Shell/lldb-server/TestErrorMessages.test
@@ -0,0 +1,14 @@
+RUN: lldb-server gdbserver --fd 2>&1 | FileCheck --check-prefixes=FD1,ALL %s
+FD1: error: --fd: missing argument
+
+RUN: lldb-server gdbserver --fd three 2>&1 | FileCheck 
--check-prefixes=FD2,ALL %s
+FD2: error: invalid '--fd' argument
+
+RUN: lldb-server gdbserver --bogus 2>&1 | FileCheck --check-prefixes=BOGUS,ALL 
%s
+BOGUS: error: unknown argument '--bogus'
+
+RUN: lldb-server gdbserver 2>&1 | FileCheck --check-prefixes=CONN,ALL %s
+CONN: error: no connection arguments
+
+ALL: Use '{{.*}} g[dbserver] --help' for a complete list of options.
+

diff  --git a/lldb/tools/lldb-server/CMakeLists.txt 
b/lldb/tools/lldb-server/CMakeLists.txt
index 6e7b30df5c58..930c327cf072 100644
--- a/lldb/tools/lldb-server/CMakeLists.txt
+++ b/lldb/tools/lldb-server/CMakeLists.txt
@@ -1,3 +1,8 @@
+set(LLVM_TARGET_DEFINITIONS LLGSOptions.td)
+tablegen(LLVM LLGSOptions.inc -gen-opt-parser-defs)
+add_public_tablegen_target(LLGSOptionsTableGen)
+set_target_properties(LLGSOptionsTableGen PROPERTIES FOLDER "lldb misc")
+
 set(LLDB_PLUGINS)
 
 if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
@@ -53,8 +58,13 @@ add_lldb_tool(lldb-server
   ${LLDB_SYSTEM_LIBS}
 
 LINK_COMPONENTS
+  Option
   Support
 )
 
+add_dependencies(lldb-server
+  LLGSOptionsTableGen
+  ${tablegen_deps}
+)
 target_include_directories(lldb-server PRIVATE "${LLDB_SOURCE_DIR}/source")
 target_link_libraries(lldb-server PRIVATE ${LLDB_SYSTEM_LIBS})

diff  --git a/lldb/tools/lldb-server/LLGSOptions.td 
b/lldb/tools/lldb-server/LLGSOptions.td
new file mode 100644
index ..429a4671764f
--- /dev/null
+++ b/lldb/tools/lldb-server/LLGSOptions.td
@@ -0,0 +1,62 @@
+include "llvm/Option/OptParser.td"
+
+class F: Flag<["--", "-"], name>;
+class R prefixes, string name>
+  : Option;
+
+multiclass SJ {
+  def NAME: Separate<["--", "-"], name>,
+HelpText;
+  def NAME # _eq: Joined<["--", "-"], name # "=">,
+Alias(NAME)>;
+}
+
+def grp_connect : OptionGroup<"connection">, HelpText<"CONNECTION">;
+
+defm fd: SJ<"fd", "Communicate over the given file descriptor.">,
+  MetaVarName<"">,
+  Group;
+
+defm named_pipe: SJ<"named-pipe", "Write port lldb-server will listen on to 
the given named pipe.">,
+  MetaVarName<"">,
+  Group;
+
+defm pipe: SJ<"pipe", "Write port lldb-server will listen on to the given file 
descriptor.">,
+  MetaVarName<"">,
+  Group;
+
+def reverse_connect: F<"reverse-connect">,
+  HelpText<"Connect to the client instead of passively waiti

[Lldb-commits] [lldb] 26459e6 - Fix "Unknown arguments specified" to if in lldb

[Christopher Tetreault via lldb-commits]

Author: Christopher Tetreault
Date: 2020-10-21T07:24:53-07:00
New Revision: 26459e6d8eeb3c2e61c70d207de7e10a00b4ed1c

URL: 
https://github.com/llvm/llvm-project/commit/26459e6d8eeb3c2e61c70d207de7e10a00b4ed1c
DIFF: 
https://github.com/llvm/llvm-project/commit/26459e6d8eeb3c2e61c70d207de7e10a00b4ed1c.diff

LOG: Fix "Unknown arguments specified" to if in lldb

Reviewed By: labath

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

Added: 


Modified: 
lldb/cmake/modules/FindPythonAndSwig.cmake

Removed: 




diff  --git a/lldb/cmake/modules/FindPythonAndSwig.cmake 
b/lldb/cmake/modules/FindPythonAndSwig.cmake
index de274ede5dbf..dcbc386b70dd 100644
--- a/lldb/cmake/modules/FindPythonAndSwig.cmake
+++ b/lldb/cmake/modules/FindPythonAndSwig.cmake
@@ -48,7 +48,7 @@ else()
   get_property(MULTI_CONFIG GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
   if ("${Python3_VERSION}" VERSION_GREATER_EQUAL "3.7" AND
   "${SWIG_VERSION}" VERSION_LESS "4.0" AND WIN32 AND (
-  ${MULTI_CONFIG} OR (${uppercase_CMAKE_BUILD_TYPE} STREQUAL "DEBUG")))
+  ${MULTI_CONFIG} OR (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")))
 # Technically this can happen with non-Windows builds too, but we are not
 # able to detect whether Python was built with assertions, and only Windows
 # has the requirement that Debug LLDB must use Debug Python.



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

[Lldb-commits] [PATCH] D89807: Fix "Unknown arguments specified" to if in lldb

[Christopher Tetreault via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG26459e6d8eeb: Fix "Unknown arguments specified" to 
if in lldb (authored by ctetreau).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89807/new/

https://reviews.llvm.org/D89807

Files:
  lldb/cmake/modules/FindPythonAndSwig.cmake


Index: lldb/cmake/modules/FindPythonAndSwig.cmake
===
--- lldb/cmake/modules/FindPythonAndSwig.cmake
+++ lldb/cmake/modules/FindPythonAndSwig.cmake
@@ -48,7 +48,7 @@
   get_property(MULTI_CONFIG GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
   if ("${Python3_VERSION}" VERSION_GREATER_EQUAL "3.7" AND
   "${SWIG_VERSION}" VERSION_LESS "4.0" AND WIN32 AND (
-  ${MULTI_CONFIG} OR (${uppercase_CMAKE_BUILD_TYPE} STREQUAL "DEBUG")))
+  ${MULTI_CONFIG} OR (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")))
 # Technically this can happen with non-Windows builds too, but we are not
 # able to detect whether Python was built with assertions, and only Windows
 # has the requirement that Debug LLDB must use Debug Python.


Index: lldb/cmake/modules/FindPythonAndSwig.cmake
===
--- lldb/cmake/modules/FindPythonAndSwig.cmake
+++ lldb/cmake/modules/FindPythonAndSwig.cmake
@@ -48,7 +48,7 @@
   get_property(MULTI_CONFIG GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
   if ("${Python3_VERSION}" VERSION_GREATER_EQUAL "3.7" AND
   "${SWIG_VERSION}" VERSION_LESS "4.0" AND WIN32 AND (
-  ${MULTI_CONFIG} OR (${uppercase_CMAKE_BUILD_TYPE} STREQUAL "DEBUG")))
+  ${MULTI_CONFIG} OR (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")))
 # Technically this can happen with non-Windows builds too, but we are not
 # able to detect whether Python was built with assertions, and only Windows
 # has the requirement that Debug LLDB must use Debug Python.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89334: [lldb] Support Python imports relative the to the current file being sourced

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89334#2341889 , @JDevlieghere 
wrote:

> In D89334#2341561 , @labath wrote:
>
>> I'm afraid you've lost me there. Yes, we turn relative paths into absolute 
>> ones, but I don't see how that translates into adding less entries into 
>> sys.path. For each module that we import, we add `dirname(module_path)` to 
>> sys.path. If we import two modules from different directories, we will get 
>> two sys.path entries, even if the modules were specified as paths relative 
>> to the same directory. Canonicalizing the path to that directory will mean 
>> less entries.
>
> It doesn't, it translates to more: one for the `.` and one for 
> `dirname(module_path)`. Which basically translates to `n+1` entries because 
> the working directory doesn't change. I was saying we could do the exact same 
> thing for modules relative to the source-dir, but that means `2n` entries 
> because for every module we add `source_dir` and `dirname(module_path)` and 
> generally both will be different. My point was that it would solve the issue 
> of a relative import that you described last week, but at the cost at adding 
> twice as much entries, which means double the chance of an ambiguous (not 
> ambitious ;-) import. I think that was your main objection to the patch as it 
> is right now and I'm arguing that I think it's the best of the different 
> trade-offs.

Why would we be adding both? We've already checked the path and know if the 
file exists in the cwd or not. What's the point in adding that? My impression 
was that this patch already avoids adding two paths in this case...

Actually, this guessing of what they user meant to say is one of the things I 
don't like about this approach. I think it would be better if there was some 
way (command option or something) to specify where the module should be 
imported from. The docstring for that option could also explain the rule for 
how the module is being imported.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89334/new/

https://reviews.llvm.org/D89334

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D89874#2344561 , @labath wrote:

> I like this, and I think this is in a pretty good shape as it stands. The 
> thing I'm not sure of is the naming of the class. I'd probably name it 
> something like NativeRegisterContext_x86 (and hope that it can include 
> non-watchpoint functionality in the future). As it stands now, this is not 
> really a mix-in but a full register context class and in theory one can 
> imagine that some target which only supports a single architecture will not 
> bother with the NativeRegisterContextLinux stuff, but inherit straight from 
> `NativeRegisterContext_x86`. (It's comment can allude to the fact that it is 
> indented to be mixed with subclasses implementing os-specific functionality, 
> which will also help explaining the virtual inheritance.)

I don't have a strong opinion here. I went for the mixin approach since that 
was your suggest wrt register caching. I suppose in this case it doesn't matter 
much but it could make things easier (or harder) as we have more potentially 
disjoint functions and partially transitioned source tree.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89334: [lldb] Support Python imports relative the to the current file being sourced

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D89334#2344610 , @labath wrote:

> In D89334#2341889 , @JDevlieghere 
> wrote:
>
>> In D89334#2341561 , @labath wrote:
>>
>>> I'm afraid you've lost me there. Yes, we turn relative paths into absolute 
>>> ones, but I don't see how that translates into adding less entries into 
>>> sys.path. For each module that we import, we add `dirname(module_path)` to 
>>> sys.path. If we import two modules from different directories, we will get 
>>> two sys.path entries, even if the modules were specified as paths relative 
>>> to the same directory. Canonicalizing the path to that directory will mean 
>>> less entries.
>>
>> It doesn't, it translates to more: one for the `.` and one for 
>> `dirname(module_path)`. Which basically translates to `n+1` entries because 
>> the working directory doesn't change. I was saying we could do the exact 
>> same thing for modules relative to the source-dir, but that means `2n` 
>> entries because for every module we add `source_dir` and 
>> `dirname(module_path)` and generally both will be different. My point was 
>> that it would solve the issue of a relative import that you described last 
>> week, but at the cost at adding twice as much entries, which means double 
>> the chance of an ambiguous (not ambitious ;-) import. I think that was your 
>> main objection to the patch as it is right now and I'm arguing that I think 
>> it's the best of the different trade-offs.
>
> Why would we be adding both? We've already checked the path and know if the 
> file exists in the cwd or not. What's the point in adding that? My impression 
> was that this patch already avoids adding two paths in this case...
>
> Actually, this guessing of what they user meant to say is one of the things I 
> don't like about this approach. I think it would be better if there was some 
> way (command option or something) to specify where the module should be 
> imported from. The docstring for that option could also explain the rule for 
> how the module is being imported.



In D89334#2344610 , @labath wrote:

> In D89334#2341889 , @JDevlieghere 
> wrote:
>
>> In D89334#2341561 , @labath wrote:
>>
>>> I'm afraid you've lost me there. Yes, we turn relative paths into absolute 
>>> ones, but I don't see how that translates into adding less entries into 
>>> sys.path. For each module that we import, we add `dirname(module_path)` to 
>>> sys.path. If we import two modules from different directories, we will get 
>>> two sys.path entries, even if the modules were specified as paths relative 
>>> to the same directory. Canonicalizing the path to that directory will mean 
>>> less entries.
>>
>> It doesn't, it translates to more: one for the `.` and one for 
>> `dirname(module_path)`. Which basically translates to `n+1` entries because 
>> the working directory doesn't change. I was saying we could do the exact 
>> same thing for modules relative to the source-dir, but that means `2n` 
>> entries because for every module we add `source_dir` and 
>> `dirname(module_path)` and generally both will be different. My point was 
>> that it would solve the issue of a relative import that you described last 
>> week, but at the cost at adding twice as much entries, which means double 
>> the chance of an ambiguous (not ambitious ;-) import. I think that was your 
>> main objection to the patch as it is right now and I'm arguing that I think 
>> it's the best of the different trade-offs.
>
> Why would we be adding both? We've already checked the path and know if the 
> file exists in the cwd or not. What's the point in adding that? My impression 
> was that this patch already avoids adding two paths in this case...

For paths relative to the CWD we add both already:

1. We add "." to the sys path once: 
https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L3234
2. We "resolve" (make absolute) the relative path 
(https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2747)
 and if it exists add its dir to the sys path 
(https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2785).

For source-relative imports all this was a hypothetical solution to the 
relative import from Python issue you described.

> Actually, this guessing of what they user meant to say is one of the things I 
> don't like about this approach. I think it would be better if there was some 
> way (command option or something) to specify where the module should be 
> imported fro

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 4 inline comments as done.
mgorny added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:9
+
+#if defined(__i386__) || defined(__x86_64__)
+

labath wrote:
> This class won't be using any os- or arch-specific features, so we can just 
> drop these.
Does it make sense to compile code that isn't going to be used on other 
platforms?



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:146-155
+  // clear dr6 if address or bits changed (i.e. we're not reenabling the same
+  // watchpoint)
+  if (drN_value.GetAsUInt64() != addr ||
+  (dr7_value.GetAsUInt64() & bit_mask) != (rw_bits | size_bits)) {
+ClearWatchpointHit(wp_index);
+
+error = WriteRegister(reg_info_drN, RegisterValue(addr));

labath wrote:
> mgorny wrote:
> > This is the DR6 cleanup I was talking about.
> This seems fine. Maybe worth mentioning that this is here to avoid confusing 
> the NetBSD kernel.
I'm going to go through all the code and see what can be improved and/or 
documented better.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:183
+
+Status WatchpointMixin_x86::ClearWatchpointHit(uint32_t wp_index) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())

labath wrote:
> mgorny wrote:
> > On NetBSD, we have to call this explicitly when processing watchpoint hit 
> > (in SIGTRAP handling). Not sure if Linux would need that too (in which case 
> > we'd have to add a virtual method to the base class, I guess), or if the 
> > kernel takes care of it.
> No clue. If it's not done currently, then I guess it's not needed.
I think it might have been done as part of the implicit disable/reenable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 299704.
mgorny marked an inline comment as done.
mgorny added a comment.

Deleted unnecessary constructor and switched public/virtual order. More changes 
coming.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h

Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
@@ -0,0 +1,53 @@
+//===-- WatchpointMixin_x86.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#ifndef lldb_WatchpointMixin_x86_h
+#define lldb_WatchpointMixin_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class WatchpointMixin_x86 : public virtual NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  // TODO: do we need to call it explicitly like on netbsd?
+  Status ClearWatchpointHit(uint32_t wp_index);
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+uint32_t watch_flags,
+uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  int GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_WatchpointMixin_x86_h
+
+#endif // defined(__i386__) || defined(__x86_64__)
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
@@ -0,0 +1,261 @@
+//===-- WatchpointMixin_x86.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#include "WatchpointMixin_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+
+using namespace lldb_private;
+
+int WatchpointMixin_x86::GetDR(int num) const {
+  assert(num >= 0 && num <= 7);
+  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
+  case llvm::Triple::x86:
+return lldb_dr0_i386 + num;
+  case llvm::Triple::x86_64:
+return lldb_dr0_x86_64 + num;
+  default:
+llvm_unreachable("Unhandled target architecture.");
+  }
+}
+
+Status WatchpointMixin_x86::IsWatchpointHit(uint32_t wp_index,
+   bool &is_hit) {
+  if (wp_index >= NumSupportedHardwareWatchpoints())
+return Status("Watchpoint index out of range");
+
+  RegisterValue reg_value;
+  const RegisterInfo *const reg_info = GetRegisterInfoAtIndex(GetDR(6));
+  Status error = ReadRegister(reg_info, reg_value);
+  if (error.Fail()) {
+is_hit = false;
+return error;
+  }
+
+  uint64_t status_bits = reg_value.GetAsUInt64();
+
+  is_hit = status_bits & (1 << wp_index);
+
+  return error;
+}
+
+Status WatchpointMixin_x86::GetWatchpointHitIndex(
+uint32_t &wp_index, lldb::addr_t trap_addr) {
+  uint32_t num_hw_wps = NumSupportedHardwareWatchpoints();
+  for (wp_index = 0; wp_index < num_hw_wps; ++wp_index) {
+bool is_hit;
+Status error = IsWatchpointHit(wp_index, is_hit);
+if (error.Fail()) {
+  wp_index = LLDB_INVALID_INDEX32;
+  r

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

For the record, I'm planning to `clang-format` the code at the very end.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [lldb] d900b75 - [lldb] Fix windows build for fa5fa63fd140f

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-10-21T17:51:11+02:00
New Revision: d900b755ed003967d1c9675b62293414831db1b6

URL: 
https://github.com/llvm/llvm-project/commit/d900b755ed003967d1c9675b62293414831db1b6
DIFF: 
https://github.com/llvm/llvm-project/commit/d900b755ed003967d1c9675b62293414831db1b6.diff

LOG: [lldb] Fix windows build for fa5fa63fd140f

Added: 


Modified: 
lldb/tools/lldb-server/lldb-gdbserver.cpp

Removed: 




diff  --git a/lldb/tools/lldb-server/lldb-gdbserver.cpp 
b/lldb/tools/lldb-server/lldb-gdbserver.cpp
index 0fbb13800bf7..9b8c67af14db 100644
--- a/lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ b/lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -451,10 +451,12 @@ int main_gdbserver(int argc, char *argv[]) {
   reverse_connect = Args.hasArg(OPT_reverse_connect);
   attach_target = Args.getLastArgValue(OPT_attach).str();
   if (Args.hasArg(OPT_pipe)) {
-if (!llvm::to_integer(Args.getLastArgValue(OPT_pipe), unnamed_pipe)) {
+uint64_t Arg;
+if (!llvm::to_integer(Args.getLastArgValue(OPT_pipe), Arg)) {
   WithColor::error() << "invalid '--pipe' argument\n" << HelpText;
   return 1;
 }
+unnamed_pipe = (pipe_t)Arg;
   }
   if (Args.hasArg(OPT_fd)) {
 if (!llvm::to_integer(Args.getLastArgValue(OPT_fd), connection_fd)) {



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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:580
 
-  if (IsGPR(reg_index))
+  if (IsGPROrDR(reg_index))
 return WriteRegisterRaw(reg_index, reg_value);

Can we have `IsGPR(reg_index) || IsDR(reg_index)`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 299731.
mgorny added a comment.

Revamped the whole file to hopefully make it a bit less magical.

- Moved all the constants into inline functions and one constexpr var.
- Added more 'visual' comments what bits are touched.
- Added comments about NetBSD-specific hacks.
- Inlined RegisterInfo magic into `GetDR()`.
- Reduced the number of local variables, and renamed reg_value variables to 
clearly indicate which reg it is.
- Removed duplicate cleaning of DR6 when clearing all hw watchpoints — the 
cleanup is done on adding new watchpoints anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

Files:
  lldb/include/lldb/lldb-defines.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2618,17 +2618,17 @@
 want_breakpoint = true;
 break;
   case eWatchpointWrite:
-watch_flags = 1;
+watch_flags = LLDB_GDB_WATCH_TYPE_WRITE;
 want_hardware = true;
 want_breakpoint = false;
 break;
   case eWatchpointRead:
-watch_flags = 2;
+watch_flags = LLDB_GDB_WATCH_TYPE_READ;
 want_hardware = true;
 want_breakpoint = false;
 break;
   case eWatchpointReadWrite:
-watch_flags = 3;
+watch_flags = LLDB_GDB_WATCH_TYPE_READ_WRITE;
 want_hardware = true;
 want_breakpoint = false;
 break;
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
@@ -0,0 +1,53 @@
+//===-- WatchpointMixin_x86.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#ifndef lldb_WatchpointMixin_x86_h
+#define lldb_WatchpointMixin_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class WatchpointMixin_x86 : public virtual NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  // TODO: do we need to call it explicitly like on netbsd?
+  Status ClearWatchpointHit(uint32_t wp_index);
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+uint32_t watch_flags,
+uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  const RegisterInfo *GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_WatchpointMixin_x86_h
+
+#endif // defined(__i386__) || defined(__x86_64__)
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
@@ -0,0 +1,269 @@
+//===-- WatchpointMixin_x86.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#include "WatchpointMixin_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utilit

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/include/lldb/lldb-defines.h:57-58
 #define LLDB_WATCH_ID_IS_VALID(uid) ((uid) != (LLDB_INVALID_WATCH_ID))
 #define LLDB_WATCH_TYPE_READ (1u << 0)
 #define LLDB_WATCH_TYPE_WRITE (1u << 1)
 #define LLDB_WATCH_TYPE_IS_VALID(type) 
\

I'm wondering if we could change these two values to avoid having separate 
`LLDB_GDB...`. I suppose it could break if API clients relied on them.



Comment at: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp:59-60
+if (error.Fail()) {
+  wp_index = LLDB_INVALID_INDEX32;
+  return error;
+} else if (is_hit) {

Technically this could be replaced by `break`, and `return error` below but I 
suppose it's more readable as-is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny added reviewers: emaste, krytarowski.
mgorny added a comment.

Adding more people in case they wanted to chime in, as this code will be reused 
on FreeBSD and NetBSD too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

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

[Lldb-commits] [PATCH] D89874: [lldb] Unify x86 watchpoint implementation on Linux and NetBSD (and future FreeBSD plugin) [WIP]

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 299800.
mgorny added a comment.

Migrate NetBSD plugin as well. This implied adding `ClearWatchpointHit()` as a 
virtual method to the base class.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89874/new/

https://reviews.llvm.org/D89874

Files:
  lldb/include/lldb/Host/common/NativeRegisterContext.h
  lldb/include/lldb/lldb-defines.h
  lldb/source/Host/common/NativeRegisterContext.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
  lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2618,17 +2618,17 @@
 want_breakpoint = true;
 break;
   case eWatchpointWrite:
-watch_flags = 1;
+watch_flags = LLDB_GDB_WATCH_TYPE_WRITE;
 want_hardware = true;
 want_breakpoint = false;
 break;
   case eWatchpointRead:
-watch_flags = 2;
+watch_flags = LLDB_GDB_WATCH_TYPE_READ;
 want_hardware = true;
 want_breakpoint = false;
 break;
   case eWatchpointReadWrite:
-watch_flags = 3;
+watch_flags = LLDB_GDB_WATCH_TYPE_READ_WRITE;
 want_hardware = true;
 want_breakpoint = false;
 break;
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.h
@@ -0,0 +1,52 @@
+//===-- WatchpointMixin_x86.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#ifndef lldb_WatchpointMixin_x86_h
+#define lldb_WatchpointMixin_x86_h
+
+#include "Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h"
+
+namespace lldb_private {
+
+class WatchpointMixin_x86 : public virtual NativeRegisterContextRegisterInfo {
+public:
+  Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
+
+  Status GetWatchpointHitIndex(uint32_t &wp_index,
+   lldb::addr_t trap_addr) override;
+
+  Status IsWatchpointVacant(uint32_t wp_index, bool &is_vacant) override;
+
+  bool ClearHardwareWatchpoint(uint32_t wp_index) override;
+
+  Status ClearWatchpointHit(uint32_t wp_index) override;
+
+  Status ClearAllHardwareWatchpoints() override;
+
+  Status SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size,
+uint32_t watch_flags,
+uint32_t wp_index);
+
+  uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
+ uint32_t watch_flags) override;
+
+  lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
+
+  uint32_t NumSupportedHardwareWatchpoints() override;
+
+private:
+  const RegisterInfo *GetDR(int num) const;
+};
+
+} // namespace lldb_private
+
+#endif // #ifndef lldb_WatchpointMixin_x86_h
+
+#endif // defined(__i386__) || defined(__x86_64__)
Index: lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/WatchpointMixin_x86.cpp
@@ -0,0 +1,269 @@
+//===-- WatchpointMixin_x86.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#include "WatchpointMixin_x86.h"
+
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Process/Utility/lldb-x86-register-enums.h"
+
+using namespace lldb_private;
+

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 299826.
zequanwu marked 4 inline comments as done.
zequanwu added a comment.

- address comments.
- add tests.
- move `loadPDBFile` from `SymbolFileNativePDB.cpp` to `ObjectFilePDB.cpp` and 
use it to create unique_ptr of `PDBFile` so that we can move it around.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89812/new/

https://reviews.llvm.org/D89812

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/PDB/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp
  lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
  lldb/test/Shell/ObjectFile/PDB/object.test
  lldb/test/Shell/ObjectFile/PDB/symbol.test
  lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp
@@ -0,0 +1,22 @@
+// clang-format off
+// REQUIRES: lld, x86
+
+// Test that lldb load PDB file by command `target symbols add`
+
+// RUN: mkdir -p %t/executable
+// RUN: rm -f %t/executable/foo.exe %t/executable/bar.pdb
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t/executable/foo.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main %t/executable/foo.obj \
+// RUN: -out:%t/executable/foo.exe -pdb:%t/executable/foo.pdb
+// Rename the PDB file so that the name is different from the name inside the executable (foo.exe).
+// RUN: mv %t/executable/foo.pdb %t/executable/bar.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -O \
+// RUN: "target create %t/executable/foo.exe" -o \
+// RUN: "target symbols add %t/executable/bar.pdb" -o "quit" | FileCheck %s
+
+int main(int argc, char** argv) {
+  return 0;
+}
+
+// CHECK: (lldb) target symbols add {{.*}}bar.pdb
+// CHECK: symbol file '{{.*}}/bar.pdb' has been added to '{{.*}}/foo.exe'
Index: lldb/test/Shell/ObjectFile/PDB/symbol.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/symbol.test
@@ -0,0 +1,10 @@
+# REQUIRES: lld
+# RUN: yaml2obj %p/Inputs/pdb.yaml -o %t.obj
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test symbols %t.pdb | FileCheck %s
+
+# CHECK: Types:
+# CHECK-NEXT: {{.*}}:   Type{0x00010024} , size = 0, compiler_type = {{.*}} int (int, char **)
+# CHECK: Compile units:
+# CHECK-NEXT: {{.*}}:   CompileUnit{0x}, language = "c++", file = '/tmp/a.cpp'
+# CHECK-NEXT: {{.*}}: Function{0x0841}, demangled = main, type = {{.*}}
\ No newline at end of file
Index: lldb/test/Shell/ObjectFile/PDB/object.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/object.test
@@ -0,0 +1,14 @@
+# REQUIRES: lld
+# RUN: yaml2obj %p/Inputs/pdb.yaml -o %t.obj
+# RUN: lld-link %t.obj /debug /pdb:%t.pdb /entry:main /nodefaultlib
+# RUN: lldb-test object-file %t.pdb | FileCheck %s
+
+# CHECK: Plugin name: pdb
+# CHECK: Architecture: x86_64-pc-windows-msvc
+# CHECK: UUID:
+# CHECK: Executable: false
+# CHECK: Stripped: false
+# CHECK: Type: debug info
+# CHECK: Strata: user
+# CHECK: Base VM address: 0x0
+# CHECK: There are no sections
\ No newline at end of file
Index: lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/PDB/Inputs/pdb.yaml
@@ -0,0 +1,314 @@
+--- !COFF
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+Alignment:   16
+SectionData: 4883EC1831C0C74424144889542408894C24044883C418C3
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+  - Name:.bss
+Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+SizeOfRawData:   0
+  - Name:'.debug$S'
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   4
+SectionData: 0400F10080007E003C110100DC00E02E636C616E672076657273696F6E2031322E302E302028676974406769746875622E636F6D3A6C6C766D2F6C6C766D2D70726F6A6563742E676974203861303865303864623663326534613564623438353235336633313836623066396537333965313529F10090002A0047111

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

[Zequan Wu via Phabricator via lldb-commits]

zequanwu marked an inline comment as done.
zequanwu added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:176-197
+  switch (dbi_stream->getMachineType()) {
+  case PDB_Machine::Amd64:
+spec.SetTriple("x86_64-pc-windows");
+specs.Append(module_spec);
+break;
+  case PDB_Machine::x86:
+spec.SetTriple("i386-pc-windows");

labath wrote:
> Could this use the same algorithm as `ObjectFilePDB::GetArchitecture` ?
No. This part is the same algorithm as the part in 
`ObjectFilePECOFF::GetModuleSpecifications`, 
https://github.com/llvm/llvm-project/blob/master/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp#L194-L215.

The `ObjectFilePDB::GetArchitecture` is same as 
`ObjectFilePECOFF::GetArchitecture`.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:207-209
+Address ObjectFilePDB::GetBaseAddress() {
+  return Address(GetSectionList()->GetSectionAtIndex(0), 0);
+}

labath wrote:
> Leave this unimplemented. PDBs don't have sections and are not loaded into 
> memory, so the function does not apply.
This is actually necessary. It's called at 
`SymbolFileNativePDB::InitializeObject`. Otherwise, setting breakpoint won't 
work.



Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:215-216
+  m_sections_up = std::make_unique();
+  for (auto s : unified_section_list)
+m_sections_up->AddSection(s);
+}

labath wrote:
> This shouldn't be necessary. What "normally" happens here is that an object 
> file is adding own sections *into* the "unified" section list -- not the 
> other way around. Since we're pretending that the pdb file has no sections 
> (and that's kind of true) I think you should just return an empty section 
> list here, and not touch the unified list at all.
The reason why I copy section list from `unified_section_list` to 
`m_sections_up` is to let `GetBaseAddress` be able to return the right base 
address.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:299
 std::unique_ptr file_up =
 loadMatchingPDBFile(m_objfile_sp->GetFileSpec().GetPath(), 
m_allocator);
 

labath wrote:
> Instead of changing `loadMatchingPDBFile` I think we ought to change this to 
> something like:
> ```
> if (auto *pdb = llvm::dyn_cast(m_objfile_sp)) {
>   file_up = pdb->giveMeAPDBFile(); 
>   // PS: giveMeAPDBFile is a new pdb-specific public method on ObjectFilePDB
>   // PS: possibly this should not return a PDBFile, but a PDBIndex directly 
> -- I don't know the details but the general idea is that it could/should 
> reuse the pdb parsing state that the objectfile class has already created
> } else 
>   file_up = do_the_loadMatchingPDBFile_fallback();
> ```
> 
> The reason for that is that it avoids opening the pdb file twice (once in the 
> object file and once here). Another reason is that I believe the entire 
> `loadMatchingPDBFile` function should disappear. Finding the pdb file should 
> not be the job of the symbol file class -- we have symbol vendors for that. 
> However, we should keep the fallback for now to keep the patch small...
From what I see, it seems like PDBFile is designed to be created when opening 
the pdb file. It doesn't allow copying it around.
`NativeSession` holds a unique_ptr of PDBFile and `NativeSession::getPDBFile` 
returns the reference of object, so creating unique_ptr from it is not allowed.

I moved `loadPDBFile` to `ObjectFilePDB` as static function, so we can move 
`PDBFile` easily.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89812/new/

https://reviews.llvm.org/D89812

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

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, jingham.
Herald added a subscriber: pengfei.
Herald added a project: LLDB.
JDevlieghere requested review of this revision.

In a new Range class was introduced to simplify and the Disassembler API and 
reduce duplication. It unintentionally broke the `SBFrame::Disassemble` 
functionality because it unconditionally converts the number of instructions to 
a `Range{Limit::Instructions, num_instructions}`. This is subtly different from 
the previous behavior, where now we're passing a Range and assume it's valid in 
the callee, the original code would propagate `num_instructions` and the callee 
would compare the value and decided between disassembling instructions or bytes.

Unfortunately the existing tests was not particularly strict:

  disassembly = frame.Disassemble()
  self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")

This would pass because without this patch we'd disassemble zero instructions, 
resulting in an error:

  ./bin/lldb /tmp/a.out -o 'b main' -o r -o 'script 
print(lldb.frame.Disassemble())'
  (lldb) target create "/tmp/a.out"
  Current executable set to '/tmp/a.out' (x86_64).
  (lldb) b main
  Breakpoint 1: where = a.out`main + 11 at foo.c:2:3, address = 
0x00013fab
  (lldb) r
  Process 22141 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  frame #0: 0x00013fab a.out`main at foo.c:2:3
 1int main() {
  -> 2  return 10;
 3}
  
  Process 22141 launched: '/tmp/a.out' (x86_64)
  (lldb) script print(lldb.frame.Disassemble())
  error: error reading data from section __text


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89925

Files:
  lldb/source/Core/Disassembler.cpp
  lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
  lldb/test/API/commands/disassemble/basic/main.cpp


Index: lldb/test/API/commands/disassemble/basic/main.cpp
===
--- lldb/test/API/commands/disassemble/basic/main.cpp
+++ lldb/test/API/commands/disassemble/basic/main.cpp
@@ -2,6 +2,7 @@
 sum (int a, int b)
 {
 int result = a + b; // Set a breakpoint here
+asm("nop");
 return result;
 }
 
Index: lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
===
--- lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
+++ lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
@@ -57,4 +57,6 @@
 
 frame = threads[0].GetFrameAtIndex(0)
 disassembly = frame.Disassemble()
-self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")
+self.assertNotEqual(disassembly, "")
+self.assertNotIn("error", disassembly)
+self.assertIn(": nop", disassembly)
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -564,10 +564,16 @@
   range.SetByteSize(DEFAULT_DISASM_BYTE_SIZE);
   }
 
-  return Disassemble(
-  debugger, arch, plugin_name, flavor, exe_ctx, range.GetBaseAddress(),
-  {Limit::Instructions, num_instructions}, mixed_source_and_assembly,
-  num_mixed_context_lines, options, strm);
+  Disassembler::Limit limit = {Limit::Instructions, num_instructions};
+  if (num_instructions == 0) {
+limit = {Disassembler::Limit::Bytes, range.GetByteSize()};
+if (limit.value == 0)
+  limit.value = DEFAULT_DISASM_BYTE_SIZE;
+  }
+
+  return Disassemble(debugger, arch, plugin_name, flavor, exe_ctx,
+ range.GetBaseAddress(), limit, mixed_source_and_assembly,
+ num_mixed_context_lines, options, strm);
 }
 
 Instruction::Instruction(const Address &address, AddressClass addr_class)


Index: lldb/test/API/commands/disassemble/basic/main.cpp
===
--- lldb/test/API/commands/disassemble/basic/main.cpp
+++ lldb/test/API/commands/disassemble/basic/main.cpp
@@ -2,6 +2,7 @@
 sum (int a, int b)
 {
 int result = a + b; // Set a breakpoint here
+asm("nop");
 return result;
 }
 
Index: lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
===
--- lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
+++ lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py
@@ -57,4 +57,6 @@
 
 frame = threads[0].GetFrameAtIndex(0)
 disassembly = frame.Disassemble()
-self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")
+self.assertNotEqual(disassembly, "")
+self.assertNotIn("error", disassembly)
+self.assertIn(": nop", disassembly)
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/s

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

So in this patch you're setting the limit to be the size of the range (for 
someone who did 'disass -s 0x100 -e 0x200' they would be requesting 100 
instructions) but the disassembly returned would be limited by the byte range 
requested, so this works OK.  The asm(nop) is to guarantee that sum() has at 
least one instruction?  There's no documentation for the Disassembler ctors 
where we might mention that the use case is either (1) an AddressRange, or (2) 
a start address and instruction count, but maybe there should be?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89925/new/

https://reviews.llvm.org/D89925

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

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

(*cough* "requesting 256 instructions", wrote too quickly, or rather I am 
speaking in gdb RSP and don't always prefix my base16 numbers).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89925/new/

https://reviews.llvm.org/D89925

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

[Lldb-commits] [PATCH] D89925: [lldb] Fix a regression introduced by D75730

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D89925#2346290 , @jasonmolenda 
wrote:

> So in this patch you're setting the limit to be the size of the range (for 
> someone who did 'disass -s 0x100 -e 0x200' they would be requesting 100 
> instructions) but the disassembly returned would be limited by the byte range 
> requested, so this works OK.

Not exactly. The patch uses the range of the current symbol when the number of 
lines to disassemble == 0. You can think of it as just `disass` (even though 
that goes through a different code path and continued to work).

> The asm(nop) is to guarantee that sum() has at least one instruction?

The `nop` is just there so we can check that the output actually contains the 
instructions from the current frame. I think a `nop` should work everywhere our 
test suite runs.

> There's no documentation for the Disassembler ctors where we might mention 
> that the use case is either (1) an AddressRange, or (2) a start address and 
> instruction count, but maybe there should be?

Once converted to a `Range` object it has to be either so I think it's implicit 
in the API. This API doesn't take a range but a `uint32_t num_instructions` 
which is expected to give you all the instructions for the current frame/symbol 
when 0.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89925/new/

https://reviews.llvm.org/D89925

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