[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't have any objections to the contents of this patch per se.  But I wonder 
if having to do all this work to separate the uses of Args from the options 
parser so we don't drag it in in some low level uses doesn't rather mean the 
Args class was not appropriate for use at that level, when what they really 
meant was just a collection of strings.  For instance, we use the Args class to 
hold arguments to pass to running processes.  That's convenient when we parse 
them from commands into run-args, but hardly essential to the function of 
passing a collection of strings to posix_spawnp or its like.

I haven't gone through and audited all the uses you are trying to separate out 
from the options part of Args, but if possible it would be cleaner to have the 
class that's supposed to be cheek to jowl with the command line parsing, and 
another to store a list of strings.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Getting watchpoints to propagate to threads that are created after the 
watchpoint has been set is one of the trickier parts of watchpoint propagation. 
 On macOS, (where we don't get new thread creation notification) we rely on the 
kernel to propagate  the control register settings to the thread contexts of 
newly created threads - a feature nobody but the debugger uses so we need to 
make sure it doesn't regress.  On systems that don't have kernel cooperation 
you have to make sure that you catch new thread events and by hand set the 
control registers, again something we need to make sure we keep doing.  If 
there's no other test that this works, then we should keep testing that in this 
test.  It is the most fragile part of the interaction between watchpoints and 
threads.

OTOH making this a debug info independent test seems correct.


https://reviews.llvm.org/D43857



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

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek created this revision.
kubamracek added reviewers: jingham, jasonmolenda.

This patch teaches LLDB about more fields on NSException Obj-C objects, 
specifically we can now retrieve the "name" and "reason" of an NSException. The 
goal is to eventually be able to have SB API that can provide details about the 
currently thrown/caught/processed exception.


https://reviews.llvm.org/D43884

Files:
  packages/Python/lldbsuite/test/lang/objc/exceptions/Makefile
  packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
  packages/Python/lldbsuite/test/lang/objc/exceptions/main.m
  source/Plugins/Language/ObjC/NSArray.cpp
  source/Plugins/Language/ObjC/NSException.cpp

Index: source/Plugins/Language/ObjC/NSException.cpp
===
--- source/Plugins/Language/ObjC/NSException.cpp
+++ source/Plugins/Language/ObjC/NSException.cpp
@@ -33,56 +33,75 @@
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
-bool lldb_private::formatters::NSException_SummaryProvider(
-ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
+static bool ExtractFields(ValueObject &valobj, ValueObjectSP *name_sp,
+  ValueObjectSP *reason_sp, ValueObjectSP *userinfo_sp,
+  ValueObjectSP *reserved_sp) {
   ProcessSP process_sp(valobj.GetProcessSP());
-  if (!process_sp)
-return false;
+  if (!process_sp) return false;
 
-  lldb::addr_t ptr_value = LLDB_INVALID_ADDRESS;
+  lldb::addr_t ptr = LLDB_INVALID_ADDRESS;
 
   CompilerType valobj_type(valobj.GetCompilerType());
   Flags type_flags(valobj_type.GetTypeInfo());
   if (type_flags.AllClear(eTypeHasValue)) {
 if (valobj.IsBaseClass() && valobj.GetParent())
-  ptr_value = valobj.GetParent()->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
+  ptr = valobj.GetParent()->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
   } else
-ptr_value = valobj.GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
+ptr = valobj.GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
 
-  if (ptr_value == LLDB_INVALID_ADDRESS)
+  if (ptr == LLDB_INVALID_ADDRESS)
 return false;
   size_t ptr_size = process_sp->GetAddressByteSize();
-  lldb::addr_t name_location = ptr_value + 1 * ptr_size;
-  lldb::addr_t reason_location = ptr_value + 2 * ptr_size;
 
   Status error;
-  lldb::addr_t name = process_sp->ReadPointerFromMemory(name_location, error);
-  if (error.Fail() || name == LLDB_INVALID_ADDRESS)
-return false;
-
-  lldb::addr_t reason =
-  process_sp->ReadPointerFromMemory(reason_location, error);
-  if (error.Fail() || reason == LLDB_INVALID_ADDRESS)
-return false;
+  auto name = process_sp->ReadPointerFromMemory(ptr + 1 * ptr_size, error);
+  if (error.Fail() || name == LLDB_INVALID_ADDRESS) return false;
+  auto reason = process_sp->ReadPointerFromMemory(ptr + 2 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto userinfo = process_sp->ReadPointerFromMemory(ptr + 3 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto reserved = process_sp->ReadPointerFromMemory(ptr + 4 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
 
   InferiorSizedWord name_isw(name, *process_sp);
   InferiorSizedWord reason_isw(reason, *process_sp);
+  InferiorSizedWord userinfo_isw(userinfo, *process_sp);
+  InferiorSizedWord reserved_isw(reserved, *process_sp);
 
   CompilerType voidstar = process_sp->GetTarget()
-  .GetScratchClangASTContext()
-  ->GetBasicType(lldb::eBasicTypeVoid)
-  .GetPointerType();
-
-  ValueObjectSP name_sp = ValueObject::CreateValueObjectFromData(
-  "name_str", name_isw.GetAsData(process_sp->GetByteOrder()),
-  valobj.GetExecutionContextRef(), voidstar);
-  ValueObjectSP reason_sp = ValueObject::CreateValueObjectFromData(
-  "reason_str", reason_isw.GetAsData(process_sp->GetByteOrder()),
-  valobj.GetExecutionContextRef(), voidstar);
-
-  if (!name_sp || !reason_sp)
+  .GetScratchClangASTContext()
+  ->GetBasicType(lldb::eBasicTypeVoid)
+  .GetPointerType();
+
+  if (name_sp)
+*name_sp = ValueObject::CreateValueObjectFromData(
+"name", name_isw.GetAsData(process_sp->GetByteOrder()),
+valobj.GetExecutionContextRef(), voidstar);
+  if (reason_sp)
+*reason_sp = ValueObject::CreateValueObjectFromData(
+"reason", reason_isw.GetAsData(process_sp->GetByteOrder()),
+valobj.GetExecutionContextRef(), voidstar);
+  if (userinfo_sp)
+*userinfo_sp = ValueObject::CreateValueObjectFromData(
+"userInfo", userinfo_isw.GetAsData(process_sp->GetByteOrder()),
+valobj.GetExecutionContextRef(), voidstar);
+  if (reserved_sp)
+*reserved_sp = ValueObject::CreateValueObjectFromData(
+"reserved", reserved_isw.GetAsData(process_sp->GetByteOrder()),
+valobj

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D43837#1022303, @jingham wrote:

> I don't have any objections to the contents of this patch per se.  But I 
> wonder if having to do all this work to separate the uses of Args from the 
> options parser so we don't drag it in in some low level uses doesn't rather 
> mean the Args class was not appropriate for use at that level, when what they 
> really meant was just a collection of strings.  For instance, we use the Args 
> class to hold arguments to pass to running processes.  That's convenient when 
> we parse them from commands into run-args, but hardly essential to the 
> function of passing a collection of strings to posix_spawnp or its like.
>
> I haven't gone through and audited all the uses you are trying to separate 
> out from the options part of Args, but if possible it would be cleaner to 
> have the class that's supposed to be cheek to jowl with the command line 
> parsing, and another to store a list of strings.


I was thinking about that as well, but eventually I chose this approach. The 
reason for that is that there is already functionality in the Args class that 
is useful for posix_spawn and friends, and that's the ability to turn itself 
into an argv vector. So, the new class couldn't just be a `vector`, but 
it would need some additional smartness. So after implementing that, I think I 
would need to find a way to rip that code out of the Args class (maybe by 
making the new class a member of Args). So the end result may end up being very 
similar, we would just reach in it a different way.

The other reason I want to move this out is the single responsibility 
principle. Right now, I can identify about 4 responsibilities of the Args class:

- being a representation of a list of arguments (I put it as a separate item 
because of the argv requirement)
- parsing a string into a list of arguments
- parsing a list of arguments into options
- parsing a string into random other objects (via various static functions)

Now we probably don't want to go all out and split this into 4 classes, but I 
figured the first two items are enough for a single class (one could even argue 
the two items are actually a single thing). I think parsing a string into args 
and parsing args into options are two sufficiently complicated algorithms that 
makes sense to keep them separate.

The thing I'm not 100% clear on is whether the Options class is the best home 
for these functions. The reason I chose this at the end (instead of e,g, 
putting it in a new class) was because I saw this pattern in the CommandObject:

  options->NotifyOptionParsingStarting(&exe_ctx);
  ...
  options->Parse(...);
  ...
  options->NotifyOptionParsingFinished(&exe_ctx);

It seemed to be that having these functions in the Options class would open up 
possibilities for simplifying the Options interface by folding the `Notify` 
functions into the `Parse` call.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43886: [lldb] Add GetCurrentException and GetCurrentExceptionBacktrace APIs to SBThread

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek created this revision.
kubamracek added reviewers: jasonmolenda, jingham.
Herald added a subscriber: mgorny.

This adds new APIs and commands to deal with exceptions (mostly Obj-C 
exceptions):

- SBThread and Thread get GetCurrentException API, which returns an 
SBValue/ValueObjectSP with the current exception for a thread. "Current" means 
an exception that is currently being thrown, caught or otherwise processed. In 
this patch, we only know about the exception when in objc_exception_throw, but 
subsequent patches will expand this.
- SBThread and Thread get GetCurrentExceptionBacktrace, which return an 
SBThread/ThreadSP containing a historical thread backtrace retrieved from the 
exception object. Currently unimplemented, subsequent patches will implement 
this.

To be able to extract the exception when inside objc_exception_throw, this 
patch introduces a concept of "frame recognizer" and "recognized frame". This 
should be an extensible mechanism that hardcodes knowledge about special frames 
(like objc_exception_throw) where we know the ABI, arguments or other special 
properties of that frame, even without source code. In this patch, we only 
handle objc_exception_throw frame.


https://reviews.llvm.org/D43886

Files:
  include/lldb/API/SBThread.h
  include/lldb/Target/FrameRecognizer.h
  include/lldb/Target/Platform.h
  include/lldb/Target/StackFrame.h
  include/lldb/Target/Thread.h
  lldb.xcodeproj/project.pbxproj
  packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py
  source/API/SBThread.cpp
  source/Commands/CommandObjectThread.cpp
  source/Plugins/Platform/MacOSX/CMakeLists.txt
  source/Plugins/Platform/MacOSX/DarwinFrameRecognizer.cpp
  source/Plugins/Platform/MacOSX/DarwinFrameRecognizer.h
  source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  source/Plugins/Platform/MacOSX/PlatformDarwin.h
  source/Target/StackFrame.cpp
  source/Target/Thread.cpp

Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -2209,3 +2209,20 @@
   }
   return error;
 }
+
+ValueObjectSP Thread::GetCurrentException() {
+  StackFrameSP frame_sp(GetStackFrameAtIndex(0));
+  if (!frame_sp) return ValueObjectSP();
+
+  RecognizedStackFrameSP recognized_frame(frame_sp->GetRecognizedFrame());
+  if (!recognized_frame) return ValueObjectSP();
+
+  if (!recognized_frame->IsThrowingObjCException()) return ValueObjectSP();
+
+  return recognized_frame->GetObjCExceptionObject();
+}
+
+ThreadSP Thread::GetCurrentExceptionBacktrace() {
+  // TODO
+  return ThreadSP();
+}
Index: source/Target/StackFrame.cpp
===
--- source/Target/StackFrame.cpp
+++ source/Target/StackFrame.cpp
@@ -1918,3 +1918,14 @@
   }
   return true;
 }
+
+RecognizedStackFrameSP StackFrame::GetRecognizedFrame() {
+  if (!m_recognized_frame) {
+lldb_private::FrameRecognizerSP recognizer_sp =
+CalculateTarget()->GetPlatform()->GetFrameRecognizer();
+if (recognizer_sp) {
+  m_recognized_frame = recognizer_sp->RecognizeFrame(CalculateStackFrame());
+}
+  }
+  return m_recognized_frame;
+}
Index: source/Plugins/Platform/MacOSX/PlatformDarwin.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwin.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwin.h
@@ -85,6 +85,8 @@
   static std::tuple
   ParseVersionBuildDir(llvm::StringRef str);
 
+  lldb_private::FrameRecognizerSP GetFrameRecognizer() override;
+
 protected:
   void ReadLibdispatchOffsetsAddress(lldb_private::Process *process);
 
@@ -137,6 +139,7 @@
 
   std::string m_developer_directory;
 
+  lldb_private::FrameRecognizerSP m_frame_recognizer;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformDarwin);
Index: source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -42,6 +42,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
+#include "Plugins/Platform/MacOSX/DarwinFrameRecognizer.h"
 
 #if defined(__APPLE__)
 #include  // for TARGET_OS_TV, TARGET_OS_WATCH
@@ -1825,3 +1826,9 @@
   }
   return Status();
 }
+
+FrameRecognizerSP PlatformDarwin::GetFrameRecognizer() {
+  if (!m_frame_recognizer)
+m_frame_recognizer.reset(new DarwinFrameRecognizer());
+  return m_frame_recognizer;
+}
Index: source/Plugins/Platform/MacOSX/DarwinFrameRecognizer.h
===
--- /dev/null
+++ source/Plugins/Platform/MacOSX/DarwinFrameRecognizer.h
@@ -0,0 +1,27 @@
+//===-- DarwinFrameRecognizer.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illi

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

That's a good point. I think that at least TestConcurrentEvents do it that way, 
but they are testing a different thing. I think it makes sense to test both 
things here actually. I'll create one test which sets the watchpoint before 
thread creation and one after.

BTW, do you think there's any value in having three threads running around 
here? I think the test would be more obvious (and deterministic) if we had just 
one thread tripping the watchpoint.


https://reviews.llvm.org/D43857



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I still worry a bit because there's another unstated responsibility for Args 
which is that even though it is going to get used at a very high level in lldb 
it has to NOT depend on anything you don't want lldb-server to depend on.  That 
seems like a more slippery responsibility, and one that's worth stating 
explicitly by making the part of Args that gets used in lldb-server its own 
class.  But on the implementation wins principle, as long as this doesn't worry 
you, I'm content.

BTW, most the string -> int/address/whatever conversion functions don't belong 
in Args at all.  It makes these fairly useful functions hard to find, and we 
should have some string conversion utility to hold all these convenience 
functions.  There are a few (like picking an enum option string from a set of 
enums) that belong more properly in options.  But most of them have no real 
relation to Args.  But that's an orthogonal issue.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: include/lldb/Interpreter/Options.h:123-126
+  llvm::Expected Parse(const Args &args,
+ ExecutionContext *execution_context,
+ lldb::PlatformSP platform_sp,
+ bool require_validation);

It appears that all of these could be static functions.  Can we do that?


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Thanks!

While it seems possible somebody could write code that only sets the control 
registers on one thread and then gets tired and goes out for a beer, that seems 
like a really unlikely error.  I don't think it's worth complicating the test 
to catch that possibility.


https://reviews.llvm.org/D43857



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D43837#1022430, @jingham wrote:

> I still worry a bit because there's another unstated responsibility for Args 
> which is that even though it is going to get used at a very high level in 
> lldb it has to NOT depend on anything you don't want lldb-server to depend on.


This will be enforced by moving the class (once I get rid of the extra static 
functions like you mentioned) into the Utility module. Nothing in the Utility 
module can depend on anything outside of that module.




Comment at: include/lldb/Interpreter/Options.h:123-126
+  llvm::Expected Parse(const Args &args,
+ ExecutionContext *execution_context,
+ lldb::PlatformSP platform_sp,
+ bool require_validation);

zturner wrote:
> It appears that all of these could be static functions.  Can we do that?
They can't be. All of them access the `this` object. If you look at the 
original functions, they were taking an `Options&` as an argument and `Args` as 
`this`. These have that inverted.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

It would be nice if we could eventually get rid of the need to pass in a 
platform and execution context here, but that's work for another day.




Comment at: include/lldb/Interpreter/Options.h:123-126
+  llvm::Expected Parse(const Args &args,
+ ExecutionContext *execution_context,
+ lldb::PlatformSP platform_sp,
+ bool require_validation);

labath wrote:
> zturner wrote:
> > It appears that all of these could be static functions.  Can we do that?
> They can't be. All of them access the `this` object. If you look at the 
> original functions, they were taking an `Options&` as an argument and `Args` 
> as `this`. These have that inverted.
I originally searched for `m_` and didn't find anything so assumed they could 
be static.  But it looks like they are calling member functions, which is why I 
didn't see it.  It's too bad it can't even be `const`, given that it returns a 
copy of the args.  Seems like an awkward interface, maybe future cleanup can 
try to tackle that though.  Anyway, ignore my comment.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: include/lldb/Interpreter/Options.h:123-126
+  llvm::Expected Parse(const Args &args,
+ ExecutionContext *execution_context,
+ lldb::PlatformSP platform_sp,
+ bool require_validation);

zturner wrote:
> labath wrote:
> > zturner wrote:
> > > It appears that all of these could be static functions.  Can we do that?
> > They can't be. All of them access the `this` object. If you look at the 
> > original functions, they were taking an `Options&` as an argument and 
> > `Args` as `this`. These have that inverted.
> I originally searched for `m_` and didn't find anything so assumed they could 
> be static.  But it looks like they are calling member functions, which is why 
> I didn't see it.  It's too bad it can't even be `const`, given that it 
> returns a copy of the args.  Seems like an awkward interface, maybe future 
> cleanup can try to tackle that though.  Anyway, ignore my comment.
The returning of args is kind of a side effect of the function. The return 
value contains the arguments that could not be parsed into options (more 
precisely, the positional arguments). The main effect of the function is that 
it populates the Options object with the options that have been parsed. (I 
should probably add that to the comment).


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814
 
   value = value - header->p_vaddr;
   found_offset = true;

owenpshaw wrote:
> labath wrote:
> > Ok so the issue is that here we use the virtual address to compute the load 
> > bias, but at line 830 we add the bias to the physical address. This breaks 
> > as soon as these two addresses start to differ.
> > 
> > Changing this to use the physical address as well fixes things, but as I 
> > said before, I'm not sure we should be using physical addresses here.
> I don't know if there's another use case besides flash load that should 
> definitely use the physical address, so I can't be much help answering that 
> question.  I was mainly relying on tests to catch any issue caused by using 
> physical addresses.
> 
> Could the value_is_offset flag be a tell here?  Looks like that load bias is 
> only calculated if value_is_offset == false.  Would it make sense to always 
> use virtual address in such a case?  It wouldn't affect the "target modules 
> load" case, which always sets value_is_offset to true.
> I don't know if there's another use case besides flash load that should 
> definitely use the physical address, so I can't be much help answering that 
> question. I was mainly relying on tests to catch any issue caused by using 
> physical addresses.

Yeah, unfortunately we don't have tests which would catch borderline conditions 
like that, and as you've said, most elf files have these the same, so you 
couldn't have noticed that. The only reason I this is that one of the android 
devices we test on has a kernel whose vdso has these different. 

The good news is that with your test suite, we should be able to write a test 
for it, when we figure out what the behavior should be here.

> Could the value_is_offset flag be a tell here? Looks like that load bias is 
> only calculated if value_is_offset == false. Would it make sense to always 
> use virtual address in such a case? It wouldn't affect the "target modules 
> load" case, which always sets value_is_offset to true.

The is_offset flag is orthogonal to this. It tells you whether the value 
represents a bias or an absolute value. When we read the module list from the 
linker rendezvous structure, we get a load bias; when we read it from 
/proc/pid/maps, we get an absolute value. It does *not* tell you what the value 
is relative to. So while doing that like you suggest would fix things, I don't 
think it's the right thing to do.

In fact, the more I think about this, the more I'm convinced using physical 
addresses is not the right solution here.  The main user of this function is 
the dynamic linker plugin, which uses it to notify the debugger about modules 
that have already been loaded into the process. That definitely sounds like a 
virtual address thing. Also, if you look at the documentation of the "target 
modules load --slide", it explicitly states that the slide should be relative 
to the virtual address.

And I'm not even completely clear about your case. I understand you write the 
module to the physical address, but what happens when you actually go around to 
debugging it? Is it still there or does it get copied/mapped/whatever to the 
virtual address?

So it seems to me the physical addresses should really be specific to the 
"target modules load --load" case. I've never used that command so I don't know 
if it can just use physical addresses unconditionally, or whether we need an 
extra option for that "target modules load --load --physical". I think I'll 
leave that decision up to you (or anyone else who has an opinion on this). But 
for the other use cases, I believe we should keep using virtual addresses.

So I think we should revert this and then split out the loading part of this 
patch from the vFlash thingy so we can focus on the virtual/physical address 
situation and how to handle that.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Okay, that sounds good then.  Will you enforce the rule about the Utilities 
directory socially or by some mechanism?


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D43837#1022554, @jingham wrote:

> Okay, that sounds good then.  Will you enforce the rule about the Utilities 
> directory socially or by some mechanism?


If you mean the rule that Utility can't depend on anything else, I think it's 
enforced implicitly.  because we have a `UtilityTests` unit test that only 
links against `Utility` and nothing else.  If anything else gets brought in, 
this binary will fail to link.

That said, a while back I wrote a python script (checked in under 
`lldb/scripts`) which dumps the set of header dependencies for every project.  
It would perhaps be a useful exercise to write a lit test that enforces this by 
dumping the deps of `Utility` and ensuring that they're empty.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D43837#1022554, @jingham wrote:

> Okay, that sounds good then.  Will you enforce the rule about the Utilities 
> directory socially or by some mechanism?


You will get a link error because the Utility unittest executable will have 
missing symbols. I'm not sure how xcode builds the unit test executables; if it 
goes through cmake you will definitely get it locally; if not, at the very 
latest you will get it from one of the bots.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Pavel Labath via Phabricator via lldb-commits]

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

