[Lldb-commits] [PATCH] D60325: [lldb] [Process/NetBSD] Fix wrongly mapping mm* registers

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I really like how you've approached testing this. There's just one thing that 
bothers me. Using a .s file means it's going to be hard to make this test work 
on other OSs (and we should be able to test something like this in a cross-OS 
manner). All elf targets should probably be fine, but I am pretty sure darwin 
and windows will choke on this.

I think it would be better to write this as a C(++) file and move the register 
setting code to inline asm. That should be more-or-less supported on all 
platforms (when compiling with clang). You should be able to test that by 
trying a cross-compilation with clang (`clang -target x86_64-pc-windows -c 
foo.c`). I've tried it with a simplified snipped from your test and it seemed 
to work fine.

(Note that I still expect this test to fail on windows, because AFAIK we don't 
support reading these registers on windows yet, but it would be good if at 
least the source code compiled there.)




Comment at: lldb/lit/Register/x86-mm-xmm-read.test:1
+# REQUIRES: x86
+# RUN: %clang %p/Inputs/x86-mm-xmm-read.s -o %t

Does this mean "the host is an x86 system", or "x86 is a configured llvm 
target"? My impression is that in means the latter 
, but we 
obviously want the former here.



Comment at: lldb/lit/Register/x86-mm-xmm-read.test:7-22
+# CHECK: mm0 = 0x0102030405060708
+# CHECK-NEXT: mm1 = 0x1112131415161718
+# CHECK-NEXT: mm2 = 0x2122232425262728
+# CHECK-NEXT: mm3 = 0x3132333435363738
+# CHECK-NEXT: mm4 = 0x4142434445464748
+# CHECK-NEXT: mm5 = 0x5152535455565758
+# CHECK-NEXT: mm6 = 0x6162636465666768

Maybe you could drop the `-NEXT`s here (or even replace them by `-DAG`s), 
because (I presume) you are not interested in the order the registers get 
printed, just their values.


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

https://reviews.llvm.org/D60325



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

[Lldb-commits] [PATCH] D60180: [CMake] Don't explicitly use LLVM_LIBRARY_DIR in standalone builds

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D60180#1456263 , @sgraenitz wrote:

> > No, everything is being built for android. Cross-compiling lldb-server 
> > involves cross-compiling llvm libraries, clang libraries, and if you've got 
> > swift in the picture, swift host libraries. LLVM and clang libraries are 
> > built against the libc++ from the android NDK, but in standalone builds, 
> > LLDB will try to link against the freshly built libc++ from LLVM. You get 
> > loads of undefined references to symbols from the NDK's libc++.
>
> For the `link_directories` line: if you cross-compile the entire tree and 
> this tree contains libc++, wouldn't it be natural that LLDB will try to link 
> against the freshly built libc++?
>  If this is not your intention, then maybe we need a `LLDB_EXTERNAL_LIBCXX` 
> option instead?


Well.. you definitely shouldn't link against the fresh libc++, but still use 
the headers from the system one (which I suspect is the cause of the undefined 
symbols Alex is seeing).

FWIW, I don't think cross-compilation should be a factor here. I can see 
somebody may want to build a fresh libc++ and then link llvm (all of it) 
against that, but that is true regardless of whether you're cross-compiling or 
not. However, I wouldn't expect this to be the default because then the built 
binaries wouldn't play well with anything else on this system, if the system 
happens to use a different C++ library (or just an incompatible version of 
libc++).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60180



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

[Lldb-commits] [lldb] r357890 - modify-python-lldb.py: Remove ifdef SWIG-removing code

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Apr  8 01:43:07 2019
New Revision: 357890

URL: http://llvm.org/viewvc/llvm-project?rev=357890&view=rev
Log:
modify-python-lldb.py: Remove ifdef SWIG-removing code

There are no patterns like that in the generated swig files (there
probably were some back in the days when we were running swig over the
header files directly), so this is dead code and has no effect on the
generated file.

Modified:
lldb/trunk/scripts/Python/modify-python-lldb.py

Modified: lldb/trunk/scripts/Python/modify-python-lldb.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/scripts/Python/modify-python-lldb.py?rev=357890&r1=357889&r2=357890&view=diff
==
--- lldb/trunk/scripts/Python/modify-python-lldb.py (original)
+++ lldb/trunk/scripts/Python/modify-python-lldb.py Mon Apr  8 01:43:07 2019
@@ -9,17 +9,13 @@
 # As a cleanup step, it also removes the 'residues' from the autodoc features 
of
 # swig.  For an example, take a look at SBTarget.h header file, where we take
 # advantage of the already existing doxygen C++-docblock and make it the Python
-# docstring for the same method.  The 'residues' in this context include the
-# '#endif', the '#ifdef SWIG', the c comment marker, the trailing blank (SPC's)
-# line, and the doxygen comment start marker.
+# docstring for the same method.  The 'residues' in this context include the c
+# comment marker, the trailing blank (SPC's) line, and the doxygen comment 
start
+# marker.
 #
 # In addition to the 'residues' removal during the cleanup step, it also
 # transforms the 'char' data type (which was actually 'char *' but the 
'autodoc'
 # feature of swig removes ' *' from it) into 'str' (as a Python str type).
-#
-# It also calls SBDebugger.Initialize() to initialize the lldb debugger
-# subsystem.
-#
 
 # System modules
 import sys
@@ -47,8 +43,6 @@ else:
 #
 # Residues to be removed.
 #
-c_endif_swig = "#endif"
-c_ifdef_swig = "#ifdef SWIG"
 c_comment_marker = "//"
 # The pattern for recognizing the doxygen comment block line.
 doxygen_comment_start = re.compile("^\s*(/// ?)")
@@ -133,10 +127,7 @@ for line in content.splitlines():
 state |= CLEANUP_DOCSTRING
 
 if (state & CLEANUP_DOCSTRING):
-# Cleanse the lldb.py of the autodoc'ed residues.
-if c_ifdef_swig in line or c_endif_swig in line:
-continue
-# As well as the comment marker line.
+# Remove the comment marker line.
 if c_comment_marker in line:
 continue
 


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

[Lldb-commits] [PATCH] D60400: Remove unneeded #ifdef SWIGs

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: JDevlieghere, clayborg, jingham.

Some of these were present in files which should never be read by swig
(and we also had one in the interface file, which is only read by swig).
They are probably leftovers from the time when we were running swig over
lldb headers directly.

The reason I'm putting this up for review is that the patch also
contains mild functional changes. While writing this patch, I noticed
that some of the #ifdefs were guarding public functions that were
operating on lldb_private data types. While it wasn't strictly
necessary, I made these private, as nobody should really be accessing
them. This can potentially break existing code if it happened to use
these methods, though it will only break at build time -- if someone
builds against an old header, he should still be able to link to a new
lldb library, since the functions are still there.

We could keep these public for backward compatbility, but I would argue
that if anyone was actually using these functions for anything, his code
is already broken.


https://reviews.llvm.org/D60400

Files:
  include/lldb/API/SBDefines.h
  include/lldb/API/SBThread.h
  include/lldb/API/SBThreadPlan.h
  include/lldb/Core/Address.h
  include/lldb/Core/SourceManager.h
  include/lldb/Target/Process.h
  scripts/interface/SBFrame.i
  source/API/SBThread.cpp
  source/API/SBThreadPlan.cpp

Index: source/API/SBThreadPlan.cpp
===
--- source/API/SBThreadPlan.cpp
+++ source/API/SBThreadPlan.cpp
@@ -89,11 +89,7 @@
 //--
 SBThreadPlan::~SBThreadPlan() {}
 
-lldb_private::ThreadPlan *SBThreadPlan::get() {
-  LLDB_RECORD_METHOD_NO_ARGS(lldb_private::ThreadPlan *, SBThreadPlan, get);
-
-  return LLDB_RECORD_RESULT(m_opaque_sp.get());
-}
+lldb_private::ThreadPlan *SBThreadPlan::get() { return m_opaque_sp.get(); }
 
 bool SBThreadPlan::IsValid() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBThreadPlan, IsValid);
@@ -402,7 +398,6 @@
   LLDB_REGISTER_CONSTRUCTOR(SBThreadPlan, (lldb::SBThread &, const char *));
   LLDB_REGISTER_METHOD(const lldb::SBThreadPlan &,
SBThreadPlan, operator=,(const lldb::SBThreadPlan &));
-  LLDB_REGISTER_METHOD(lldb_private::ThreadPlan *, SBThreadPlan, get, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBThreadPlan, IsValid, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBThreadPlan, operator bool, ());
   LLDB_REGISTER_METHOD(void, SBThreadPlan, Clear, ());
Index: source/API/SBThread.cpp
===
--- source/API/SBThread.cpp
+++ source/API/SBThread.cpp
@@ -1398,21 +1398,11 @@
 }
 
 lldb_private::Thread *SBThread::operator->() {
-  LLDB_RECORD_METHOD_NO_ARGS(lldb_private::Thread *, SBThread, operator->);
-
-  ThreadSP thread_sp(m_opaque_sp->GetThreadSP());
-  if (thread_sp)
-return LLDB_RECORD_RESULT(thread_sp.get());
-  return nullptr;
+  return get();
 }
 
 lldb_private::Thread *SBThread::get() {
-  LLDB_RECORD_METHOD_NO_ARGS(lldb_private::Thread *, SBThread, get);
-
-  ThreadSP thread_sp(m_opaque_sp->GetThreadSP());
-  if (thread_sp)
-return LLDB_RECORD_RESULT(thread_sp.get());
-  return nullptr;
+  return m_opaque_sp->GetThreadSP().get();
 }
 
 namespace lldb_private {
@@ -1516,8 +1506,6 @@
   LLDB_REGISTER_METHOD(lldb::SBThread, SBThread, GetCurrentExceptionBacktrace,
());
   LLDB_REGISTER_METHOD(bool, SBThread, SafeToCallFunctions, ());
-  LLDB_REGISTER_METHOD(lldb_private::Thread *, SBThread, operator->,());
-  LLDB_REGISTER_METHOD(lldb_private::Thread *, SBThread, get, ());
 }
 
 }
Index: scripts/interface/SBFrame.i
===
--- scripts/interface/SBFrame.i
+++ scripts/interface/SBFrame.i
@@ -210,15 +210,12 @@
 void
 Clear();
 
-#ifndef SWIG
 bool
 operator == (const lldb::SBFrame &rhs) const;
 
 bool
 operator != (const lldb::SBFrame &rhs) const;
 
