[Lldb-commits] [PATCH] D66249: [JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

A bunch more comments from me. So far, I've only tried to highlight the most 
obvious problems or style issues.
 It's not clear to me whether this is still prototype code or not... If it 
isn't, I'll have a bunch more.. :)




Comment at: lldb/include/lldb/Breakpoint/BreakpointSite.h:54-56
+   static bool classof(const BreakpointSite *bp_site) {
+ return bp_site->getKind() == eKindBreakpointSite;
+   }

mib wrote:
> labath wrote:
> > This is weird. So, in OO terminology, `BreakpointInjectedSite` "is-a" 
> > `BreakpointSite`, but if you ask 
> > `llvm::isa(injected_site)`, it will return false?
> No, it returns **true**.
Then does `llvm::isa(injected_site)` return false? They 
can't both return true, given the current implementations (but they should, if 
the dynamic type of `injected_site` really is `BreakpointInjectedSite`...



Comment at: lldb/include/lldb/Expression/ExpressionVariable.h:210
+public:
+  typedef AdaptedIterable

mib wrote:
> labath wrote:
> > What is the purpose of this `AdaptedIterable`, and why is it better than 
> > just returning  say `ArrayRef Given a class with a container, `AdaptedIterable` will generate the proper 
> iterator accessor for this class.
> 
> In this case, it's pretty convenient to do:
> ```
> for (auto var : expr_vars)
> { }
> ```
> 
> instead of:
> ```
> for (auto var : expr_vars.GetContainer())
> { }
> ```
> 
> It doesn't require to have a getter on the container to iterate over it.
> 
> Most of LLDB container classes are built upon `std` containers (vector, map 
> ...).
> I could change it to `ArrayRef`, but I don't see 
> why using it would be better, + it might break things (e.g. ArrayRef doesn't 
> implement some of the `std` containers method like `erase()` or `clear()`).
You are adding this function in this very patch. It can't "break" anything if 
nobody is using it. If you want to expose the full feature set of the 
underlying container, then why wrap it in this thing, why not return a 
reference to the container itself?

Though I am not sure if you actually want to do that -- having somebody from 
the outside tinker with the contents of an internal container seems like it 
would break encapsulation in most cases.

If you look at the llvm classes, you'll find that they usually just do 
`iterator_range variables() { return 
make_iterator_range(m_variables.begin(), m_variables.end()); }` in situations 
like these...



Comment at: lldb/include/lldb/Target/ABI.h:163
+
+  virtual llvm::ArrayRef GetJumpOpcode() { return 0; }
+

I don't think this does what you think it does... it returns an ArrayRef 
containing a single element -- zero. (And a dangling ArrayRef even...)



Comment at: lldb/include/lldb/Target/ABI.h:165
+
+  virtual size_t GetJumpSize() { return 0; }
+

The point of my earlier suggestion to use ArrayRef is that you don't need to 
have a separate `XXXSize` function. `ArrayRef` will encode both the 
size and the contents.

That said, I'm not convinced that this is a good api anyway. I think it would 
be extremely hard to make anything on top of these functions that would *not* 
be target specific (e.g. how would you handle the fact that on arm the jump 
target is expressed as a relative offset to the address of the jump instruction 
**+ 8**?).

I think it would be better to just have the ABI plugin just return the fully 
constructed trampoline jump sequence (say by giving it the final address of the 
sequence, and the address it should jump to) instead of somebody trying to 
create a target-independent assembler on top of it.



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+  "\n"
+  "   typedef unsigned intuint32_t;\n"
+  "   typedef unsigned long long  uint64_t ;\n"

aprantl wrote:
> This seems awfully specific. Shouldn't this be target-dependent, and is there 
> no pre-existing table in LLDB that we could derive this from?
This entire function is target specific. I would struggle to find a single line 
in this expression that could be reused on linux for instance (and let's not 
even talk about windows..).
I still believe that the simplest approach here would be to allocate memory for 
this structure on the stack, side-stepping the whole "how to allocate memory in 
a target-independent manner" discussion completely.



Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:421
+case DW_OP_addr: {
+  int64_t operand = op.getRawOperand(0);
+  expr += "   src_addr = " + std::to_string(operand) +

Note that this will generally not be the correct address for PIC (so, pretty 
much everything nowadays). You also need consider the actual address that the 
object file got loaded at, not just the thing that's written in the file itself.



Com

[Lldb-commits] [PATCH] D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D66250#1638338 , @jasonmolenda 
wrote:

> In D66250#1633455 , @clayborg wrote:
>
> > Why are we not just using ObjectFileJIT? I am guessing these breakpoint 
> > expressions create one of these by compiling the breakpoint expression and 
> > JIT'ing it just like any other expression. If this is the case, then why do 
> > we need to create a ObjectFileTrampoline? Seems like we could add .cfi 
> > directives to the assembly we use for the breakpoint condition function so 
> > that we can unwind out of it?
>
>
> I hadn't looked at ObjectFileJIT, we should look at whether we can that 
> existing plugin.  I agree they're conceptually doing the same thing, but one 
> difference is that the trampoline is only a function, it's not a mach-o 
> binary that we've written into memory.  I think it would be possible to 
> represent the trampoline unwind plan in eh_frame using a DW_OP_addr encoding 
> for the caller's pc value, but we would have to manually write out the bytes 
> of the eh_frame header, CIE and FDE entries, then the DW_OP_addr.  I think 
> it's easier to stuff in an UnwindPlan with this unwind rule added manually.  
> Using the existing eh_frame format is laudable but I think it hand-writing 
> the binary format into memory and then reading it back out again would be 
> less maintainable IMO.


Setting the return address is one thing, but I'm not sure it stops there... For 
instance, I would expect you'll want to also set the "restore" rules for the GP 
registers too, so that their values come out "right" when you select the 
original frame in the debugger. We could do something special there too, but if 
we go the assembly route (which I believe has a lot of other advantages too), 
then all we'd need is to add some .cfi directives into the asm, and the 
eh_frame would fall out automatically..




Comment at: lldb/source/Target/ABI.cpp:10
 #include "lldb/Target/ABI.h"
+#include "Plugins/ObjectFile/Trampoline/ObjectFileTrampoline.h"
+#include "lldb/Core/Module.h"

jasonmolenda wrote:
> mib wrote:
> > labath wrote:
> > > If this is going to be something that is called directly from core lldb 
> > > code, then it not a "plugin" by any stretch of imagination. I think we 
> > > should put this file some place else.
> > Any suggestion on where to put it ?
> Maybe in source/Symbol along with the ObjectFile.cpp base class.  I agree 
> with Pavel that this isn't a plugin; it subclasses ObjectFile so it probably 
> has to implement the plugin methods, but it's not something that will ever be 
> created by iterating through the active ObjectFile plugins or anything like 
> that.
Yeah, either that, or next to the place that actually uses it (which would be 
`Target/ABI` right now). I think you have to implement some plugin methods 
(like `GetPluginName`, which is fine), but I am pretty sure you don't have the 
implement the CreateInstance stuff or register yourself with the plugin manager 
(as we will never be creating the objects this way).

That said, I'm still not convinced we really need this class in the first 
place. I would like to explore the possiblity of injecting the trampoline code 
as inline asm into the compiled expression. That way, it should automatically 
become a part of the resuling ObjectFileJIT, and it will solve a couple of 
other problems along the way (e.g., the "how to allocate memory in a 
target-independent manner" discussion in the other thread).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66250



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

[Lldb-commits] [PATCH] D65952: SymbolVendor: Have plugins return symbol files directly

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D65952#1641539 , @clayborg wrote:

> In D65952#1624799 , @labath wrote:
>
> > In D65952#1623297 , @clayborg 
> > wrote:
> >
> > > So I am confused. Are we keeping SymbolVendor around for locating symbols 
> > > files or are we getting rid of it entirely?
> >
> >
> > Well... right now my idea was to keep it around as an class with just some 
> > static methods for the purposes of locating symbol files, though I'm open 
> > to other ideas too. After this patch the SymbolVendor class will contain 
> > just one method (FindPlugin), but I think that in the future it could be 
> > used to host functionality that is common for different symbol vendors. 
> > Here, I'm mainly thinking of `Symbols/LocateSymbolFile.cpp` (formerly 
> > `Host/Symbols.cpp`), which contains a bunch of functionality for searching 
> > for symbol files (so it could fall under the symbol vendor mandate), and it 
> > is currently being called by other symbol vendors to do their job. However, 
> > it is also being called from placed other than symbol vendors, and so more 
> > refactoring would be needed for that to fit in neatly.
>
>
> IMHO: The functionality that locates symbol files should be moved into the 
> Platform and we can get rid of SymbolVendor completely.


This sounded like an interesting idea at first, because Platforms already do 
some work with finding files and stuff. However, after thinking about it a bit, 
I am not so sure anymore :). The reason for that is twofold:

- Platform objects are tied to a Debugger (they don't reference it directly, 
but their instances are owned by the Debugger object), where as Modules are 
trying to be independent of the Debuggers. This would make it tricky for 
example to fetch a platform instance when searching for a symbol file.
- even if we work around that (say by making the search logic a static member 
of the platform class), then there still the issue that some symbol finding 
logic is not really tied to any particular platform. It is true that the two 
current symbol vendors could be assigned to platforms relatively cleanly 
(SymbolVendorELF -> PlatformPOSIX, SymbolVendorMacOSX -> PlatformDarwin), but 
then we'd have a problem with where to put the Breakpad finding logic (it isn't 
implemented yet, but I am hoping to get around to it eventually). Breakpad 
system for searching for symbol files involves directory structures like 
`$SYMBOL_NAME/$UUID/$SYMBOL_NAME.sym` (e.g. 
`ConsoleApplication1.pdb/897DD83EA8C8411897F3A925EE4BF7411/ConsoleApplication1.sym`).
 I think this system is inspired by some Microsoft scheme, but breakpad uses it 
everywhere, which means there isn't a single platform class that could house 
this functionality.

I also considered putting this functionality inside the SymbolFile classes. 
That would solve the `Debugger` problem, and it would definitely work for 
breakpad. However, it would mean munging ELF and MacOS symbol vendors into the 
DWARF symbol file, which has enough on its plate already (though maybe there is 
a case to be made for moving SymbolFileDWARFDebugMap into a separate plugin, in 
which case the symbol vendor could go there -- however that's pretty hard to do 
right now).

So, overall, I think its still best to keep symbol finding/vending as a 
separate piece of functionality, albeit without a a real (isntantiatable) class 
backing it. Theoretically, we could keep this as a separate entry point in the 
PluginManager, but get rid of the `Plugins/SymbolVendor` directories by 
spreading the implementation over various other plugin kinds (breakpad vendor 
-> SymbolFileBreakpad, "elf" vendor -> ObjectFileELF (?), macos vendor -> 
PlatformDarwin), but given that they these folders already exist, I don't see 
much of a reason for changing them.


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

https://reviews.llvm.org/D65952



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

[Lldb-commits] [PATCH] D65952: SymbolVendor: Have plugins return symbol files directly

[Pavel Labath via Phabricator via lldb-commits]

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

merge m_old_symfiles and m_symfiles_up. I am not entirely satisfied with how I 
implemented that, but then again the original implementation wasn't totally 
clean either.


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

https://reviews.llvm.org/D65952

Files:
  include/lldb/Core/Module.h
  include/lldb/Core/PluginManager.h
  include/lldb/Symbol/SymbolVendor.h
  include/lldb/lldb-private-interfaces.h
  source/Core/Module.cpp
  source/Core/PluginManager.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
  source/Symbol/SymbolContext.cpp
  source/Symbol/SymbolVendor.cpp

Index: source/Symbol/SymbolVendor.cpp
===
--- source/Symbol/SymbolVendor.cpp
+++ source/Symbol/SymbolVendor.cpp
@@ -7,13 +7,10 @@
 //===--===//
 
 #include "lldb/Symbol/SymbolVendor.h"
-
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
-#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolFile.h"
-#include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -23,20 +20,20 @@
 // Platforms can register a callback to use when creating symbol vendors to
 // allow for complex debug information file setups, and to also allow for
 // finding separate debug information files.
-SymbolVendor *SymbolVendor::FindPlugin(const lldb::ModuleSP &module_sp,
-   lldb_private::Stream *feedback_strm) {
-  std::unique_ptr instance_up;
+std::unique_ptr
+SymbolVendor::FindPlugin(const lldb::ModuleSP &module_sp,
+ lldb_private::Stream *feedback_strm) {
+  std::unique_ptr instance_up;
   SymbolVendorCreateInstance create_callback;
 
   for (size_t idx = 0;
(create_callback = PluginManager::GetSymbolVendorCreateCallbackAtIndex(
 idx)) != nullptr;
++idx) {
-instance_up.reset(create_callback(module_sp, feedback_strm));
+instance_up = create_callback(module_sp, feedback_strm);
 
-if (instance_up) {
-  return instance_up.release();
-}
+if (instance_up)
+  return instance_up;
   }
   // The default implementation just tries to create debug information using
   // the file representation for the module.
@@ -51,32 +48,6 @@
   }
   if (!sym_objfile_sp)
 sym_objfile_sp = module_sp->GetObjectFile()->shared_from_this();
-  instance_up.reset(new SymbolVendor(module_sp));
-  instance_up->AddSymbolFileRepresentation(sym_objfile_sp);
-  return instance_up.release();
+  return std::unique_ptr(
+  SymbolFile::FindPlugin(std::move(sym_objfile_sp)));
 }
-
-// SymbolVendor constructor
-SymbolVendor::SymbolVendor(const lldb::ModuleSP &module_sp)
-: ModuleChild(module_sp), m_sym_file_up() {}
-
-// Destructor
-SymbolVendor::~SymbolVendor() {}
-
-// Add a representation given an object file.
-void SymbolVendor::AddSymbolFileRepresentation(const ObjectFileSP &objfile_sp) {
-  ModuleSP module_sp(GetModule());
-  if (module_sp) {
-std::lock_guard guard(module_sp->GetMutex());
-if (objfile_sp)
-  m_sym_file_up.reset(SymbolFile::FindPlugin(objfile_sp));
-  }
-}
-
-// PluginInterface protocol
-lldb_private::ConstString SymbolVendor::GetPluginName() {
-  static ConstString g_name("vendor-default");
-  return g_name;
-}
-
-uint32_t SymbolVendor::GetPluginVersion() { return 1; }
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -18,6 +18,7 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
+#include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
Index: source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
===
--- source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
+++ source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
@@ -19,26 +19,9 @@
 
   static void Terminate();
 
-  static lldb_private::ConstString GetPluginNameStatic();
-
-  static const char *GetPluginDescriptionStatic();
-
-  static lldb_private::SymbolVendor *
+  static std::unique_ptr
   CreateInstance(const lldb::ModuleSP &module_sp,
  lldb_private::Stream *feedback_strm);
-
-  // Constructors and Destructors
-  SymbolVendorMacOSX(const lldb::ModuleSP &module_sp);
-
-  virtual ~SymbolVendorMacOSX();
-
-  // PluginInterface protocol
-  virt

[Lldb-commits] [PATCH] D66633: Breakpad: Add support for parsing STACK WIN records

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: amccarth, markmentovai.

The fields that aren't useful for us right now are simply ignored.


https://reviews.llvm.org/D66633

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

Index: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
===
--- unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
+++ unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
@@ -20,6 +20,7 @@
   EXPECT_EQ(Record::Func, Record::classify("FUNC"));
   EXPECT_EQ(Record::Public, Record::classify("PUBLIC"));
   EXPECT_EQ(Record::StackCFI, Record::classify("STACK CFI"));
+  EXPECT_EQ(Record::StackWin, Record::classify("STACK WIN"));
 
   // Any obviously incorrect lines will be classified as such.
   EXPECT_EQ(llvm::None, Record::classify("STACK"));
@@ -119,3 +120,34 @@
   EXPECT_EQ(llvm::None, StackCFIRecord::parse("FILE 47 foo"));
   EXPECT_EQ(llvm::None, StackCFIRecord::parse("42 47"));
 }
+
+TEST(StackWinRecord, parse) {
+  EXPECT_EQ(
+  StackWinRecord(0x47, 0x8, 3, 4, 5, llvm::StringRef("$eip $esp ^ =")),
+  StackWinRecord::parse("STACK WIN 4 47 8 1 2 3 4 5 6 1 $eip $esp ^ ="));
+
+  EXPECT_EQ(llvm::None, StackWinRecord::parse(
+"STACK WIN 0 47 8 1 0 0 0 0 0 1 $eip $esp ^ ="));
+  EXPECT_EQ(llvm::None,
+StackWinRecord::parse("STACK WIN 4 47 8 1 0 0 0 0 0 0 1"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse(
+"STACK WIN 3 47 8 1 0 0 0 0 0 1 $eip $esp ^ ="));
+  EXPECT_EQ(llvm::None,
+StackWinRecord::parse("STACK WIN 3 47 8 1 0 0 0 0 0 0 1"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse(
+"STACK WIN 4 47 8 1 0 0 0 0 1 $eip $esp ^ ="));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1 0 0 0 0 0"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1 0 0 0 0"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1 0 0 0"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1 0 0"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1 0"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8 1"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47 8"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4 47"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN 4"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK WIN"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse("STACK"));
+  EXPECT_EQ(llvm::None, StackWinRecord::parse(""));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("FILE 47 foo"));
+  EXPECT_EQ(llvm::None, StackCFIRecord::parse("42 47"));
+}
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, StackCFI };
+  enum Kind { Module, Info, File, Func, Line, Public, StackCFI, StackWin };
 
   /// 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
@@ -157,6 +157,29 @@
 bool operator==(const StackCFIRecord &L, const StackCFIRecord &R);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const StackCFIRecord &R);
 
+class StackWinRecord : public Record {
+public:
+  static llvm::Optional parse(llvm::StringRef Line);
+
+  StackWinRecord(lldb::addr_t RVA, lldb::addr_t CodeSize,
+ lldb::addr_t ParameterSize, lldb::addr_t SavedRegisterSize,
+ lldb::addr_t LocalSize, llvm::StringRef ProgramString)
+  : Record(StackWin), RVA(RVA), CodeSize(CodeSize),
+ParameterSize(ParameterSize), SavedRegisterSize(SavedRegisterSize),
+LocalSize(LocalSize), ProgramString(ProgramString) {}
+
+  enum class FrameType : uint8_t { FPO = 0, FrameData = 4 };
+  lldb::addr_t RVA;
+  lldb::addr_t CodeSize;
+  lldb::addr_t ParameterSize;
+  lldb::addr_t SavedRegisterSize;
+  lldb::addr_t LocalSize;
+  llvm::StringRef ProgramString;
+};
+
+bool operator==(const StackWinRecord &L, const StackWinRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const StackWinRecord &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
@@ -16,7 +16,19 @@
 using namespace lldb_private::breakpad;
 
 namespace {
-enum class Token { Unknown, Module, Info, CodeID, File, Func, Public, Stack, CFI, Init };
+enum

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: amccarth, aleksandr.urakov.
Herald added a subscriber: aprantl.

Previously we moved the code which parses a single expression out of the PDB
plugin, because that was useful for DWARF expressions in breakpad. However, FPO
programs are used in breakpad files too (when unwinding on windows), so this
completes the job, and moves the rest of the FPO parser too.


https://reviews.llvm.org/D66634

Files:
  include/lldb/Symbol/PostfixExpression.h
  source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
  source/Symbol/PostfixExpression.cpp
  unittests/Symbol/PostfixExpressionTest.cpp

Index: unittests/Symbol/PostfixExpressionTest.cpp
===
--- unittests/Symbol/PostfixExpressionTest.cpp
+++ unittests/Symbol/PostfixExpressionTest.cpp
@@ -12,6 +12,7 @@
 #include "lldb/Utility/StreamString.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
@@ -71,40 +72,68 @@
   }
 };
 
-static std::string ParseAndStringify(llvm::StringRef expr) {
+static std::string ParseOneAndStringify(llvm::StringRef expr) {
   llvm::BumpPtrAllocator alloc;
-  return ASTPrinter::Print(Parse(expr, alloc));
+  return ASTPrinter::Print(ParseOneExpression(expr, alloc));
 }
 
-TEST(PostfixExpression, Parse) {
-  EXPECT_EQ("int(47)", ParseAndStringify("47"));
-  EXPECT_EQ("$foo", ParseAndStringify("$foo"));
-  EXPECT_EQ("+(int(1), int(2))", ParseAndStringify("1 2 +"));
-  EXPECT_EQ("-(int(1), int(2))", ParseAndStringify("1 2 -"));
-  EXPECT_EQ("@(int(1), int(2))", ParseAndStringify("1 2 @"));
-  EXPECT_EQ("+(int(1), +(int(2), int(3)))", ParseAndStringify("1 2 3 + +"));
-  EXPECT_EQ("+(+(int(1), int(2)), int(3))", ParseAndStringify("1 2 + 3 +"));
-  EXPECT_EQ("^(int(1))", ParseAndStringify("1 ^"));
-  EXPECT_EQ("^(^(int(1)))", ParseAndStringify("1 ^ ^"));
-  EXPECT_EQ("^(+(int(1), ^(int(2", ParseAndStringify("1 2 ^ + ^"));
-  EXPECT_EQ("-($foo, int(47))", ParseAndStringify("$foo 47 -"));
-  EXPECT_EQ("+(int(47), int(-42))", ParseAndStringify("47 -42 +"));
-
-  EXPECT_EQ("nullptr", ParseAndStringify("+"));
-  EXPECT_EQ("nullptr", ParseAndStringify("^"));
-  EXPECT_EQ("nullptr", ParseAndStringify("1 +"));
-  EXPECT_EQ("nullptr", ParseAndStringify("1 2 ^"));
-  EXPECT_EQ("nullptr", ParseAndStringify("1 2 3 +"));
-  EXPECT_EQ("nullptr", ParseAndStringify("^ 1"));
-  EXPECT_EQ("nullptr", ParseAndStringify("+ 1 2"));
-  EXPECT_EQ("nullptr", ParseAndStringify("1 + 2"));
-  EXPECT_EQ("nullptr", ParseAndStringify("1 2"));
-  EXPECT_EQ("nullptr", ParseAndStringify(""));
+TEST(PostfixExpression, ParseOneExpression) {
+  EXPECT_EQ("int(47)", ParseOneAndStringify("47"));
+  EXPECT_EQ("$foo", ParseOneAndStringify("$foo"));
+  EXPECT_EQ("+(int(1), int(2))", ParseOneAndStringify("1 2 +"));
+  EXPECT_EQ("-(int(1), int(2))", ParseOneAndStringify("1 2 -"));
+  EXPECT_EQ("@(int(1), int(2))", ParseOneAndStringify("1 2 @"));
+  EXPECT_EQ("+(int(1), +(int(2), int(3)))", ParseOneAndStringify("1 2 3 + +"));
+  EXPECT_EQ("+(+(int(1), int(2)), int(3))", ParseOneAndStringify("1 2 + 3 +"));
+  EXPECT_EQ("^(int(1))", ParseOneAndStringify("1 ^"));
+  EXPECT_EQ("^(^(int(1)))", ParseOneAndStringify("1 ^ ^"));
+  EXPECT_EQ("^(+(int(1), ^(int(2", ParseOneAndStringify("1 2 ^ + ^"));
+  EXPECT_EQ("-($foo, int(47))", ParseOneAndStringify("$foo 47 -"));
+  EXPECT_EQ("+(int(47), int(-42))", ParseOneAndStringify("47 -42 +"));
+
+  EXPECT_EQ("nullptr", ParseOneAndStringify("+"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("^"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("1 +"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("1 2 ^"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("1 2 3 +"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("^ 1"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("+ 1 2"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("1 + 2"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify("1 2"));
+  EXPECT_EQ("nullptr", ParseOneAndStringify(""));
+}
+
+static std::vector>
+ParseFPOAndStringify(llvm::StringRef prog) {
+  llvm::BumpPtrAllocator alloc;
+  std::vector> parsed =
+  ParseFPOProgram(prog, alloc);
+  auto range = llvm::map_range(
+  parsed, [](const std::pair &pair) {
+return std::make_pair(pair.first, ASTPrinter::Print(pair.second));
+  });
+  return std::vector>(range.begin(),
+  range.end());
+}
+
+TEST(PostfixExpression, ParseFPOProgram) {
+  EXPECT_THAT(ParseFPOAndStringify("a 1 ="),
+  testing::ElementsAre(std::make_pair("a", "int(1)")));
+  EXPECT_THAT(ParseFPOAndStringify("a 1 = b 2 3 + ="),
+  testing::ElementsAre(std::make_pair("a", "int(1)"),
+   std::make_pair("b", "+(int(2), int(3))")));
+
+  EXPECT_THAT(ParseFPOAndSt

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, JDevlieghere, granata.enrico.
jankratochvil added a project: LLDB.
Herald added a subscriber: lldb-commits.

The sanity checking part 
 is improved here to 
use some better error channel than original `llvm::errs()`.
With this patch (and without its `TestDataFormatterAdv.py` fix) one gets:

  runCmd: frame variable int_array
  output: (int [5]) int_array = 

I understand one could pass some better abstracted error-reporting interface 
instead of `ValueObject` itself but that would require some more code 
(interface definition, inheritance etc.).
With this alternative patch 
 one 
gets:

  runCmd: frame variable int_array
  output: (int [5]) int_array = ([0] = -1, [1] = 2, [2] = 3, [3] = 4, [4] = 5)
   = no formatter applied
  and on console:
  error: a.out Two regexes ("int \[[0-9]+\]" and "int \[[0-9]\]") ambiguously 
match the same string "int [5]".

But I think the alternative patch is worse than this one - for example maybe 
the variable does not come from a file/module (can it really happen?) and then 
there would be no way to print the error.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66654

Files:
  lldb/include/lldb/Core/ValueObject.h
  lldb/include/lldb/DataFormatters/FormattersContainer.h
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-advanced/TestDataFormatterAdv.py
  lldb/source/DataFormatters/TypeCategory.cpp

Index: lldb/source/DataFormatters/TypeCategory.cpp
===
--- lldb/source/DataFormatters/TypeCategory.cpp
+++ lldb/source/DataFormatters/TypeCategory.cpp
@@ -101,9 +101,10 @@
lldb::TypeFormatImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeFormatsContainer()->Get(candidates, entry, reason))
+  if (GetTypeFormatsContainer()->Get(candidates, entry, reason, &valobj))
 return true;
-  bool regex = GetRegexTypeFormatsContainer()->Get(candidates, entry, reason);
+  bool regex =
+  GetRegexTypeFormatsContainer()->Get(candidates, entry, reason, &valobj);
   if (regex && reason)
 *reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
   return regex;
@@ -114,9 +115,10 @@
lldb::TypeSummaryImplSP &entry, uint32_t *reason) {
   if (!IsEnabled() || !IsApplicable(valobj))
 return false;
-  if (GetTypeSummariesContainer()->Get(candidates, entry, reason))
+  if (GetTypeSummariesContainer()->Get(candidates, entry, reason, &valobj))
 return true;
-  bool regex = GetRegexTypeSummariesContainer()->Get(candidates, entry, reason);
+  bool regex =
+  GetRegexTypeSummariesContainer()->Get(candidates, entry, reason, &valobj);
   if (regex && reason)
 *reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummary;
   return regex;
@@ -132,17 +134,19 @@
   bool regex_filter = false;
   // first find both Filter and Synth, and then check which is most recent
 
-  if (!GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter))
+  if (!GetTypeFiltersContainer()->Get(candidates, filter_sp, &reason_filter,
+  &valobj))
 regex_filter = GetRegexTypeFiltersContainer()->Get(candidates, filter_sp,
-   &reason_filter);
+   &reason_filter, &valobj);
 
   bool regex_synth = false;
   uint32_t reason_synth = 0;
   bool pick_synth = false;
   ScriptedSyntheticChildren::SharedPointer synth;
-  if (!GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth))
-regex_synth = GetRegexTypeSyntheticsContainer()->Get(candidates, synth,
- &reason_synth);
+  if (!GetTypeSyntheticsContainer()->Get(candidates, synth, &reason_synth,
+ &valobj))
+regex_synth = GetRegexTypeSyntheticsContainer()->Get(
+candidates, synth, &reason_synth, &valobj);
   if (!filter_sp.get() && !synth.get())
 return false;
   else if (!filter_sp.get() && synth.get())
@@ -174,10 +178,10 @@
lldb::TypeValidatorImplSP &entry, uint32_t *reason) {
   if (!IsEnabled())
 return false;
-  if (GetTypeValidatorsContainer()->Get(candidates, entry, reason))
+  if (GetTypeValidatorsContainer()->Get(candidates, entry, reason, &valobj))
 return true;
-  bool regex =
-  GetRegexTypeValidatorsContainer()->Get(candidates, entry, reason);
+  bool regex = GetRegexTypeValidatorsContainer()->Get(candidates, entry, reason,
+  &valobj);
   if (regex && reason)
 *reason |= lldb_private::eFormatterChoiceCriterionRegularExpressionSummar

[Lldb-commits] [PATCH] D66655: [lldb] Fix x86 compilation

[Leonid Mashinskiy via Phabricator via lldb-commits]

leonid.mashinskiy created this revision.
leonid.mashinskiy added a reviewer: asmith.
leonid.mashinskiy added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66655

Files:
  source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
  source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h


Index: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
===
--- source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
+++ source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
@@ -6,7 +6,7 @@
 //
 
//===--===//
 
-#if defined(_WIN64)
+#if defined(_WIN32) && !defined(_WIN64)
 #ifndef liblldb_NativeRegisterContextWindows_i386_h_
 #define liblldb_NativeRegisterContextWindows_i386_h_
 
@@ -71,4 +71,4 @@
 } // namespace lldb_private
 
 #endif // liblldb_NativeRegisterContextWindows_i386_h_
-#endif // defined(_WIN64)
+#endif // defined(_WIN32) && !defined(_WIN64)
Index: 
source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
===
--- source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
+++ source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
@@ -242,7 +242,6 @@
 NativeRegisterContextWindows_i386::ReadRegister(const RegisterInfo *reg_info,
 RegisterValue ®_value) {
   Status error;
-  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_REGISTERS);
 
   if (!reg_info) {
 error.SetErrorString("reg_info NULL");
@@ -267,7 +266,6 @@
 
 Status NativeRegisterContextWindows_i386::WriteRegister(
 const RegisterInfo *reg_info, const RegisterValue ®_value) {
-  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_REGISTERS);
   Status error;
 
   if (!reg_info) {
@@ -286,12 +284,12 @@
   }
 
   if (IsGPR(reg))
-return GPRRead(reg, reg_value);
+return GPRWrite(reg, reg_value);
 
   return Status("unimplemented");
 }
 
-Status NativeRegisterContextWindows_x86_64::ReadAllRegisterValues(
+Status NativeRegisterContextWindows_i386::ReadAllRegisterValues(
 lldb::DataBufferSP &data_sp) {
   const size_t data_size = REG_CONTEXT_SIZE;
   data_sp = std::make_shared(data_size, 0);


Index: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
===
--- source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
+++ source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.h
@@ -6,7 +6,7 @@
 //
 //===--===//
 
-#if defined(_WIN64)
+#if defined(_WIN32) && !defined(_WIN64)
 #ifndef liblldb_NativeRegisterContextWindows_i386_h_
 #define liblldb_NativeRegisterContextWindows_i386_h_
 
@@ -71,4 +71,4 @@
 } // namespace lldb_private
 
 #endif // liblldb_NativeRegisterContextWindows_i386_h_
-#endif // defined(_WIN64)
+#endif // defined(_WIN32) && !defined(_WIN64)
Index: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
===
--- source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
+++ source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_i386.cpp
@@ -242,7 +242,6 @@
 NativeRegisterContextWindows_i386::ReadRegister(const RegisterInfo *reg_info,
 RegisterValue ®_value) {
   Status error;
-  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_REGISTERS);
 
   if (!reg_info) {
 error.SetErrorString("reg_info NULL");
@@ -267,7 +266,6 @@
 
 Status NativeRegisterContextWindows_i386::WriteRegister(
 const RegisterInfo *reg_info, const RegisterValue ®_value) {
-  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_REGISTERS);
   Status error;
 
   if (!reg_info) {
@@ -286,12 +284,12 @@
   }
 
   if (IsGPR(reg))
-return GPRRead(reg, reg_value);
+return GPRWrite(reg, reg_value);
 
   return Status("unimplemented");
 }
 
-Status NativeRegisterContextWindows_x86_64::ReadAllRegisterValues(
+Status NativeRegisterContextWindows_i386::ReadAllRegisterValues(
 lldb::DataBufferSP &data_sp) {
   const size_t data_size = REG_CONTEXT_SIZE;
   data_sp = std::make_shared(data_size, 0);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Jonas Devlieghere via Phabricator via lldb-commits]

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

> I understand one could pass some better abstracted error-reporting interface 
> instead of ValueObject itself but that would require some more code 
> (interface definition, inheritance etc.).

How much work would it be to return an `llvm::Expected` from `Get_Impl`? 
I'd really prefer that over the current approach.




Comment at: lldb/include/lldb/Core/ValueObject.h:459
 
+  void SetError(Status &&error) { m_error = std::move(error); }
+

This allows overwriting the error without checking if it's already set. Maybe 
it's better to use `GetError`, check that it's not set, and then either set it 
or append to it?



Comment at: lldb/include/lldb/DataFormatters/FormattersContainer.h:296
 
-  bool Get_Impl(ConstString key, MapValueType &value,
+  bool Get_Impl(ConstString key, MapValueType &value, ValueObject *valobj,
 lldb::RegularExpressionSP *dummy) {

If returning an expected isn't feasible, we should pass at least a `Status*` 
instead of setting the error in the `ValueObject` directly. 



Comment at: lldb/source/DataFormatters/TypeCategory.cpp:303
+if (GetTypeFormatsContainer()->Get(type_name, format_sp,
+   nullptr /*valobj*/)) {
   if (matching_category)

It's really unfortunate that so many call sites now require a null pointer. Can 
we make it a default argument?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654



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

[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Enrico Granata via Phabricator via lldb-commits]

granata.enrico resigned from this revision.
granata.enrico added a comment.

I'm not working on LLDB anymore. Sorry about that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654



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

[Lldb-commits] [lldb] r369788 - Windows: explicitly cast constants to `DWORD`

[Saleem Abdulrasool via lldb-commits]

Author: compnerd
Date: Fri Aug 23 10:58:53 2019
New Revision: 369788

URL: http://llvm.org/viewvc/llvm-project?rev=369788&view=rev
Log:
Windows: explicitly cast constants to `DWORD`

STATUS_SINGLE_STEP and STATUS_BREAKPOINT are defined as 0x8-- which
is negative and thus can't be implicitly narrowed to a DWORD which is
unsigned.  The value is defined differently across winnt.h and ntstatus.h.

Patch by Gwen Mittertreiner!

Modified:
lldb/trunk/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp

Modified: 
lldb/trunk/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp?rev=369788&r1=369787&r2=369788&view=diff
==
--- lldb/trunk/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp 
Fri Aug 23 10:58:53 2019
@@ -430,7 +430,7 @@ NativeProcessWindows::OnDebugException(b
 
   ExceptionResult result = ExceptionResult::SendToApplication;
   switch (record.GetExceptionCode()) {
-  case STATUS_SINGLE_STEP:
+  case DWORD(STATUS_SINGLE_STEP):
   case STATUS_WX86_SINGLE_STEP:
 StopThread(record.GetThreadID(), StopReason::eStopReasonTrace);
 SetState(eStateStopped, true);
@@ -438,7 +438,7 @@ NativeProcessWindows::OnDebugException(b
 // Continue the debugger.
 return ExceptionResult::MaskException;
 
-  case STATUS_BREAKPOINT:
+  case DWORD(STATUS_BREAKPOINT):
   case STATUS_WX86_BREAKPOINT:
 if (FindSoftwareBreakpoint(record.GetExceptionAddress())) {
   LLDB_LOG(log, "Hit non-loader breakpoint at address {0:x}.",


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

[Lldb-commits] [PATCH] D66445: Explicitly Cast Constants to DWORD

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r369788


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66445



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

[Lldb-commits] [PATCH] D66633: Breakpad: Add support for parsing STACK WIN records

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.


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

https://reviews.llvm.org/D66633



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

[Lldb-commits] [PATCH] D66633: Breakpad: Add support for parsing STACK WIN records

[Mark Mentovai via Phabricator via lldb-commits]

markmentovai accepted this revision.
markmentovai added a comment.

LGTM too.


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

https://reviews.llvm.org/D66633



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

[Lldb-commits] [PATCH] D66249: [JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline

[Med Ismail Bennani via Phabricator via lldb-commits]

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

In D66249#1642316 , @labath wrote:

> A bunch more comments from me. So far, I've only tried to highlight the most 
> obvious problems or style issues.
>
>   It's not clear to me whether this is still prototype code or not... If it 
> isn't, I'll have a bunch more.. :)


This is prototype code that I'm trying to upstream to build upon.

From the comments I already gathered, I started thinking of a better 
implementation of this feature (maybe v2).
There are still some parts that need do be done, such as resolving the 
variables on optimized code or patching dynamically the copied instructions.

But, I'd like to have a baseline that is upstream to start working on these 
parts.

**All feedback is welcome !** :)




Comment at: lldb/include/lldb/Breakpoint/BreakpointSite.h:54-56
+   static bool classof(const BreakpointSite *bp_site) {
+ return bp_site->getKind() == eKindBreakpointSite;
+   }

labath wrote:
> mib wrote:
> > labath wrote:
> > > This is weird. So, in OO terminology, `BreakpointInjectedSite` "is-a" 
> > > `BreakpointSite`, but if you ask 
> > > `llvm::isa(injected_site)`, it will return false?
> > No, it returns **true**.
> Then does `llvm::isa(injected_site)` return false? 
> They can't both return true, given the current implementations (but they 
> should, if the dynamic type of `injected_site` really is 
> `BreakpointInjectedSite`...
`llvm::isa(injected_site) ` & 
`llvm::isa(injected_site)` both return **true**.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249



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

[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointOptions.h:364
+  void SetInjectCondition(bool inject_condition) {
+m_inject_condition = inject_condition;
+m_set_flags.Set(eInjectCondition);

JDevlieghere wrote:
> Any reason this should be in the header?
Most of the other options setters are defined in the header the same way ...
I just kept the same style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66248



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

[Lldb-commits] [PATCH] D66248: [JIT][Command] Add "inject-condition" flag to conditional breakpoints

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 216947.
mib added a comment.

Rename BreakpointOption name to "InjectCondition"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66248

Files:
  lldb/include/lldb/API/SBBreakpoint.h
  lldb/include/lldb/API/SBBreakpointLocation.h
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/BreakpointLocation.h
  lldb/include/lldb/Breakpoint/BreakpointOptions.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/lldb-enumerations.h
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/Makefile
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/main.c
  lldb/scripts/interface/SBBreakpoint.i
  lldb/scripts/interface/SBBreakpointLocation.i
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointLocation.cpp
  lldb/source/Breakpoint/BreakpointOptions.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/Options.td

Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -87,6 +87,9 @@
 Arg<"Command">,
 Desc<"A command to run when the breakpoint is hit, can be provided more "
 "than once, the commands will get run in order left to right.">;
+  def breakpoint_modify_inject_condition : Option<"inject-condition", "I">,
+  Desc<"The breakpoint injects the condition expression into the process "
+  "machine code. Enables Fast Conditional Breakpoints.">;
 }
 
 let Command = "breakpoint dummy" in {
Index: lldb/source/Commands/CommandObjectBreakpoint.cpp
===
--- lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -102,6 +102,13 @@
 m_bp_opts.SetIgnoreCount(ignore_count);
 }
 break;
+case 'I': {
+  if (!m_bp_opts.IsOptionSet(BreakpointOptions::eCondition))
+error.SetErrorString("inject-condition option only available for "
+ "conditional breakpoints");
+  else
+m_bp_opts.SetInjectCondition(true);
+} break;
 case 'o': {
   bool value, success;
   value = OptionArgParser::ToBoolean(option_arg, false, &success);
Index: lldb/source/Breakpoint/BreakpointOptions.cpp
===
--- lldb/source/Breakpoint/BreakpointOptions.cpp
+++ lldb/source/Breakpoint/BreakpointOptions.cpp
@@ -109,8 +109,8 @@
 
 const char *BreakpointOptions::g_option_names[(
 size_t)BreakpointOptions::OptionNames::LastOptionName]{
-"ConditionText", "IgnoreCount", 
-"EnabledState", "OneShotState", "AutoContinue"};
+"ConditionText", "IgnoreCount",  "EnabledState",
+"OneShotState",  "AutoContinue", "InjectCondition"};
 
 bool BreakpointOptions::NullCallback(void *baton,
  StoppointCallbackContext *context,
@@ -124,25 +124,23 @@
 : m_callback(BreakpointOptions::NullCallback), m_callback_baton_sp(),
   m_baton_is_command_baton(false), m_callback_is_synchronous(false),
   m_enabled(true), m_one_shot(false), m_ignore_count(0), m_thread_spec_up(),
-  m_condition_text(), m_condition_text_hash(0), m_auto_continue(false),
-  m_set_flags(0) {
+  m_condition_text(), m_condition_text_hash(0), m_inject_condition(false),
+  m_auto_continue(false), m_set_flags(0) {
   if (all_flags_set)
 m_set_flags.Set(~((Flags::ValueType)0));
 }
 
 BreakpointOptions::BreakpointOptions(const char *condition, bool enabled,
- int32_t ignore, bool one_shot, 
- bool auto_continue)
+ int32_t ignore, bool one_shot,
+ bool auto_continue, bool inject_condition)
 : m_callback(nullptr), m_baton_is_command_baton(false),
   m_callback_is_synchronous(false), m_enabled(enabled),
-  m_one_shot(one_shot), m_ignore_count(ignore),
-  m_condition_text_hash(0), m_auto_continue(auto_continue)
-{
-m_set_flags.Set(eEnabled | eIgnoreCount | eOneShot 
-   | eAutoContinue);
-if (condition && *condition != '\0') {
-  SetCondition(condition);
-}
+  m_one_shot(one_shot), m_ignore_count(ignore), m_condition_text_hash(0),
+  m_inject_condition(inject_condition), m_auto_continue(auto_continue) {
+  m_set_flags.Set(eEnabled | eIgnoreCount | eOneShot | eAutoContinue);
+  if (condition && *condition != '\0') {
+SetCondition(condition);
+  }
 }
 
 // BreakpointOptions copy constr

[Lldb-commits] [lldb] r369814 - Upstream support for macCatalyst Mach-O binaries.

[Adrian Prantl via lldb-commits]

Author: adrian
Date: Fri Aug 23 14:28:14 2019
New Revision: 369814

URL: http://llvm.org/viewvc/llvm-project?rev=369814&view=rev
Log:
Upstream support for macCatalyst Mach-O binaries.

On macOS one Mach-O slice can contain multiple load commands: One load
command for being loaded into a macOS process and one load command for
being loaded into a macCatalyst process. This patch adds support for
the new load command and makes sure ObjectFileMachO returns the
Architecture that matches the Module.

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

Modified:
lldb/trunk/lit/Modules/MachO/lc_build_version.yaml
lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Modified: lldb/trunk/lit/Modules/MachO/lc_build_version.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/MachO/lc_build_version.yaml?rev=369814&r1=369813&r2=369814&view=diff
==
--- lldb/trunk/lit/Modules/MachO/lc_build_version.yaml (original)
+++ lldb/trunk/lit/Modules/MachO/lc_build_version.yaml Fri Aug 23 14:28:14 2019
@@ -2,14 +2,15 @@
 # RUN: lldb-test symbols %t.out | FileCheck %s
 # Test that the deployment target is parsed from the load commands.
 # CHECK: x86_64-apple-macosx10.14.0
+# CHECK: x86_64-apple-ios12.0.0-macabi
 --- !mach-o
 FileHeader:  
   magic:   0xFEEDFACF
   cputype: 0x0107
   cpusubtype:  0x8003
   filetype:0x0002
-  ncmds:   14
-  sizeofcmds:  744
+  ncmds:   15
+  sizeofcmds:  776
   flags:   0x00200085
   reserved:0x
 LoadCommands:
@@ -150,6 +151,15 @@ LoadCommands:
 cmdsize: 16
 dataoff: 4152
 datasize:0
+  - cmd: LC_BUILD_VERSION
+cmdsize: 32
+platform:6
+minos:   786432
+sdk: 786432
+ntools:  1
+Tools:
+  - tool:3
+version: 26738944
 LinkEditData:
   ExportTrie:  
 TerminalSize:0

Modified: lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp?rev=369814&r1=369813&r2=369814&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp Fri Aug 23 
14:28:14 2019
@@ -913,16 +913,11 @@ size_t ObjectFileMachO::GetModuleSpecifi
 data_offset = MachHeaderSizeFromMagic(header.magic);
   }
   if (data_sp) {
-ModuleSpec spec;
-spec.GetFileSpec() = file;
-spec.SetObjectOffset(file_offset);
-spec.SetObjectSize(length);
-
-spec.GetArchitecture() = GetArchitecture(header, data, data_offset);
-if (spec.GetArchitecture().IsValid()) {
-  spec.GetUUID() = GetUUID(header, data, data_offset);
-  specs.Append(spec);
-}
+ModuleSpec base_spec;
+base_spec.GetFileSpec() = file;
+base_spec.SetObjectOffset(file_offset);
+base_spec.SetObjectSize(length);
+GetAllArchSpecs(header, data, data_offset, base_spec, specs);
   }
 }
   }
@@ -1104,11 +1099,19 @@ bool ObjectFileMachO::ParseHeader() {
 if (can_parse) {
   m_data.GetU32(&offset, &m_header.cputype, 6);
 
-  if (ArchSpec mach_arch = GetArchitecture()) {
+  ModuleSpecList all_specs;
+  ModuleSpec base_spec;
+  GetAllArchSpecs(m_header, m_data, 
MachHeaderSizeFromMagic(m_header.magic),
+  base_spec, all_specs);
+
+  for (unsigned i = 0, e = all_specs.GetSize(); i != e; ++i) {
+ArchSpec mach_arch =
+all_specs.GetModuleSpecRefAtIndex(i).GetArchitecture();
+
 // Check if the module has a required architecture
 const ArchSpec &module_arch = module_sp->GetArchitecture();
 if (module_arch.IsValid() && !module_arch.IsCompatibleMatch(mach_arch))
-  return false;
+  continue;
 
 if (SetModulesArchitecture(mach_arch)) {
   const size_t header_and_lc_size =
@@ -1123,7 +1126,7 @@ bool ObjectFileMachO::ParseHeader() {
   // Read in all only the load command data from the file on disk
   data_sp = MapFileData(m_file, header_and_lc_size, m_file_offset);
   if (data_sp->GetByteSize() != header_and_lc_size)
-return false;
+continue;
 }
 if (data_sp)
   m_data.SetData(data_sp);
@@ -1131,6 +1134,8 @@ bool ObjectFileMachO::ParseHeader() {
 }
 return true;
   }
+  // None found.
+  return false;
 } else {
   memset(&m_header, 0, sizeof(struct mach_header));
 }
@@ -4831,11 +4836,22 @@ void ObjectFileMachO

[Lldb-commits] [lldb] r369821 - Skip tail call frame tests when dwarf_version < 4

[Vedant Kumar via lldb-commits]

Author: vedantk
Date: Fri Aug 23 15:28:46 2019
New Revision: 369821

URL: http://llvm.org/viewvc/llvm-project?rev=369821&view=rev
Log:
Skip tail call frame tests when dwarf_version < 4

rdar://problem/54656572

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq1/TestAmbiguousTailCallSeq1.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/TestDisambiguatePathsToCommonSink.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_tail_call_seq/TestDisambiguateTailCallSeq.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/inlining_and_tail_calls/TestInliningAndTailCalls.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/thread_step_out_message/TestArtificialFrameStepOutMessage.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/unambiguous_sequence/TestUnambiguousTailCalls.py
lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq1/TestAmbiguousTailCallSeq1.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq1/TestAmbiguousTailCallSeq1.py?rev=369821&r1=369820&r2=369821&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq1/TestAmbiguousTailCallSeq1.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq1/TestAmbiguousTailCallSeq1.py
 Fri Aug 23 15:28:46 2019
@@ -2,4 +2,5 @@ from lldbsuite.test import lldbinline
 from lldbsuite.test import decorators
 
 lldbinline.MakeInlineTest(__file__, globals(),
-[decorators.skipUnlessHasCallSiteInfo])
+[decorators.skipUnlessHasCallSiteInfo,
+ decorators.skipIf(dwarf_version=['<', '4'])])

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py?rev=369821&r1=369820&r2=369821&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py
 Fri Aug 23 15:28:46 2019
@@ -2,4 +2,5 @@ from lldbsuite.test import lldbinline
 from lldbsuite.test import decorators
 
 lldbinline.MakeInlineTest(__file__, globals(),
-[decorators.skipUnlessHasCallSiteInfo])
+[decorators.skipUnlessHasCallSiteInfo,
+ decorators.skipIf(dwarf_version=['<', '4'])])

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py?rev=369821&r1=369820&r2=369821&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_call_site/TestDisambiguateCallSite.py
 Fri Aug 23 15:28:46 2019
@@ -2,4 +2,5 @@ from lldbsuite.test import lldbinline
 from lldbsuite.test import decorators
 
 lldbinline.MakeInlineTest(__file__, globals(),
-[decorators.skipUnlessHasCallSiteInfo])
+[decorators.skipUnlessHasCallSiteInfo,
+ decorators.skipIf(dwarf_version=['<', '4'])])

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/TestDisambiguatePathsToCommonSink.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/TestDisambiguatePathsToCommonSink.py?rev=369821&r1=369820&r2=369821&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/tail_call_f

[Lldb-commits] [lldb] r369827 - [NFC] Fix comments and formatting.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Fri Aug 23 16:56:19 2019
New Revision: 369827

URL: http://llvm.org/viewvc/llvm-project?rev=369827&view=rev
Log:
[NFC] Fix comments and formatting.

Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h

Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp?rev=369827&r1=369826&r2=369827&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp Fri Aug 23 
16:56:19 2019
@@ -125,7 +125,7 @@ DWARFMappedHash::Prologue::Prologue(dw_o
 : die_base_offset(_die_base_offset), atoms(), atom_mask(0),
   min_hash_data_byte_size(0), hash_data_has_fixed_byte_size(true) {
   // Define an array of DIE offsets by first defining an array, and then define
-  // the atom type for the array, in this case we have an array of DIE offsets
+  // the atom type for the array, in this case we have an array of DIE offsets.
   AppendAtom(eAtomTypeDIEOffset, DW_FORM_data4);
 }
 
@@ -208,9 +208,10 @@ DWARFMappedHash::Prologue::Read(const ll
 
   const uint32_t atom_count = data.GetU32(&offset);
   if (atom_count == 0x00060003u) {
-// Old format, deal with contents of old pre-release format
-while (data.GetU32(&offset))
+// Old format, deal with contents of old pre-release format.
+while (data.GetU32(&offset)) {
   /* do nothing */;
+}
 
 // Hardcode to the only known value for now.
 AppendAtom(eAtomTypeDIEOffset, DW_FORM_data4);
@@ -226,7 +227,7 @@ DWARFMappedHash::Prologue::Read(const ll
 
 size_t DWARFMappedHash::Prologue::GetByteSize() const {
   // Add an extra count to the atoms size for the zero termination Atom that
-  // gets written to disk
+  // gets written to disk.
   return sizeof(die_base_offset) + sizeof(uint32_t) +
  atoms.size() * sizeof(Atom);
 }
@@ -286,7 +287,7 @@ bool DWARFMappedHash::Header::Read(const
   break;
 
 default:
-  // We can always skip atoms we don't know about
+  // We can always skip atoms we don't know about.
   break;
 }
   }
@@ -308,8 +309,8 @@ DWARFMappedHash::MemoryTable::GetStringF
 bool DWARFMappedHash::MemoryTable::ReadHashData(uint32_t hash_data_offset,
 HashData &hash_data) const {
   lldb::offset_t offset = hash_data_offset;
-  offset += 4; // Skip string table offset that contains offset of hash name in
-   // .debug_str
+  // Skip string table offset that contains offset of hash name in .debug_str.
+  offset += 4;
   const uint32_t count = m_data.GetU32(&offset);
   if (count > 0) {
 hash_data.resize(count);
@@ -335,7 +336,7 @@ DWARFMappedHash::MemoryTable::GetHashDat
 return eResultEndOfHashData;
 
   // There definitely should be a string for this string offset, if there
-  // isn't, there is something wrong, return and error
+  // isn't, there is something wrong, return and error.
   const char *strp_cstr = m_string_table.PeekCStr(pair.key);
   if (strp_cstr == nullptr) {
 *hash_data_offset_ptr = UINT32_MAX;
@@ -345,9 +346,8 @@ DWARFMappedHash::MemoryTable::GetHashDat
   const uint32_t count = m_data.GetU32(hash_data_offset_ptr);
   const size_t min_total_hash_data_size =
   count * m_header.header_data.GetMinimumHashDataByteSize();
-  if (count > 0 &&
-  m_data.ValidOffsetForDataOfSize(*hash_data_offset_ptr,
-  min_total_hash_data_size)) {
+  if (count > 0 && m_data.ValidOffsetForDataOfSize(*hash_data_offset_ptr,
+   min_total_hash_data_size)) {
 // We have at least one HashData entry, and we have enough data to parse at
 // least "count" HashData entries.
 
@@ -370,21 +370,22 @@ DWARFMappedHash::MemoryTable::GetHashDat
   if (match)
 pair.value.push_back(die_info);
 } else {
-  // Something went wrong while reading the data
+  // Something went wrong while reading the data.
   *hash_data_offset_ptr = UINT32_MAX;
   return eResultError;
 }
   }
 }
 // Return the correct response depending on if the string matched or not...
-if (match)
-  return eResultKeyMatch; // The key (cstring) matches and we have lookup
-  // results!
-else
-  return eResultKeyMismatch; // The key doesn't match, this function will
- // get called
-// again for the next key/value or the key terminator which in our case is
-// a zero .debug_str offset.
+if (match) {
+  // The key (cstring) matches and we have lookup results!
+  return eResultKeyMatch;
+} else {
+  // The key doesn't match