Add a slightly longer comment to the Parse function operation.


https://reviews.llvm.org/D43837

Files:
  include/lldb/Interpreter/Args.h
  include/lldb/Interpreter/Options.h
  packages/Python/lldbsuite/test/functionalities/completion/TestCompletion.py
  source/Commands/CommandObjectSettings.cpp
  source/Interpreter/Args.cpp
  source/Interpreter/CommandAlias.cpp
  source/Interpreter/CommandObject.cpp
  source/Interpreter/Options.cpp
  source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp

Index: source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
===
--- source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -1013,6 +1013,7 @@
 };
 
 EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS);
   // We are abusing the options data model here so that we can parse
   // options without requiring the Debugger instance.
 
@@ -1051,15 +1052,16 @@
   args.Shift();
   }
 
-  // ParseOptions calls getopt_long_only, which always skips the zero'th item in
-  // the array and starts at position 1,
-  // so we need to push a dummy value into position zero.
-  args.Unshift(llvm::StringRef("dummy_string"));
   bool require_validation = false;
-  error = args.ParseOptions(*options_sp.get(), &exe_ctx, PlatformSP(),
-require_validation);
-  if (!error.Success())
+  llvm::Expected args_or =
+  options_sp->Parse(args, &exe_ctx, PlatformSP(), require_validation);
+  if (!args_or) {
+LLDB_LOG_ERROR(
+log, args_or.takeError(),
+"Parsing plugin.structured-data.darwin-log.auto-enable-options value "
+"failed: {0}");
 return EnableOptionsSP();
+  }
 
   if (!options_sp->VerifyOptions(result))
 return EnableOptionsSP();
Index: source/Interpreter/Options.cpp
===
--- source/Interpreter/Options.cpp
+++ source/Interpreter/Options.cpp
@@ -951,3 +951,527 @@
   }
   return error;
 }