-#endif
-
 %feature("docstring", "
 /// The version that doesn't supply a 'use_dynamic' value will use the
 /// target's default.
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -418,7 +418,6 @@
 /// \see RegisterNotificationCallbacks (const Notifications&) @see
 /// UnregisterNotificationCallbacks (const Notifications&)
 //--
-#ifndef SWIG
   typedef struct {
 void *baton;
 void (*initialize)(void *baton, Process *process);
@@ -503,7 +502,6 @@
 
 DISALLOW_COPY_AND_ASSIGN(ProcessEventData);
   };
-#endif // SWIG
 
   //--
   /// Construct with a shared pointer to a target, and the Process listener.
@@ -824,9 +822,7 @@
 ///
 /// \see Process::Notific

[Lldb-commits] [lldb] r357893 - Fix signed-unsigned comparison warning in Driver.cpp

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Apr  8 02:17:56 2019
New Revision: 357893

URL: http://llvm.org/viewvc/llvm-project?rev=357893&view=rev
Log:
Fix signed-unsigned comparison warning in Driver.cpp

Modified:
lldb/trunk/tools/driver/Driver.cpp

Modified: lldb/trunk/tools/driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/Driver.cpp?rev=357893&r1=357892&r2=357893&view=diff
==
--- lldb/trunk/tools/driver/Driver.cpp (original)
+++ lldb/trunk/tools/driver/Driver.cpp Mon Apr  8 02:17:56 2019
@@ -491,7 +491,7 @@ static ::FILE *PrepareCommandsForSourcin
   }
 
   ssize_t nrwr = write(fds[WRITE], commands_data, commands_size);