+
+// OptionParser permutes the arguments while processing them, so we create a
+// temporary array holding to avoid modification of the input arguments. The
+// options themselves are never modified, but the API expects a char * anyway,
+// hence the const_cast.
+static std::vector GetArgvForParsing(const Args &args) {
+  std::vector result;
+  // OptionParser always skips the first argument as it is based on getopt().
+  result.push_back(const_cast(""));
+  for (const Args::ArgEntry &entry : args)
+result.push_back(const_cast(entry.c_str()));
+  return result;
+}
+
+// Given a permuted argument, find it's position in the original Args vector.
+static Args::const_iterator FindOriginalIter(const char *arg,
+ const Args &original) {
+  return llvm::find_if(
+  original, [arg](const Args::ArgEntry &D) { return D.c_str() == arg; });
+}
+
+// Given a permuted argument, find it's index in the original Args vector.
+static size_t FindOriginalIndex(const char *arg, const Args &original) {
+  return std::distance(original.begin(), FindOriginalIter(arg, original));
+}
+
+// Construct a new Args object, consisting of the entries from the original
+// arguments, but in the permuted order.
+static Args ReconstituteArgsAfterParsing(llvm::ArrayRef parsed,
+ const Args &original) {
+  Args result;
+  for (const char *arg : parsed) {
+auto pos = FindOriginalIter(arg, original);
+assert(pos != original.end());
+result.AppendArgument(pos->ref, pos->quote);
+  }
+  return result;
+}
+
+static size_t FindArgumentIndexForOption(const Args &args,
+ const Option &long_option) {
+  std::string short_opt = llvm::formatv("-{0}", char(long_option.val)).str();
+  std::string long_opt =
+  llvm::formatv("--{0}", long_option.definition->long_option);
+  for (const auto &entry : llvm::enumerate(args)) {
+if (entry.value().ref.startswith(short_opt) ||
+entry.value().ref.startswith(long_opt))
+  return entry.index();
+  }
+
+  return size_t(-1);
+}
+
+llvm::Expected Options::ParseAlias(const Args &args,
+ OptionArgVector *option_arg_vector,
+ std::string &input_line) {
+  StreamString sstr;
+  int i;
+  Option *long_options = GetLongOptions();
+
+  if (long_options == nullptr) {
+return llvm::make_error("Invalid long options",
+   llvm::inconvertibleErrorCode());
+  }
+
+  for (i = 0; long_options[i].definition != nullptr; ++i) {
+if (long_options[i].flag == nullptr) {
+  sstr << (char)long_options[i].val;
+  switch (long_options[i].defin

[Lldb-commits] [PATCH] D43837: Move option parsing out of the Args class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The separate gtest directories don't get build separately the way Todd set the 
Xcode project up.  The tests get built into one combo .a file that links to 
liblldbcore.a, and then into the test binary.  So the Xcode build wouldn't see 
that failure.

Since you are building .a files not dylibs from Utility, the UtilityTest won't 
show a failure for a class that has a dependency that the tests binary doesn't 
use, so this sort of thing could creep in.  Another reason to be vigilant about 
the test coverage.

Once your whole project is done, lldb-server shouldn't build if something it 
uses from Utility uses something not in Utility (or in some other .a file that 
lldb-server depends on).  That doesn't help altogether, since macOS only builds 
the platform part of lldb-server, so the macOS builds won't check usage in the 
other parts of lldb-server.  But the bots should be good enough.


https://reviews.llvm.org/D43837



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

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Pavel Labath via Phabricator via lldb-commits]

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

This makes the inferior more simple and deterministic by using a single thread.
Now we have one test which sets the watchpoint before the thread is created and
one that sets it after.  I've also simplified the test a bit with new lldbutil
goodies.


https://reviews.llvm.org/D43857

Files:
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp

Index: packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
===
--- packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
+++ packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
@@ -7,73 +7,33 @@
 //
 //===--===//
 
-#include 
 #include 
-#include 
-#include 
 #include 
+#include "pseudo_barrier.h"
 
-std::default_random_engine g_random_engine{std::random_device{}()};
-std::uniform_int_distribution<> g_distribution{0, 300};
-
-uint32_t g_val = 0;
-
-
-uint32_t
-access_pool (bool flag = false)
-{
-static std::mutex g_access_mutex;
-g_access_mutex.lock();
-
-uint32_t old_val = g_val;
-if (flag)
-{
-printf("changing g_val to %d...\n", old_val + 1);
-g_val = old_val + 1;
-}
-
-g_access_mutex.unlock();
-return g_val;
-}
+volatile uint32_t g_val = 0;
+pseudo_barrier_t g_barrier;
 
 void
-thread_func (uint32_t thread_index)
+thread_func ()
 {
-printf ("%s (thread index = %u) starting...\n", __FUNCTION__, thread_index);
-
-uint32_t count = 0;
-uint32_t val;
-while (count++ < 15)
-{
-// random micro second sleep from zero to 3 seconds
-int usec = g_distribution(g_random_engine);
-printf ("%s (thread = %u) doing a usleep (%d)...\n", __FUNCTION__, thread_index, usec);
-std::this_thread::sleep_for(std::chrono::microseconds{usec});
-
-if (count < 7)
-val = access_pool ();
-else
-val = access_pool (true);
-
-printf ("%s (thread = %u) after usleep access_pool returns %d (count=%d)...\n", __FUNCTION__, thread_index, val, count);
-}
-printf ("%s (thread index = %u) exiting...\n", __FUNCTION__, thread_index);
+pseudo_barrier_wait(g_barrier);
+printf ("%s starting...\n", __FUNCTION__);
+for (uint32_t i = 0; i < 10; ++i )
+  g_val = i;
 }
 
 
 int main (int argc, char const *argv[])
 {
-std::thread threads[3];
+  printf("Before running the thread\n");
+pseudo_barrier_init(g_barrier, 2);
+std::thread thread(thread_func);
 
-printf ("Before turning all three threads loose...\n"); // Set break point at this line,
-// in order to set our watchpoint.
-// Create 3 threads
-for (auto &thread : threads)
-thread = std::thread{thread_func, std::distance(threads, &thread)};
+printf("After running the thread\n");
+pseudo_barrier_wait(g_barrier);
 
-// Join all of our threads
-for (auto &thread : threads)
-thread.join();
+thread.join();
 
 return 0;
 }
Index: packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
===
--- packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
+++ packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
@@ -17,51 +17,26 @@
 class WatchpointForMultipleThreadsTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+main_spec = lldb.SBFileSpec("main.cpp", False)
 
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
-def test_watchpoint_multiple_threads(self):
-"""Test that lldb watchpoint works for multiple threads."""
-self.build()
-self.setTearDownCleanup()
-self.hello_multiple_threads()
+def test_watchpoint_before_thread_start(self):
+"""Test that we can hit a watchpoint we set before starting another thread"""
+	self.do_watchpoint_test("Before running the thread")
 
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
-def test_watchpoint_multiple_threads_wp_set_and_then_delete(self):
-"""Test that lldb watchpoint works for multiple threads, and after the watchpoint is deleted, the watchpoint event should no longer fires."""
+def test_watchpoint_after_thread_start(self):
+"""Test that we can hit a watchpo

[Lldb-commits] [lldb] r326367 - Revert "[lldb] Use vFlash commands when writing to target's flash memory regions"

[Pavel Labath via lldb-commits]

Author: labath
Date: Wed Feb 28 12:42:29 2018
New Revision: 326367

URL: http://llvm.org/viewvc/llvm-project?rev=326367&view=rev
Log:
Revert "[lldb] Use vFlash commands when writing to target's flash memory 
regions"

This reverts commit r326261 as it introduces inconsistencies in the
handling of load addresses for ObjectFileELF -- some parts of the class
use physical addresses, and some use virtual. This has manifested itself
as us not being able to set the load address of the vdso "module" on
android.

Removed:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
Modified:
lldb/trunk/include/lldb/Host/XML.h
lldb/trunk/include/lldb/Target/MemoryRegionInfo.h
lldb/trunk/include/lldb/Target/Process.h
lldb/trunk/source/Host/common/XML.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/trunk/source/Symbol/ObjectFile.cpp
lldb/trunk/source/Target/Process.cpp

Modified: lldb/trunk/include/lldb/Host/XML.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/XML.h?rev=326367&r1=326366&r2=326367&view=diff
==
--- lldb/trunk/include/lldb/Host/XML.h (original)
+++ lldb/trunk/include/lldb/Host/XML.h Wed Feb 28 12:42:29 2018
@@ -82,9 +82,6 @@ public:
   llvm::StringRef GetAttributeValue(const char *name,
 const char *fail_value = nullptr) const;
 
-  bool GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
-   uint64_t fail_value = 0, int base = 0) 
const;
-
   XMLNode FindFirstChildElementWithName(const char *name) const;
 
   XMLNode GetElementForPath(const NamePath &path);

Modified: lldb/trunk/include/lldb/Target/MemoryRegionInfo.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/MemoryRegionInfo.h?rev=326367&r1=326366&r2=326367&view=diff
==
--- lldb/trunk/include/lldb/Target/MemoryRegionInfo.h (original)
+++ lldb/trunk/include/lldb/Target/MemoryRegionInfo.h Wed Feb 28 12:42:29 2018
@@ -25,7 +25,7 @@ public:
 
   MemoryRegionInfo()
   : m_range(), m_read(eDontKnow), m_write(eDontKnow), m_execute(eDontKnow),
-m_mapped(eDontKnow), m_flash(eDontKnow), m_blocksize(0) {}
+m_mapped(eDontKnow) {}
 
   ~MemoryRegionInfo() {}
 
@@ -58,14 +58,6 @@ public:
 
   void SetName(const char *name) { m_name = ConstString(name); }
 
-  OptionalBool GetFlash() const { return m_flash; }
-
-  void SetFlash(OptionalBool val) { m_flash = val; }
-
-  lldb::offset_t GetBlocksize() const { return m_blocksize; }
-
-  void SetBlocksize(lldb::offset_t blocksize) { m_blocksize = blocksize; }
-
   //--
   // Get permissions as a uint32_t that is a mask of one or more bits from
   // the lldb::Permissions
@@ -106,8 +98,6 @@ protected:
   OptionalBool m_execute;
   OptionalBool m_mapped;
   ConstString m_name;
-  OptionalBool m_flash;
-  lldb::offset_t m_blocksize;
 };
 }
 

Modified: lldb/trunk/include/lldb/Target/Process.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=326367&r1=326366&r2=326367&view=diff
==
--- lldb/trunk/include/lldb/Target/Process.h (original)
+++ lldb/trunk/include/lldb/Target/Process.h Wed Feb 28 12:42:29 2018
@@ -556,11 +556,6 @@ public:
 return GetStaticBroadcasterClass();
   }
 
-  struct WriteEntry {
-lldb::addr_t Dest;
-llvm::ArrayRef Contents;
-  };
-
 //--
 /// A notification structure that can be used by clients to listen
 /// for changes in a process's lifetime.
@@ -1955,8 +1950,6 @@ public:
 return LLDB_INVALID_ADDRESS;
   }
 
-  virtual Status WriteObjectFile(std::vector entries);
-
   //--
   /// The public interface to allocating memory in the process.
   ///

Removed: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py?rev=326366&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

Thanks for taking the time to do this!

Can you please double check some of the formatting in the python code? It looks 
like it's not indented properly.


https://reviews.llvm.org/D43857



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

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Pavel Labath via Phabricator via lldb-commits]

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

Replace all tabs in the python file and clang-format the cpp file.


https://reviews.llvm.org/D43857

Files:
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
  
packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp

Index: packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
===
--- packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
+++ packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/main.cpp
@@ -7,73 +7,29 @@
 //
 //===--===//
 
-#include 
+#include "pseudo_barrier.h"
 #include 
-#include 
-#include 
 #include 
 
-std::default_random_engine g_random_engine{std::random_device{}()};
-std::uniform_int_distribution<> g_distribution{0, 300};
+volatile uint32_t g_val = 0;
+pseudo_barrier_t g_barrier;
 
-uint32_t g_val = 0;
-
-
-uint32_t
-access_pool (bool flag = false)
-{
-static std::mutex g_access_mutex;
-g_access_mutex.lock();
-
-uint32_t old_val = g_val;
-if (flag)
-{
-printf("changing g_val to %d...\n", old_val + 1);
-g_val = old_val + 1;
-}
-
-g_access_mutex.unlock();
-return g_val;
-}
-
-void
-thread_func (uint32_t thread_index)
-{
-printf ("%s (thread index = %u) starting...\n", __FUNCTION__, thread_index);
-
-uint32_t count = 0;
-uint32_t val;
-while (count++ < 15)
-{
-// random micro second sleep from zero to 3 seconds
-int usec = g_distribution(g_random_engine);
-printf ("%s (thread = %u) doing a usleep (%d)...\n", __FUNCTION__, thread_index, usec);
-std::this_thread::sleep_for(std::chrono::microseconds{usec});
-
-if (count < 7)
-val = access_pool ();
-else
-val = access_pool (true);
-
-printf ("%s (thread = %u) after usleep access_pool returns %d (count=%d)...\n", __FUNCTION__, thread_index, val, count);
-}
-printf ("%s (thread index = %u) exiting...\n", __FUNCTION__, thread_index);
+void thread_func() {
+  pseudo_barrier_wait(g_barrier);
+  printf("%s starting...\n", __FUNCTION__);
+  for (uint32_t i = 0; i < 10; ++i)
+g_val = i;
 }
 
+int main(int argc, char const *argv[]) {
+  printf("Before running the thread\n");
+  pseudo_barrier_init(g_barrier, 2);
+  std::thread thread(thread_func);
 
-int main (int argc, char const *argv[])
-{
-std::thread threads[3];
-
-printf ("Before turning all three threads loose...\n"); // Set break point at this line,
-// in order to set our watchpoint.
-// Create 3 threads
-for (auto &thread : threads)
-thread = std::thread{thread_func, std::distance(threads, &thread)};
+  printf("After running the thread\n");
+  pseudo_barrier_wait(g_barrier);
 
-// Join all of our threads
-for (auto &thread : threads)
-thread.join();
+  thread.join();
 
-return 0;
+  return 0;
 }
Index: packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
===
--- packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
+++ packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_threads/TestWatchpointMultipleThreads.py
@@ -17,51 +17,26 @@
 class WatchpointForMultipleThreadsTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+main_spec = lldb.SBFileSpec("main.cpp", False)
 
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
-def test_watchpoint_multiple_threads(self):
-"""Test that lldb watchpoint works for multiple threads."""
-self.build()
-self.setTearDownCleanup()
-self.hello_multiple_threads()
+def test_watchpoint_before_thread_start(self):
+"""Test that we can hit a watchpoint we set before starting another thread"""
+self.do_watchpoint_test("Before running the thread")
 
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not supported on Windows")
-def test_watchpoint_multiple_threads_wp_set_and_then_delete(self):
-"""Test that lldb watchpoint works for multiple threads, and after the watchpoint is deleted, the watchpoint event should no longer fires."""
+def test_watchpoint_after_thread_start(self):
+"""Test that we can hit a watchpoint we set after starting another thread"""
+self.do_watchpoint_test("After running the thread")
+
+def do_watchpoint_tes

[Lldb-commits] [lldb] r326369 - Adapt some tests to work with PPC64le architecture

[Pavel Labath via lldb-commits]

Author: labath
Date: Wed Feb 28 12:57:26 2018
New Revision: 326369

URL: http://llvm.org/viewvc/llvm-project?rev=326369&view=rev
Log:
Adapt some tests to work with PPC64le architecture

Summary: Merge branch 'master' into adaptPPC64tests

Reviewers: clayborg, alexandreyy, labath

Reviewed By: clayborg, alexandreyy

Subscribers: luporl, lbianc, alexandreyy, lldb-commits

Differential Revision: https://reviews.llvm.org/D42917
Patch by Ana Julia Caetano .

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py?rev=326369&r1=326368&r2=326369&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
 Wed Feb 28 12:57:26 2018
@@ -21,7 +21,7 @@ class MultipleHitsTestCase(TestBase):
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not 
supported on Windows")
-@skipIf(bugnumber="llvm.org/pr30758", oslist=["linux"], archs=["arm", 
"aarch64"])
+@skipIf(bugnumber="llvm.org/pr30758", oslist=["linux"], archs=["arm", 
"aarch64", "powerpc64le"])
 def test(self):
 self.build()
 exe = self.getBuildArtifact("a.out")

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c?rev=326369&r1=326368&r2=326369&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c 
(original)
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c 
Wed Feb 28 12:57:26 2018
@@ -1,6 +1,6 @@
 #include 
 
-#if defined(__arm__) || defined(__aarch64__) || defined (__mips__)
+#if defined(__arm__) || defined(__aarch64__) || defined (__mips__) || 
defined(__powerpc64__)
 // Clang does not accept regparm attribute on these platforms.
 // Fortunately, the default calling convention passes arguments in registers
 // anyway.

Modified: lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py?rev=326369&r1=326368&r2=326369&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py Wed Feb 28 
12:57:26 2018
@@ -33,6 +33,8 @@ def check_first_register_readable(test_c
 test_case.expect("register read zero", substrs=['zero = 0x'])
 elif arch in ['s390x']:
 test_case.expect("register read r0", substrs=['r0 = 0x'])
+elif arch in ['powerpc64le']:
+test_case.expect("register read r0", substrs=['r0 = 0x'])
 else:
 # TODO: Add check for other architectures
 test_case.fail(

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py?rev=326369&r1=326368&r2=326369&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
 Wed Feb 28 12:57:26 2018
@@ -330,8 +330,9 @@ class LldbGdbServerTestCase(gdbremote_te
 # Ensure we have a program counter register.
 self.assertTrue('pc' in generic_regs)
 
-# Ensure we have a frame pointer register.
-self.assertTrue('fp' in generic_regs)
+# Ensure we have a frame pointer register. PPC64le's FP is the same as 
SP
+if(self.getArchitecture() != 'powerpc64le'):
+self.assertTrue('fp' in generic_regs)
 
 # Ensure we have a stack pointer register.
 self.assertTrue('sp' in generic_regs)


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

[Lldb-commits] [PATCH] D42917: Adapt some tests to work with PPC64le architecture

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326369: Adapt some tests to work with PPC64le architecture 
(authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42917?vs=132857&id=136371#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42917

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
  lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
  lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
  
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py


Index: 
lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
@@ -1,6 +1,6 @@
 #include 
 
-#if defined(__arm__) || defined(__aarch64__) || defined (__mips__)
+#if defined(__arm__) || defined(__aarch64__) || defined (__mips__) || 
defined(__powerpc64__)
 // Clang does not accept regparm attribute on these platforms.
 // Fortunately, the default calling convention passes arguments in registers
 // anyway.
Index: lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -33,6 +33,8 @@
 test_case.expect("register read zero", substrs=['zero = 0x'])
 elif arch in ['s390x']:
 test_case.expect("register read r0", substrs=['r0 = 0x'])
+elif arch in ['powerpc64le']:
+test_case.expect("register read r0", substrs=['r0 = 0x'])
 else:
 # TODO: Add check for other architectures
 test_case.fail(
Index: 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
+++ 
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
@@ -330,8 +330,9 @@
 # Ensure we have a program counter register.
 self.assertTrue('pc' in generic_regs)
 
-# Ensure we have a frame pointer register.
-self.assertTrue('fp' in generic_regs)
+# Ensure we have a frame pointer register. PPC64le's FP is the same as 
SP
+if(self.getArchitecture() != 'powerpc64le'):
+self.assertTrue('fp' in generic_regs)
 
 # Ensure we have a stack pointer register.
 self.assertTrue('sp' in generic_regs)
Index: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/multiple_hits/TestMultipleHits.py
@@ -21,7 +21,7 @@
 @expectedFailureAll(
 oslist=["windows"],
 bugnumber="llvm.org/pr24446: WINDOWS XFAIL TRIAGE - Watchpoints not 
supported on Windows")
-@skipIf(bugnumber="llvm.org/pr30758", oslist=["linux"], archs=["arm", 
"aarch64"])
+@skipIf(bugnumber="llvm.org/pr30758", oslist=["linux"], archs=["arm", 
"aarch64", "powerpc64le"])
 def test(self):
 self.build()
 exe = self.getBuildArtifact("a.out")


Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/register_variables/test.c
@@ -1,6 +1,6 @@
 #include 
 
-#if defined(__arm__) || defined(__aarch64__) || defined (__mips__)
+#if defined(__arm__) || defined(__aarch64__) || defined (__mips__) || defined(__powerpc64__)
 // Clang does not accept regparm attribute on these platforms.
 // Fortunately, the default calling convention passes arguments in registers
 // anyway.
Index: lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -33,6 +33,8 @@
 test_case.expect("register read zero", substrs=['zero = 0x'])
 elif arch in ['s390x']:
 test_case.expect("register read r0", substrs=['r0 = 0x'])
+elif arch in ['powerpc64le']:
+test_case.expect("register read r0", substrs=['r0 = 0x'])
 else:
 # TODO: Add check for other architectures
 test_case.fail(
I

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> And I'm not even completely clear about your case. I understand you write the 
> module to the physical address, but what happens when you actually go around 
> to debugging it? Is it still there or does it get copied/mapped/whatever to 
> the virtual address?

In  my case it's a hardware target with ROM and RAM.  Only the data section has 
different virtual and physical addresses.  That data section should be loaded 
to its physical address in ROM so it'll be preserved across resets.  The first 
code to execute on boot copies the data section from ROM to its virtual address 
in RAM.

> So it seems to me the physical addresses should really be specific to the 
> "target modules load --load" case. I've never used that command so I don't 
> know if it can just use physical addresses unconditionally, or whether we 
> need an extra option for that "target modules load --load --physical". I 
> think I'll leave that decision up to you (or anyone else who has an opinion 
> on this). But for the other use cases, I believe we should keep using virtual 
> addresses.

The "target modules load --load" case was originally intended to accomplish 
what gdb's "load" command does (see https://reviews.llvm.org/D28804).  I know 
gdb uses the physical address when writes the sections.

> So I think we should revert this and then split out the loading part of this 
> patch from the vFlash thingy so we can focus on the virtual/physical address 
> situation and how to handle that.

It's like we need the original all-virtual SectionLoadList in 
ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the 
ObjectFile::LoadInMemory case, a function which is only used by "target modules 
load --load".

Or instead instead of a second list, maybe SectionLoadList gets a second pair 
of addr<->section maps that contain physical addresses.  Then 
ObjectFile::LoadInMemory can ask for the physical instead of virtual.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D42145#1022717, @owenpshaw wrote:

> > And I'm not even completely clear about your case. I understand you write 
> > the module to the physical address, but what happens when you actually go 
> > around to debugging it? Is it still there or does it get 
> > copied/mapped/whatever to the virtual address?
>
> In  my case it's a hardware target with ROM and RAM.  Only the data section 
> has different virtual and physical addresses.  That data section should be 
> loaded to its physical address in ROM so it'll be preserved across resets.  
> The first code to execute on boot copies the data section from ROM to its 
> virtual address in RAM.


Ok, so it seems to me that we do want the rest of the debugger to operate on 
virtual addresses, as that's what you will be debugging (except when you are 
actually debugging the boot code itself, but we can't be correct all the time.

> It's like we need the original all-virtual SectionLoadList in 
> ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the 
> ObjectFile::LoadInMemory case, a function which is only used by "target 
> modules load --load".
> 
> Or instead instead of a second list, maybe SectionLoadList gets a second pair 
> of addr<->section maps that contain physical addresses.  Then 
> ObjectFile::LoadInMemory can ask for the physical instead of virtual.

Since there is just one caller of this function maybe we don't even need to 
that fancy. Could the LoadInMemory function do the shifting itself?
I'm thinking of something like where it takes the load bias as the argument and 
then adds that to the physical address it obtains from the object file. 
Thinking about that, I'm not even sure the function should be operating at the 
section level. If it's going to be simulating a loader, shouldn't it be based 
on program headers and segments completely (and not just use them for obtaining 
the physical address)?


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [lldb] r326374 - Add ability to collect memory limit.

[Han Ming Ong via lldb-commits]

Author: hanming
Date: Wed Feb 28 14:18:45 2018
New Revision: 326374

URL: http://llvm.org/viewvc/llvm-project?rev=326374&view=rev
Log:
Add ability to collect memory limit.

Reviewer: Jason Molenda



Modified:
lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj
lldb/trunk/tools/debugserver/source/DNBDefs.h
lldb/trunk/tools/debugserver/source/MacOSX/MachTask.mm
lldb/trunk/tools/debugserver/source/MacOSX/MachVMMemory.cpp
lldb/trunk/tools/debugserver/source/MacOSX/MachVMMemory.h
lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist

Modified: lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj?rev=326374&r1=326373&r2=326374&view=diff
==
--- lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj 
(original)
+++ lldb/trunk/tools/debugserver/debugserver.xcodeproj/project.pbxproj Wed Feb 
28 14:18:45 2018
@@ -875,6 +875,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
OTHER_LDFLAGS = "";
@@ -973,6 +975,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
OTHER_LDFLAGS = "";
@@ -1070,6 +1074,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
OTHER_LDFLAGS = "";
@@ -1504,6 +1510,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
"OTHER_LDFLAGS[sdk=iphoneos*][arch=*]" = (
@@ -1644,6 +1652,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
"OTHER_LDFLAGS[sdk=iphoneos*][arch=*]" = (
@@ -1855,6 +1865,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
"OTHER_CPLUSPLUSFLAGS[sdk=iphoneos*][arch=*]" = 
"$(OTHER_CFLAGS)";
OTHER_LDFLAGS = "";
@@ -1987,6 +1999,8 @@
"$(LLDB_COMPRESSION_CFLAGS)",
"$(LLDB_ZLIB_CFLAGS)",
"$(LLDB_OS_LOG_CFLAGS)",
+   "-isystem",
+   
"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
);
   

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: labath, zturner, davide, aprantl, lhames.

LLDB CompilerTypes may be invalid, i.e they should be checked for
validity before use.

Compared to a more typical model in which only valid types exist [1],
this has a few disadvantages:

- The debugger is guaranteed to operate on invalid types as if they were valid, 
leading to unexpected or unpredictable results. Because type validity checks 
are not mandatory, they can and will be skipped.

  E.g in this patch I chose a random function which returns a type. Its callers 
checked for invalid results inconsistently, hiding a bug.
- Operations on types have high complexity, because each operation has fallback 
paths to deal with invalid types [2]. Beyond making code hard to read, this 
results in redundant double-checking of type validity.

Going forward, we should transition to a model in which CompilerTypes
are either valid or do not exist. This is a first, incremental step
towards that goal. Ultimately we should delete "Type::IsValid()".

[1] See llvm::Type, clang::Type, swift::Type, etc. All of these either
exist and are valid, or do not exist.

[2] See the methods in CompilerType. Each of these branch on the type's
validity.

[3] For an overview of the principles and advantages of llvm::Error, see
Lang Hames's talk: https://www.youtube.com/watch?v=Wq8fNK98WGw.


https://reviews.llvm.org/D43912

Files:
  include/lldb/Symbol/CompilerType.h
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  source/Symbol/CompilerType.cpp

Index: source/Symbol/CompilerType.cpp
===
--- source/Symbol/CompilerType.cpp
+++ source/Symbol/CompilerType.cpp
@@ -1080,3 +1080,5 @@
   return lhs.GetTypeSystem() != rhs.GetTypeSystem() ||
  lhs.GetOpaqueQualType() != rhs.GetOpaqueQualType();
 }
+
+char lldb_private::InvalidType::ID;
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -625,10 +625,11 @@
   /// The type as it appears in the source context.
   ///
   /// @return
-  /// Returns the moved type, or an empty type if there was a problem.
+  /// Returns the moved type, or an error.
   //--
-  TypeFromUser DeportType(ClangASTContext &target, ClangASTContext &source,
-  TypeFromParser parser_type);
+  llvm::Expected DeportType(ClangASTContext &target,
+  ClangASTContext &source,
+  TypeFromParser parser_type);
 
   ClangASTContext *GetClangASTContext();
 };
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -277,9 +277,10 @@
   return ret;
 }
 
-TypeFromUser ClangExpressionDeclMap::DeportType(ClangASTContext &target,
-ClangASTContext &source,
-TypeFromParser parser_type) {
+llvm::Expected
+ClangExpressionDeclMap::DeportType(ClangASTContext &target,
+   ClangASTContext &source,
+   TypeFromParser parser_type) {
   assert (&target == m_target->GetScratchClangASTContext());
   assert ((TypeSystem*)&source == parser_type.GetTypeSystem());
   assert (source.getASTContext() == m_ast_context);
@@ -302,10 +303,10 @@
 source_file,
 clang::QualType::getFromOpaquePtr(parser_type.GetOpaqueQualType()));
 return TypeFromUser(exported_type.getAsOpaquePtr(), &target);
-  } else {
-lldbassert(0 && "No mechanism for deporting a type!");
-return TypeFromUser();
   }
+
+  lldbassert(0 && "No mechanism for deporting a type!");
+  return llvm::make_error("Could not deport type from context");
 }
 
 bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
@@ -315,6 +316,7 @@
bool is_lvalue) {
   assert(m_parser_vars.get());
 
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
   ClangASTContext *ast =
   llvm::dyn_cast_or_null(parser_type.GetTypeSystem());
   if (ast == nullptr)
@@ -328,8 +330,18 @@
 if (target == nullptr)
   return false;
 
-TypeFromUser user_type =
+llvm::Expected user_type_or_err =
 DeportType(*target->GetScratchClangASTContext(), *ast, parser_type);
+if (llvm::Error E = user_type_or_err.takeError()) {
+  std::string Reason = llvm::toString(std::move(E));

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> Going forward, we should transition to a model in which CompilerTypes are 
> either valid or do not exist.

I don't understand very well how the LLDB type system works so excuse my naive 
questions: Does this account for lazyness? I.e., could there be value in having 
an unverified type that might be sufficient for what LLDB is trying to do with 
it, where validating it (which may involve recursively materializing all of its 
children) might fail? I could imagine that for some use-cases just knowing the 
size of a type would be sufficient.
I guess what I'm trying to say is: Are there code paths in LLDB that do 
something useful with a type where `type.IsValid()==false` ?


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: include/lldb/Symbol/CompilerType.h:446
+/// A force-checked error used to describe type construction failures.
+class InvalidType : public llvm::ErrorInfo {
+public:

I think this should be called `InvalidTypeError`?


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43857: Speed up TestWatchpointMultipleThreads

[Jim Ingham via Phabricator via lldb-commits]

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

Great, thanks!


https://reviews.llvm.org/D43857



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

[Lldb-commits] [lldb] r326378 - Fix up the gtest targets for changes in the UnwindAssembly tests.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Wed Feb 28 14:41:11 2018
New Revision: 326378

URL: http://llvm.org/viewvc/llvm-project?rev=326378&view=rev
Log:
Fix up the gtest targets for changes in the UnwindAssembly tests.

Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj

Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=326378&r1=326377&r2=326378&view=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Wed Feb 28 14:41:11 2018
@@ -746,6 +746,9 @@
4CE4EFB41E899A4000A80C06 /* RegisterContextOpenBSD_x86_64.cpp 
in Sources */ = {isa = PBXBuildFile; fileRef = 4CE4EFAD1E899A1200A80C06 /* 
RegisterContextOpenBSD_x86_64.cpp */; };
4CE4F673162C971A00F75CB3 /* SBExpressionOptions.h in Headers */ 
= {isa = PBXBuildFile; fileRef = 4CE4F672162C971A00F75CB3 /* 
SBExpressionOptions.h */; settings = {ATTRIBUTES = (Public, ); }; };
4CE4F675162C973F00F75CB3 /* SBExpressionOptions.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = 4CE4F674162C973F00F75CB3 /* 
SBExpressionOptions.cpp */; };
+   4CEC86A4204738C5009B37B1 /* TestArm64InstEmulation.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = 4CEC86A3204738C5009B37B1 /* 
TestArm64InstEmulation.cpp */; };
+   4CEC86A7204738EB009B37B1 /* TestPPC64InstEmulation.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = 4CEC86A6204738EA009B37B1 /* 
TestPPC64InstEmulation.cpp */; };
+   4CEC86A92047395F009B37B1 /* Python.framework in Frameworks */ = 
{isa = PBXBuildFile; fileRef = 2326CF3F1BDD613E00A5CEAC /* Python.framework */; 
};
4CF3D80C15AF4DC800845BF3 /* Security.framework in Frameworks */ 
= {isa = PBXBuildFile; fileRef = EDB919B414F6F10D008FF64B /* Security.framework 
*/; };
4CF52AF51428291E0051E832 /* SBFileSpecList.h in Headers */ = 
{isa = PBXBuildFile; fileRef = 4CF52AF41428291E0051E832 /* SBFileSpecList.h */; 
settings = {ATTRIBUTES = (Public, ); }; };
4CF52AF8142829390051E832 /* SBFileSpecList.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = 4CF52AF7142829390051E832 /* SBFileSpecList.cpp 
*/; };
@@ -954,7 +957,6 @@
AF235EB41FBE7858009C5541 /* RegisterInfoPOSIX_ppc64le.h in 
Headers */ = {isa = PBXBuildFile; fileRef = AF235EB21FBE7857009C5541 /* 
RegisterInfoPOSIX_ppc64le.h */; };
AF235EB51FBE7858009C5541 /* RegisterInfoPOSIX_ppc64le.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = AF235EB31FBE7858009C5541 /* 
RegisterInfoPOSIX_ppc64le.cpp */; };
AF23B4DB19009C66003E2A58 /* FreeBSDSignals.cpp in Sources */ = 
{isa = PBXBuildFile; fileRef = AF23B4D919009C66003E2A58 /* FreeBSDSignals.cpp 
*/; };
-   AF248A4D1DA71C77000B814D /* TestArm64InstEmulation.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = AF248A4C1DA71C77000B814D /* 
TestArm64InstEmulation.cpp */; };
AF254E31170CCC33007AE5C9 /* PlatformDarwinKernel.cpp in Sources 
*/ = {isa = PBXBuildFile; fileRef = AF254E2F170CCC33007AE5C9 /* 
PlatformDarwinKernel.cpp */; };
AF25AB26188F685C0030DEC3 /* AppleGetQueuesHandler.cpp in 
Sources */ = {isa = PBXBuildFile; fileRef = AF25AB24188F685C0030DEC3 /* 
AppleGetQueuesHandler.cpp */; };
AF26703A1852D01E00B6CC36 /* Queue.cpp in Sources */ = {isa = 
PBXBuildFile; fileRef = AF2670381852D01E00B6CC36 /* Queue.cpp */; };
@@ -2627,6 +2629,8 @@
4CE4F672162C971A00F75CB3 /* SBExpressionOptions.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = 
SBExpressionOptions.h; path = include/lldb/API/SBExpressionOptions.h; 
sourceTree = ""; };
4CE4F674162C973F00F75CB3 /* SBExpressionOptions.cpp */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; 
name = SBExpressionOptions.cpp; path = source/API/SBExpressionOptions.cpp; 
sourceTree = ""; };
4CE4F676162CE1E100F75CB3 /* SBExpressionOptions.i */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.c.c.preprocessed; path = SBExpressionOptions.i; sourceTree = 
""; };
+   4CEC86A3204738C5009B37B1 /* TestArm64InstEmulation.cpp */ = 
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.cpp.cpp; name = TestArm64InstEmulation.cpp; path = 
UnwindAssembly/ARM64/TestArm64InstEmulation.cpp; sourceTree = ""; };
+   4CEC86A6204738EA009B37B1 /* TestPPC64InstEmulation.cpp */ = 
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = 
sourcecode.cpp.cpp; name = TestPPC64InstEmulation.cpp; path = 
UnwindAssembly/PPC64/TestPPC64InstEmulation.cpp; sourceTree = ""; };
4CEDAED311754F5E00E875A6 /* ThreadPlanStepUntil.h */ = {isa = 
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; na

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I agree that we should start using this in a lot more places. I think you 
should be able to shorten the logging code a bit by using the LLDB_LOG_ERROR 
macro, which I've created for this purpose.




Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340
+if (llvm::Error E = user_type_or_err.takeError()) {
+  std::string Reason = llvm::toString(std::move(E));
+  if (log)
+log->Printf("%s", Reason.c_str());
+  return false;
+}

How about:
```
if (!user_type_or_err) {
  LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
  return false;
}
```


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:382-387
+  if (llvm::Error E = user_type_or_err.takeError()) {
+std::string Reason = llvm::toString(std::move(E));
 if (log)
-  log->Printf("Persistent variable's type wasn't copied successfully");
+  log->Printf("%s", Reason.c_str());
 return false;
   }

I wonder if we need a macro like 

```
#define RETURN_IF_UNEXPECTED(item, ret_val, log_categories)   \
  if (auto E = item.takeError()) {\
std::string Reason = llvm::toString(std::move(E));\
Log *log(lldb_private::GetLogIfAllCategoriesSet(log_categories)); \
if (log) \
  log->Printf("%s", Reason.c_str());
return ret_val;
  }
```

then we say:

```
RETURN_IF_UNEXPECTED(user_type_or_err, false, LIBLLDB_LOG_EXPRESSIONS);
```

I don't have too strong of an opinion, but as this pattern becomes more 
pervasive, it's annoying to have to write out 5-6 lines of code every time you 
call a function that returns an `Expected`.


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> Since there is just one caller of this function maybe we don't even need to 
> that fancy. Could the LoadInMemory function do the shifting itself?
>  I'm thinking of something like where it takes the load bias as the argument 
> and then adds that to the physical address it obtains from the object file. 
> Thinking about that, I'm not even sure the function should be operating at 
> the section level. If it's going to be simulating a loader, shouldn't it be 
> based on program headers and segments completely (and not just use them for 
> obtaining the physical address)?

The original LoadInMemory thread from over a year ago discusses segments vs 
sections a little bit: 
http://lists.llvm.org/pipermail/lldb-dev/2016-December/011758.html.  Sounds 
like gdb used sections, so that was deemed ok for lldb.  It also fit with the 
pre-existing "target modules load" command that allowed section addresses to be 
specified individually.  At one point, Greg suggests:

> Since the arguments to this command are free form, we could pass these 
> arguments to ObjectFile::LoadInMemory() and let each loader (ObjectFileELF 
> and ObjectFileMach) determine what arguments are valid

But that didn't end up in the implementation.  Perhaps it should, and 
ObjectFileELF could disallow any --load that specified sections individually.  
And if we're doing a special ObjectFileELF::LoadInMemory anyway, it could go 
ahead and use segments.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In https://reviews.llvm.org/D43912#1022916, @aprantl wrote:

> > Going forward, we should transition to a model in which CompilerTypes are 
> > either valid or do not exist.
>
> I don't understand very well how the LLDB type system works so excuse my 
> naive questions: Does this account for lazyness? I.e., could there be value 
> in having an unverified type that might be sufficient for what LLDB is trying 
> to do with it, where validating it (which may involve recursively 
> materializing all of its children) might fail? I could imagine that for some 
> use-cases just knowing the size of a type would be sufficient.
>  I guess what I'm trying to say is: Are there code paths in LLDB that do 
> something useful with a type where `type.IsValid()==false` ?


No, I don't think so.  Laziness might make a type go from valid (we only got 
the forward type) to invalid if the attempt to complete it fails for some 
reason (base class missing, for instance).  But a not yet fully completed type 
will call itself valid, not invalid.


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340
+if (llvm::Error E = user_type_or_err.takeError()) {
+  std::string Reason = llvm::toString(std::move(E));
+  if (log)
+log->Printf("%s", Reason.c_str());
+  return false;
+}

labath wrote:
> How about:
> ```
> if (!user_type_or_err) {
>   LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
>   return false;
> }
> ```
I think it'd be even more convenient to include the return into the macro, as 
Zachary suggested.



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:382-387
+  if (llvm::Error E = user_type_or_err.takeError()) {
+std::string Reason = llvm::toString(std::move(E));
 if (log)
-  log->Printf("Persistent variable's type wasn't copied successfully");
+  log->Printf("%s", Reason.c_str());
 return false;
   }

zturner wrote:
> I wonder if we need a macro like 
> 
> ```
> #define RETURN_IF_UNEXPECTED(item, ret_val, log_categories)   \
>   if (auto E = item.takeError()) {\
> std::string Reason = llvm::toString(std::move(E));\
> Log *log(lldb_private::GetLogIfAllCategoriesSet(log_categories)); \
> if (log) \
>   log->Printf("%s", Reason.c_str());
> return ret_val;
>   }
> ```
> 
> then we say:
> 
> ```
> RETURN_IF_UNEXPECTED(user_type_or_err, false, LIBLLDB_LOG_EXPRESSIONS);
> ```
> 
> I don't have too strong of an opinion, but as this pattern becomes more 
> pervasive, it's annoying to have to write out 5-6 lines of code every time 
> you call a function that returns an `Expected`.
Thanks for the suggestion. I've introduced a macro which is pretty similar to 
what you've shown here.


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk updated this revision to Diff 136433.
vsk marked an inline comment as done.
vsk edited the summary of this revision.
vsk added a comment.

- Address some review feedback.


https://reviews.llvm.org/D43912

Files:
  include/lldb/Symbol/CompilerType.h
  include/lldb/Utility/Log.h
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  source/Symbol/CompilerType.cpp

Index: source/Symbol/CompilerType.cpp
===
--- source/Symbol/CompilerType.cpp
+++ source/Symbol/CompilerType.cpp
@@ -1080,3 +1080,5 @@
   return lhs.GetTypeSystem() != rhs.GetTypeSystem() ||
  lhs.GetOpaqueQualType() != rhs.GetOpaqueQualType();
 }
+
+char lldb_private::InvalidTypeError::ID;
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -625,10 +625,11 @@
   /// The type as it appears in the source context.
   ///
   /// @return
-  /// Returns the moved type, or an empty type if there was a problem.
+  /// Returns the moved type, or an error.
   //--
-  TypeFromUser DeportType(ClangASTContext &target, ClangASTContext &source,
-  TypeFromParser parser_type);
+  llvm::Expected DeportType(ClangASTContext &target,
+  ClangASTContext &source,
+  TypeFromParser parser_type);
 
   ClangASTContext *GetClangASTContext();
 };
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -277,9 +277,10 @@
   return ret;
 }
 
-TypeFromUser ClangExpressionDeclMap::DeportType(ClangASTContext &target,
-ClangASTContext &source,
-TypeFromParser parser_type) {
+llvm::Expected
+ClangExpressionDeclMap::DeportType(ClangASTContext &target,
+   ClangASTContext &source,
+   TypeFromParser parser_type) {
   assert (&target == m_target->GetScratchClangASTContext());
   assert ((TypeSystem*)&source == parser_type.GetTypeSystem());
   assert (source.getASTContext() == m_ast_context);
@@ -302,10 +303,10 @@
 source_file,
 clang::QualType::getFromOpaquePtr(parser_type.GetOpaqueQualType()));
 return TypeFromUser(exported_type.getAsOpaquePtr(), &target);
-  } else {
-lldbassert(0 && "No mechanism for deporting a type!");
-return TypeFromUser();
   }
+
+  lldbassert(0 && "No mechanism for deporting a type!");
+  return llvm::make_error("Could not deport type");
 }
 
 bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
@@ -315,6 +316,7 @@
bool is_lvalue) {
   assert(m_parser_vars.get());
 
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
   ClangASTContext *ast =
   llvm::dyn_cast_or_null(parser_type.GetTypeSystem());
   if (ast == nullptr)
@@ -328,8 +330,12 @@
 if (target == nullptr)
   return false;
 
-TypeFromUser user_type =
+llvm::Expected user_type_or_err =
 DeportType(*target->GetScratchClangASTContext(), *ast, parser_type);
+RETURN_IF_UNEXPECTED(user_type_or_err, false, log);
+TypeFromUser &user_type = *user_type_or_err;
+assert(user_type.IsValid() &&
+   "Persistent variable's type wasn't copied successfully");
 
 uint32_t offset = m_parser_vars->m_materializer->AddResultVariable(
 user_type, is_lvalue, m_keep_result_in_memory, m_result_delegate, err);
@@ -358,21 +364,19 @@
 return true;
   }
 
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
   ExecutionContext &exe_ctx = m_parser_vars->m_exe_ctx;
   Target *target = exe_ctx.GetTargetPtr();
   if (target == NULL)
 return false;
 
   ClangASTContext *context(target->GetScratchClangASTContext());
 
-  TypeFromUser user_type = DeportType(*context, *ast, parser_type);
-
-  if (!user_type.GetOpaqueQualType()) {
-if (log)
-  log->Printf("Persistent variable's type wasn't copied successfully");
-return false;
-  }
+  llvm::Expected user_type_or_err =
+  DeportType(*context, *ast, parser_type);
+  RETURN_IF_UNEXPECTED(user_type_or_err, false, log);
+  TypeFromUser &user_type = *user_type_or_err;
+  assert(user_type.IsValid() &&
+ "Persistent variable's type wasn't copied successfully");
 
   if (!m_parser_vars->m_target_info.IsValid())
 return false;
Index

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

In https://reviews.llvm.org/D43912#1023078, @jingham wrote:

> In https://reviews.llvm.org/D43912#1022916, @aprantl wrote:
>
> > > Going forward, we should transition to a model in which CompilerTypes are 
> > > either valid or do not exist.
> >
> > I don't understand very well how the LLDB type system works so excuse my 
> > naive questions: Does this account for lazyness? I.e., could there be value 
> > in having an unverified type that might be sufficient for what LLDB is 
> > trying to do with it, where validating it (which may involve recursively 
> > materializing all of its children) might fail? I could imagine that for 
> > some use-cases just knowing the size of a type would be sufficient.
> >  I guess what I'm trying to say is: Are there code paths in LLDB that do 
> > something useful with a type where `type.IsValid()==false` ?
>


If we discover code like this, transitioning the code into a model where types 
are force-checked will require caution, but would still be possible/desirable. 
For example, we could refactor the operations on invalid type s.t they use a 
different type, i.e one without the same validity requirement as CompilerType.

> No, I don't think so.  Laziness might make a type go from valid (we only got 
> the forward type) to invalid if the attempt to complete it fails for some 
> reason (base class missing, for instance).  But a not yet fully completed 
> type will call itself valid, not invalid.

Thanks for the explanation Jim!


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The problem with your RETURN_IF_UNEXPECTED macro is that it make it impossible 
to put a breakpoint only on the "type was invalid case."  To catch that 
happening while debugging you'd have to write a conditional breakpoint that 
also checks the error.

That seems like something you would really want to be able to do.  If you leave 
the return outside the macro, then you can break on the return.  IMHO that's 
worth having to write

if (CHECK_IF_ERROR)

  return;


https://reviews.llvm.org/D43912



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

[Lldb-commits] [lldb] r326399 - Add another entitlement that we need for debugserver.

[Jason Molenda via lldb-commits]

Author: jmolenda
Date: Wed Feb 28 17:04:07 2018
New Revision: 326399

URL: http://llvm.org/viewvc/llvm-project?rev=326399&view=rev
Log:
Add another entitlement that we need for debugserver.
 

Modified:
lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist

Modified: lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist?rev=326399&r1=326398&r2=326399&view=diff
==
--- lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist 
(original)
+++ lldb/trunk/tools/debugserver/source/debugserver-entitlements.plist Wed Feb 
28 17:04:07 2018
@@ -26,5 +26,7 @@
 
 com.apple.private.memorystatus
 
+com.apple.private.cs.debugger
+
 
 


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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: include/lldb/Utility/Log.h:249-254
+  ::lldb_private::Log *log_private = (log);
\
+  if (log_private) 
\
+log_private->FormatError(::std::move(error_private), __FILE__, 
\
+ __func__, "{0}"); 
\
+  else 
\
+::llvm::consumeError(::std::move(error_private));  
\

it looks like this could be just handled by delegating to the LLDB_LOG_ERROR 
macro.



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340
+if (llvm::Error E = user_type_or_err.takeError()) {
+  std::string Reason = llvm::toString(std::move(E));
+  if (log)
+log->Printf("%s", Reason.c_str());
+  return false;
+}

vsk wrote:
> labath wrote:
> > How about:
> > ```
> > if (!user_type_or_err) {
> >   LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
> >   return false;
> > }
> > ```
> I think it'd be even more convenient to include the return into the macro, as 
> Zachary suggested.
Yeah, I'm fine with the new macro as well. I see it mostly as a transitionary 
measure. Once most of the code uses llvm::Error, we should be able to just 
propagate that (or do something interesting with the error).


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The phase where we are converting everything over seems exactly when we want to 
be able to easily stop on the error cases when debugging, which using a macro 
that contains the return makes impossible.


https://reviews.llvm.org/D43912



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

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk updated this revision to Diff 136450.
vsk added a comment.

> The problem with your RETURN_IF_UNEXPECTED macro is that it make it 
> impossible to put a breakpoint only on the "type was invalid case." To catch 
> that happening while debugging you'd have to write a conditional breakpoint 
> that also checks the error.

Yes, we'd need a conditional breakpoint if more than one InvalidTypeError 
instance is created (otherwise we'd just break on the constructor). While I 
hope we never have to deal with that, I'm happy to keep things more explicit 
for now.


https://reviews.llvm.org/D43912

Files:
  include/lldb/Symbol/CompilerType.h
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  source/Symbol/CompilerType.cpp

Index: source/Symbol/CompilerType.cpp
===
--- source/Symbol/CompilerType.cpp
+++ source/Symbol/CompilerType.cpp
@@ -1080,3 +1080,5 @@
   return lhs.GetTypeSystem() != rhs.GetTypeSystem() ||
  lhs.GetOpaqueQualType() != rhs.GetOpaqueQualType();
 }
+
+char lldb_private::InvalidTypeError::ID;
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -625,10 +625,11 @@
   /// The type as it appears in the source context.
   ///
   /// @return
-  /// Returns the moved type, or an empty type if there was a problem.
+  /// Returns the moved type, or an error.
   //--
-  TypeFromUser DeportType(ClangASTContext &target, ClangASTContext &source,
-  TypeFromParser parser_type);
+  llvm::Expected DeportType(ClangASTContext &target,
+  ClangASTContext &source,
+  TypeFromParser parser_type);
 
   ClangASTContext *GetClangASTContext();
 };
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -277,9 +277,10 @@
   return ret;
 }
 
-TypeFromUser ClangExpressionDeclMap::DeportType(ClangASTContext &target,
-ClangASTContext &source,
-TypeFromParser parser_type) {
+llvm::Expected
+ClangExpressionDeclMap::DeportType(ClangASTContext &target,
+   ClangASTContext &source,
+   TypeFromParser parser_type) {
   assert (&target == m_target->GetScratchClangASTContext());
   assert ((TypeSystem*)&source == parser_type.GetTypeSystem());
   assert (source.getASTContext() == m_ast_context);
@@ -302,10 +303,10 @@
 source_file,
 clang::QualType::getFromOpaquePtr(parser_type.GetOpaqueQualType()));
 return TypeFromUser(exported_type.getAsOpaquePtr(), &target);
-  } else {
-lldbassert(0 && "No mechanism for deporting a type!");
-return TypeFromUser();
   }
+
+  lldbassert(0 && "No mechanism for deporting a type!");
+  return llvm::make_error("Could not deport type");
 }
 
 bool ClangExpressionDeclMap::AddPersistentVariable(const NamedDecl *decl,
@@ -315,6 +316,7 @@
bool is_lvalue) {
   assert(m_parser_vars.get());
 
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
   ClangASTContext *ast =
   llvm::dyn_cast_or_null(parser_type.GetTypeSystem());
   if (ast == nullptr)
@@ -328,8 +330,15 @@
 if (target == nullptr)
   return false;
 
-TypeFromUser user_type =
+llvm::Expected user_type_or_err =
 DeportType(*target->GetScratchClangASTContext(), *ast, parser_type);
+if (!user_type_or_err) {
+  LLDB_LOG_ERROR(log, user_type_or_err.takeError(), "{0}");
+  return false;
+}
+TypeFromUser &user_type = *user_type_or_err;
+assert(user_type.IsValid() &&
+   "Persistent variable's type wasn't copied successfully");
 
 uint32_t offset = m_parser_vars->m_materializer->AddResultVariable(
 user_type, is_lvalue, m_keep_result_in_memory, m_result_delegate, err);
@@ -358,21 +367,22 @@
 return true;
   }
 
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
   ExecutionContext &exe_ctx = m_parser_vars->m_exe_ctx;
   Target *target = exe_ctx.GetTargetPtr();
   if (target == NULL)
 return false;
 
   ClangASTContext *context(target->GetScratchClangASTContext());
 
-  TypeFromUser user_type = DeportType(*context, *ast, parser_type);
-
-  if (!user_type.GetOpaqueQualType()) {
-if (log)
-  log->Printf("Persistent variabl

[Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.



Comment at: include/lldb/Utility/Log.h:249-254
+  ::lldb_private::Log *log_private = (log);
\
+  if (log_private) 
\
+log_private->FormatError(::std::move(error_private), __FILE__, 
\
+ __func__, "{0}"); 
\
+  else 
\
+::llvm::consumeError(::std::move(error_private));  
\

labath wrote:
> it looks like this could be just handled by delegating to the LLDB_LOG_ERROR 
> macro.
I missed this earlier because I mistakenly named two variables error_private. 
At any rate, there's an objection to having a return_if macro so I'm setting it 
aside.


https://reviews.llvm.org/D43912



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

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

[Vedant Kumar via lldb-commits]

Hey Adrian,

Did you mean to write cls.getBuildArtifact() here?

I'm seeing a Python exception locally when I run check-lldb-single:

Traceback (most recent call last):
  File 
"/Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 584, in tearDownClass
cls.classCleanup()
  File 
"/Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py",
 line 25, in classCleanup
cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
NameError: global name 'self' is not defined

vedant

> On Jan 30, 2018, at 10:29 AM, Adrian Prantl via lldb-commits 
>  wrote:
> 
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
> (original)
> +++ lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
> Tue Jan 30 10:29:16 2018
> @@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
> @classmethod
> def classCleanup(cls):
> """Cleanup the test byproducts."""
> -cls.RemoveTempFile("stdout.txt")
> +cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))

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

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

[Adrian Prantl via lldb-commits]

That looks reasonable, since this is a class method, yes. Thanks for spotting 
it! Would you mind committing the change?

-- adrian

> On Feb 28, 2018, at 6:46 PM, Vedant Kumar  wrote:
> 
> Hey Adrian,
> 
> Did you mean to write cls.getBuildArtifact() here?
> 
> I'm seeing a Python exception locally when I run check-lldb-single:
> 
> Traceback (most recent call last):
>   File "/Users/vsk/src/llvm.org 
> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py",
>  line 584, in tearDownClass
> cls.classCleanup()
>   File "/Users/vsk/src/llvm.org 
> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py",
>  line 25, in classCleanup
> cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
> NameError: global name 'self' is not defined
> 
> vedant
> 
>> On Jan 30, 2018, at 10:29 AM, Adrian Prantl via lldb-commits 
>> mailto:lldb-commits@lists.llvm.org>> wrote:
>> 
>> ==
>> --- 
>> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
>> (original)
>> +++ 
>> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
>> Tue Jan 30 10:29:16 2018
>> @@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
>> @classmethod
>> def classCleanup(cls):
>> """Cleanup the test byproducts."""
>> -cls.RemoveTempFile("stdout.txt")
>> +cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
> 

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

[Lldb-commits] [lldb] r326412 - We were getting the wrong dynamic type if there were two classes with the same basename.

[Jim Ingham via lldb-commits]

Author: jingham
Date: Wed Feb 28 18:44:34 2018
New Revision: 326412

URL: http://llvm.org/viewvc/llvm-project?rev=326412&view=rev
Log:
We were getting the wrong dynamic type if there were two classes with the same 
basename.

There's a bug in FindTypes, it ignores the exact flag if you pass a name that 
doesn't begin with
:: and pass eTypeClassAny for the type.

In this case we always know that the name we get from the vtable name is 
absolute so we can
work around the bug by prepending the "::".  This doesn't fix the FindTypes bug.



Added:

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/Makefile

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/main.cpp
Modified:

lldb/trunk/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/Makefile?rev=326412&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/Makefile
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/Makefile
 Wed Feb 28 18:44:34 2018
@@ -0,0 +1,5 @@
+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py?rev=326412&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
 Wed Feb 28 18:44:34 2018
@@ -0,0 +1,62 @@
+"""
+Make sure if we have two classes with the same base name the
+dynamic value calculator doesn't confuse them
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class RenameThisSampleTestTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the 
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_same_basename_this(self):
+"""Test that the we use the full name to resolve dynamic types."""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.cpp")
+self.sample_test()
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def sample_test(self):
+(target, process, thread, bkpt) = 
lldbutil.run_to_source_breakpoint(self,
+   "Break here to get started", 
self.main_source_file) 
+
+# Set breakpoints in the two class methods and run to them:
+namesp_bkpt = target.BreakpointCreateBySourceRegex("namesp function 
did something.", self.main_source_file)
+self.assertEqual(namesp_bkpt.GetNumLocations(), 1, "Namespace 
breakpoint invalid")
+
+virtual_bkpt = target.BreakpointCreateBySourceRegex("Virtual function 
did something.", self.main_source_file)
+self.assertEqual(virtual_bkpt.GetNumLocations(), 1, "Virtual 
breakpoint invalid")
+
+threads = lldbutil.continue_to_breakpoint(process, namesp_bkpt)
+self.assertEqual(len(threads), 1, "Didn't stop at namespace 
breakpoint")
+
+frame = threads[0].frame[0]
+namesp_this = frame.FindVariable("this", lldb.eDynamicCanRunTarget)
+self.assertEqual(namesp_this.GetTypeName(), "namesp::Virtual *", 
"Didn't get the right dynamic type")
+
+threads = lldbutil.continue_to_breakpoint(process, virtual_bkpt)
+self.assertEqual(len(threads), 1, "Didn't stop at virtual breakpoint")
+
+frame = threads[0].frame[0]
+virtual_this = frame.FindVariable("this", lldb.eDynamicCanRunTarget)
+self.assertEqual(virtual_this.GetTypeName(), "Virtual *", "Didn't get 
the right dynamic type")
+
+
+

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/main.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic-value-same-basename/main.cpp?rev=326412&view=auto

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

[Vedant Kumar via lldb-commits]

Will test and commit momentarily, thanks!

vedant

> On Feb 28, 2018, at 6:47 PM, Adrian Prantl  wrote:
> 
> That looks reasonable, since this is a class method, yes. Thanks for spotting 
> it! Would you mind committing the change?
> 
> -- adrian
> 
>> On Feb 28, 2018, at 6:46 PM, Vedant Kumar > > wrote:
>> 
>> Hey Adrian,
>> 
>> Did you mean to write cls.getBuildArtifact() here?
>> 
>> I'm seeing a Python exception locally when I run check-lldb-single:
>> 
>> Traceback (most recent call last):
>>   File "/Users/vsk/src/llvm.org 
>> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py",
>>  line 584, in tearDownClass
>> cls.classCleanup()
>>   File "/Users/vsk/src/llvm.org 
>> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py",
>>  line 25, in classCleanup
>> cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
>> NameError: global name 'self' is not defined
>> 
>> vedant
>> 
>>> On Jan 30, 2018, at 10:29 AM, Adrian Prantl via lldb-commits 
>>> mailto:lldb-commits@lists.llvm.org>> wrote:
>>> 
>>> ==
>>> --- 
>>> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
>>> (original)
>>> +++ 
>>> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
>>> Tue Jan 30 10:29:16 2018
>>> @@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
>>> @classmethod
>>> def classCleanup(cls):
>>> """Cleanup the test byproducts."""
>>> -cls.RemoveTempFile("stdout.txt")
>>> +cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
>> 
> 

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

[Lldb-commits] [lldb] r326414 - [test] Restore cleanup behavior in TestQuoting.py

[Vedant Kumar via lldb-commits]

Author: vedantk
Date: Wed Feb 28 19:03:38 2018
New Revision: 326414

URL: http://llvm.org/viewvc/llvm-project?rev=326414&view=rev
Log:
[test] Restore cleanup behavior in TestQuoting.py

Before the change to compile tests out-of-tree, the cleanup classmethod
in TestQuoting.py would remove a temp file. After the change it threw an
exception due to a malformed call to getBuildArtifact().

Bring back the old behavior.

Modified:
lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py?rev=326414&r1=326413&r2=326414&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
(original)
+++ lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
Wed Feb 28 19:03:38 2018
@@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
 @classmethod
 def classCleanup(cls):
 """Cleanup the test byproducts."""
-cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
+cls.RemoveTempFile("stdout.txt")
 
 @no_debug_info_test
 def test_no_quote(self):


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

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

[Vedant Kumar via lldb-commits]

Done, r326414

> On Feb 28, 2018, at 6:48 PM, Vedant Kumar  wrote:
> 
> Will test and commit momentarily, thanks!
> 
> vedant
> 
>> On Feb 28, 2018, at 6:47 PM, Adrian Prantl > > wrote:
>> 
>> That looks reasonable, since this is a class method, yes. Thanks for 
>> spotting it! Would you mind committing the change?
>> 
>> -- adrian
>> 
>>> On Feb 28, 2018, at 6:46 PM, Vedant Kumar >> > wrote:
>>> 
>>> Hey Adrian,
>>> 
>>> Did you mean to write cls.getBuildArtifact() here?
>>> 
>>> I'm seeing a Python exception locally when I run check-lldb-single:
>>> 
>>> Traceback (most recent call last):
>>>   File "/Users/vsk/src/llvm.org 
>>> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py",
>>>  line 584, in tearDownClass
>>> cls.classCleanup()
>>>   File "/Users/vsk/src/llvm.org 
>>> -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py",
>>>  line 25, in classCleanup
>>> cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
>>> NameError: global name 'self' is not defined
>>> 
>>> vedant
>>> 
 On Jan 30, 2018, at 10:29 AM, Adrian Prantl via lldb-commits 
 mailto:lldb-commits@lists.llvm.org>> wrote:
 
 ==
 --- 
 lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
 (original)
 +++ 
 lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
 Tue Jan 30 10:29:16 2018
 @@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
 @classmethod
 def classCleanup(cls):
 """Cleanup the test byproducts."""
 -cls.RemoveTempFile("stdout.txt")
 +cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
>>> 
>> 
> 

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

Re: [Lldb-commits] [lldb] r323803 - Compile the LLDB tests out-of-tree.

[Adrian Prantl via lldb-commits]

Thanks!

> On Feb 28, 2018, at 7:06 PM, Vedant Kumar  wrote:
> 
> Done, r326414
> 
>> On Feb 28, 2018, at 6:48 PM, Vedant Kumar > > wrote:
>> 
>> Will test and commit momentarily, thanks!
>> 
>> vedant
>> 
>>> On Feb 28, 2018, at 6:47 PM, Adrian Prantl >> > wrote:
>>> 
>>> That looks reasonable, since this is a class method, yes. Thanks for 
>>> spotting it! Would you mind committing the change?
>>> 
>>> -- adrian
>>> 
 On Feb 28, 2018, at 6:46 PM, Vedant Kumar >>> > wrote:
 
 Hey Adrian,
 
 Did you mean to write cls.getBuildArtifact() here?
 
 I'm seeing a Python exception locally when I run check-lldb-single:
 
 Traceback (most recent call last):
   File "/Users/vsk/src/llvm.org 
 -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lldbtest.py",
  line 584, in tearDownClass
 cls.classCleanup()
   File "/Users/vsk/src/llvm.org 
 -lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py",
  line 25, in classCleanup
 cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
 NameError: global name 'self' is not defined
 
 vedant
 
> On Jan 30, 2018, at 10:29 AM, Adrian Prantl via lldb-commits 
> mailto:lldb-commits@lists.llvm.org>> wrote:
> 
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
> (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py 
> Tue Jan 30 10:29:16 2018
> @@ -22,7 +22,7 @@ class SettingsCommandTestCase(TestBase):
> @classmethod
> def classCleanup(cls):
> """Cleanup the test byproducts."""
> -cls.RemoveTempFile("stdout.txt")
> +cls.RemoveTempFile(self.getBuildArtifact("stdout.txt"))
 
>>> 
>> 
> 

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

Re: [Lldb-commits] [PATCH] D43912: [Symbol] Add InvalidType, a force-checked recoverable error

[Zachary Turner via lldb-commits]

What about having the macro do the log printing in a function so you can
put the breakpoint in the function?
On Wed, Feb 28, 2018 at 5:56 PM Vedant Kumar via Phabricator <
revi...@reviews.llvm.org> wrote:

> vsk added inline comments.
>
>
> 
> Comment at: include/lldb/Utility/Log.h:249-254
> +  ::lldb_private::Log *log_private = (log);
>   \
> +  if (log_private)
>  \
> +log_private->FormatError(::std::move(error_private), __FILE__,
>  \
> + __func__, "{0}");
>  \
> +  else
>  \
> +::llvm::consumeError(::std::move(error_private));
>   \
> 
> labath wrote:
> > it looks like this could be just handled by delegating to the
> LLDB_LOG_ERROR macro.
> I missed this earlier because I mistakenly named two variables
> error_private. At any rate, there's an objection to having a return_if
> macro so I'm setting it aside.
>
>
> https://reviews.llvm.org/D43912
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I like the way this patch is structured, some comments inline.




Comment at: 
packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4
+# encoding: utf-8
+"""
+Test lldb Obj-C exception support.
+"""

This looks like it doesn't need any interactivity whatsoever, so you can 
probably convert this to a lit test. See the examples in `lit/` (and please let 
me know if you have any questions.



Comment at: source/Plugins/Language/ObjC/NSArray.cpp:221-228
+namespace CallStackArray {
+  struct DataDescriptor_32 {
+uint32_t _data;
+uint32_t _used;
+uint32_t _offset;
+const uint32_t _size = 0;
+  };

Do you mind to add comments to explain what the structure fields represent (or 
what this structure is for)?



Comment at: source/Plugins/Language/ObjC/NSException.cpp:57-64
+  auto name = process_sp->ReadPointerFromMemory(ptr + 1 * ptr_size, error);
+  if (error.Fail() || name == LLDB_INVALID_ADDRESS) return false;
+  auto reason = process_sp->ReadPointerFromMemory(ptr + 2 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto userinfo = process_sp->ReadPointerFromMemory(ptr + 3 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto reserved = process_sp->ReadPointerFromMemory(ptr + 4 * ptr_size, error);

The fact we have to calculate this by hand is slightly annoying, but I guess 
that's a bug to fix another day.


https://reviews.llvm.org/D43884



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

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek added inline comments.



Comment at: 
packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4
+# encoding: utf-8
+"""
+Test lldb Obj-C exception support.
+"""

davide wrote:
> This looks like it doesn't need any interactivity whatsoever, so you can 
> probably convert this to a lit test. See the examples in `lit/` (and please 
> let me know if you have any questions.
Will do! I didn't realize we already have the lit infrastructure ready. Quick 
question: Can I test the SB layer from a lit test?



Comment at: source/Plugins/Language/ObjC/NSArray.cpp:221-228
+namespace CallStackArray {
+  struct DataDescriptor_32 {
+uint32_t _data;
+uint32_t _used;
+uint32_t _offset;
+const uint32_t _size = 0;
+  };

davide wrote:
> Do you mind to add comments to explain what the structure fields represent 
> (or what this structure is for)?
There's many definitions of array layout throughout this file, so I don't see a 
comment to be necessary here. Basically, this just defines another layout of an 
array, see above for other instances.



Comment at: source/Plugins/Language/ObjC/NSException.cpp:57-64
+  auto name = process_sp->ReadPointerFromMemory(ptr + 1 * ptr_size, error);
+  if (error.Fail() || name == LLDB_INVALID_ADDRESS) return false;
+  auto reason = process_sp->ReadPointerFromMemory(ptr + 2 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto userinfo = process_sp->ReadPointerFromMemory(ptr + 3 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto reserved = process_sp->ReadPointerFromMemory(ptr + 4 * ptr_size, error);

davide wrote:
> The fact we have to calculate this by hand is slightly annoying, but I guess 
> that's a bug to fix another day.
I'll be happy to send follow-up patches. Do you have suggestions how to avoid 
these manual pointer calculations?


https://reviews.llvm.org/D43884



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

[Lldb-commits] [PATCH] D43884: [lldb] Extract more fields from NSException values

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

LG modulo the test. Update that, I'll take another look and approve it. Thanks 
for your contribution!




Comment at: 
packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py:1-4
+# encoding: utf-8
+"""
+Test lldb Obj-C exception support.
+"""

kubamracek wrote:
> davide wrote:
> > This looks like it doesn't need any interactivity whatsoever, so you can 
> > probably convert this to a lit test. See the examples in `lit/` (and please 
> > let me know if you have any questions.
> Will do! I didn't realize we already have the lit infrastructure ready. Quick 
> question: Can I test the SB layer from a lit test?
No not really. The `lit` tests are a super recent addition :)



Comment at: source/Plugins/Language/ObjC/NSArray.cpp:221-228
+namespace CallStackArray {
+  struct DataDescriptor_32 {
+uint32_t _data;
+uint32_t _used;
+uint32_t _offset;
+const uint32_t _size = 0;
+  };

kubamracek wrote:
> davide wrote:
> > Do you mind to add comments to explain what the structure fields represent 
> > (or what this structure is for)?
> There's many definitions of array layout throughout this file, so I don't see 
> a comment to be necessary here. Basically, this just defines another layout 
> of an array, see above for other instances.
Fair enough.



Comment at: source/Plugins/Language/ObjC/NSException.cpp:57-64
+  auto name = process_sp->ReadPointerFromMemory(ptr + 1 * ptr_size, error);
+  if (error.Fail() || name == LLDB_INVALID_ADDRESS) return false;
+  auto reason = process_sp->ReadPointerFromMemory(ptr + 2 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto userinfo = process_sp->ReadPointerFromMemory(ptr + 3 * ptr_size, error);
+  if (error.Fail() || reason == LLDB_INVALID_ADDRESS) return false;
+  auto reserved = process_sp->ReadPointerFromMemory(ptr + 4 * ptr_size, error);

kubamracek wrote:
> davide wrote:
> > The fact we have to calculate this by hand is slightly annoying, but I 
> > guess that's a bug to fix another day.
> I'll be happy to send follow-up patches. Do you have suggestions how to avoid 
> these manual pointer calculations?
The way we do that in swift (or well, we're trying to do) is that of using 
remote mirrors. I'm not really aware of an equivalent mechanism in Obj-C, FWIW, 
but we should definitely think about it and have a chat (if you want, I don't 
think you should be signed up for cleaning up this).


https://reviews.llvm.org/D43884



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