-  if (nrwr != commands_size) {
+  if (size_t(nrwr) != commands_size) {
 WithColor::error()
 << format(
 "write(%i, %p, %" PRIu64


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

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Pavel Labath via Phabricator via lldb-commits]

labath marked 5 inline comments as done.
labath added inline comments.



Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:393
+  llvm::StringRef LHS, RHS;
+  while (std::tie(Str, Line) = getToken(Line), !Str.empty()) {
+if (Str.back() == ':') { // regN

clayborg wrote:
> clayborg wrote:
> > Do we really need to pull the content apart into separate strings for each 
> > register? Seems like a lot of work and 99% of these we will never accessed. 
> > Maybe just store the entire string for all registers and be done? 
> You can add an iterator method to the StackCFIRecord record maybe for when 
> you do want to parse each register?
That's a good point. I'll remove the register parsing code from this patch and 
just store the entire expression as one string(ref). I'll see what's the best 
api for splitting these up when I get to connecting this with the postfix 
expression parser.



Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:394
+  while (std::tie(Str, Line) = getToken(Line), !Str.empty()) {
+if (Str.back() == ':') { // regN
+  // Flush the previous expression, if there is one.

clayborg wrote:
> Does the format specify no space between the register name and the colon?
That is how I interpret the format "spec" 

```
Each registeri is the name of a register or pseudoregister. Each expression is 
a Breakpad postfix expression, which may contain spaces, but never ends with a 
colon.
```
That's not a very good definition because there are many ways one can split up 
these strings such they "don't end with a colon", but I chose this 
interpretation, as that's what made most sense to me (and it works for the 
example files I have lying around). We can always change that if we find a 
producer doing things slightly differently.


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

https://reviews.llvm.org/D60268



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

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 194108.
labath marked 2 inline comments as done.
labath added a comment.

Remove the fancy parsing of register expressions.


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

https://reviews.llvm.org/D60268

Files:
  source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
  source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
  unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp

Index: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
===
--- unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
+++ unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
@@ -19,7 +19,6 @@
   EXPECT_EQ(Record::File, Record::classify("FILE"));
   EXPECT_EQ(Record::Func, Record::classify("FUNC"));
   EXPECT_EQ(Record::Public, Record::classify("PUBLIC"));
-  EXPECT_EQ(Record::StackCFIInit, Record::classify("STACK CFI INIT"));
   EXPECT_EQ(Record::StackCFI, Record::classify("STACK CFI"));
 
   // Any obviously incorrect lines will be classified as such.
@@ -97,3 +96,26 @@
   EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC m"));
   EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC"));
 }
+
+TEST(StackCFIRecord, parse) {
+  EXPECT_EQ(StackCFIRecord(0x47, 0x8, ".cfa: $esp 4 + $eip: .cfa 4 - ^"),
+StackCFIRecord::parse(
+"STACK CFI INIT 47 8 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+
+  EXPECT_EQ(StackCFIRecord(0x47, 0x8, ".cfa: $esp 4 +"),
+StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +  "));
+
+  EXPECT_EQ(StackCFIRecord(0x47, llvm::None, ".cfa: $esp 4 +"),
+StackCFIRecord::parse("STACK CFI 47 .cfa: $esp 4 +"));
+
+  // The validity of the register value expressions is not checked
+  EXPECT_EQ(StackCFIRecord(0x47, 0x8, ".cfa: ^ ^ ^"),
+StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: ^ ^ ^"));
+
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("STACK CFI INIT 47"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("STACK CFI INIT"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("STACK CFI"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("STACK"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("FILE 47 foo"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("42 47"));
+}
Index: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
===
--- source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
+++ source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
@@ -153,9 +153,6 @@
   // Line records logically belong to the preceding Func record, so we put
   // them in the same section.
   next_section = Record::Func;
-} else if (next_section == Record::StackCFI) {
-  // Same goes for StackCFI and StackCFIInit
-  next_section = Record::StackCFIInit;
 }
 if (next_section == current_section)
   continue;
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
===
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
@@ -20,7 +20,7 @@
 
 class Record {
 public:
-  enum Kind { Module, Info, File, Func, Line, Public, StackCFIInit, StackCFI };
+  enum Kind { Module, Info, File, Func, Line, Public, StackCFI };
 
   /// Attempt to guess the kind of the record present in the argument without
   /// doing a full parse. The returned kind will always be correct for valid
@@ -141,6 +141,22 @@
 bool operator==(const PublicRecord &L, const PublicRecord &R);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PublicRecord &R);
 
+class StackCFIRecord : public Record {
+public:
+  static llvm::Optional parse(llvm::StringRef Line);
+  StackCFIRecord(lldb::addr_t Address, llvm::Optional Size,
+ llvm::StringRef UnwindRules)
+  : Record(StackCFI), Address(Address), Size(Size),
+UnwindRules(UnwindRules) {}
+
+  lldb::addr_t Address;
+  llvm::Optional Size;
+  llvm::StringRef UnwindRules;
+};
+
+bool operator==(const StackCFIRecord &L, const StackCFIRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const StackCFIRecord &R);
+
 } // namespace breakpad
 } // namespace lldb_private
 
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
===
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
@@ -143,8 +143,7 @@
 Tok = consume(Line);
 switch (Tok) {
 case Token::CFI:
-  Tok = consume(Line);
-  return Tok == Token::Init ? Record::StackCFIInit : Record::StackCFI;
+  return Record::StackCFI;
 default:
   return llvm::None;
 }
@@ -359,6 +358,55 @@
  R.Name);
 }
 
+llvm::Optional StackCFIRecord::parse(llvm::StringRef Line) {
+  // STACK CFI 

[Lldb-commits] [lldb] r357895 - PDBFPO: add dyn_cast support

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Apr  8 02:52:57 2019
New Revision: 357895

URL: http://llvm.org/viewvc/llvm-project?rev=357895&view=rev
Log:
PDBFPO: add dyn_cast support

This adds the necessary glue so we can use llvm::dyn_cast, instead of
doing a manual type-check followed by a cast. NFC.

Modified:

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

Modified: 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp?rev=357895&r1=357894&r2=357895&view=diff
==
--- 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
 (original)
+++ 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
 Mon Apr  8 02:52:57 2019
@@ -18,6 +18,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/DebugInfo/CodeView/CodeView.h"
 #include "llvm/DebugInfo/CodeView/EnumTables.h"
+#include "llvm/Support/Casting.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -70,6 +71,10 @@ public:
 
   llvm::StringRef GetName() const { return m_name; }
 
+  static bool classof(const FPOProgramNode *node) {
+return node->GetKind() == Symbol;
+  }
+
 private:
   llvm::StringRef m_name;
 };
@@ -83,6 +88,10 @@ public:
 
   uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
+  static bool classof(const FPOProgramNode *node) {
+return node->GetKind() == Register;
+  }
+
 private:
   uint32_t m_lldb_reg_num;
 };
@@ -96,6 +105,10 @@ public:
 
   uint32_t GetValue() const { return m_value; }
 
+  static bool classof(const FPOProgramNode *node) {
+return node->GetKind() == IntegerLiteral;
+  }
+
 private:
   uint32_t m_value;
 };
@@ -123,6 +136,10 @@ public:
   const FPOProgramNode *Right() const { return m_right; }
   FPOProgramNode *&Right() { return m_right; }
 
+  static bool classof(const FPOProgramNode *node) {
+return node->GetKind() == BinaryOp;
+  }
+
 private:
   OpType m_op_type;
   FPOProgramNode *m_left;
@@ -145,6 +162,10 @@ public:
   const FPOProgramNode *Operand() const { return m_operand; }
   FPOProgramNode *&Operand() { return m_operand; }
 
+  static bool classof(const FPOProgramNode *node) {
+return node->GetKind() == UnaryOp;
+  }
+
 private:
   OpType m_op_type;
   FPOProgramNode *m_operand;
@@ -216,10 +237,8 @@ void FPOProgramASTVisitorMergeDependent:
 void FPOProgramASTVisitorMergeDependent::TryReplace(
 FPOProgramNode *&node_ref) const {
 
-  while (node_ref->GetKind() == FPOProgramNode::Symbol) {
-auto *node_symbol_ref = static_cast(node_ref);
-
-auto it = m_dependent_programs.find(node_symbol_ref->GetName());
+  while (auto *symbol = llvm::dyn_cast(node_ref)) {
+auto it = m_dependent_programs.find(symbol->GetName());
 if (it == m_dependent_programs.end()) {
   break;
 }
@@ -277,11 +296,10 @@ static uint32_t ResolveLLDBRegisterNum(l
 
 bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
 FPOProgramNode *&node_ref) {
-  if (node_ref->GetKind() != FPOProgramNode::Symbol)
+  auto *symbol = llvm::dyn_cast(node_ref);
+  if (!symbol)
 return true;
 
-  auto *symbol = static_cast(node_ref);
-
   // Look up register reference as lvalue in preceding assignments.
   auto it = m_dependent_programs.find(symbol->GetName());
   if (it != m_dependent_programs.end()) {


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

[Lldb-commits] [lldb] r357896 - MinidumpParser: parse SystemInfo stream via llvm

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Apr  8 02:53:03 2019
New Revision: 357896

URL: http://llvm.org/viewvc/llvm-project?rev=357896&view=rev
Log:
MinidumpParser: parse SystemInfo stream via llvm

I also update the tests for SystemInfo parsing to use the yaml2minidump
capabilities in llvm instead of relying on checked-in binaries.

Modified:
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=357896&r1=357895&r2=357896&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Mon Apr  8 
02:53:03 2019
@@ -12,6 +12,7 @@
 
 #include "Plugins/Process/Utility/LinuxProcMaps.h"
 #include "lldb/Utility/LLDBAssert.h"
+#include "lldb/Utility/Log.h"
 
 // C includes
 // C++ includes
@@ -180,29 +181,19 @@ MinidumpParser::GetThreadContextWow64(co
   // stored in the first slot of the 64-bit TEB (wow64teb.Reserved1[0]).
 }
 
-const SystemInfo *MinidumpParser::GetSystemInfo() {
-  llvm::ArrayRef data = GetStream(StreamType::SystemInfo);
-
-  if (data.size() == 0)
-return nullptr;
-  const SystemInfo *system_info;
-
-  Status error = consumeObject(data, system_info);
-  if (error.Fail())
-return nullptr;
-
-  return system_info;
-}
-
 ArchSpec MinidumpParser::GetArchitecture() {
   if (m_arch.IsValid())
 return m_arch;
 
   // Set the architecture in m_arch
-  const SystemInfo *system_info = GetSystemInfo();
+  llvm::Expected system_info = m_file->getSystemInfo();
 
-  if (!system_info)
+  if (!system_info) {
+LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS),
+   system_info.takeError(),
+   "Failed to read SystemInfo stream: {0}");
 return m_arch;
+  }
 
   // TODO what to do about big endiand flavors of arm ?
   // TODO set the arm subarch stuff if the minidump has info about it

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h?rev=357896&r1=357895&r2=357896&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h Mon Apr  8 
02:53:03 2019
@@ -64,8 +64,6 @@ public:
 
   llvm::ArrayRef GetThreadContextWow64(const MinidumpThread &td);
 
-  const SystemInfo *GetSystemInfo();
-
   ArchSpec GetArchitecture();
 
   const MinidumpMiscInfo *GetMiscInfo();

Modified: lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp?rev=357896&r1=357895&r2=357896&view=diff
==
--- lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp (original)
+++ lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp Mon Apr  8 
02:53:03 2019
@@ -51,6 +51,22 @@ public:
 ASSERT_GT(parser->GetData().size(), 0UL);
   }
 
+  llvm::Error SetUpFromYaml(llvm::StringRef yaml) {
+std::string data;
+llvm::raw_string_ostream os(data);
+if (llvm::Error E = llvm::MinidumpYAML::writeAsBinary(yaml, os))
+  return E;
+
+os.flush();
+auto data_buffer_sp =
+std::make_shared(data.data(), data.size());
+auto expected_parser = MinidumpParser::Create(std::move(data_buffer_sp));
+if (!expected_parser)
+  return expected_parser.takeError();
+parser = std::move(*expected_parser);
+return llvm::Error::success();
+  }
+
   llvm::Optional parser;
 };
 
@@ -159,7 +175,22 @@ TEST_F(MinidumpParserTest, GetMemoryList
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
-  SetUpData("linux-x86_64.dmp");
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams: 
+  - Type:SystemInfo
+Processor Arch:  AMD64
+Processor Level: 6
+Processor Revision: 16130
+Number of Processors: 1
+Platform ID: Linux
+CPU: 
+  Vendor ID:   GenuineIntel
+  Version Info:0x
+  Feature Info:0x
+...
+)"),
+llvm::Succeeded());
   ASSERT_EQ(llvm::Triple::ArchType::x86_64,
 parser->GetArchitecture().GetMachine());
   ASSERT_EQ(llvm::Triple::OSType::Linux,
@@ -404,15 +435,37 @@ TEST_F(MinidumpParserTest, GetMemoryRegi
 }
 
 // Windows Minidump tests
-// fizzbuzz_no_heap.dmp is copied from the WinMiniDump tests
 TEST_F(MinidumpParserTest, GetArchitectureWindows) {
-  SetUpData("fizzbuzz_no_heap.dmp");
+  ASSE

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357897: Object/Minidump: Add support for reading the 
ModuleList stream (authored by labath, committed by ).

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60121

Files:
  llvm/trunk/include/llvm/BinaryFormat/Minidump.h
  llvm/trunk/include/llvm/Object/Minidump.h
  llvm/trunk/lib/Object/Minidump.cpp
  llvm/trunk/unittests/Object/MinidumpTest.cpp

Index: llvm/trunk/include/llvm/BinaryFormat/Minidump.h
===
--- llvm/trunk/include/llvm/BinaryFormat/Minidump.h
+++ llvm/trunk/include/llvm/BinaryFormat/Minidump.h
@@ -124,6 +124,37 @@
 };
 static_assert(sizeof(SystemInfo) == 56, "");
 
+struct VSFixedFileInfo {
+  support::ulittle32_t Signature;
+  support::ulittle32_t StructVersion;
+  support::ulittle32_t FileVersionHigh;
+  support::ulittle32_t FileVersionLow;
+  support::ulittle32_t ProductVersionHigh;
+  support::ulittle32_t ProductVersionLow;
+  support::ulittle32_t FileFlagsMask;
+  support::ulittle32_t FileFlags;
+  support::ulittle32_t FileOS;
+  support::ulittle32_t FileType;
+  support::ulittle32_t FileSubtype;
+  support::ulittle32_t FileDateHigh;
+  support::ulittle32_t FileDateLow;
+};
+static_assert(sizeof(VSFixedFileInfo) == 52, "");
+
+struct Module {
+  support::ulittle64_t BaseOfImage;
+  support::ulittle32_t SizeOfImage;
+  support::ulittle32_t Checksum;
+  support::ulittle32_t TimeDateStamp;
+  support::ulittle32_t ModuleNameRVA;
+  VSFixedFileInfo VersionInfo;
+  LocationDescriptor CvRecord;
+  LocationDescriptor MiscRecord;
+  support::ulittle64_t Reserved0;
+  support::ulittle64_t Reserved1;
+};
+static_assert(sizeof(Module) == 108, "");
+
 } // namespace minidump
 
 template <> struct DenseMapInfo {
Index: llvm/trunk/include/llvm/Object/Minidump.h
===
--- llvm/trunk/include/llvm/Object/Minidump.h
+++ llvm/trunk/include/llvm/Object/Minidump.h
@@ -55,14 +55,21 @@
 return getStream(minidump::StreamType::SystemInfo);
   }
 
+  /// Returns the module list embedded in the ModuleList stream. An error is
+  /// returned if the file does not contain this stream, or if the stream is
+  /// not large enough to contain the number of modules declared in the stream
+  /// header. The consistency of the Module entries themselves is not checked in
+  /// any way.
+  Expected> getModuleList() const;
+
 private:
-  static Error createError(StringRef Str,
-   object_error Err = object_error::parse_failed) {
-return make_error(Str, Err);
+  static Error createError(StringRef Str) {
+return make_error(Str, object_error::parse_failed);
   }
 
   static Error createEOFError() {
-return createError("Unexpected EOF", object_error::unexpected_eof);
+return make_error("Unexpected EOF",
+  object_error::unexpected_eof);
   }
 
   /// Return a slice of the given data array, with bounds checking.
@@ -101,9 +108,9 @@
   if (auto OptionalStream = getRawStream(Stream)) {
 if (OptionalStream->size() >= sizeof(T))
   return *reinterpret_cast(OptionalStream->data());
-return createError("Malformed stream", object_error::unexpected_eof);
+return createEOFError();
   }
-  return createError("No such stream", object_error::invalid_section_index);
+  return createError("No such stream");
 }
 
 template 
Index: llvm/trunk/lib/Object/Minidump.cpp
===
--- llvm/trunk/lib/Object/Minidump.cpp
+++ llvm/trunk/lib/Object/Minidump.cpp
@@ -53,6 +53,27 @@
   return Result;
 }
 
+Expected> MinidumpFile::getModuleList() const {
+  auto OptionalStream = getRawStream(StreamType::ModuleList);
+  if (!OptionalStream)
+return createError("No such stream");
+  auto ExpectedSize =
+  getDataSliceAs(*OptionalStream, 0, 1);
+  if (!ExpectedSize)
+return ExpectedSize.takeError();
+
+  size_t ListSize = ExpectedSize.get()[0];
+
+  size_t ListOffset = 4;
+  // Some producers insert additional padding bytes to align the module list to
+  // 8-byte boundary. Check for that by comparing the module list size with the
+  // overall stream size.
+  if (ListOffset + sizeof(Module) * ListSize < OptionalStream->size())
+ListOffset = 8;
+
+  return getDataSliceAs(*OptionalStream, ListOffset, ListSize);
+}
+
 Expected>
 MinidumpFile::getDataSlice(ArrayRef Data, size_t Offset, size_t Size) {
   // Check for overflow.
Index: llvm/trunk/unittests/Object/MinidumpTest.cpp
===
--- llvm/trunk/unittests/Object/MinidumpTest.cpp
+++ llvm/trunk/unittests/Object/MinidumpTest.cpp
@@ -284,3 +284,115 @@
   EXPECT_THAT_EXPECTED(File.getString(ManyStrings.size() - 2),
Failed());
 }
+
+TEST(MinidumpFile, getModuleList) {
+  std::vector

[Lldb-commits] [PATCH] D60400: Remove unneeded #ifdef SWIGs

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D60400



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

[Lldb-commits] [PATCH] D60405: MinidumpYAML: Add support for ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: amccarth, jhenderson, clayborg, zturner.
Herald added a project: LLVM.

This patch adds support for yaml (de)serialization of the minidump
ModuleList stream. It's a fairly straight forward-application of the
existing patterns to the ModuleList structures defined in previous
patches.

One thing, which may be interesting to call out explicitly is the
addition of "new" allocation functions to the helper BlobAllocator
class. The reason for this was, that there was an emerging pattern of a
need to allocate space for entities, which do not have a suitable
lifetime for use with the existing allocation functions. A typical
example of that was the "size" of various lists, which is only available
as a temporary returned by the .size() method of some container. For
these cases, one can use the new set of allocation functions, which
will take a temporary object, and store it in an allocator-managed
buffer until it is written to disk.


Repository:
  rL LLVM

https://reviews.llvm.org/D60405

Files:
  include/llvm/BinaryFormat/Minidump.h
  include/llvm/Object/Minidump.h
  include/llvm/ObjectYAML/MinidumpYAML.h
  lib/ObjectYAML/MinidumpYAML.cpp
  test/tools/obj2yaml/basic-minidump.yaml

Index: test/tools/obj2yaml/basic-minidump.yaml
===
--- test/tools/obj2yaml/basic-minidump.yaml
+++ test/tools/obj2yaml/basic-minidump.yaml
@@ -15,6 +15,33 @@
   400d9000-400db000 r-xp  b3:04 227/system/bin/app_process
   400db000-400dc000 r--p 1000 b3:04 227/system/bin/app_process
 
+  - Type:ModuleList
+Modules: 
+  - Base of Image:   0x0001020304050607
+Size of Image:   0x08090A0B
+Checksum:0x0C0D0E0F
+Time Date Stamp: 47
+Module Name: a.out
+Version Info:
+  Signature:   0x10111213
+  Struct Version:  0x14151617
+  File Version High: 0x18191A1B
+  File Version Low: 0x1C1D1E1F
+  Product Version High: 0x20212223
+  Product Version Low: 0x24252627
+  File Flags Mask: 0x28292A2B
+  File Flags:  0x2C2D2E2F
+  File OS: 0x30313233
+  File Type:   0x34353637
+  File Subtype:0x38393A3B
+  File Date High:  0x3C3D3E3F
+  File Date Low:   0x40414243
+CodeView Record: '44454647'
+Misc Record: 48494A4B
+  - Base of Image:   0x4C4D4E4F50515253
+Size of Image:   0x54555657
+Module Name: libb.so
+CodeView Record: 58595A5B
 ...
 
 # CHECK:  --- !minidump
@@ -32,4 +59,31 @@
 # CHECK-NEXT:   400d9000-400db000 r-xp  b3:04 227/system/bin/app_process
 # CHECK-NEXT:   400db000-400dc000 r--p 1000 b3:04 227/system/bin/app_process
 # CHECK-EMPTY:
+# CHECK-NEXT:   - Type:ModuleList
+# CHECK-NEXT: Modules: 
+# CHECK-NEXT:   - Base of Image:   0x0001020304050607
+# CHECK-NEXT: Size of Image:   0x08090A0B
+# CHECK-NEXT: Checksum:0x0C0D0E0F
+# CHECK-NEXT: Time Date Stamp: 47
+# CHECK-NEXT: Module Name: a.out
+# CHECK-NEXT: Version Info:
+# CHECK-NEXT:   Signature:   0x10111213
+# CHECK-NEXT:   Struct Version:  0x14151617
+# CHECK-NEXT:   File Version High: 0x18191A1B
+# CHECK-NEXT:   File Version Low: 0x1C1D1E1F
+# CHECK-NEXT:   Product Version High: 0x20212223
+# CHECK-NEXT:   Product Version Low: 0x24252627
+# CHECK-NEXT:   File Flags Mask: 0x28292A2B
+# CHECK-NEXT:   File Flags:  0x2C2D2E2F
+# CHECK-NEXT:   File OS: 0x30313233
+# CHECK-NEXT:   File Type:   0x34353637
+# CHECK-NEXT:   File Subtype:0x38393A3B
+# CHECK-NEXT:   File Date High:  0x3C3D3E3F
+# CHECK-NEXT:   File Date Low:   0x40414243
+# CHECK-NEXT: CodeView Record: '44454647'
+# CHECK-NEXT: Misc Record: 48494A4B
+# CHECK-NEXT:   - Base of Image:   0x4C4D4E4F50515253
+# CHECK-NEXT: Size of Image:   0x54555657
+# CHECK-NEXT: Module Name: libb.so
+# CHECK-NEXT: CodeView Record: 58595A5B
 # CHECK-NEXT: ...
Index: lib/ObjectYAML/MinidumpYAML.cpp
===
--- lib/ObjectYAML/MinidumpYAML.cpp
+++ lib/ObjectYAML/MinidumpYAML.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/ObjectYAML/MinidumpYAML.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/ConvertUTF.h"
 
 using namespace llvm;
@@ -14,6 +15,16 @@
 using namespace llvm::minidump;
 
 namespace {
+/// A helper class to manage the placement of various structures into the final
+/// minidump binary. Space for objects can be allocated via various allocate***
+/// methods, while the final minidump file is writte

[Lldb-commits] [PATCH] D60405: MinidumpYAML: Add support for ModuleList stream

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM. Probably should get one more ok




Comment at: lib/ObjectYAML/MinidumpYAML.cpp:91
+
 size_t BlobAllocator::allocateString(StringRef Str) {
   SmallVector WStr;

Might be nice to unique the strings in here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60405



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

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: aleksandr.urakov, amccarth.

This patch attempts to solve two issues made this code hard to follow
for me.

The first issue was that a lot of what these visitors do is mutate the
AST. The visitor pattern is not particularly good for that because by
the time you have performed the dynamic type dispatch, it's too late to
go back to the parent node, and change its pointer. The previous code
dealt with that relatively elegantly, but it still meant that one had to
perform manual type checks, which is what the visitor pattern is
supposed to avoid.

The second issue was not being able to return values from the Visit
functions, which meant that one had to store function results in member
variables (a common problem with visitor patterns).

Here, I try to solve both problems by making the visitor a CRTP
template. This allows one to parameterize the visitor based on the
return type and pass function results as function results. The mutation
is fascilitated by having each Visit function take two arguments -- a
reference to the object itself (with the correct dynamic type), and a
reference to the parent's pointer to this object.

Although this wasn't my explicit goal here, the fact that we're not
using virtual dispatch anymore (not possible due to return type
templatization) allows us to make the AST nodes trivially destructible,
which is a good thing, since we were not destroying them anyway.


https://reviews.llvm.org/D60410

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,12 +25,11 @@
 
 namespace {
 
-class FPOProgramNode;
-class FPOProgramASTVisitor;
-
 class NodeAllocator {
 public:
   template  T *makeNode(Args &&... args) {
+static_assert(std::is_trivially_destructible::value,
+  "This object will not be destroyed!");
 void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
 return new (new_node_mem) T(std::forward(args)...);
   }
@@ -53,9 +52,6 @@
   FPOProgramNode(Kind kind) : m_token_kind(kind) {}
 
 public:
-  virtual ~FPOProgramNode() = default;
-  virtual void Accept(FPOProgramASTVisitor &visitor) = 0;
-
   Kind GetKind() const { return m_token_kind; }
 
 private:
@@ -67,8 +63,6 @@
   FPOProgramNodeSymbol(llvm::StringRef name)
   : FPOProgramNode(Symbol), m_name(name) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   llvm::StringRef GetName() const { return m_name; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -84,8 +78,6 @@
   FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
   : FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -101,8 +93,6 @@
   FPOProgramNodeIntegerLiteral(uint32_t value)
   : FPOProgramNode(IntegerLiteral), m_value(value) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetValue() const { return m_value; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -126,8 +116,6 @@
   : FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(&left),
 m_right(&right) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Left() const { return m_left; }
@@ -155,8 +143,6 @@
   FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode &operand)
   : FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(&operand) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Operand() const { return m_operand; }
@@ -171,84 +157,101 @@
   FPOProgramNode *m_operand;
 };
 
+template 
 class FPOProgramASTVisitor {
-public:
-  virtual ~FPOProgramASTVisitor() = default;
+protected:
+  ResultT Dispatch(FPOProgramNode *&node) {
+switch (node->GetKind()) {
+case FPOProgramNode::Register:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+case FPOProgramNode::Symbol:
+  return getDerived().Visit(llvm::cast(*node), node);
+
+case FPOProgramNode::IntegerLiteral:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+case FPOProgramNode::UnaryOp:
+  return getDerived().Visit(llvm::cast(*node), node);
+case FPOProgramNode::BinaryOp:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+}
+llvm_unreachable("Fully covered switch!");
+  }
 
-  virtual void Visit(FPOProgramNodeSymbol &node) {}
-  virtual void Visit(FP

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I find this version easier to follow than the old one, but that could be simply 
because I didn't write it. :P
So if you believe the old one is better, I am happy to just drop this and carry 
on with the old one.


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

https://reviews.llvm.org/D60410



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

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 194160.
labath added a comment.

Fix a bug which meant we weren't replacing the root node correctly (I somehow
forgot to run tests before uploading).


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

https://reviews.llvm.org/D60410

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,12 +25,11 @@
 
 namespace {
 
-class FPOProgramNode;
-class FPOProgramASTVisitor;
-
 class NodeAllocator {
 public:
   template  T *makeNode(Args &&... args) {
+static_assert(std::is_trivially_destructible::value,
+  "This object will not be destroyed!");
 void *new_node_mem = m_alloc.Allocate(sizeof(T), alignof(T));
 return new (new_node_mem) T(std::forward(args)...);
   }
@@ -53,9 +52,6 @@
   FPOProgramNode(Kind kind) : m_token_kind(kind) {}
 
 public:
-  virtual ~FPOProgramNode() = default;
-  virtual void Accept(FPOProgramASTVisitor &visitor) = 0;
-
   Kind GetKind() const { return m_token_kind; }
 
 private:
@@ -67,8 +63,6 @@
   FPOProgramNodeSymbol(llvm::StringRef name)
   : FPOProgramNode(Symbol), m_name(name) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   llvm::StringRef GetName() const { return m_name; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -84,8 +78,6 @@
   FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
   : FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -101,8 +93,6 @@
   FPOProgramNodeIntegerLiteral(uint32_t value)
   : FPOProgramNode(IntegerLiteral), m_value(value) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   uint32_t GetValue() const { return m_value; }
 
   static bool classof(const FPOProgramNode *node) {
@@ -126,8 +116,6 @@
   : FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(&left),
 m_right(&right) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Left() const { return m_left; }
@@ -155,8 +143,6 @@
   FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode &operand)
   : FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(&operand) {}
 
-  void Accept(FPOProgramASTVisitor &visitor) override;
-
   OpType GetOpType() const { return m_op_type; }
 
   const FPOProgramNode *Operand() const { return m_operand; }
@@ -171,84 +157,101 @@
   FPOProgramNode *m_operand;
 };
 
+template 
 class FPOProgramASTVisitor {
-public:
-  virtual ~FPOProgramASTVisitor() = default;
+protected:
+  ResultT Dispatch(FPOProgramNode *&node) {
+switch (node->GetKind()) {
+case FPOProgramNode::Register:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+case FPOProgramNode::Symbol:
+  return getDerived().Visit(llvm::cast(*node), node);
+
+case FPOProgramNode::IntegerLiteral:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+case FPOProgramNode::UnaryOp:
+  return getDerived().Visit(llvm::cast(*node), node);
+case FPOProgramNode::BinaryOp:
+  return getDerived().Visit(llvm::cast(*node),
+node);
+}
+llvm_unreachable("Fully covered switch!");
+  }
 
-  virtual void Visit(FPOProgramNodeSymbol &node) {}
-  virtual void Visit(FPOProgramNodeRegisterRef &node) {}
-  virtual void Visit(FPOProgramNodeIntegerLiteral &node) {}
-  virtual void Visit(FPOProgramNodeBinaryOp &node) {}
-  virtual void Visit(FPOProgramNodeUnaryOp &node) {}
+private:
+  DerivedT &getDerived() { return static_cast(*this); }
 };
 
-void FPOProgramNodeSymbol::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
-}
-
-void FPOProgramNodeRegisterRef::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
-}
+class FPOProgramASTVisitorMergeDependent
+: public FPOProgramASTVisitor {
+public:
+  void Visit(FPOProgramNodeBinaryOp &node, FPOProgramNode *&) {
+Dispatch(node.Left());
+Dispatch(node.Right());
+  }
 
-void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
-}
+  void Visit(FPOProgramNodeUnaryOp &node, FPOProgramNode *&ref) {
+Dispatch(node.Operand());
+  }
 
-void FPOProgramNodeBinaryOp::Accept(FPOProgramASTVisitor &visitor) {
-  visitor.Visit(*this);
-}
+  void Visit(FPOProgramNodeRegisterRef &, FPOProgramNode *&) {}
+  void Visit(FPOProgramNodeIntegerLiteral &, FPOProgramNode *&) {}
+  void Visit(FPOProgramNodeSymbol &node, FPOProgramNode *&ref);
 
-void 

[Lldb-commits] [PATCH] D60325: [lldb] [Process/NetBSD] Fix wrongly mapping mm* registers

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

mgorny marked 2 inline comments as done.
mgorny added a comment.

Thanks. I'll try converting it to C now.




Comment at: lldb/lit/Register/x86-mm-xmm-read.test:1
+# REQUIRES: x86
+# RUN: %clang %p/Inputs/x86-mm-xmm-read.s -o %t

labath wrote:
> Does this mean "the host is an x86 system", or "x86 is a configured llvm 
> target"? My impression is that in means the latter 
> , but we 
> obviously want the former here.
Do you have any idea how to achieve the former? I can't find anything looking 
like it. Though I suppose it's entirely possible that there's no such thing atm.


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

https://reviews.llvm.org/D60325



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

[Lldb-commits] [PATCH] D60325: [lldb] [Process/NetBSD] Fix wrongly mapping mm* registers

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

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

Rewritten the test to C++ with inline assembly, and switched to `CHECK-DAG`. 
Still need to figure out how to filter it for correct platform.


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

https://reviews.llvm.org/D60325

Files:
  lldb/lit/Register/Inputs/x86-mm-xmm-read.cpp
  lldb/lit/Register/x86-mm-xmm-read.test
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -406,7 +406,7 @@
   case lldb_mm5_x86_64:
   case lldb_mm6_x86_64:
   case lldb_mm7_x86_64:
-reg_value.SetBytes(&m_fpr_x86_64.fxstate.fx_xmm[reg - lldb_mm0_x86_64],
+reg_value.SetBytes(&m_fpr_x86_64.fxstate.fx_87_ac[reg - lldb_mm0_x86_64],
reg_info->byte_size, endian::InlHostByteOrder());
 break;
   case lldb_xmm0_x86_64:
@@ -602,7 +602,7 @@
   case lldb_mm5_x86_64:
   case lldb_mm6_x86_64:
   case lldb_mm7_x86_64:
-::memcpy(&m_fpr_x86_64.fxstate.fx_xmm[reg - lldb_mm0_x86_64],
+::memcpy(&m_fpr_x86_64.fxstate.fx_87_ac[reg - lldb_mm0_x86_64],
  reg_value.GetBytes(), reg_value.GetByteSize());
 break;
   case lldb_xmm0_x86_64:
Index: lldb/lit/Register/x86-mm-xmm-read.test
===
--- /dev/null
+++ lldb/lit/Register/x86-mm-xmm-read.test
@@ -0,0 +1,22 @@
+# REQUIRES: x86
+# RUN: %clang++ %p/Inputs/x86-mm-xmm-read.cpp -o %t
+# RUN: %lldb -b -s %s %t | FileCheck %s
+process launch
+
+register read --all
+# CHECK-DAG: mm0 = 0x0102030405060708
+# CHECK-DAG: mm1 = 0x1112131415161718
+# CHECK-DAG: mm2 = 0x2122232425262728
+# CHECK-DAG: mm3 = 0x3132333435363738
+# CHECK-DAG: mm4 = 0x4142434445464748
+# CHECK-DAG: mm5 = 0x5152535455565758
+# CHECK-DAG: mm6 = 0x6162636465666768
+# CHECK-DAG: mm7 = 0x7172737475767778
+# CHECK-DAG: xmm0 = {0x01 0x0e 0x0c 0x0a 0x08 0x06 0x04 0x02 0x00 0x0f 0x0d 0x0b 0x09 0x07 0x05 0x03}
+# CHECK-DAG: xmm1 = {0x11 0x1e 0x1c 0x1a 0x18 0x16 0x14 0x12 0x10 0x1f 0x1d 0x1b 0x19 0x17 0x15 0x13}
+# CHECK-DAG: xmm2 = {0x21 0x2e 0x2c 0x2a 0x28 0x26 0x24 0x22 0x20 0x2f 0x2d 0x2b 0x29 0x27 0x25 0x23}
+# CHECK-DAG: xmm3 = {0x31 0x3e 0x3c 0x3a 0x38 0x36 0x34 0x32 0x30 0x3f 0x3d 0x3b 0x39 0x37 0x35 0x33}
+# CHECK-DAG: xmm4 = {0x41 0x4e 0x4c 0x4a 0x48 0x46 0x44 0x42 0x40 0x4f 0x4d 0x4b 0x49 0x47 0x45 0x43}
+# CHECK-DAG: xmm5 = {0x51 0x5e 0x5c 0x5a 0x58 0x56 0x54 0x52 0x50 0x5f 0x5d 0x5b 0x59 0x57 0x55 0x53}
+# CHECK-DAG: xmm6 = {0x61 0x6e 0x6c 0x6a 0x68 0x66 0x64 0x62 0x60 0x6f 0x6d 0x6b 0x69 0x67 0x65 0x63}
+# CHECK-DAG: xmm7 = {0x71 0x7e 0x7c 0x7a 0x78 0x76 0x74 0x72 0x70 0x7f 0x7d 0x7b 0x79 0x77 0x75 0x73}
Index: lldb/lit/Register/Inputs/x86-mm-xmm-read.cpp
===
--- /dev/null
+++ lldb/lit/Register/Inputs/x86-mm-xmm-read.cpp
@@ -0,0 +1,55 @@
+#include 
+
+struct alignas(16) xmm_t {
+  uint64_t a, b;
+};
+
+int main() {
+  uint64_t mm0 = 0x0102030405060708;
+  uint64_t mm1 = 0x1112131415161718;
+  uint64_t mm2 = 0x2122232425262728;
+  uint64_t mm3 = 0x3132333435363738;
+  uint64_t mm4 = 0x4142434445464748;
+  uint64_t mm5 = 0x5152535455565758;
+  uint64_t mm6 = 0x6162636465666768;
+  uint64_t mm7 = 0x7172737475767778;
+
+  xmm_t xmm0[2] = { 0x020406080A0C0E01, 0x030507090B0D0F00 };
+  xmm_t xmm1[2] = { 0x121416181A1C1E11, 0x131517191B1D1F10 };
+  xmm_t xmm2[2] = { 0x222426282A2C2E21, 0x232527292B2D2F20 };
+  xmm_t xmm3[2] = { 0x323436383A3C3E31, 0x333537393B3D3F30 };
+  xmm_t xmm4[2] = { 0x424446484A4C4E41, 0x434547494B4D4F40 };
+  xmm_t xmm5[2] = { 0x525456585A5C5E51, 0x535557595B5D5F50 };
+  xmm_t xmm6[2] = { 0x626466686A6C6E61, 0x636567696B6D6F60 };
+  xmm_t xmm7[2] = { 0x727476787A7C7E71, 0x737577797B7D7F70 };
+
+  asm volatile(
+"movq%0, %%mm0\n\t"
+"movq%1, %%mm1\n\t"
+"movq%2, %%mm2\n\t"
+"movq%3, %%mm3\n\t"
+"movq%4, %%mm4\n\t"
+"movq%5, %%mm5\n\t"
+"movq%6, %%mm6\n\t"
+"movq%7, %%mm7\n\t"
+"\n\t"
+"movaps  %8, %%xmm0\n\t"
+"movaps  %9, %%xmm1\n\t"
+"movaps  %10, %%xmm2\n\t"
+"movaps  %11, %%xmm3\n\t"
+"movaps  %12, %%xmm4\n\t"
+"movaps  %13, %%xmm5\n\t"
+"movaps  %14, %%xmm6\n\t"
+"movaps  %15, %%xmm7\n\t"
+"\n\t"
+"int3"
+:
+: "g"(mm0), "g"(mm1), "g"(mm2), "g"(mm3), "g"(mm4), "g"(mm5), "g"(mm6),
+  "g"(mm7), "m"(xmm0), "m"(xmm1), "m"(xmm2), "m"(xmm3), "m"(xmm4),
+  "m"(xmm5), "m"(xmm6), "m"(xmm7)
+: "%mm0", "%mm1", "%mm2", "%mm3", "%mm4", "%mm5", "%mm6", "%mm7",
+  "%xmm0", "%xmm1", "%xmm2", "%xmm3", "%xmm4", "%xmm5", "%xmm6", "%xmm7"
+  );
+
+  return 0;
+}
___
lld

[Lldb-commits] [lldb] r357948 - Fix a stack buffer overflow found by ASAN.

[Adrian Prantl via lldb-commits]

Author: adrian
Date: Mon Apr  8 14:58:36 2019
New Revision: 357948

URL: http://llvm.org/viewvc/llvm-project?rev=357948&view=rev
Log:
Fix a stack buffer overflow found by ASAN.

llvm::StringRef host_and_port is not guaranteed to be null-terminated.
Generally, it is not safe at all to convert a StringRef into a char *
by calling data() on it.



Modified:
lldb/trunk/source/Host/common/Socket.cpp

Modified: lldb/trunk/source/Host/common/Socket.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/Socket.cpp?rev=357948&r1=357947&r2=357948&view=diff
==
--- lldb/trunk/source/Host/common/Socket.cpp (original)
+++ lldb/trunk/source/Host/common/Socket.cpp Mon Apr  8 14:58:36 2019
@@ -124,7 +124,7 @@ Status Socket::TcpConnect(llvm::StringRe
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_COMMUNICATION));
   if (log)
 log->Printf("Socket::%s (host/port = %s)", __FUNCTION__,
-host_and_port.data());
+host_and_port.str().c_str());
 
   Status error;
   std::unique_ptr connect_socket(
@@ -144,7 +144,7 @@ Status Socket::TcpListen(llvm::StringRef
  Predicate *predicate, int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   if (log)
-log->Printf("Socket::%s (%s)", __FUNCTION__, host_and_port.data());
+log->Printf("Socket::%s (%s)", __FUNCTION__, host_and_port.str().c_str());
 
   Status error;
   std::string host_str;
@@ -184,7 +184,7 @@ Status Socket::UdpConnect(llvm::StringRe
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   if (log)
 log->Printf("Socket::%s (host/port = %s)", __FUNCTION__,
-host_and_port.data());
+host_and_port.str().c_str());
 
   return UDPSocket::Connect(host_and_port, child_processes_inherit, socket);
 }
@@ -275,7 +275,8 @@ bool Socket::DecodeHostAndPort(llvm::Str
   // port is too large
   if (error_ptr)
 error_ptr->SetErrorStringWithFormat(
-"invalid host:port specification: '%s'", host_and_port.data());
+"invalid host:port specification: '%s'",
+host_and_port.str().c_str());
   return false;
 }
   }
@@ -293,7 +294,7 @@ bool Socket::DecodeHostAndPort(llvm::Str
 
   if (error_ptr)
 error_ptr->SetErrorStringWithFormat("invalid host:port specification: 
'%s'",
-host_and_port.data());
+host_and_port.str().c_str());
   return false;
 }
 


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

[Lldb-commits] [lldb] r357954 - Experiment with a larger packet timeout.

[Adrian Prantl via lldb-commits]

Author: adrian
Date: Mon Apr  8 16:02:11 2019
New Revision: 357954

URL: http://llvm.org/viewvc/llvm-project?rev=357954&view=rev
Log:
Experiment with a larger packet timeout.

This is a follow-up to r357829 (https://reviews.llvm.org/D60340) to
see whether increasing the packet timeout for non-asan builds could
also positively affect the stability of non-asan bots.

Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=357954&r1=357953&r2=357954&view=diff
==
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Mon Apr  
8 16:02:11 2019
@@ -111,10 +111,10 @@ void DumpProcessGDBRemotePacketHistory(v
 namespace {
 
 static constexpr PropertyDefinition g_properties[] = {
-{"packet-timeout", OptionValue::eTypeUInt64, true, 1
+{"packet-timeout", OptionValue::eTypeUInt64, true, 5
 #if defined(__has_feature)
 #if __has_feature(address_sanitizer)
-  + 4
+  * 2
 #endif
 #endif
   , NULL, {},


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

[Lldb-commits] [lldb] r357955 - Rename Target::GetSharedModule to Target::GetOrCreateModule.

[Jason Molenda via lldb-commits]

Author: jmolenda
Date: Mon Apr  8 16:03:02 2019
New Revision: 357955

URL: http://llvm.org/viewvc/llvm-project?rev=357955&view=rev
Log:
Rename Target::GetSharedModule to Target::GetOrCreateModule.

Add a flag to control whether the ModulesDidLoad notification is
called when a module is added.  If the notifications are disabled,
the caller must call ModulesDidLoad after adding all the new modules,
but postponing this notification until they're all batched up can
allow for better efficiency than notifying one-by-one.

Change the name of the ModuleList notifier functions that a subclass
can implement to start with 'Notify' to make it clear what they are.
Add a NotifyModulesRemoved.

Add header documentation for the changed/updated methods.

Added defaulted-value 'notify' argument to ModuleList Append,
AppendIfNeeded, and Remove because callers working with a local
ModuleList don't have an obvious idea of what notify means in this
context.  When the ModuleList is a part of the Target class, the
notify behavior matters.

DynamicLoaderDarwin has been updated so that libraries being
added/removed are correctly batched up before notifications are
sent.  Added the TestModuleLoadedNotifys.py test to run on 
Darwin to test this.

 

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


Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/

lldb/trunk/packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile

lldb/trunk/packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
Modified:
lldb/trunk/include/lldb/Core/ModuleList.h
lldb/trunk/include/lldb/Target/Target.h
lldb/trunk/source/API/SBTarget.cpp
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Core/DynamicLoader.cpp
lldb/trunk/source/Core/ModuleList.cpp
lldb/trunk/source/Expression/FunctionCaller.cpp

lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

lldb/trunk/source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
lldb/trunk/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp

lldb/trunk/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/source/Target/Target.cpp

Modified: lldb/trunk/include/lldb/Core/ModuleList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ModuleList.h?rev=357955&r1=357954&r2=357955&view=diff
==
--- lldb/trunk/include/lldb/Core/ModuleList.h (original)
+++ lldb/trunk/include/lldb/Core/ModuleList.h Mon Apr  8 16:03:02 2019
@@ -96,14 +96,16 @@ public:
   public:
 virtual ~Notifier() = default;
 
-virtual void ModuleAdded(const ModuleList &module_list,
- const lldb::ModuleSP &module_sp) = 0;
-virtual void ModuleRemoved(const ModuleList &module_list,
-   const lldb::ModuleSP &module_sp) = 0;
-virtual void ModuleUpdated(const ModuleList &module_list,
-   const lldb::ModuleSP &old_module_sp,
-   const lldb::ModuleSP &new_module_sp) = 0;
-virtual void WillClearList(const ModuleList &module_list) = 0;
+virtual void NotifyModuleAdded(const ModuleList &module_list,
+   const lldb::ModuleSP &module_sp) = 0;
+virtual void NotifyModuleRemoved(const ModuleList &module_list,
+ const lldb::ModuleSP &module_sp) = 0;
+virtual void NotifyModuleUpdated(const ModuleList &module_list,
+ const lldb::ModuleSP &old_module_sp,
+ const lldb::ModuleSP &new_module_sp) = 0;
+virtual void NotifyWillClearList(const ModuleList &module_list) = 0;
+
+virtual void NotifyModulesRemoved(lldb_private::ModuleList &module_list) = 
0;
   };
 
   //--
@@ -146,12 +148,20 @@ public:
   //--
   /// Append a module to the module list.
   ///
-  /// Appends the module to the collection.
-  ///
   /// \param[in] module_sp
   /// A shared pointer to a module to add to this collection.
+  ///
+  /// \param[in] notify
+  /// If true, and a notifier function is set, the notifier function
+  /// will be called.  Defaults to true.
+  ///
+  /// When this ModuleList is the Target's ModuleList, t

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Phabricator via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB357955: Rename Target::GetSharedModule to 
Target::GetOrCreateModule. (authored by jmolenda, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60172?vs=194000&id=194216#toc

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Target/Target.h
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
  source/API/SBTarget.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/DynamicLoader.cpp
  source/Core/ModuleList.cpp
  source/Expression/FunctionCaller.cpp
  source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
  source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Target.cpp

Index: packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile
===
--- packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile
+++ packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile
@@ -0,0 +1,5 @@
+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules
Index: packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
===
--- packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
+++ packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
@@ -0,0 +1,6 @@
+#include 
+int main ()
+{
+  puts("running"); // breakpoint here
+  return 0;
+}
Index: packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
===
--- packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
+++ packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
@@ -0,0 +1,114 @@
+"""
+Test how many times newly loaded binaries are notified;
+they should be delivered in batches instead of one-by-one.
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ModuleLoadedNotifysTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+# DyanmicLoaderDarwin should batch up notifications about
+# newly added/removed libraries.  Other DynamicLoaders may
+# not be written this way.
+@skipUnlessDarwin
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.cpp', '// breakpoint')
+
+def test_launch_notifications(self):
+"""Test that lldb broadcasts newly loaded libraries in batches."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.dbg.SetAsync(False)
+
+listener = self.dbg.GetListener()
+listener.StartListeningForEventClass(
+self.dbg,
+lldb.SBTarget.GetBroadcasterClassName(),
+lldb.SBTarget.eBroadcastBitModulesLoaded | lldb.SBTarget.eBroadcastBitModulesUnloaded)
+
+# Create a target by the debugger.
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+# break on main
+breakpoint = target.BreakpointCreateByName('main', 'a.out')
+
+event = lldb.SBEvent()
+# CreateTarget() generated modules-loaded events; consume them & toss
+while listener.GetNextEvent(event):
+True
+
+error = lldb.SBError()
+process = target.Launch(listener,
+None,  # argv
+None,  # envp
+None,  # stdin_path
+None,  # stdout_path
+None,  # stderr_path
+

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

[Adrian McCarthy via Phabricator via lldb-commits]

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

Overall, I'm ambivalent.

The patch description makes a good case for the change.  I find the visitor 
pattern hard to follow (partly because the `visit` and `accept` naming is not 
intuitive for me).  I find CRTP more understandable.  And you make a good 
argument about the visitor pattern not being right for a mutator.  So I'm 
inclined to like this change

But in practice, I don't find the change any easier to read, and the switch on 
the type _seems_ like a code smell even though it's justified here.

I'm OK with it if nobody else objects.




Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:284
 
-bool FPOProgramASTVisitorResolveRegisterRefs::TryReplace(
-FPOProgramNode *&node_ref) {
-  auto *symbol = llvm::dyn_cast(node_ref);
-  if (!symbol)
-return true;
-
+bool FPOProgramASTVisitorResolveRegisterRefs::Visit(FPOProgramNodeSymbol &node,
+FPOProgramNode *&ref) {

I actually preferred `symbol` for this parameter name.  `node` is very generic. 
 At this point, we know the node is a symbol, so using the more specific name 
would be more consistent.


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

https://reviews.llvm.org/D60410



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

[Lldb-commits] [lldb] r357963 - Get the run locker before you ask if your thread is valid.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Mon Apr  8 18:33:23 2019
New Revision: 357963

URL: http://llvm.org/viewvc/llvm-project?rev=357963&view=rev
Log:
Get the run locker before you ask if your thread is valid.

I have occasional crashes coming from SBThread::GetExtendedBacktraceThread.  
The 
symptom is that we got true back from HasThreadScope - so we should have a valid
live thread, but then when we go to use the thread, it is not good anymore and 
we
crash.
I can't spot any obvious cause for this crash, but in looking for same I noticed
that in the current code we check that the thread is valid, THEN we take the 
stop 
locker.  We really should do that in the other order, and ensure that the 
process 
will stay stopped before we check our thread is still good.  That's what this 
patch does.



Modified:
lldb/trunk/source/API/SBThread.cpp

Modified: lldb/trunk/source/API/SBThread.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBThread.cpp?rev=357963&r1=357962&r2=357963&view=diff
==
--- lldb/trunk/source/API/SBThread.cpp (original)
+++ lldb/trunk/source/API/SBThread.cpp Mon Apr  8 18:33:23 2019
@@ -1329,9 +1329,9 @@ SBThread SBThread::GetExtendedBacktraceT
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
   SBThread sb_origin_thread;
 
-  if (exe_ctx.HasThreadScope()) {
-Process::StopLocker stop_locker;
-if (stop_locker.TryLock(&exe_ctx.GetProcessPtr()->GetRunLock())) {
+  Process::StopLocker stop_locker;
+  if (stop_locker.TryLock(&exe_ctx.GetProcessPtr()->GetRunLock())) {
+if (exe_ctx.HasThreadScope()) {
   ThreadSP real_thread(exe_ctx.GetThreadSP());
   if (real_thread) {
 ConstString type_const(type);


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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan updated this revision to Diff 194244.
rocallahan added a comment.

Addressed all comments AFAIK. I'll post some performance numbers in a moment.


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

https://reviews.llvm.org/D54747

Files:
  lld/ELF/Driver.cpp
  lld/ELF/InputFiles.h
  lld/ELF/InputSection.cpp
  lld/ELF/InputSection.h
  lld/ELF/MarkLive.cpp
  lld/test/ELF/linkerscript/comdat-gc.s
  lld/test/ELF/linkerscript/debuginfo-gc.s

Index: lld/test/ELF/linkerscript/debuginfo-gc.s
===
--- /dev/null
+++ lld/test/ELF/linkerscript/debuginfo-gc.s
@@ -0,0 +1,14 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/comdat-gc.s -o %t1
+# RUN: echo "SECTIONS { .text : { *(.text*) } }" > %t.script
+# RUN: ld.lld --gc-sections --script %t.script %t %t1 -o %t2
+# RUN: llvm-readobj -sections -symbols %t2 | FileCheck %s
+
+# CHECK-NOT: Name: .debug_line
+
+.file 1 "test/ELF/linkerscript/comdat_gc.s"
+.section  .text._Z3fooIiEvv,"axG",@progbits,_Z3fooIiEvv,comdat
+.loc 1 14
+  ret
Index: lld/test/ELF/linkerscript/comdat-gc.s
===
--- lld/test/ELF/linkerscript/comdat-gc.s
+++ lld/test/ELF/linkerscript/comdat-gc.s
@@ -8,6 +8,9 @@
 
 # GC1: Name: .debug_line
 
+# Add .ctors section so all debuginfo isn't GCed
+.section  .ctors,"ax",@progbits
+
 .file 1 "test/ELF/linkerscript/comdat_gc.s"
 .section  .text._Z3fooIiEvv,"axG",@progbits,_Z3fooIiEvv,comdat
 .loc 1 14
Index: lld/ELF/MarkLive.cpp
===
--- lld/ELF/MarkLive.cpp
+++ lld/ELF/MarkLive.cpp
@@ -47,7 +47,7 @@
   void run();
 
 private:
-  void enqueue(InputSectionBase *Sec, uint64_t Offset);
+  void enqueue(InputSectionBase *Sec, uint64_t Offset, bool IsLSDA);
   void markSymbol(Symbol *Sym);
 
   template 
@@ -97,7 +97,7 @@
   Offset += getAddend(Sec, Rel);
 
 if (!IsLSDA || !(RelSec->Flags & SHF_EXECINSTR))
-  enqueue(RelSec, Offset);
+  enqueue(RelSec, Offset, IsLSDA);
 return;
   }
 
@@ -106,7 +106,7 @@
   SS->getFile().IsNeeded = true;
 
   for (InputSectionBase *Sec : CNamedSections.lookup(Sym.getName()))
-enqueue(Sec, 0);
+enqueue(Sec, 0, IsLSDA);
 }
 
 // The .eh_frame section is an unfortunate special case.
@@ -169,7 +169,7 @@
 }
 
 template 
-void MarkLive::enqueue(InputSectionBase *Sec, uint64_t Offset) {
+void MarkLive::enqueue(InputSectionBase *Sec, uint64_t Offset, bool IsLSDA) {
   // Skip over discarded sections. This in theory shouldn't happen, because
   // the ELF spec doesn't allow a relocation to point to a deduplicated
   // COMDAT section directly. Unfortunately this happens in practice (e.g.
@@ -183,19 +183,28 @@
   if (auto *MS = dyn_cast(Sec))
 MS->getSectionPiece(Offset)->Live = true;
 
+  InputSection *S = dyn_cast(Sec);
+  // LSDA does not count as "live code or data" in the object file.
+  // The section may already have been marked live for LSDA in which
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+  Sec->getFile()->HasLiveCodeOrData = true;
+
   if (Sec->Live)
 return;
-  Sec->Live = true;
 
+  Sec->Live = true;
   // Add input section to the queue.
-  if (InputSection *S = dyn_cast(Sec))
+  if (S) {
 Queue.push_back(S);
+  }
 }
 
 template  void MarkLive::markSymbol(Symbol *Sym) {
   if (auto *D = dyn_cast_or_null(Sym))
 if (auto *IS = dyn_cast_or_null(D->Section))
-  enqueue(IS, D->Value);
+  enqueue(IS, D->Value, false);
 }
 
 // This is the main function of the garbage collector.
@@ -239,7 +248,7 @@
   continue;
 
 if (isReserved(Sec) || Script->shouldKeep(Sec)) {
-  enqueue(Sec, 0);
+  enqueue(Sec, 0, false);
 } else if (isValidCIdentifier(Sec->Name)) {
   CNamedSections[Saver.save("__start_" + Sec->Name)].push_back(Sec);
   CNamedSections[Saver.save("__stop_" + Sec->Name)].push_back(Sec);
@@ -259,7 +268,7 @@
 }
 
 for (InputSectionBase *IS : Sec.DependentSections)
-  enqueue(IS, 0);
+  enqueue(IS, 0, false);
   }
 }
 
@@ -285,7 +294,7 @@
   // The -gc-sections option works only for SHF_ALLOC sections
   // (sections that are memory-mapped at runtime). So we can
   // unconditionally make non-SHF_ALLOC sections alive except
-  // SHF_LINK_ORDER and SHT_REL/SHT_RELA sections.
+  // SHF_LINK_ORDER and SHT_REL/SHT_RELA sections and .debug sections.
   //
   // Usually, SHF_ALLOC sections are not removed even if they are
   // unreachable through relocations because reachability is not
@@ -301,7 +310,12 @@
   // or -emit-reloc were given. And they are subject of garbage
   // collection because, if we remove a text section, we also
   // remove its relocation section.
+  bool AnyDebugSections = false;
   for (InputSectionBase *Sec : InputSections) {
+  

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan added a comment.

Updated results for the rusoto test in 
https://github.com/rust-lang/rust/issues/56068#issue-382175735. The test 
changed a bit because I'm using an updated Rust toolchain and `rusoto_core` 
0.37.0.

| Linker | Size (bytes) | Real time (ms) |
| GNU ld version 2.29.1-23.fc28  | 48,554,736   | 2234   |
| GNU gold (version 2.29.1-23.fc28) 1.14 | 49,888,392   | 813|
| lld-6.0.1-1.fc28.x86_64| 49,800,824   | 247|
| lld d918d74461724a22cedd0b76dc1237392f295656  
| 49,873,960   | 224|
| lld d918d74461724a22cedd0b76dc1237392f295656 + this patch 
| 5,390,632| 158|


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan added a comment.

I am also pleased to report that for my real project 
, switching from 
`ld.bfd` to `lld` + this patch reduces the total size of our `dist` built 
binaries from 2.9GB to 2.0GB.

I'm not sure why it's become more effective, but several variables have 
changed, including some significant restructuring of the build artifacts for 
our project. Still, this seems like a pretty good result. (Perhaps even 
suspiciously good! But I can't find any missing debuginfo with cursory checks.)


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Rui Ueyama via Phabricator via lldb-commits]

ruiu added a comment.

Nice. One more thing you might want to try is to add `-O2` to the linker 
command line option. When that option is given, lld attempts to tail-merge 
strings in the string table. That's not very effective, but you might be able 
to shave off 0.2% or something like that.


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Rui Ueyama via Phabricator via lldb-commits]

ruiu added a comment.

Overall looking good.




Comment at: lld/ELF/MarkLive.cpp:190
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))

ruiu wrote:
> S is true only when it is an InputSection, so you have duplicate tests for 
> this. It also looks like you don't have to care about whether a section is an 
> input section or a merged section. Isn't this condition sufficient?
> 
>   if (Sec->File && !IsLSDA)
> Sec->getFile()->HasLiveCodeOrData = true;
Move this code after `Sec->Live = true` so that when we visit the same section 
more than once, this piece of code runs only once.



Comment at: lld/ELF/MarkLive.cpp:190-191
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+  Sec->getFile()->HasLiveCodeOrData = true;

S is true only when it is an InputSection, so you have duplicate tests for 
this. It also looks like you don't have to care about whether a section is an 
input section or a merged section. Isn't this condition sufficient?

  if (Sec->File && !IsLSDA)
Sec->getFile()->HasLiveCodeOrData = true;



Comment at: lld/ELF/MarkLive.cpp:313
   // remove its relocation section.
+  bool AnyDebugSections = false;
   for (InputSectionBase *Sec : InputSections) {

Let's remove this optimization -- I don't think we need this.



Comment at: lld/ELF/MarkLive.cpp:321
 bool IsLinkOrder = (Sec->Flags & SHF_LINK_ORDER);
 bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);

Then you can simplify the condition to

  if (!IsAlloc && !IsLinkOrder && !IsRel && !Sec->Debug)
Sec->Live = true;



Comment at: lld/ELF/MarkLive.cpp:330
 
+  if (AnyDebugSections)
+// Mark debug sections as live in any object file that has a live

Remove this variable and just visit all sections even if there's no debug info 
(which shouldn't take too much time anyway.)


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via lldb-commits]

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



Comment at: lld/ELF/MarkLive.cpp:190
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))

ruiu wrote:
> ruiu wrote:
> > S is true only when it is an InputSection, so you have duplicate tests for 
> > this. It also looks like you don't have to care about whether a section is 
> > an input section or a merged section. Isn't this condition sufficient?
> > 
> >   if (Sec->File && !IsLSDA)
> > Sec->getFile()->HasLiveCodeOrData = true;
> Move this code after `Sec->Live = true` so that when we visit the same 
> section more than once, this piece of code runs only once.
We can't do that. The comment I added tries to explain why. Is it unclear?



Comment at: lld/ELF/MarkLive.cpp:190-191
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+  Sec->getFile()->HasLiveCodeOrData = true;

rocallahan wrote:
> ruiu wrote:
> > ruiu wrote:
> > > S is true only when it is an InputSection, so you have duplicate tests 
> > > for this. It also looks like you don't have to care about whether a 
> > > section is an input section or a merged section. Isn't this condition 
> > > sufficient?
> > > 
> > >   if (Sec->File && !IsLSDA)
> > > Sec->getFile()->HasLiveCodeOrData = true;
> > Move this code after `Sec->Live = true` so that when we visit the same 
> > section more than once, this piece of code runs only once.
> We can't do that. The comment I added tries to explain why. Is it unclear?
I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the 
advice to use `isa` here was wrong and I should revert to checking `kind() == 
Regular || kind() == Merge`?



Comment at: lld/ELF/MarkLive.cpp:313
   // remove its relocation section.
+  bool AnyDebugSections = false;
   for (InputSectionBase *Sec : InputSections) {

ruiu wrote:
> Let's remove this optimization -- I don't think we need this.
OK



Comment at: lld/ELF/MarkLive.cpp:321
 bool IsLinkOrder = (Sec->Flags & SHF_LINK_ORDER);
 bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);

ruiu wrote:
> Then you can simplify the condition to
> 
>   if (!IsAlloc && !IsLinkOrder && !IsRel && !Sec->Debug)
> Sec->Live = true;
Yep.


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Rui Ueyama via Phabricator via lldb-commits]

ruiu added inline comments.



Comment at: lld/ELF/MarkLive.cpp:190
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))

ruiu wrote:
> rocallahan wrote:
> > rocallahan wrote:
> > > ruiu wrote:
> > > > ruiu wrote:
> > > > > S is true only when it is an InputSection, so you have duplicate 
> > > > > tests for this. It also looks like you don't have to care about 
> > > > > whether a section is an input section or a merged section. Isn't this 
> > > > > condition sufficient?
> > > > > 
> > > > >   if (Sec->File && !IsLSDA)
> > > > > Sec->getFile()->HasLiveCodeOrData = true;
> > > > Move this code after `Sec->Live = true` so that when we visit the same 
> > > > section more than once, this piece of code runs only once.
> > > We can't do that. The comment I added tries to explain why. Is it unclear?
> > I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the 
> > advice to use `isa` here was wrong and I should revert to checking `kind() 
> > == Regular || kind() == Merge`?
> But you don't really care even if it is `EhInputSection` or 
> `SyntheticSection`, no? I mean an assigned value would not be used but 
> doesn't harm.
Ah, OK, got it.



Comment at: lld/ELF/MarkLive.cpp:190-191
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+  Sec->getFile()->HasLiveCodeOrData = true;

rocallahan wrote:
> rocallahan wrote:
> > ruiu wrote:
> > > ruiu wrote:
> > > > S is true only when it is an InputSection, so you have duplicate tests 
> > > > for this. It also looks like you don't have to care about whether a 
> > > > section is an input section or a merged section. Isn't this condition 
> > > > sufficient?
> > > > 
> > > >   if (Sec->File && !IsLSDA)
> > > > Sec->getFile()->HasLiveCodeOrData = true;
> > > Move this code after `Sec->Live = true` so that when we visit the same 
> > > section more than once, this piece of code runs only once.
> > We can't do that. The comment I added tries to explain why. Is it unclear?
> I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the 
> advice to use `isa` here was wrong and I should revert to checking `kind() == 
> Regular || kind() == Merge`?
But you don't really care even if it is `EhInputSection` or `SyntheticSection`, 
no? I mean an assigned value would not be used but doesn't harm.


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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D60440: [lldb-server] Introduce Socket::Initialize and Terminate to simply WSASocket setup

[Aaron Smith via Phabricator via lldb-commits]

asmith created this revision.
asmith added reviewers: zturner, labath.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60440

Files:
  include/lldb/Host/Socket.h
  source/Host/common/Socket.cpp
  source/Initialization/SystemInitializerCommon.cpp
  source/Plugins/Platform/Windows/PlatformWindows.cpp
  unittests/Host/MainLoopTest.cpp
  unittests/Host/SocketAddressTest.cpp
  unittests/Host/SocketTest.cpp
  unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp

Index: unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
===
--- unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
+++ unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
@@ -7,26 +7,17 @@
 //===--===//
 
 #include "GDBRemoteTestUtils.h"
-
-#if defined(_MSC_VER)
-#include "lldb/Host/windows/windows.h"
-#include 
-#endif
+#include "lldb/Host/Socket.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 void GDBRemoteTest::SetUpTestCase() {
-#if defined(_MSC_VER)
-  WSADATA data;
-  ::WSAStartup(MAKEWORD(2, 2), &data);
-#endif
+  ASSERT_TRUE(!Socket::Initialize().operator bool());
 }
 
 void GDBRemoteTest::TearDownTestCase() {
-#if defined(_MSC_VER)
-  ::WSACleanup();
-#endif
+  Socket::Terminate();
 }
 
 } // namespace process_gdb_remote
Index: unittests/Host/SocketTest.cpp
===
--- unittests/Host/SocketTest.cpp
+++ unittests/Host/SocketTest.cpp
@@ -28,16 +28,11 @@
 class SocketTest : public testing::Test {
 public:
   void SetUp() override {
-#if defined(_MSC_VER)
-WSADATA data;
-::WSAStartup(MAKEWORD(2, 2), &data);
-#endif
+ASSERT_TRUE(!Socket::Initialize().operator bool());
   }
 
   void TearDown() override {
-#if defined(_MSC_VER)
-::WSACleanup();
-#endif
+Socket::Terminate();
   }
 
 protected:
Index: unittests/Host/SocketAddressTest.cpp
===
--- unittests/Host/SocketAddressTest.cpp
+++ unittests/Host/SocketAddressTest.cpp
@@ -8,27 +8,23 @@
 
 #include "gtest/gtest.h"
 
+#include "lldb/Host/Socket.h"
 #include "lldb/Host/SocketAddress.h"
 
+using namespace lldb_private;
+
 namespace {
 class SocketAddressTest : public testing::Test {
 public:
   static void SetUpTestCase() {
-#ifdef _MSC_VER
-WSADATA data;
-ASSERT_EQ(0, WSAStartup(MAKEWORD(2, 2), &data));
-#endif
+ASSERT_TRUE(!Socket::Initialize().operator bool());
   }
   static void TearDownTestCase() {
-#ifdef _MSC_VER
-ASSERT_EQ(0, WSACleanup());
-#endif
+Socket::Terminate();
   }
 };
 } // namespace
 
-using namespace lldb_private;
-
 TEST_F(SocketAddressTest, Set) {
   SocketAddress sa;
   ASSERT_TRUE(sa.SetToLocalhost(AF_INET, 1138));
Index: unittests/Host/MainLoopTest.cpp
===
--- unittests/Host/MainLoopTest.cpp
+++ unittests/Host/MainLoopTest.cpp
@@ -19,16 +19,11 @@
 class MainLoopTest : public testing::Test {
 public:
   static void SetUpTestCase() {
-#ifdef _MSC_VER
-WSADATA data;
-ASSERT_EQ(0, WSAStartup(MAKEWORD(2, 2), &data));
-#endif
+ASSERT_TRUE(!Socket::Initialize().operator bool());
   }
 
   static void TearDownTestCase() {
-#ifdef _MSC_VER
-ASSERT_EQ(0, WSACleanup());
-#endif
+Socket::Terminate();
   }
 
   void SetUp() override {
Index: source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -125,8 +125,6 @@
 
   if (g_initialize_count++ == 0) {
 #if defined(_WIN32)
-WSADATA dummy;
-WSAStartup(MAKEWORD(2, 2), &dummy);
 // Force a host flag to true for the default platform object.
 PlatformSP default_platform_sp(new PlatformWindows(true));
 default_platform_sp->SetSystemArchitecture(HostInfo::GetArchitecture());
@@ -142,9 +140,6 @@
 void PlatformWindows::Terminate(void) {
   if (g_initialize_count > 0) {
 if (--g_initialize_count == 0) {
-#ifdef _WIN32
-  WSACleanup();
-#endif
   PluginManager::UnregisterPlugin(PlatformWindows::CreateInstance);
 }
   }
Index: source/Initialization/SystemInitializerCommon.cpp
===
--- source/Initialization/SystemInitializerCommon.cpp
+++ source/Initialization/SystemInitializerCommon.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/Timer.h"
@@ -114,6 +115,10 @@
   ProcessWindowsLog::Initialize();
 #endif
 
+  llvm::Error error = Socket::Initialize();
+  if (error)
+return error;
+
   return llvm::Error::success();
 }
 

[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for lldb-server on Windows

[Aaron Smith via Phabricator via lldb-commits]

asmith marked 6 inline comments as done.
asmith added inline comments.



Comment at: tools/lldb-server/SystemInitializerLLGS.cpp:37-52
+#if defined(_WIN32)
+  if (g_ref_count++ == 0) {
+// Require Windows Sockets version 2.2.
+auto wVersion = MAKEWORD(2, 2);
+WSADATA wsaData;
+auto err = WSAStartup(wVersion, &wsaData);
+if (err == 0) {

labath wrote:
> zturner wrote:
> > labath wrote:
> > > I think a better option here would be to move this code to something like 
> > > `Socket::Initialize`. Then we could call this from 
> > > `SystemInitializerCommon::Initialize`, and it would be available to 
> > > everyone. This would allow us to remove the copy of this code in 
> > > PlatformWindows, and replace the `WSAStartup` calls in various unittests 
> > > with `Socket::Initialize()`
> > Also, why do we need the `g_ref_count`?  Can't we just print an error 
> > message and call exit if the version is not what we expect?
> I still believe this should go to Socket::Initialize (as a separate patch).
This is done in https://reviews.llvm.org/D60440


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

https://reviews.llvm.org/D56233



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Robert O'Callahan via Phabricator via lldb-commits]

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



Comment at: lld/ELF/MarkLive.cpp:190-191
+  // case we still need to mark the file.
+  if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+  Sec->getFile()->HasLiveCodeOrData = true;

ruiu wrote:
> ruiu wrote:
> > rocallahan wrote:
> > > rocallahan wrote:
> > > > ruiu wrote:
> > > > > ruiu wrote:
> > > > > > S is true only when it is an InputSection, so you have duplicate 
> > > > > > tests for this. It also looks like you don't have to care about 
> > > > > > whether a section is an input section or a merged section. Isn't 
> > > > > > this condition sufficient?
> > > > > > 
> > > > > >   if (Sec->File && !IsLSDA)
> > > > > > Sec->getFile()->HasLiveCodeOrData = true;
> > > > > Move this code after `Sec->Live = true` so that when we visit the 
> > > > > same section more than once, this piece of code runs only once.
> > > > We can't do that. The comment I added tries to explain why. Is it 
> > > > unclear?
> > > I want to skip `EhInputSection` and `SyntheticSection` here. So I guess 
> > > the advice to use `isa` here was wrong and I should revert to checking 
> > > `kind() == Regular || kind() == Merge`?
> > But you don't really care even if it is `EhInputSection` or 
> > `SyntheticSection`, no? I mean an assigned value would not be used but 
> > doesn't harm.
> Ah, OK, got it.
I'm assuming that `.eh_frame` sections don't have associated debuginfo so even 
if such a section is enqueued somehow, it should not cause debuginfo to be 
included for that object file. Is it impossible for a `.eh_frame` section to be 
enqueued here? Ditto for a `SyntheticSection`?

To put it another way, the logic here is supposed to be: "debuginfo can only be 
associated with Regular or Merge sections, so it only makes sense to mark files 
as having 'live code or data' possibly associated with debuginfo for those 
section types."


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

https://reviews.llvm.org/D54747



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