[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think the conversion of different string types should be handled differently. 
Please see inline comment for details. Otherwise, this looks fine.




Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:326-338
   llvm::Expected GetAttribute(const char *name) const {
 if (!m_py_obj)
   return nullDeref();
 PyObject *obj = PyObject_GetAttrString(m_py_obj, name);
 if (!obj)
   return exception();
 return python::Take(obj);

This is the annoying part about interacting with external "c" apis in llvm. 
Llvm really likes to have StringRefs everywhere, but since they're not 
guaranteed to be null-terminated, it usually ends up requiring a string copy 
(`.str().c_str()`).
Having the wrapper APIs take `const char *` is not really a good option because 
that pushes the problem down to its users (and/or creates redundant strlen()s 
everywhere), whereas the purpose of these APIs should be to create an interface 
that interacts nicely with the rest of the codebase.

There are two ways  to get around that, depending on how much you want to 
optimize stuff:
# Just have your functions take a StringRef and call `.str().c_str()` when you 
need a null-terminated version of it
# Have your functions take a llvm::Twine. This class has a method 
,
 which enables you to avoid a string copy in the cases where the user passed in 
a null-terminated string, while avoiding to define multiple overloads of all 
your functions.
It is used as such:
```
auto my_foo_wrapper(const llvm::Twine &str) {
  llvm::SmallString<32 /*or whatever is the "typical" size*/> storage;
  llvm::StringRef s = str.toNullTerminatedStringRef(storage); // s is either 
the original const char *, or points to "storage"
  return foo(s.data() /*null terminated*/);
}
```
For this code I'd be fine with either option, but I'd like to avoid having two 
overloads of all functions (particularly if none of them is a StringRef).



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:401
 
-  void Reset(PyRefType type, PyObject *py_obj) {
-Reset();
+  void Reset() { PythonObject::Reset(); }
+

Is this needed, given the "using" declaration above?



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:406
+  TypedPythonObject(PyRefType type, PyObject *py_obj) {
+m_py_obj = nullptr;
 if (!py_obj)

Isn't this already set by the superclass constructor?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:720-728
+// This is only here to help incrementally migrate old, exception-unsafe
+// code.
+template  T unwrapIgnoringErrors(llvm::Expected expected) {
+  if (expected)
+return std::move(expected.get());
+  llvm::consumeError(expected.takeError());
+  return T();

Could you put this into the "python" namespace, since we already have it? 
Otherwise, it looks like a generic lldb_private function (which I would very 
much like to avoid it becoming).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133



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

[Lldb-commits] [PATCH] D69080: eliminate one form of PythonObject::Reset()

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:235-245
   PythonObject &operator=(const PythonObject &other) {
 Reset(PyRefType::Borrowed, other.get());
 return *this;
   }
 
-  void Reset(PythonObject &&other) {
+  PythonObject &operator=(PythonObject &&other) {
 Reset();

lawrence_danna wrote:
> labath wrote:
> > lawrence_danna wrote:
> > > labath wrote:
> > > > lawrence_danna wrote:
> > > > > labath wrote:
> > > > > > lawrence_danna wrote:
> > > > > > > labath wrote:
> > > > > > > > You can consider simplifying this further down to a 
> > > > > > > > "universal"/"sink" `operator=(PythonObject other)`. Since the 
> > > > > > > > object is really just a pointer, the extra object being created 
> > > > > > > > won't hurt (in fact, the removal of `&`-indirection might make 
> > > > > > > > things faster).
> > > > > > > wouldn't that result in an extra retain and release every time a 
> > > > > > > PythonObject was copied instead of referenced or moved?
> > > > > > No, it shouldn't, because the temporary PythonObject will be 
> > > > > > move-constructed (== no refcount traffic), if the operator= is 
> > > > > > called with an xvalue (if the rhs was not an xvalue, then you 
> > > > > > wouldn't end up calling the `&&` overload anyway). Then you can 
> > > > > > move the temporary object into *this, and avoid refcount traffic 
> > > > > > again.
> > > > > > 
> > > > > > So, there is an additional PythonObject created, but it's 
> > > > > > move-constructed if possible, which should be efficient, if I 
> > > > > > understand these classes correctly. This is the recommended 
> > > > > > practice (at least by some) when you don't want to squeeze every 
> > > > > > last nanosecond of performance..
> > > > > How do you move the temporary object into *this, if you only have 
> > > > > `operator=(PythonObject other)` to assign with?
> > > > In case that wasn't clear, the idea is to replace two operator= 
> > > > overloads with a single universal one taking a temporary. The advantage 
> > > > of that is less opportunities to implement move/copy incorrectly. The 
> > > > cost is one temporary move-constructed object more.
> > > so does that amount to just deleting the copy-assign, and keeping the 
> > > move-assign how it is?
> > > How do you move the temporary object into *this, if you only have 
> > > operator=(PythonObject other) to assign with?
> > 
> > You need to do it manually, like the current `&&` overload does, but you 
> > don't also need to implement the copy semantics in the const& overload.
> > 
> > > so does that amount to just deleting the copy-assign, and keeping the 
> > > move-assign how it is?
> > Almost. The implementation of move-assign would remain the same, but you'd 
> > drop the `&&` (otherwise you'd lose the ability to copy-assign) from the 
> > signature. I.e., 
> > ```
> > PythonObject &operator=(PythonObject other) {
> > Reset();
> > m_py_obj = std::exchange(other.m_py_obj, nullptr); // I just learned of 
> > this today so I have to show off.
> > return *this;
> >   }
> > ```
> oooh, i get it.   It didn't occur to me that you could treat other as a 
> rvalue without passing it as one in the parameter list, but of course you can 
> because if it's pass-by-value then it's just a local variable by the time 
> operator= gets its hands on it.
Yeah, isn't c++ fun? :)

I actually think this is one of the best features of c++11, as it allows you to 
write a single function that takes ownership of something more-or-less 
efficiently without any superfluous overloads or fancy `&&`s. The only problem 
is overcoming the learned "wisdom" that `const &` is the most efficient way to 
pass objects around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69080



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

[Lldb-commits] [PATCH] D69031: [LLDB] [test] Use %clang_cl instead of build.py in a few tests

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D69031#1713900 , @aprantl wrote:

> Check out  `lldb/test/Shell/helper/toolchain.py`, you probably need to filter 
> out some options for clang_cl specifically.


Yeah, I later found that. The issue is that it's passed to usr_clang in i 
llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags 
on both %clang, %clangxx, %clang_cc1 and %clang_cl.

Maybe the additional_flags param needs to be split, into one common for all, 
one for gcc-style driver, one for clang_cl, and maybe also a separate one for 
cc1 (where not all driver level flags may fit either)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69031



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

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for doing the investigation, and the detailed summary. I'm going to 
respond inline.

In D68968#1713788 , @wallace wrote:

> Thanks for your feedback and to @clayborg  for an offline discussion we had.
>
> I'm going to add three new attributes to the ProcessInfo class, with 
> corresponding parameters in the gdb-remote packet
>
> - display_name: This is a custom display name that the server can set for a 
> given process. This will supersede any other other attribute in the 
> GetProcessName call. There are interesting cases that are worth mentioning 
> for Android:
>   - Kernel threads and system processes are displayed like [kworker/u:0] 
> [khubd] by ps on Android. At least on modern devices ps.c identify those 
> processes as the ones with unreadable /proc//cmdline and unreadable 
> /proc//exe. The name is gotten from /proc//stat. There's some 
> generic information about this here 
> https://unix.stackexchange.com/questions/22121/what-do-the-brackets-around-processes-mean.
>  In this case both the actual executable and args are not known, so it's 
> better to leave those fields empty and store the display name in this new 
> field.
>   - Custom apk process names: apks can launch processes and subprocesses with 
> custom names. The custom name has the form com.example.app:custom_name. It's 
> always that way and it's documented here 
> https://developer.android.com/guide/topics/manifest/application-element#proc 
> (go to the android:process section). A given apk can have several 
> subprocesses, each one with a different custom name, e.g. 
> com.samsung.services:Core, com.samsung.services:Monitor, etc. In this case, 
> the package name is known and a display_name field would be useful to store 
> the entire custom name.


I think this is an excellent idea. It makes it clear that this is a separate 
concept, and that one should not rely on this being identical to any other 
concept (exe path, bundle it, etc.)

> 
> 
> - bundle_id: This will be exactly the apk package name or the iOS bundle name 
> inferred by the server. I plan to make a best effort guess for modern android 
> devices, leaving old devices as cases to implement as needed. I plan to use 
> `pm list packages` for this, given that this is the only reliable way to get 
> all the apk names. Besides, I'll check /proc/stat for the process name 
> and not arg0. It seems that /stat is not modifiable the same way arg0 is.

Having a bundle_id field in this struct seems fine. I'm much less enthusiastic 
about lldb-server "inferring" its contents. In fact, after reading the android 
docs you linked above, I'm starting to become opposed to the idea. I'll copy 
the interesting bits of that document:

  android:process
  The name of a process where all components of the application should run. 
Each component can override this default by setting its own process attribute.
  
  By default, Android creates a process for an application when the first 
of its components needs to run.

The way I read this is that this gives an application a fully documented way of 
changing it's process name to _anything_ it wants, including the name of 
another package (or at least, something that looks very similar to it). The app 
doesn't even have to go native to scribble into some random bytes in memory 
(which could be considered unsupported). I also think that going for 
/proc//stat is a downgrade, as the name there comes from /proc//comm, 
which is a file that can be written to by anyone with appropriate permissions 
(as opposed to argv[0], which requires permissions to write to the memory of 
that process -- a harder thing to come by). It is also limited to 16 
characters, so it likely will not contain the full package name.

What's the end goal here? My impression was that you're trying to to make 
"platform process list" output useful on android, but it seems to me like this 
is already achieved by having the "display name" field. Can we postpone this 
part until we actually need it? I'm hoping that at that point it will become 
clearer whether these hac^H^Heuristics are really needed, or if there's another 
way to achieve the same goal. For instance when running doing a "process attach 
PID", we can (reliably) get the user id that PID runs under, and then use "pm 
list" to tie that user id to a package name that can be given to the "run-as" 
command -- no need to guess something in the implementation of GetProcessInfo.

> 
> 
> - bundle_path: On android this is the path the apk path. This will also be 
> useful for iOS, because app bundles are also stored somewhere. Fortunately, 
> the command `pm list packages -f`, shows the list of apks along with their 
> corresponding paths on disk. This command doesn't work on old devices, but 
> still, I wouldn't prioritize them now.

Likewise, the bundle_path concept seems fine, but I have doubts about the 
methods used to retrieve it, part

[Lldb-commits] [PATCH] D67793: new api class: SBFile

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D67793#1713981 , @lawrence_danna 
wrote:

> In D67793#1692544 , @labath wrote:
>
> > BTW, I've had to add (or rather, extend) a fairly ugly hack in r373573 in 
> > order to get the new tests running on non-darwin platforms. The reason for 
> > that is that the FILE* out typemap needs to get the "mode" of the object in 
> > order to construct the python wrapper.
> >
> > My hope is that we can be improved once you're finished your work here. For 
> > instance by reimplementing `GetOutputFileHandle` on top of `GetOutputFile` 
> > in swig, as the latter will (hopefully) be able to get the mode of a file 
> > correctly. Does that sound reasonable, or am I totally off-track here?
>
>
> oops, sorry I missed this.   Are you happy with how it all turned out?


Yes, very much. Thank you for cleaning that up.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67793



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

[Lldb-commits] [PATCH] D69031: [LLDB] [test] Use %clang_cl instead of build.py in a few tests

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D69031#1714107 , @mstorsjo wrote:

> In D69031#1713900 , @aprantl wrote:
>
> > Check out  `lldb/test/Shell/helper/toolchain.py`, you probably need to 
> > filter out some options for clang_cl specifically.
>
>
> Yeah, I later found that. The issue is that it's passed to usr_clang in i 
> llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional 
> flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.
>
> Maybe the additional_flags param needs to be split, into one common for all, 
> one for gcc-style driver, one for clang_cl, and maybe also a separate one for 
> cc1 (where not all driver level flags may fit either)?


Actually, it seems to be that these arguments should not be added to the 
command line by default. All of the existing tests that do "%clang -target 
whatever" don't need the arguments, and they "only" work because the arguments 
do no harm because the tests don't do anything which would depend on them. I 
think we should leave `additional_flags` argument mostly empty, and instead 
have a separate way of invoking clang when building for the host. Either 
%clang_host (achievable with a recursive substitution %clang_host -> %clang 
), or using something like %clang %host_cflags.

I can have a stab at that, if you like, but I'm not sure I'll get around to 
that before the llvm conference is over...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69031



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

[Lldb-commits] [PATCH] D68961: Add support for DW_AT_export_symbols for anonymous structs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D68961#1713566 , @shafik wrote:

> We also have the lldb-cmake-matrix 
>  bot which 
> runs the test suite using older versions of clang which is there for exactly 
> this purpose to catch regressions due to features not supported by older 
> compiler versions. Which would catch any regressions here.


I'm afraid I have to disagree with that. Like I said the last time https://reviews.llvm.org/D66370>> when this topic came up, I don't think that 
this "matrix" testing is the way to test dwarf features. The "matrix" bot is 
very useful to catch regressions in parsing old debug info (because there will 
always be some quirk that you forget to think of), but I don't think there 
should be any piece of code that is _only_ tested this way, particularly when 
it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on 
on the mailing lists right now. One of the main questions being debated is 
whether it is ok to have a test which checks the assembly generated from some 
c(++) source code. And there's a lot of pushback saying that this is testing 
too much. However, even such a test is peanuts compared to 
clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the 
simplest lldb test we have, it runs the compiler front end, back end *and* 
assembler, and then reads the output hoping that the pipeline has generated the 
thing it wants to test. (And btw, it will fail on windows.)  It doesn't even 
check that the compiler has generated the thing it wants to check, so if the 
compiler output later changes to not include DW_AT_export_symbols, it will 
start to test something completely different, leaving the code added here 
uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds 
of changes happen all the time. Just last week, llvm started generating a very 
different style of location lists. Both lldb https://reviews.llvm.org/rL374769>> and dsymutil https://reviews.llvm.org/D69005>> had to be fixed to handle that. Both patches 
had tests, which were not based on running clang to generate the debug info 
(the dsymutil test had a checked in binary, which I am not a fan of, but that's 
a different story). Even so, it's very likely that this has regressed our 
coverage of the previous forms of location lists because we had no tests 
(except some basic ones I've added two or three months ago) which tested this 
code directly. And we only know about this because the initial implementation 
was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. 
Maybe it's not fair to ask you to add this kind of a test for the "old" style 
structs because that is past and not really "on you". However, I think we 
should be trying to add these kinds of tests for new features whenever is 
possible (another thing -- I imagine debugging failures from the matrix bot can 
be hard, though I've fortunately had not had to do that yet). I've been trying 
to do that for the past year, and while it takes a while to get used to, after 
some time I've gotten fairly efficient at generating/reducing "raw" dwarf. 
Besides being able to generate "stable" tests, this also enables one to 
generate tests for "incorrect" dwarf, which is one of the things which is 
impossible with the "clang -g" approach.


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

https://reviews.llvm.org/D68961



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

Re: [Lldb-commits] [lldb] r375146 - [Reproducer] Surface error if setting the cwd fails

[Pavel Labath via lldb-commits]

On 17/10/2019 19:58, Jonas Devlieghere via lldb-commits wrote:

+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+ cwd->end());


What is this hacking around? I can imagine a .rtrim() would be needed to 
remove a trailing cr/lf (though it would still be better and probably 
easy to just make sure the crlf is not added to the file), but this 
seems way over the top.


Btw, control characters can appear in file/directory names on most posix 
systems.

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

[Lldb-commits] [PATCH] D69105: minidump: Create memory regions from the sections of loaded modules

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:348-349
+  bool is_complete;
+  std::tie(*m_memory_regions, is_complete) =
+  m_minidump_parser->BuildMemoryRegions();
+

clayborg wrote:
> Might be nice to just assign memory regions and then ask the minidump parser 
> what its source of memory regions was? I am thinking of:
> 
> ```
> m_memory_regions = m_minidump_parser->BuildMemoryRegions();
> switch (m_minidump_parser->GetMemoryRegionsSource()) {
>   case MemoryInfoList:
>   case LinuxProcMaps:
> break;
>   case MemoryList:
> // Memory list is not always a exhaustive list of memory regions in a 
> process...
> // Insert code below the "if (is_complete)" in switch?
> ```
> 
> Actually thinking of this a bit more, we should really just do this work 
> inside "m_minidump_parser->BuildMemoryRegions();". The minidump itself can 
> then make the call and we don't need to return the "is_complete". Otherwise 
> if anyone else uses the info from "m_minidump_parser->BuildMemoryRegions();" 
> they would have to do the same thing as this code which would be bad. 
> 
I don't think that doing this work in the MinidumpParser is right, as the main 
reason for it's existence was to isolate minidump parsing from the stuff 
happening elsewhere in lldb (to enable unit testing and potentially be able to 
reuse it elsewhere). Now that a lot of the parsing has moved to llvm the second 
reason probably doesn't apply, but the unit testing separation is still nice to 
have. Even if we ignore that (and say have unit tests pass in an empty 
ModuleList), it still doesn't seem fully right because it's ProcessMinidump who 
creates the Module objects (MinidumpParser only works with minidump "modules"), 
so it seems natural to me that it should be the one who handles their 
regionizing too.


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

https://reviews.llvm.org/D69105



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

[Lldb-commits] [PATCH] D69106: MemoryRegion: Print "don't know" permission values as such

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

In D69106#1712949 , @clayborg wrote:

> Looks good. Do we maybe want to use "unknown" instead of "don't know" when 
> printing out the long for of a MemoryRegionInfo::OptionalBool? Or maybe "???"?


"don't know" is the name of the enum, and the same thing that the formatter 
printed previously. I don't think this value is be printed anywhere right now 
(except maybe some test failure messages), but if you want (I agree the name is 
somewhat informal), I can rename the change the enum and the formatter string 
at the same time.




Comment at: test/Shell/Minidump/memory-region-from-module.yaml:22
 # CHECK1: [0x4000-0x40b0) r-x 
{{.*}}memory-region-from-module.exe PT_LOAD[0]
-# TODO: This output does not give any indication that the region is only 
"potentially" writable.
-# CHECK2: [0x4000-0x4010) rwx
+# CHECK2: [0x4000-0x4010) r??
 # ALL-LABEL: (lldb) memory region 0x5000

clayborg wrote:
> How do we get two responses for a single address here?
These are two separate checks for the two FileCheck invocations above. The test 
runs the same lldb commands on two minidumps (their yaml below) which differ 
only in that the second one has a MemoryList stream covering the 0x4000 area. 
The test shows that in the first case (no MemoryList) we get the permissions 
from the module while in the second one, the minidump data "wins". (In this 
case I'm not really concerned with who wins, as that shouldn't happen in 
practice -- I'm mainly interested in making sure that we don't end up with a 
corrupted/overlapping memory regions)


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

https://reviews.llvm.org/D69106



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

[Lldb-commits] [lldb] r375221 - SystemInitializerCommon fix compilation on linux

[Pavel Labath via lldb-commits]

Author: labath
Date: Fri Oct 18 04:47:23 2019
New Revision: 375221

URL: http://llvm.org/viewvc/llvm-project?rev=375221&view=rev
Log:
SystemInitializerCommon fix compilation on linux

C++ defines two overloads of std::iscntrl. One in  and one in
. On linux we seem to include both which makes the std::erase_if
call ambiguous.

Wrap std::iscntrl call in a lambda to ensure regular overload
resolution.

Modified:
lldb/trunk/source/Initialization/SystemInitializerCommon.cpp

Modified: lldb/trunk/source/Initialization/SystemInitializerCommon.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Initialization/SystemInitializerCommon.cpp?rev=375221&r1=375220&r2=375221&view=diff
==
--- lldb/trunk/source/Initialization/SystemInitializerCommon.cpp (original)
+++ lldb/trunk/source/Initialization/SystemInitializerCommon.cpp Fri Oct 18 
04:47:23 2019
@@ -80,7 +80,8 @@ llvm::Error SystemInitializerCommon::Ini
 }
 if (llvm::Expected cwd =
 loader->LoadBuffer()) {
-  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(),
+[](char c) { return std::iscntrl(c); }),
  cwd->end());
   if (std::error_code ec = FileSystem::Instance()
.GetVirtualFileSystem()


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

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

[Pavel Labath via Phabricator via lldb-commits]

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

Address review comments


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

https://reviews.llvm.org/D69100

Files:
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  test/Shell/ObjectFile/PECOFF/export-dllfunc.yaml
  test/Shell/ObjectFile/PECOFF/sections.yaml
  test/Shell/ObjectFile/PECOFF/subsections.yaml

Index: test/Shell/ObjectFile/PECOFF/sections.yaml
===
--- test/Shell/ObjectFile/PECOFF/sections.yaml
+++ test/Shell/ObjectFile/PECOFF/sections.yaml
@@ -2,36 +2,36 @@
 # RUN: lldb-test object-file %t | FileCheck %s
 
 
-# CHECK:  Showing 1 sections
+# CHECK:  Showing 3 sections
 # CHECK-NEXT:   Index: 0
 # CHECK-NEXT:   ID: 0x
-# CHECK-NEXT:   Name:
-# CHECK-NEXT:   Type: container
+# CHECK-NEXT:   Name: PECOFF header
+# CHECK-NEXT:   Type: regular
 # CHECK-NEXT:   Permissions: ---
 # CHECK-NEXT:   Thread specific: no
 # CHECK-NEXT:   VM address: 0x4000
-# CHECK-NEXT:   VM size: 12288
-# CHECK-NEXT:   File size: 0
-# CHECK-NEXT:   Showing 2 subsections
-# CHECK-NEXT: Index: 0
-# CHECK-NEXT: ID: 0x1
-# CHECK-NEXT: Name: .text
-# CHECK-NEXT: Type: code
-# CHECK-NEXT: Permissions: ---
-# CHECK-NEXT: Thread specific: no
-# CHECK-NEXT: VM address: 0x40001000
-# CHECK-NEXT: VM size: 64
-# CHECK-NEXT: File size: 512
+# CHECK-NEXT:   VM size: 512
+# CHECK-NEXT:   File size: 512
 # CHECK-EMPTY: 
-# CHECK-NEXT: Index: 1
-# CHECK-NEXT: ID: 0x2
-# CHECK-NEXT: Name: .data
-# CHECK-NEXT: Type: data
-# CHECK-NEXT: Permissions: ---
-# CHECK-NEXT: Thread specific: no
-# CHECK-NEXT: VM address: 0x40002000
-# CHECK-NEXT: VM size: 64
-# CHECK-NEXT: File size: 512
+# CHECK-NEXT:   Index: 1
+# CHECK-NEXT:   ID: 0x1
+# CHECK-NEXT:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: ---
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x40001000
+# CHECK-NEXT:   VM size: 64
+# CHECK-NEXT:   File size: 512
+# CHECK-EMPTY:
+# CHECK-NEXT:   Index: 2
+# CHECK-NEXT:   ID: 0x2
+# CHECK-NEXT:   Name: .data
+# CHECK-NEXT:   Type: data
+# CHECK-NEXT:   Permissions: ---
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x40002000
+# CHECK-NEXT:   VM size: 64
+# CHECK-NEXT:   File size: 512
 
 
 --- !COFF
Index: test/Shell/ObjectFile/PECOFF/export-dllfunc.yaml
===
--- test/Shell/ObjectFile/PECOFF/export-dllfunc.yaml
+++ test/Shell/ObjectFile/PECOFF/export-dllfunc.yaml
@@ -11,20 +11,24 @@
 # UUID should not be empty if the module is built with debug info.
 # BASIC-CHECK-DAG: UUID: {{[0-9A-F]{7,}[0-9A-F]}}-{{.*}}
 
-# BASIC-CHECK: Showing 3 subsections
+# BASIC-CHECK: Showing 4 sections
+#
 # BASIC-CHECK:  Index: 0
+# BASIC-CHECK:  Name: PECOFF header
+#
+# BASIC-CHECK:  Index: 1
 # BASIC-CHECK:  Name: .text
 # BASIC-CHECK:  Type: code
 # BASIC-CHECK:  VM size: 22
 # BASIC-CHECK:  File size: 512
 #
-# BASIC-CHECK:  Index: 1
+# BASIC-CHECK:  Index: 2
 # BASIC-CHECK:  Name: .rdata
 # BASIC-CHECK:  Type: data
 # BASIC-CHECK:  VM size: {{.}}
 # BASIC-CHECK:  File size: 512
 #
-# BASIC-CHECK:  Index: 2
+# BASIC-CHECK:  Index: 3
 # BASIC-CHECK:  Name: .pdata
 # BASIC-CHECK:  Type: data
 # BASIC-CHECK:  VM size: 12
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -790,13 +790,15 @@
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
 
-SectionSP image_sp = std::make_shared(
-module_sp, this, ~user_id_t(0), ConstString(), eSectionTypeContainer,
-m_coff_header_opt.image_base, m_coff_header_opt.image_size,
-/*file_offset*/ 0, /*file_size*/ 0, m_coff_header_opt.sect_alignment,
+SectionSP header_sp = std::make_shared(
+module_sp, this, ~user_id_t(0), ConstString("PECOFF header"),
+eSectionTypeOther, m_coff_header_opt.image_base,
+m_coff_header_opt.header_size,
+/*file_offset*/ 0, m_coff_header_opt.header_size,
+m_coff_header_opt.sect_alignment,
 /*flags*/ 0);
-m_sections_up->AddSection(image_sp);
-unified_section_list.AddSection(image_sp);
+m_sections_up->AddSection(header_sp);
+unified_section_list.AddSection(header_sp);
 
 const uint32_t nsects = m_sect_headers.size();
 ModuleSP module_sp(GetModule());
@@ -901,15 +903,15 @@
   }
 
   SectionSP section_sp(new Section(
-  image_sp,// Parent section
   module_sp,   // Module to which this section belongs
   this,// Object file to which this section belongs
   idx + 1, // Section ID is the 1 based section inde

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:794
+SectionSP header_sp = std::make_shared(
+module_sp, this, ~user_id_t(0), ConstString("header"),
+eSectionTypeOther, m_coff_header_opt.image_base,

clayborg wrote:
> maybe name this "COFF header" or "PECOFF header"?
good idea.



Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:797
+m_coff_header_opt.header_size,
+/*file_offset*/ 0, m_coff_header_opt.header_size,
+m_coff_header_opt.sect_alignment,

jankratochvil wrote:
> dropped ///*file_size*///
somehow I thought that a non-trivial argument value does not need a comment, 
but you're right that this is still ambiguous. Adding it back.


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

https://reviews.llvm.org/D69100



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

[Lldb-commits] [PATCH] D69031: [LLDB] [test] Use %clang_cl instead of build.py in a few tests

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D69031#1714143 , @labath wrote:

> In D69031#1714107 , @mstorsjo wrote:
>
> > In D69031#1713900 , @aprantl wrote:
> >
> > > Check out  `lldb/test/Shell/helper/toolchain.py`, you probably need to 
> > > filter out some options for clang_cl specifically.
> >
> >
> > Yeah, I later found that. The issue is that it's passed to usr_clang in i 
> > llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional 
> > flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.
> >
> > Maybe the additional_flags param needs to be split, into one common for 
> > all, one for gcc-style driver, one for clang_cl, and maybe also a separate 
> > one for cc1 (where not all driver level flags may fit either)?
>
>
> Actually, it seems to be that these arguments should not be added to the 
> command line by default. All of the existing tests that do "%clang -target 
> whatever" don't need the arguments, and they "only" work because the 
> arguments do no harm because the tests don't do anything which would depend 
> on them. I think we should leave `additional_flags` argument mostly empty, 
> and instead have a separate way of invoking clang when building for the host. 
> Either %clang_host (achievable with a recursive substitution %clang_host -> 
> %clang ), or using something like %clang %host_cflags.
>
> I can have a stab at that, if you like, but I'm not sure I'll get around to 
> that before the llvm conference is over...


Ok, that makes sense.

I'd appreciate if you do that. I have no hurry wrt this patch as it's just a 
collateral fix I picked up along the way :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69031



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

[Lldb-commits] [PATCH] D69166: [LLDB] Do not take address of std::iscntrl in std::remove_if

[Kyrylo Bohdanenko via Phabricator via lldb-commits]

KyrBoh created this revision.
KyrBoh added a reviewer: MyDeveloperDay.
Herald added a reviewer: bollu.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Due to the fact, that getting an address of function in the standard library, 
LLDB fails to compile in my environment with en error saying "unable to find 
function to call for remove_if(..., )".

Just use the fix suggested by cppreference: wrap function into a lambda: 
https://en.cppreference.com/w/cpp/string/byte/iscntrl

Environment used:
OS: Ubuntu Eoan (19.10), x86_64
Compiler: g++ (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
CMake invocation:
cmake \

  
-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;libunwind;lldb;compiler-rt;lld;polly"
 \
  -DLLVM_TARGETS_TO_BUILD="X86" \
  -DLLVM_BUILD_LLVM_DYLIB=ON \
  -DCMAKE_BUILD_TYPE=Release \
  -DCMAKE_INSTALL_PREFIX=/opt/llvm-git \
  ../llvm-project/llvm


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69166

Files:
  lldb/source/Initialization/SystemInitializerCommon.cpp


Index: lldb/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/source/Initialization/SystemInitializerCommon.cpp
@@ -80,7 +80,7 @@
 }
 if (llvm::Expected cwd =
 loader->LoadBuffer()) {
-  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), [](char c) { return 
std::iscntrl(c); }),
  cwd->end());
   if (std::error_code ec = FileSystem::Instance()
.GetVirtualFileSystem()


Index: lldb/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/source/Initialization/SystemInitializerCommon.cpp
@@ -80,7 +80,7 @@
 }
 if (llvm::Expected cwd =
 loader->LoadBuffer()) {
-  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), [](char c) { return std::iscntrl(c); }),
  cwd->end());
   if (std::error_code ec = FileSystem::Instance()
.GetVirtualFileSystem()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69166: [LLDB] Do not take address of std::iscntrl in std::remove_if

[Kyrylo Bohdanenko via Phabricator via lldb-commits]

KyrBoh updated this revision to Diff 225607.
KyrBoh added a comment.

Correct patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69166

Files:
  lldb/source/Initialization/SystemInitializerCommon.cpp


Index: lldb/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/source/Initialization/SystemInitializerCommon.cpp
@@ -80,7 +80,7 @@
 }
 if (llvm::Expected cwd =
 loader->LoadBuffer()) {
-  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), [](unsigned char c) 
{ return std::iscntrl(c); }),
  cwd->end());
   if (std::error_code ec = FileSystem::Instance()
.GetVirtualFileSystem()


Index: lldb/source/Initialization/SystemInitializerCommon.cpp
===
--- lldb/source/Initialization/SystemInitializerCommon.cpp
+++ lldb/source/Initialization/SystemInitializerCommon.cpp
@@ -80,7 +80,7 @@
 }
 if (llvm::Expected cwd =
 loader->LoadBuffer()) {
-  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
+  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), [](unsigned char c) { return std::iscntrl(c); }),
  cwd->end());
   if (std::error_code ec = FileSystem::Instance()
.GetVirtualFileSystem()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69166: [LLDB] Do not take address of std::iscntrl in std::remove_if

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I already submitted the very same patch in r375221.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69166



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

[Lldb-commits] [PATCH] D69166: [LLDB] Do not take address of std::iscntrl in std::remove_if

[Kyrylo Bohdanenko via Phabricator via lldb-commits]

KyrBoh abandoned this revision.
KyrBoh added a comment.

Per labath's comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69166



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

[Lldb-commits] [PATCH] D69102: COFF: Set section permissions

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:800
 /*flags*/ 0);
+header_sp->SetPermissions(ePermissionsReadable);
 m_sections_up->AddSection(header_sp);

labath wrote:
> Are these the right permissions for the header?
I've dug around in some minidumps I have around and this does appear to be 
correct. It looks like the entire block of memory for the object is first 
allocated with PAGE_EXECUTE_WRITE_COPY, and the permissions for the header 
region are later changed to PAGE_READ_ONLY.


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

https://reviews.llvm.org/D69102



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

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

Looks and sounds good to me, although I'm not familiar with this code and its 
context yet - so I'm not sure if I'm qualified to formally approve it.


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

https://reviews.llvm.org/D69100



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

[Lldb-commits] [lldb] r375234 - Add REQUIRES: x86 to more tests which need the x86 llvm target built

[Pavel Labath via lldb-commits]

Author: labath
Date: Fri Oct 18 06:49:40 2019
New Revision: 375234

URL: http://llvm.org/viewvc/llvm-project?rev=375234&view=rev
Log:
Add REQUIRES: x86 to more tests which need the x86 llvm target built

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test
lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=375234&r1=375233&r2=375234&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Fri Oct 18 06:49:40 2019
@@ -332,6 +332,7 @@ class MiniDumpNewTestCase(TestBase):
 function_name = frame.GetFunctionName()
 self.assertTrue(name in function_name)
 
+@skipIfLLVMTargetMissing("X86")
 def test_deeper_stack_in_minidump(self):
 """Test that we can examine a more interesting stack in a Minidump."""
 # Launch with the Minidump, and inspect the stack.

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py?rev=375234&r1=375233&r2=375234&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
 Fri Oct 18 06:49:40 2019
@@ -90,6 +90,7 @@ class MiniDumpTestCase(TestBase):
 "fizzbuzz.syms", "has been added to", "fizzbuzz.exe"]),
 self.assertTrue(self.target.modules[0].FindSymbol("main"))
 
+@skipIfLLVMTargetMissing("X86")
 def test_stack_info_in_mini_dump(self):
 """Test that we can see a trivial stack in a VS-generate mini dump."""
 # target create -c fizzbuzz_no_heap.dmp

Modified: lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test?rev=375234&r1=375233&r2=375234&view=diff
==
--- lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test 
(original)
+++ lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test Fri Oct 
18 06:49:40 2019
@@ -1,3 +1,5 @@
+# REQUIRES: x86
+
 # RUN: yaml2obj %S/Inputs/unwind-via-stack-cfi.yaml > %t
 # RUN: %lldb -c %t -o "target symbols add %S/Inputs/unwind-via-stack-cfi.syms" 
\
 # RUN:   -s %s -b | FileCheck %s

Modified: lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test?rev=375234&r1=375233&r2=375234&view=diff
==
--- lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test 
(original)
+++ lldb/trunk/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test Fri Oct 
18 06:49:40 2019
@@ -1,3 +1,5 @@
+# REQUIRES: x86
+
 # RUN: yaml2obj %S/Inputs/unwind-via-stack-win.yaml > %t
 # RUN: %lldb -c %t \
 # RUN:   -o "target symbols add %S/Inputs/unwind-via-stack-win.syms" \


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

[Lldb-commits] [PATCH] D69102: COFF: Set section permissions

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo accepted this revision.
mstorsjo added inline comments.
This revision is now accepted and ready to land.



Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:800
 /*flags*/ 0);
+header_sp->SetPermissions(ePermissionsReadable);
 m_sections_up->AddSection(header_sp);

labath wrote:
> labath wrote:
> > Are these the right permissions for the header?
> I've dug around in some minidumps I have around and this does appear to be 
> correct. It looks like the entire block of memory for the object is first 
> allocated with PAGE_EXECUTE_WRITE_COPY, and the permissions for the header 
> region are later changed to PAGE_READ_ONLY.
I haven't checked, but I certainly would expect it to be readonly.


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

https://reviews.llvm.org/D69102



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

[Lldb-commits] [lldb] r375242 - Update MinidumpYAML to use minidump::Exception for exception stream

[Joseph Tremoulet via lldb-commits]

Author: josepht
Date: Fri Oct 18 07:56:19 2019
New Revision: 375242

URL: http://llvm.org/viewvc/llvm-project?rev=375242&view=rev
Log:
Update MinidumpYAML to use minidump::Exception for exception stream

Reviewers: labath, jhenderson, clayborg, MaskRay, grimar

Reviewed By: grimar

Subscribers: lldb-commits, grimar, MaskRay, hiraditya, llvm-commits

Tags: #llvm, #lldb

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

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml?rev=375242&r1=375241&r2=375242&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml
 Fri Oct 18 07:56:19 2019
@@ -14,7 +14,10 @@ Streams:
 Module Name: '/tmp/test/linux-x86_64'
 CodeView Record: 4C457042E35C283BC327C28762DB788BF5A4078BE2351448
   - Type:Exception
-Content: 
DD740B00D004F831
+Thread ID:   0x74DD
+Exception Record:
+  Exception Code:  0x000B
+Thread Context:  
   - Type:SystemInfo
 Processor Arch:  AMD64
 Processor Level: 6


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

[Lldb-commits] [lldb] r375243 - LLDB: Use LLVM's type for minidump ExceptionStream [NFC]

[Joseph Tremoulet via lldb-commits]

Author: josepht
Date: Fri Oct 18 07:59:10 2019
New Revision: 375243

URL: http://llvm.org/viewvc/llvm-project?rev=375243&view=rev
Log:
LLDB: Use LLVM's type for minidump ExceptionStream [NFC]

Summary: The types defined for it in LLDB are now redundant with core types.

Reviewers: labath, clayborg

Reviewed By: clayborg

Subscribers: lldb-commits

Tags: #lldb

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

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

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=375243&r1=375242&r2=375243&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Fri Oct 18 
07:59:10 2019
@@ -314,13 +314,15 @@ std::vector Mi
   return filtered_modules;
 }
 
-const MinidumpExceptionStream *MinidumpParser::GetExceptionStream() {
-  llvm::ArrayRef data = GetStream(StreamType::Exception);
+const minidump::ExceptionStream *MinidumpParser::GetExceptionStream() {
+  auto ExpectedStream = GetMinidumpFile().getExceptionStream();
+  if (ExpectedStream)
+return &*ExpectedStream;
 
-  if (data.size() == 0)
-return nullptr;
-
-  return MinidumpExceptionStream::Parse(data);
+  LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS),
+ ExpectedStream.takeError(),
+ "Failed to read minidump exception stream: {0}");
+  return nullptr;
 }
 
 llvm::Optional

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h?rev=375243&r1=375242&r2=375243&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h Fri Oct 18 
07:59:10 2019
@@ -82,7 +82,7 @@ public:
   // have the same name, it keeps the copy with the lowest load address.
   std::vector GetFilteredModuleList();
 
-  const MinidumpExceptionStream *GetExceptionStream();
+  const llvm::minidump::ExceptionStream *GetExceptionStream();
 
   llvm::Optional FindMemoryRange(lldb::addr_t addr);
 

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp?rev=375243&r1=375242&r2=375243&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp Fri Oct 18 
07:59:10 2019
@@ -57,17 +57,6 @@ LinuxProcStatus::Parse(llvm::ArrayRef &data) {
-  const MinidumpExceptionStream *exception_stream = nullptr;
-  Status error = consumeObject(data, exception_stream);
-  if (error.Fail())
-return nullptr;
-
-  return exception_stream;
-}
-
 std::pair, uint64_t>
 MinidumpMemoryDescriptor64::ParseMemory64List(llvm::ArrayRef &data) {
   const llvm::support::ulittle64_t *mem_ranges_count;

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h?rev=375243&r1=375242&r2=375243&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h Fri Oct 18 
07:59:10 2019
@@ -118,35 +118,6 @@ private:
   LinuxProcStatus() = default;
 };
 
-// Exception stuff
-struct MinidumpException {
-  enum : unsigned {
-ExceptonInfoMaxParams = 15,
-DumpRequested = 0x,
-  };
-
-  llvm::support::ulittle32_t exception_code;
-  llvm::support::ulittle32_t exception_flags;
-  llvm::support::ulittle64_t exception_record;
-  llvm::support::ulittle64_t exception_address;
-  llvm::support::ulittle32_t number_parameters;
-  llvm::support::ulittle32_t unused_alignment;
-  llvm::support::ulittle64_t exception_information[ExceptonInfoMaxParams];
-};
-static_assert(sizeof(MinidumpException) == 152,
-  "sizeof MinidumpException is not correct!");
-
-struct MinidumpExceptionStream {
-  llvm::support::ulittle32_t thread_id;
-  llvm::support::ulittle32_t alignment;
-  MinidumpException exception_r

[Lldb-commits] [lldb] r375244 - ProcessMinidump: Suppress reporting stop for signal '0'

[Joseph Tremoulet via lldb-commits]

Author: josepht
Date: Fri Oct 18 08:02:16 2019
New Revision: 375244

URL: http://llvm.org/viewvc/llvm-project?rev=375244&view=rev
Log:
ProcessMinidump: Suppress reporting stop for signal '0'

Summary:
The minidump exception stream can report an exception record with
signal 0.  If we try to create a stop reason with signal zero, processing
of the stop event won't find anything, and the debugger will hang.
So, simply early-out of RefreshStateAfterStop in this case.

Also set the UnixSignals object in DoLoadCore as is done for
ProcessElfCore.

Reviewers: labath, clayborg, jfb

Reviewed By: labath, clayborg

Subscribers: dexonsmith, lldb-commits

Tags: #lldb

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

Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=375244&r1=375243&r2=375244&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Fri Oct 18 08:02:16 2019
@@ -153,9 +153,9 @@ class MiniDumpNewTestCase(TestBase):
 self.assertTrue(eip.IsValid())
 self.assertEqual(pc, eip.GetValueAsUnsigned())
 
-def test_snapshot_minidump(self):
+def test_snapshot_minidump_dump_requested(self):
 """Test that if we load a snapshot minidump file (meaning the process
-did not crash) there is no stop reason."""
+did not crash) with exception code "DUMP_REQUESTED" there is no stop 
reason."""
 # target create -c linux-x86_64_not_crashed.dmp
 self.dbg.CreateTarget(None)
 self.target = self.dbg.GetSelectedTarget()
@@ -163,6 +163,17 @@ class MiniDumpNewTestCase(TestBase):
 self.check_state()
 self.assertEqual(self.process.GetNumThreads(), 1)
 thread = self.process.GetThreadAtIndex(0)
+self.assertEqual(thread.GetStopReason(), lldb.eStopReasonNone)
+stop_description = thread.GetStopDescription(256)
+self.assertEqual(stop_description, "")
+
+def test_snapshot_minidump_null_exn_code(self):
+"""Test that if we load a snapshot minidump file (meaning the process
+did not crash) with exception code zero there is no stop reason."""
+self.process_from_yaml("linux-x86_64_null_signal.yaml")
+self.check_state()
+self.assertEqual(self.process.GetNumThreads(), 1)
+thread = self.process.GetThreadAtIndex(0)
 self.assertEqual(thread.GetStopReason(), lldb.eStopReasonNone)
 stop_description = thread.GetStopDescription(256)
 self.assertEqual(stop_description, "")

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml?rev=375244&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
 Fri Oct 18 08:02:16 2019
@@ -0,0 +1,25 @@
+--- !minidump
+Streams:
+  - Type:ThreadList
+Threads:
+  - Thread Id:   0x2177
+Context: 
+Stack:
+  Start of Memory Range: 0x7FFE2F689000
+  Content: 
+  - Type:Exception
+Thread ID:   0x2177
+Exception Record:
+  Exception Code:  0x
+  Exception Address: 0x00400582
+Thread Context:  
+  - Type:SystemInfo
+Processor Arch:  AMD64
+Platform ID: Linux
+  - Type:LinuxProcStatus
+Text: |
+  Name:busyloop
+  Umask:   0002
+  State:   t (tracing stop)
+  Pid: 8567
+...

Modified: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp?rev=375244&r1=375243&r2=375244&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp (original)
+++ lldb/trunk/source/Plugi

[Lldb-commits] [PATCH] D68657: Update MinidumpYAML to use minidump::Exception for exception stream

[Joseph Tremoulet via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa50272f8261f: Update MinidumpYAML to use minidump::Exception 
for exception stream (authored by JosephTremoulet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64.yaml
  llvm/include/llvm/ObjectYAML/MinidumpYAML.h
  llvm/lib/ObjectYAML/MinidumpEmitter.cpp
  llvm/lib/ObjectYAML/MinidumpYAML.cpp
  llvm/test/tools/obj2yaml/basic-minidump.yaml
  llvm/test/tools/yaml2obj/minidump-exception-missing-parameter.yaml
  llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp

Index: llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
===
--- llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
+++ llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
@@ -139,3 +139,200 @@
   (ArrayRef{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}),
   makeArrayRef(SysInfo.CPU.Other.ProcessorFeatures));
 }
+
+// Test that we can parse a normal-looking ExceptionStream.
+TEST(MinidumpYAML, ExceptionStream) {
+  SmallString<0> Storage;
+  auto ExpectedFile = toBinary(Storage, R"(
+--- !minidump
+Streams:
+  - Type:Exception
+Thread ID:  0x7
+Exception Record:
+  Exception Code:  0x23
+  Exception Flags: 0x5
+  Exception Record: 0x0102030405060708
+  Exception Address: 0x0a0b0c0d0e0f1011
+  Number of Parameters: 2
+  Parameter 0: 0x22
+  Parameter 1: 0x24
+Thread Context:  3DeadBeefDefacedABadCafe)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  object::MinidumpFile &File = **ExpectedFile;
+
+  ASSERT_EQ(1u, File.streams().size());
+
+  Expected ExpectedStream =
+  File.getExceptionStream();
+
+  ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
+
+  const minidump::ExceptionStream &Stream = *ExpectedStream;
+  EXPECT_EQ(0x7u, Stream.ThreadId);
+  const minidump::Exception &Exception = Stream.ExceptionRecord;
+  EXPECT_EQ(0x23u, Exception.ExceptionCode);
+  EXPECT_EQ(0x5u, Exception.ExceptionFlags);
+  EXPECT_EQ(0x0102030405060708u, Exception.ExceptionRecord);
+  EXPECT_EQ(0x0a0b0c0d0e0f1011u, Exception.ExceptionAddress);
+  EXPECT_EQ(2u, Exception.NumberParameters);
+  EXPECT_EQ(0x22u, Exception.ExceptionInformation[0]);
+  EXPECT_EQ(0x24u, Exception.ExceptionInformation[1]);
+
+  Expected> ExpectedContext =
+  File.getRawData(Stream.ThreadContext);
+  ASSERT_THAT_EXPECTED(ExpectedContext, Succeeded());
+  EXPECT_EQ((ArrayRef{0x3d, 0xea, 0xdb, 0xee, 0xfd, 0xef, 0xac, 0xed,
+   0xab, 0xad, 0xca, 0xfe}),
+*ExpectedContext);
+}
+
+// Test that we can parse an exception stream with no ExceptionInformation.
+TEST(MinidumpYAML, ExceptionStream_NoParameters) {
+  SmallString<0> Storage;
+  auto ExpectedFile = toBinary(Storage, R"(
+--- !minidump
+Streams:
+  - Type:Exception
+Thread ID:  0x7
+Exception Record:
+  Exception Code:  0x23
+  Exception Flags: 0x5
+  Exception Record: 0x0102030405060708
+  Exception Address: 0x0a0b0c0d0e0f1011
+Thread Context:  3DeadBeefDefacedABadCafe)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  object::MinidumpFile &File = **ExpectedFile;
+
+  ASSERT_EQ(1u, File.streams().size());
+
+  Expected ExpectedStream =
+  File.getExceptionStream();
+
+  ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
+
+  const minidump::ExceptionStream &Stream = *ExpectedStream;
+  EXPECT_EQ(0x7u, Stream.ThreadId);
+  const minidump::Exception &Exception = Stream.ExceptionRecord;
+  EXPECT_EQ(0x23u, Exception.ExceptionCode);
+  EXPECT_EQ(0x5u, Exception.ExceptionFlags);
+  EXPECT_EQ(0x0102030405060708u, Exception.ExceptionRecord);
+  EXPECT_EQ(0x0a0b0c0d0e0f1011u, Exception.ExceptionAddress);
+  EXPECT_EQ(0u, Exception.NumberParameters);
+
+  Expected> ExpectedContext =
+  File.getRawData(Stream.ThreadContext);
+  ASSERT_THAT_EXPECTED(ExpectedContext, Succeeded());
+  EXPECT_EQ((ArrayRef{0x3d, 0xea, 0xdb, 0xee, 0xfd, 0xef, 0xac, 0xed,
+   0xab, 0xad, 0xca, 0xfe}),
+*ExpectedContext);
+}
+
+// Test that we can parse an ExceptionStream where the stated number of
+// parameters is greater than the actual size of the ExceptionInformation
+// array.
+TEST(MinidumpYAML, ExceptionStream_TooManyParameters) {
+  SmallString<0> Storage;
+  auto ExpectedFile = toBinary(Storage, R"(
+--- !minidump
+Streams:
+  - Type:Exception
+Thread ID:  0x8
+Exception Record:
+  Exception Code: 0
+  Number of Parameters: 16
+  Parameter 0: 0x0
+  Parameter 1: 0xff
+  Parameter 2: 0xee
+  Parameter 3: 0xdd
+  Parameter 4: 0xcc
+  Parameter 5: 0xbb
+  Parameter 6: 0xaa
+  Parameter 7: 0x99
+  Parameter 8: 0x88
+  Parameter 9: 0x77
+  Parameter 10: 0x66
+ 

[Lldb-commits] [PATCH] D68658: LLDB: Use LLVM's type for minidump ExceptionStream [NFC]

[Joseph Tremoulet via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd094d97d0223: LLDB: Use LLVM's type for minidump 
ExceptionStream [NFC] (authored by JosephTremoulet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68658

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/MinidumpTypes.cpp
  lldb/source/Plugins/Process/minidump/MinidumpTypes.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/source/Plugins/Process/minidump/ProcessMinidump.h
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -252,10 +252,10 @@
 
 TEST_F(MinidumpParserTest, GetExceptionStream) {
   SetUpData("linux-x86_64.dmp");
-  const MinidumpExceptionStream *exception_stream =
+  const llvm::minidump::ExceptionStream *exception_stream =
   parser->GetExceptionStream();
   ASSERT_NE(nullptr, exception_stream);
-  ASSERT_EQ(11UL, exception_stream->exception_record.exception_code);
+  ASSERT_EQ(11UL, exception_stream->ExceptionRecord.ExceptionCode);
 }
 
 void check_mem_range_exists(MinidumpParser &parser, const uint64_t range_start,
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.h
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -108,7 +108,7 @@
   FileSpec m_core_file;
   lldb::DataBufferSP m_core_data;
   llvm::ArrayRef m_thread_list;
-  const MinidumpExceptionStream *m_active_exception;
+  const minidump::ExceptionStream *m_active_exception;
   lldb::CommandObjectSP m_command_sp;
   bool m_is_wow64;
 };
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -241,36 +241,45 @@
   if (!m_active_exception)
 return;
 
-  if (m_active_exception->exception_record.exception_code ==
-  MinidumpException::DumpRequested) {
+  constexpr uint32_t BreakpadDumpRequested = 0x;
+  if (m_active_exception->ExceptionRecord.ExceptionCode ==
+  BreakpadDumpRequested) {
+// This "ExceptionCode" value is a sentinel that is sometimes used
+// when generating a dump for a process that hasn't crashed.
+
+// TODO: The definition and use of this "dump requested" constant
+// in Breakpad are actually Linux-specific, and for similar use
+// cases on Mac/Windows it defines differnt constants, referring
+// to them as "simulated" exceptions; consider moving this check
+// down to the OS-specific paths and checking each OS for its own
+// constant.
 return;
   }
 
   lldb::StopInfoSP stop_info;
   lldb::ThreadSP stop_thread;
 
-  Process::m_thread_list.SetSelectedThreadByID(m_active_exception->thread_id);
+  Process::m_thread_list.SetSelectedThreadByID(m_active_exception->ThreadId);
   stop_thread = Process::m_thread_list.GetSelectedThread();
   ArchSpec arch = GetArchitecture();
 
   if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
 stop_info = StopInfo::CreateStopReasonWithSignal(
-*stop_thread, m_active_exception->exception_record.exception_code);
+*stop_thread, m_active_exception->ExceptionRecord.ExceptionCode);
   } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
 stop_info = StopInfoMachException::CreateStopReasonWithMachException(
-*stop_thread, m_active_exception->exception_record.exception_code, 2,
-m_active_exception->exception_record.exception_flags,
-m_active_exception->exception_record.exception_address, 0);
+*stop_thread, m_active_exception->ExceptionRecord.ExceptionCode, 2,
+m_active_exception->ExceptionRecord.ExceptionFlags,
+m_active_exception->ExceptionRecord.ExceptionAddress, 0);
   } else {
 std::string desc;
 llvm::raw_string_ostream desc_stream(desc);
 desc_stream << "Exception "
 << llvm::format_hex(
-   m_active_exception->exception_record.exception_code, 8)
+   m_active_exception->ExceptionRecord.ExceptionCode, 8)
 << " encountered at address "
 << llvm::format_hex(
-   m_active_exception->exception_record.exception_address,
-   8);
+   m_active_exception->ExceptionRecord.ExceptionAddress, 8);
 stop_info = StopInfo::CreateStopReasonWithException(
 *stop_thread, desc_stream.str().c_str());

[Lldb-commits] [PATCH] D68096: ProcessMinidump: Suppress reporting stop for signal '0'

[Joseph Tremoulet via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG77460d3888c5: ProcessMinidump: Suppress reporting stop for 
signal '0' (authored by JosephTremoulet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68096

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -219,6 +219,9 @@
 
   m_thread_list = m_minidump_parser->GetThreads();
   m_active_exception = m_minidump_parser->GetExceptionStream();
+
+  SetUnixSignals(UnixSignals::Create(GetArchitecture()));
+
   ReadModuleList();
 
   llvm::Optional pid = m_minidump_parser->GetPid();
@@ -238,6 +241,7 @@
 Status ProcessMinidump::DoDestroy() { return Status(); }
 
 void ProcessMinidump::RefreshStateAfterStop() {
+
   if (!m_active_exception)
 return;
 
@@ -264,8 +268,15 @@
   ArchSpec arch = GetArchitecture();
 
   if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
+uint32_t signo = m_active_exception->ExceptionRecord.ExceptionCode;
+
+if (signo == 0) {
+  // No stop.
+  return;
+}
+
 stop_info = StopInfo::CreateStopReasonWithSignal(
-*stop_thread, m_active_exception->ExceptionRecord.ExceptionCode);
+*stop_thread, signo);
   } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
 stop_info = StopInfoMachException::CreateStopReasonWithMachException(
 *stop_thread, m_active_exception->ExceptionRecord.ExceptionCode, 2,
Index: lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_null_signal.yaml
@@ -0,0 +1,25 @@
+--- !minidump
+Streams:
+  - Type:ThreadList
+Threads:
+  - Thread Id:   0x2177
+Context: 
+Stack:
+  Start of Memory Range: 0x7FFE2F689000
+  Content: 
+  - Type:Exception
+Thread ID:   0x2177
+Exception Record:
+  Exception Code:  0x
+  Exception Address: 0x00400582
+Thread Context:  
+  - Type:SystemInfo
+Processor Arch:  AMD64
+Platform ID: Linux
+  - Type:LinuxProcStatus
+Text: |
+  Name:	busyloop
+  Umask:	0002
+  State:	t (tracing stop)
+  Pid:	8567
+...
Index: lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
===
--- lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -153,9 +153,9 @@
 self.assertTrue(eip.IsValid())
 self.assertEqual(pc, eip.GetValueAsUnsigned())
 
-def test_snapshot_minidump(self):
+def test_snapshot_minidump_dump_requested(self):
 """Test that if we load a snapshot minidump file (meaning the process
-did not crash) there is no stop reason."""
+did not crash) with exception code "DUMP_REQUESTED" there is no stop reason."""
 # target create -c linux-x86_64_not_crashed.dmp
 self.dbg.CreateTarget(None)
 self.target = self.dbg.GetSelectedTarget()
@@ -167,6 +167,17 @@
 stop_description = thread.GetStopDescription(256)
 self.assertEqual(stop_description, "")
 
+def test_snapshot_minidump_null_exn_code(self):
+"""Test that if we load a snapshot minidump file (meaning the process
+did not crash) with exception code zero there is no stop reason."""
+self.process_from_yaml("linux-x86_64_null_signal.yaml")
+self.check_state()
+self.assertEqual(self.process.GetNumThreads(), 1)
+thread = self.process.GetThreadAtIndex(0)
+self.assertEqual(thread.GetStopReason(), lldb.eStopReasonNone)
+stop_description = thread.GetStopDescription(256)
+self.assertEqual(stop_description, "")
+
 def check_register_unsigned(self, set, name, expected):
 reg_value = set.GetChildMemberWithName(name)
 self.assertTrue(reg_value.IsValid(),
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Vedant Kumar via Phabricator via lldb-commits]

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



Comment at: llvm/lib/Support/Unix/Signals.inc:372
   if (Sig == SIGPIPE)
-exit(EX_IOERR);
+if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+  CurrentPipeFunction();

jordan_rose wrote:
> Should it be doing the same sort of `exchange` as the interrupt function?
The interrupt handler is apparently meant to be disabled after it’s used once 
(not sure why, the docs don’t say). As lldb is long-lived, I expect it should 
survive multiple SIGPIPEs.


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

https://reviews.llvm.org/D69148



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

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks for fixing this, Vedant!


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

https://reviews.llvm.org/D69148



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

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

man, thanks for your feedback!
Indeed, i was trying to solve some problems that don't exist yet. Later when I 
add apk debugging support, I'll figure out what's the best way to retrieve apk 
specific information, but for now display_name is more than enough


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68968



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

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Jordan Rose via Phabricator via lldb-commits]

jordan_rose added inline comments.



Comment at: llvm/lib/Support/Unix/Signals.inc:372
   if (Sig == SIGPIPE)
-exit(EX_IOERR);
+if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+  CurrentPipeFunction();

vsk wrote:
> jordan_rose wrote:
> > Should it be doing the same sort of `exchange` as the interrupt function?
> The interrupt handler is apparently meant to be disabled after it’s used once 
> (not sure why, the docs don’t say). As lldb is long-lived, I expect it should 
> survive multiple SIGPIPEs.
I guess the SIGINT one makes sense so that you can kill a stuck process with 
^C^C.


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

https://reviews.llvm.org/D69148



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

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

sounds like we have a plan!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68968



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

[Lldb-commits] [PATCH] D69106: MemoryRegion: Print "don't know" permission values as such

[Greg Clayton via Phabricator via lldb-commits]

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

Sounds good, thanks for doing this


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

https://reviews.llvm.org/D69106



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

Re: [Lldb-commits] [lldb] r375146 - [Reproducer] Surface error if setting the cwd fails

[Jonas Devlieghere via lldb-commits]

You're right. I've replaced it with rtrim in r375259.

On Fri, Oct 18, 2019 at 4:12 AM Pavel Labath  wrote:
>
> On 17/10/2019 19:58, Jonas Devlieghere via lldb-commits wrote:
> > +  cwd->erase(std::remove_if(cwd->begin(), cwd->end(), std::iscntrl),
> > + cwd->end());
>
> What is this hacking around? I can imagine a .rtrim() would be needed to
> remove a trailing cr/lf (though it would still be better and probably
> easy to just make sure the crlf is not added to the file), but this
> seems way over the top.
>
> Btw, control characters can appear in file/directory names on most posix
> systems.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

In D68130#1713620 , @teemperor wrote:

> Yeah, seems like constructing objects in expressions isn't implemented on 
> Windows. I'm not sure if there is a reliable way to test that constructors 
> aren't shadowed by these global functions if constructors themselves don't 
> work on Windows, but I filed llvm.org/pr43707 for the underlying bug and 
> x-failed the tests. Will push in a few minutes.


Thanks!

Btw, I also see TestCallOverriddenMethod failing on Ubuntu but there's no 
official bot for that.

FAIL: test_call_on_temporary_dwarf 
(TestCallOverriddenMethod.ExprCommandCallOverriddenMethod)

  Test calls to overridden methods in derived classes.

--

Traceback (most recent call last):

  File 
"/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 1748, in test_method
return attrvalue(self)
  File 
"/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/decorators.py",
 line 111, in wrapper
func(*args, **kwargs)
  File 
"/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py",
 line 71, in test_call_on_temporary
self.expect("expr Base().foo()", substrs=["1"])
  File 
"/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 2309, in expect
inHistory=inHistory)
  File 
"/home/e2admin/vstsdrive/_work/115/s/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 2068, in runCmd
msg if (msg) else CMD_MSG(cmd))

AssertionError: False is not True : Command 'expr Base().foo()
Error output:
error: Execution was interrupted, reason: signal SIGABRT.
The process has been returned to the state before expression evaluation.
' returns successfully
Config=x86_64-/home/e2admin/vstsdrive/_work/115/b/bin/clang-10


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68130



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

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Vedant Kumar via Phabricator via lldb-commits]

vsk planned changes to this revision.
vsk added a comment.

The death tests are flaky. I've noticed two issues:

1. When run under lit, the DisableExitOnSIGPIPE doesn't actually exit when it 
receives SIGPIPE. Dtrace suggests that the unit test process inherits an 
"ignore" sigaction from its parent.
2. When run in parallel, exiting causes the unit test to run atexit destructors 
for other unit tests lumped into the SupportTests binary. One of these is a 
condition_variable destructor and it hangs. Also, gtest warns: `Death tests use 
fork(), which is unsafe particularly in a threaded context. For this test, 
Google Test detected 21 threads.`

So I plan to give up on testing "EnableExitOnSIGPIPE" and will write a 
non-death test to get some coverage.


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

https://reviews.llvm.org/D69148



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

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Vedant Kumar via Phabricator via lldb-commits]

vsk updated this revision to Diff 225661.
vsk added a comment.
This revision is now accepted and ready to land.

Replace the death tests with a non-death test. I plan to commit this later in 
the day if there aren't any objections.


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

https://reviews.llvm.org/D69148

Files:
  lldb/tools/driver/Driver.cpp
  llvm/include/llvm/Support/Signals.h
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/SignalsTest.cpp

Index: llvm/unittests/Support/SignalsTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/SignalsTest.cpp
@@ -0,0 +1,53 @@
+//- unittests/Support/SignalsTest.cpp - Signal handling test =//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if !defined(_WIN32)
+#include 
+#include 
+#endif // !defined(_WIN32)
+
+#include "llvm/Support/Signals.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+#if !defined(_WIN32)
+TEST(SignalTest, IgnoreMultipleSIGPIPEs) {
+  // Ignore SIGPIPE.
+  signal(SIGPIPE, SIG_IGN);
+
+  // Disable exit-on-SIGPIPE.
+  sys::SetPipeSignalFunction(nullptr);
+
+  // Create unidirectional read/write pipes.
+  int fds[2];
+  int err = pipe(fds);
+  if (err != 0)
+return; // If we can't make pipes, this isn't testing anything.
+
+  // Close the read pipe.
+  close(fds[0]);
+
+  // Attempt to write to the write pipe. Currently we're asserting that the
+  // write fails, which isn't great.
+  //
+  // What we really want is a death test that checks that this block exits
+  // with a special exit "success" code, as opposed to unexpectedly exiting due
+  // to a kill-by-SIGNAL or due to the default SIGPIPE handler.
+  //
+  // Unfortunately llvm's unit tests aren't set up to support death tests well.
+  // For one, death tests are flaky in a multithreaded context. And sigactions
+  // inherited from llvm-lit interfere with what's being tested.
+  const void *buf = (const void *)&fds;
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+}
+#endif // !defined(_WIN32)
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -58,6 +58,7 @@
   ReverseIterationTest.cpp
   ReplaceFileTest.cpp
   ScaledNumberTest.cpp
+  SignalsTest.cpp
   SourceMgrTest.cpp
   SpecialCaseListTest.cpp
   StringPool.cpp
Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -560,6 +560,9 @@
   // Unimplemented.
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  // Unimplemented.
+}
 
 /// Add a function to be called when a signal is delivered to the process. The
 /// handler can have a cookie passed to it to identify what instance of the
Index: llvm/lib/Support/Unix/Signals.inc
===
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -82,12 +82,18 @@
 static RETSIGTYPE SignalHandler(int Sig);  // defined below.
 static RETSIGTYPE InfoSignalHandler(int Sig);  // defined below.
 
+static void DefaultPipeSignalFunction() {
+  exit(EX_IOERR);
+}
+
 using SignalHandlerFunctionType = void (*)();
 /// The function to call if ctrl-c is pressed.
 static std::atomic InterruptFunction =
 ATOMIC_VAR_INIT(nullptr);
 static std::atomic InfoSignalFunction =
 ATOMIC_VAR_INIT(nullptr);
+static std::atomic PipeSignalFunction =
+ATOMIC_VAR_INIT(DefaultPipeSignalFunction);
 
 namespace {
 /// Signal-safe removal of files.
@@ -363,7 +369,8 @@
 
   // Send a special return code that drivers can check for, from sysexits.h.
   if (Sig == SIGPIPE)
-exit(EX_IOERR);
+if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+  CurrentPipeFunction();
 
   raise(Sig);   // Execute the default handler.
   return;
@@ -403,6 +410,11 @@
   RegisterHandlers();
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  PipeSignalFunction.exchange(Handler);
+  RegisterHandlers();
+}
+
 // The public API
 bool llvm::sys::RemoveFileOnSignal(StringRef Filename,
std::string* ErrMsg) {
Index: llvm/include/llvm/Support/Signals.h
===
--- llvm/include/llvm/Support/Signals.h
+++ llvm/include/llvm/Support/Signals.h
@@ -84,6 +84,17 @@
   /// function.  Note also that

[Lldb-commits] [PATCH] D69102: COFF: Set section permissions

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:800
 /*flags*/ 0);
+header_sp->SetPermissions(ePermissionsReadable);
 m_sections_up->AddSection(header_sp);

mstorsjo wrote:
> labath wrote:
> > labath wrote:
> > > Are these the right permissions for the header?
> > I've dug around in some minidumps I have around and this does appear to be 
> > correct. It looks like the entire block of memory for the object is first 
> > allocated with PAGE_EXECUTE_WRITE_COPY, and the permissions for the header 
> > region are later changed to PAGE_READ_ONLY.
> I haven't checked, but I certainly would expect it to be readonly.
I wrote the last comment before seeing the preceding update, which is why it 
seems odd here.


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

https://reviews.llvm.org/D69102



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

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

Oh, and TestCallOverridenMethod still has some failures on Windows:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9996/steps/test/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68130



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

[Lldb-commits] [lldb] 5c28d49 - [lldb][NFC] Remove wrong tests in TestCallOverriddenMethod

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2019-10-18T19:18:41Z
New Revision: 5c28d49314c7bb84f08c9db3acd5ff64e1c4bc2d

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

LOG: [lldb][NFC] Remove wrong tests in TestCallOverriddenMethod

We call these tests in the second test function where they are
x-failed on Windows. I forgot to remove the tests from the first
test function (which is not x-failed on Windows) when extracting these
calls into their own test function, so the test is still failing on Windows.

llvm-svn: 375271

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py
 
b/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py
index f630aef9e51e..09369f43819c 100644
--- 
a/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/expression/call-overridden-method/TestCallOverriddenMethod.py
@@ -50,10 +50,6 @@ def test(self):
 # Test calling the base class.
 self.expect("expr realbase.foo()", substrs=["1"])
 
-# Test with locally constructed instances.
-self.expect("expr Base().foo()", substrs=["1"])
-self.expect("expr Derived().foo()", substrs=["2"])
-
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr43707")
 def test_call_on_temporary(self):
 """Test calls to overridden methods in derived classes."""



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

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Sorry, seems like I forgot to delete these tests when I extracted them into 
their own test function.

About the ubuntu failure: I think I have a similar failure on my Arch Linux 
machine when constructing an object in another test, so I fear that the whole 
object construction in the expression parser seems to be not working properly. 
I'll write a separate test for constructing different kind of objects and then 
we see what platform actually survives those tests. In the meanwhile I can also 
X-Fail that test on Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68130



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

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It doesn't seem to be failing here 
http://lab.llvm.org:8014/builders/lldb-x86_64-debian.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68130



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

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225687.
lawrence_danna added a comment.

spelling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -174,6 +174,27 @@
   static auto get(const T &value) { return value.get(); }
 };
 
+/* This is a helper class for functions that wrap python C API functions
+ * that want a null-terminated c string as an argument.   It's suboptimal
+ * for such wrappers to just take a StringRef, because those may not be
+ * null-terminated, so we'd wind up doing a full copy just to pass a
+ * fixed string. Instead, wrappers can just take their arguments as
+ * CStringArg, and callers can pass StringRefs, Twines, strings, or
+ * const char*, and the right thing will happen. */
+class CStringArg {
+  llvm::SmallString<32> storage;
+  const char *cstr;
+
+public:
+  CStringArg(const char *s) { cstr = s; }
+  CStringArg(const std::string &s) { cstr = s.c_str(); }
+  CStringArg(const llvm::Twine &twine) {
+llvm::StringRef ref = twine.toNullTerminatedStringRef(storage);
+cstr = ref.data();
+  }
+  const char *str() const { return cstr; }
+};
+
 class PythonObject {
 public:
   PythonObject() : m_py_obj(nul

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:401
 
-  void Reset(PyRefType type, PyObject *py_obj) {
-Reset();
+  void Reset() { PythonObject::Reset(); }
+

labath wrote:
> Is this needed, given the "using" declaration above?
yes, it seems when you delete one override of a member function, it deletes all 
the other ones that you don't explicitly mention too.   Actually it's the using 
line that should be deleted.



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:406
+  TypedPythonObject(PyRefType type, PyObject *py_obj) {
+m_py_obj = nullptr;
 if (!py_obj)

labath wrote:
> Isn't this already set by the superclass constructor?
yes, fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133



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

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225686.
lawrence_danna marked 6 inline comments as done.
lawrence_danna added a comment.

fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -174,6 +174,27 @@
   static auto get(const T &value) { return value.get(); }
 };
 
+/* This is a helper class for functions that wrap python C API functions
+ * that want a null-terminated c string as an argument.   It's suboptimal
+ * for such wrappers to just take a StringRef, because those may not be
+ * null-terminated, so we'd wind up doing a full copy just to pass a
+ * fixed string. Instead, wrappers can just take their arguments as
+ * CStringArg, an callers can pass StringRefs, Twines, strings, or
+ * const char*, and the right thing will happen. */
+class CStringArg {
+  llvm::SmallString<32> storage;
+  const char *cstr;
+
+public:
+  CStringArg(const char *s) { cstr = s; }
+  CStringArg(const std::string &s) { cstr = s.c_str(); }
+  CStringArg(const llvm::Twine &twine) {
+llvm::StringRef ref = twine.toNullTerminatedStringRef(storage);
+cstr = ref.data();
+  }
+  const char *str() const { return cstr; }
+};
+
 class PythonObjec

[Lldb-commits] [PATCH] D69014: [LLDB] bugfix: command script add -f doesn't work for some callables

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

@jingham , @labathwhat do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69014



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

[Lldb-commits] [lldb] 32ce14e - Disable exit-on-SIGPIPE in lldb

[Vedant Kumar via lldb-commits]

Author: Vedant Kumar
Date: 2019-10-18T21:05:30Z
New Revision: 32ce14e55e5a99dd99c3b4fd4bd0ccaaf2948c30

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

LOG: Disable exit-on-SIGPIPE in lldb

Occasionally, during test teardown, LLDB writes to a closed pipe.
Sometimes the communication is inherently unreliable, so LLDB tries to
avoid being killed due to SIGPIPE (it calls `signal(SIGPIPE, SIG_IGN)`).
However, LLVM's default SIGPIPE behavior overrides LLDB's, causing it to
exit with IO_ERR.

Opt LLDB out of the default SIGPIPE behavior. I expect that this will
resolve some LLDB test suite flakiness (tests randomly failing with
IO_ERR) that we've seen since r344372.

rdar://55750240

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

llvm-svn: 375288

Added: 
llvm/unittests/Support/SignalsTest.cpp

Modified: 
lldb/tools/driver/Driver.cpp
llvm/include/llvm/Support/Signals.h
llvm/lib/Support/Unix/Signals.inc
llvm/lib/Support/Windows/Signals.inc
llvm/unittests/Support/CMakeLists.txt

Removed: 




diff  --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index 4a403a7ffb46..6ab2bd93659f 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -853,6 +853,16 @@ int main(int argc, char const *argv[])
   signal(SIGCONT, sigcont_handler);
 #endif
 
+  // Occasionally, during test teardown, LLDB writes to a closed pipe.
+  // Sometimes the communication is inherently unreliable, so LLDB tries to
+  // avoid being killed due to SIGPIPE. However, LLVM's default SIGPIPE 
behavior
+  // is to exit with IO_ERR. Opt LLDB out of that.
+  //
+  // We don't disable LLVM's signal handling entirely because we still want
+  // pretty stack traces, and file cleanup (for when, say, the clang embedded
+  // in LLDB leaves behind temporary objects).
+  llvm::sys::SetPipeSignalFunction(nullptr);
+
   int exit_code = 0;
   // Create a scope for driver so that the driver object will destroy itself
   // before SBDebugger::Terminate() is called.

diff  --git a/llvm/include/llvm/Support/Signals.h 
b/llvm/include/llvm/Support/Signals.h
index a6b215a24311..a4f1fad22dd5 100644
--- a/llvm/include/llvm/Support/Signals.h
+++ b/llvm/include/llvm/Support/Signals.h
@@ -84,6 +84,17 @@ namespace sys {
   /// function.  Note also that the handler may be executed on a 
diff erent
   /// thread on some platforms.
   void SetInfoSignalFunction(void (*Handler)());
+
+  /// Registers a function to be called when a "pipe" signal is delivered to
+  /// the process.
+  ///
+  /// The "pipe" signal typically indicates a failed write to a pipe (SIGPIPE).
+  /// The default installed handler calls `exit(EX_IOERR)`, causing the process
+  /// to immediately exit with an IO error exit code.
+  ///
+  /// This function is only applicable on POSIX systems.
+  void SetPipeSignalFunction(void (*Handler)());
+
 } // End sys namespace
 } // End llvm namespace
 

diff  --git a/llvm/lib/Support/Unix/Signals.inc 
b/llvm/lib/Support/Unix/Signals.inc
index be05eabfb2e2..5e0cde4a81ed 100644
--- a/llvm/lib/Support/Unix/Signals.inc
+++ b/llvm/lib/Support/Unix/Signals.inc
@@ -82,12 +82,18 @@ using namespace llvm;
 static RETSIGTYPE SignalHandler(int Sig);  // defined below.
 static RETSIGTYPE InfoSignalHandler(int Sig);  // defined below.
 
+static void DefaultPipeSignalFunction() {
+  exit(EX_IOERR);
+}
+
 using SignalHandlerFunctionType = void (*)();
 /// The function to call if ctrl-c is pressed.
 static std::atomic InterruptFunction =
 ATOMIC_VAR_INIT(nullptr);
 static std::atomic InfoSignalFunction =
 ATOMIC_VAR_INIT(nullptr);
+static std::atomic PipeSignalFunction =
+ATOMIC_VAR_INIT(DefaultPipeSignalFunction);
 
 namespace {
 /// Signal-safe removal of files.
@@ -363,7 +369,8 @@ static RETSIGTYPE SignalHandler(int Sig) {
 
   // Send a special return code that drivers can check for, from 
sysexits.h.
   if (Sig == SIGPIPE)
-exit(EX_IOERR);
+if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+  CurrentPipeFunction();
 
   raise(Sig);   // Execute the default handler.
   return;
@@ -403,6 +410,11 @@ void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
   RegisterHandlers();
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  PipeSignalFunction.exchange(Handler);
+  RegisterHandlers();
+}
+
 // The public API
 bool llvm::sys::RemoveFileOnSignal(StringRef Filename,
std::string* ErrMsg) {

diff  --git a/llvm/lib/Support/Windows/Signals.inc 
b/llvm/lib/Support/Windows/Signals.inc
index 6a820ef22b1e..d962daf79348 100644
--- a/llvm/lib/Support/Windows/Signals.inc
+++ b/llvm/lib/Support/Windows/Signals.inc
@@ -560,6 +560,9 @@ void llvm::s

[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Vedant Kumar via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG32ce14e55e5a: Disable exit-on-SIGPIPE in lldb (authored by 
vsk).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69148

Files:
  lldb/tools/driver/Driver.cpp
  llvm/include/llvm/Support/Signals.h
  llvm/lib/Support/Unix/Signals.inc
  llvm/lib/Support/Windows/Signals.inc
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Support/SignalsTest.cpp

Index: llvm/unittests/Support/SignalsTest.cpp
===
--- /dev/null
+++ llvm/unittests/Support/SignalsTest.cpp
@@ -0,0 +1,53 @@
+//- unittests/Support/SignalsTest.cpp - Signal handling test =//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#if !defined(_WIN32)
+#include 
+#include 
+#endif // !defined(_WIN32)
+
+#include "llvm/Support/Signals.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+#if !defined(_WIN32)
+TEST(SignalTest, IgnoreMultipleSIGPIPEs) {
+  // Ignore SIGPIPE.
+  signal(SIGPIPE, SIG_IGN);
+
+  // Disable exit-on-SIGPIPE.
+  sys::SetPipeSignalFunction(nullptr);
+
+  // Create unidirectional read/write pipes.
+  int fds[2];
+  int err = pipe(fds);
+  if (err != 0)
+return; // If we can't make pipes, this isn't testing anything.
+
+  // Close the read pipe.
+  close(fds[0]);
+
+  // Attempt to write to the write pipe. Currently we're asserting that the
+  // write fails, which isn't great.
+  //
+  // What we really want is a death test that checks that this block exits
+  // with a special exit "success" code, as opposed to unexpectedly exiting due
+  // to a kill-by-SIGNAL or due to the default SIGPIPE handler.
+  //
+  // Unfortunately llvm's unit tests aren't set up to support death tests well.
+  // For one, death tests are flaky in a multithreaded context. And sigactions
+  // inherited from llvm-lit interfere with what's being tested.
+  const void *buf = (const void *)&fds;
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+}
+#endif // !defined(_WIN32)
Index: llvm/unittests/Support/CMakeLists.txt
===
--- llvm/unittests/Support/CMakeLists.txt
+++ llvm/unittests/Support/CMakeLists.txt
@@ -58,6 +58,7 @@
   ReverseIterationTest.cpp
   ReplaceFileTest.cpp
   ScaledNumberTest.cpp
+  SignalsTest.cpp
   SourceMgrTest.cpp
   SpecialCaseListTest.cpp
   StringPool.cpp
Index: llvm/lib/Support/Windows/Signals.inc
===
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -560,6 +560,9 @@
   // Unimplemented.
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  // Unimplemented.
+}
 
 /// Add a function to be called when a signal is delivered to the process. The
 /// handler can have a cookie passed to it to identify what instance of the
Index: llvm/lib/Support/Unix/Signals.inc
===
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -82,12 +82,18 @@
 static RETSIGTYPE SignalHandler(int Sig);  // defined below.
 static RETSIGTYPE InfoSignalHandler(int Sig);  // defined below.
 
+static void DefaultPipeSignalFunction() {
+  exit(EX_IOERR);
+}
+
 using SignalHandlerFunctionType = void (*)();
 /// The function to call if ctrl-c is pressed.
 static std::atomic InterruptFunction =
 ATOMIC_VAR_INIT(nullptr);
 static std::atomic InfoSignalFunction =
 ATOMIC_VAR_INIT(nullptr);
+static std::atomic PipeSignalFunction =
+ATOMIC_VAR_INIT(DefaultPipeSignalFunction);
 
 namespace {
 /// Signal-safe removal of files.
@@ -363,7 +369,8 @@
 
   // Send a special return code that drivers can check for, from sysexits.h.
   if (Sig == SIGPIPE)
-exit(EX_IOERR);
+if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+  CurrentPipeFunction();
 
   raise(Sig);   // Execute the default handler.
   return;
@@ -403,6 +410,11 @@
   RegisterHandlers();
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  PipeSignalFunction.exchange(Handler);
+  RegisterHandlers();
+}
+
 // The public API
 bool llvm::sys::RemoveFileOnSignal(StringRef Filename,
std::string* ErrMsg) {
Index: llvm/include/llvm/Support/Signals.h
===
--- llvm/include/llvm/Support/Signals.h
+++ llvm/include/llvm/Support/Signals.h
@@ -84,6 +84,17 @@
   /// function.  Note also that the 

[Lldb-commits] [lldb] 64b7d95 - [Reproducer] Improve reproducer help (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2019-10-18T21:47:31Z
New Revision: 64b7d95568607eac5336428a22e02f27b8663a79

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

LOG: [Reproducer] Improve reproducer help (NFC)

Provide a little more detail for the reproducer command.

llvm-svn: 375292

Added: 


Modified: 
lldb/source/Commands/CommandObjectReproducer.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectReproducer.cpp 
b/lldb/source/Commands/CommandObjectReproducer.cpp
index 72afed902649..dc4579c20fc2 100644
--- a/lldb/source/Commands/CommandObjectReproducer.cpp
+++ b/lldb/source/Commands/CommandObjectReproducer.cpp
@@ -161,7 +161,9 @@ class CommandObjectReproducerDump : public 
CommandObjectParsed {
 public:
   CommandObjectReproducerDump(CommandInterpreter &interpreter)
   : CommandObjectParsed(interpreter, "reproducer dump",
-"Dump the information contained in a reproducer.",
+"Dump the information contained in a reproducer. "
+"If no reproducer is specified during replay, it "
+"dumps the content of the current reproducer.",
 nullptr) {}
 
   ~CommandObjectReproducerDump() override = default;
@@ -361,7 +363,15 @@ CommandObjectReproducer::CommandObjectReproducer(
 CommandInterpreter &interpreter)
 : CommandObjectMultiword(
   interpreter, "reproducer",
-  "Commands for manipulate the reproducer functionality.",
+  "Commands for manipulating reproducers. Reproducers make it possible 
"
+  "to capture full debug sessions with all its dependencies. The "
+  "resulting reproducer is used to replay the debug session while "
+  "debugging the debugger.\n"
+  "Because reproducers need the whole the debug session from "
+  "beginning to end, you need to launch the debugger in capture or "
+  "replay mode, commonly though the command line driver.\n"
+  "Reproducers are unrelated record-replay debugging, as you cannot "
+  "interact with the debugger during replay.\n",
   "reproducer  []") {
   LoadSubCommand(
   "generate",



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

[Lldb-commits] [lldb] 06a2bea - [Reproducer] XFAIL TestWorkingDir on Windows

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2019-10-18T22:16:15Z
New Revision: 06a2beae92f5d2adcd0143a6798918418c2ea325

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

LOG: [Reproducer] XFAIL TestWorkingDir on Windows

I'm having a hard time reproducing this and it's failing on the Windows
bot. Temporarily X-failing this test while I continue to try building
LLDB on Windows.

llvm-svn: 375294

Added: 


Modified: 
lldb/test/Shell/Reproducer/TestWorkingDir.test

Removed: 




diff  --git a/lldb/test/Shell/Reproducer/TestWorkingDir.test 
b/lldb/test/Shell/Reproducer/TestWorkingDir.test
index c2f01244..fd41e1d43ad6 100644
--- a/lldb/test/Shell/Reproducer/TestWorkingDir.test
+++ b/lldb/test/Shell/Reproducer/TestWorkingDir.test
@@ -1,3 +1,5 @@
+# XFAIL: system-windows
+
 # This tests that the reproducer can deal with relative files. We create a
 # binary in a subdirectory and pass its relative path to LLDB. The subdirectory
 # is removed before replay so that it only exists in the reproducer's VFS.



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

[Lldb-commits] [lldb] 0904f92 - Skip (more) PExpect tests under ASAN, I can't get them to work reliably.

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2019-10-19T00:30:30Z
New Revision: 0904f924012db8002eec3a0533e310c1e714cca4

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

LOG: Skip (more) PExpect tests under ASAN, I can't get them to work reliably.

llvm-svn: 375312

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/iohandler/completion/TestIOHandlerCompletion.py

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/iohandler/completion/TestIOHandlerCompletion.py
 
b/lldb/packages/Python/lldbsuite/test/iohandler/completion/TestIOHandlerCompletion.py
index d202887902e9..b48585146374 100644
--- 
a/lldb/packages/Python/lldbsuite/test/iohandler/completion/TestIOHandlerCompletion.py
+++ 
b/lldb/packages/Python/lldbsuite/test/iohandler/completion/TestIOHandlerCompletion.py
@@ -13,6 +13,9 @@ class IOHandlerCompletionTest(PExpectTest):
 
 mydir = TestBase.compute_mydir(__file__)
 
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
 def test_completion(self):
 self.launch(dimensions=(100,500))
 



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

[Lldb-commits] [PATCH] D69209: [lldb] Add test for running static initializers in expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
Herald added subscribers: lldb-commits, JDevlieghere.
Herald added a project: LLDB.
teemperor added a comment.

Just dumping this here because I don't want to anger the build bots before 
going into the weekend.


While reading ClangExpressionParser source code I saw a bunch of code that
specifically handles running all the static initializers in expressions. The 
coverage
bot tells me that we never actually test this code anywhere, so let's add some
testing to that feature.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D69209

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/Makefile
  
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/TestStaticInitializers.py
  
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/main.cpp


Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/main.cpp
===
--- /dev/null
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/main.cpp
@@ -0,0 +1,11 @@
+#include 
+
+int counter = 0;
+
+void inc_counter() { ++counter; }
+
+void do_abort() { abort(); }
+
+int main() {
+  return 0; // break here
+}
Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/TestStaticInitializers.py
===
--- /dev/null
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/TestStaticInitializers.py
@@ -0,0 +1,31 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class StaticInitializers(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+""" Test a static initializer. """
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# We use counter to observe if the initializer was called.
+self.expect("expr counter", substrs=["(int) $", " = 0"])
+self.expect("expr -p -- struct Foo { Foo() { inc_counter(); } }; Foo 
f;")
+self.expect("expr counter", substrs=["(int) $", " = 1"])
+
+def test_failing_init(self):
+""" Test a static initializer that fails to execute. """
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# FIXME: This error message is not even remotely helpful.
+self.expect("expr -p -- struct Foo2 { Foo2() { do_abort(); } }; Foo2 
f;", error=True,
+substrs=["error: couldn't run static initializers: 
couldn't run static initializer:"])
Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/Makefile
===
--- /dev/null
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/main.cpp
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/main.cpp
@@ -0,0 +1,11 @@
+#include 
+
+int counter = 0;
+
+void inc_counter() { ++counter; }
+
+void do_abort() { abort(); }
+
+int main() {
+  return 0; // break here
+}
Index: lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/TestStaticInitializers.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/commands/expression/static-initializers/TestStaticInitializers.py
@@ -0,0 +1,31 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class StaticInitializers(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+""" Test a static initializer. """
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# We use counter to observe if the initializer was called.
+self.expect("expr counter", substrs=["(int) $", " = 0"])
+self.expect("expr -p -- struct Foo { Foo() { inc_counter(); } }; Foo f;")
+self.expect("expr counter", substrs=["(int) $", " = 1"])
+
+def test_failing_init(self):
+""" Test a static initializer that fails to execute. """
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# FIXME: This error message is not even remotely helpful.
+self.expect("expr -p -- struct Foo2 { Foo2() { do_

[Lldb-commits] [PATCH] D69209: [lldb] Add test for running static initializers in expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Just dumping this here because I don't want to anger the build bots before 
going into the weekend.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69209



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

[Lldb-commits] [PATCH] D68130: [lldb] Don't emit artificial constructor declarations as global functions

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Well, I'm fine with x-failing it on Linux, even though I guess at some point 
someone (i.e., probably me) has to figure out why this stuff is broken in the 
expression parser.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68130



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

[Lldb-commits] [lldb] 90c64a3 - Move endian constant from Host.h to SwapByteOrder.h, prune include

[Reid Kleckner via lldb-commits]

Author: Reid Kleckner
Date: 2019-10-19T00:48:11Z
New Revision: 90c64a3456b972432a21ef043b205c18a91e011b

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

LOG: Move endian constant from Host.h to SwapByteOrder.h, prune include

Works on this dependency chain:
  ArrayRef.h ->
  Hashing.h -> --CUT--
  Host.h ->
  StringMap.h / StringRef.h

ArrayRef is very popular, but Host.h is rarely needed. Move the
IsBigEndianHost constant to SwapByteOrder.h. Clients of that header are
more likely to need it.

llvm-svn: 375316

Added: 


Modified: 
clang-tools-extra/clangd/FileDistance.h
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
clang/lib/Driver/ToolChains/Arch/ARM.cpp
clang/lib/Driver/ToolChains/Arch/PPC.cpp
clang/lib/Driver/ToolChains/Arch/X86.cpp
lld/include/lld/Common/LLVM.h
lld/include/lld/Core/File.h
lld/lib/ReaderWriter/MachO/DebugInfo.h
lld/tools/lld/lld.cpp
lldb/include/lldb/Utility/UUID.h
llvm/include/llvm/ADT/Hashing.h
llvm/include/llvm/BinaryFormat/Wasm.h
llvm/include/llvm/Support/Host.h
llvm/include/llvm/Support/SHA1.h
llvm/include/llvm/Support/SwapByteOrder.h
llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp
llvm/lib/Support/Windows/WindowsSupport.h
llvm/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp
llvm/tools/llvm-exegesis/lib/RegisterValue.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/FileDistance.h 
b/clang-tools-extra/clangd/FileDistance.h
index e7174bccb9dd..88bb30c14270 100644
--- a/clang-tools-extra/clangd/FileDistance.h
+++ b/clang-tools-extra/clangd/FileDistance.h
@@ -43,6 +43,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Path.h"

diff  --git a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp 
b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
index 35d11f4e2d3b..3a5fe6ddeaed 100644
--- a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -12,6 +12,7 @@
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/TargetParser.h"
+#include "llvm/Support/Host.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;

diff  --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp 
b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
index b99a1b4d3694..68a57310ad40 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/TargetParser.h"
+#include "llvm/Support/Host.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;

diff  --git a/clang/lib/Driver/ToolChains/Arch/PPC.cpp 
b/clang/lib/Driver/ToolChains/Arch/PPC.cpp
index 59ff7cbc787c..3e02e57e0f6c 100644
--- a/clang/lib/Driver/ToolChains/Arch/PPC.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/PPC.cpp
@@ -13,6 +13,7 @@
 #include "clang/Driver/Options.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
+#include "llvm/Support/Host.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;

diff  --git a/clang/lib/Driver/ToolChains/Arch/X86.cpp 
b/clang/lib/Driver/ToolChains/Arch/X86.cpp
index 34be226b69e9..d2b97bf6ad71 100644
--- a/clang/lib/Driver/ToolChains/Arch/X86.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/X86.cpp
@@ -13,6 +13,7 @@
 #include "clang/Driver/Options.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
+#include "llvm/Support/Host.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;

diff  --git a/lld/include/lld/Common/LLVM.h b/lld/include/lld/Common/LLVM.h
index f7ed1d793ca7..34b7b0d194ab 100644
--- a/lld/include/lld/Common/LLVM.h
+++ b/lld/include/lld/Common/LLVM.h
@@ -17,6 +17,7 @@
 // This should be the only #include, force #includes of all the others on
 // clients.
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include 
 

diff  --git a/lld/include/lld/Core/File.h b/lld/include/lld/Core/File.h
index 492f35989f16..df014669eb62 100644
--- a/lld/include/lld/Core/File.h
+++ b/lld/include/lld/Core/File.h
@@ -16,6 +16,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/ErrorHandling.h"
 #include 
 #include 

diff  --git a/lld/lib/ReaderWriter/MachO/DebugInfo.h 
b/lld/lib/ReaderWriter/MachO/DebugInfo.h
index 959e10f9a073..591dd1ebad86 100644
--- a/lld/lib/ReaderWriter/MachO/DebugInfo.h
+++ b/lld/lib/ReaderWriter/Ma

[Lldb-commits] [PATCH] D69210: [Disassembler] Simplify MCInst predicates

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: jasonmolenda, ab.

DisassemblerLLVMC exposes a few MCInst predicates (e.g. `HasDelaySlot`).
Group the logic for evaluating the existing predicates together, to prep
for adding new ones.

This is NFC-ish: with this change, the existing predicates will return
the conservative defaults when the disassembler is unavailable instead
of false, but I'm not sure that really matters.


https://reviews.llvm.org/D69210

Files:
  lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
@@ -78,6 +78,42 @@
 };
 
 class InstructionLLVMC : public lldb_private::Instruction {
+private:
+  void VisitInstruction() {
+// Be conservative. If we didn't understand the instruction, say it:
+//   - Might branch
+//   - Does not have a delay slot
+//   - Is not a call
+m_does_branch = eLazyBoolYes;
+m_has_delay_slot = eLazyBoolNo;
+m_is_call = eLazyBoolNo;
+
+DisassemblerScope disasm(*this);
+if (!disasm)
+  return;
+
+DataExtractor data;
+if (!m_opcode.GetData(data))
+  return;
+
+bool is_alternate_isa;
+lldb::addr_t pc = m_address.GetFileAddress();
+DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
+GetDisasmToUse(is_alternate_isa, disasm);
+const uint8_t *opcode_data = data.GetDataStart();
+const size_t opcode_data_len = data.GetByteSize();
+llvm::MCInst inst;
+const size_t inst_size =
+mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
+if (inst_size == 0)
+  return;
+
+m_does_branch = mc_disasm_ptr->CanBranch(inst) ? eLazyBoolYes : eLazyBoolNo;
+m_has_delay_slot =
+mc_disasm_ptr->HasDelaySlot(inst) ? eLazyBoolYes : eLazyBoolNo;
+m_is_call = mc_disasm_ptr->IsCall(inst) ? eLazyBoolYes : eLazyBoolNo;
+  }
+
 public:
   InstructionLLVMC(DisassemblerLLVMC &disasm,
const lldb_private::Address &address,
@@ -92,68 +128,14 @@
   ~InstructionLLVMC() override = default;
 
   bool DoesBranch() override {
-if (m_does_branch == eLazyBoolCalculate) {
-  DisassemblerScope disasm(*this);
-  if (disasm) {
-DataExtractor data;
-if (m_opcode.GetData(data)) {
-  bool is_alternate_isa;
-  lldb::addr_t pc = m_address.GetFileAddress();
-
-  DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-  GetDisasmToUse(is_alternate_isa, disasm);
-  const uint8_t *opcode_data = data.GetDataStart();
-  const size_t opcode_data_len = data.GetByteSize();
-  llvm::MCInst inst;
-  const size_t inst_size =
-  mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
-  // Be conservative, if we didn't understand the instruction, say it
-  // might branch...
-  if (inst_size == 0)
-m_does_branch = eLazyBoolYes;
-  else {
-const bool can_branch = mc_disasm_ptr->CanBranch(inst);
-if (can_branch)
-  m_does_branch = eLazyBoolYes;
-else
-  m_does_branch = eLazyBoolNo;
-  }
-}
-  }
-}
+if (m_does_branch == eLazyBoolCalculate)
+  VisitInstruction();
 return m_does_branch == eLazyBoolYes;
   }
 
   bool HasDelaySlot() override {
-if (m_has_delay_slot == eLazyBoolCalculate) {
-  DisassemblerScope disasm(*this);
-  if (disasm) {
-DataExtractor data;
-if (m_opcode.GetData(data)) {
-  bool is_alternate_isa;
-  lldb::addr_t pc = m_address.GetFileAddress();
-
-  DisassemblerLLVMC::MCDisasmInstance *mc_disasm_ptr =
-  GetDisasmToUse(is_alternate_isa, disasm);
-  const uint8_t *opcode_data = data.GetDataStart();
-  const size_t opcode_data_len = data.GetByteSize();
-  llvm::MCInst inst;
-  const size_t inst_size =
-  mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
-  // if we didn't understand the instruction, say it doesn't have a
-  // delay slot...
-  if (inst_size == 0)
-m_has_delay_slot = eLazyBoolNo;
-  else {
-const bool has_delay_slot = mc_disasm_ptr->HasDelaySlot(inst);
-if (has_delay_slot)
-  m_has_delay_slot = eLazyBoolYes;
-else
-  m_has_delay_slot = eLazyBoolNo;
-  }
-}
-  }
-}
+if (m_has_delay_slot == eLazyBoolCalculate)
+  VisitInstruction();
 return m_has_delay_slot == eLazyBoolYes;
   }
 
@@ -865,32 +847,8 @@
   }
 
   bool IsCall() override {
-if (m_is_call == eLazyBoolCalculate) {
-  DisassemblerScope disasm(*this);
-  if (disasm) {
-DataExtractor data;
-if (

[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna updated this revision to Diff 225738.
lawrence_danna added a comment.

use enable_if on CStringArg so the other constructors take precedence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -27,8 +27,7 @@
   void SetUp() override {
 PythonTestSuite::SetUp();
 
-PythonString sys_module("sys");
-m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
+m_sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
 m_main_module = PythonModule::MainModule();
 m_builtins_module = PythonModule::BuiltinsModule();
   }
@@ -70,13 +69,10 @@
   PythonDictionary dict(PyInitialValue::Empty);
 
   PyObject *new_dict = PyDict_New();
-  dict.Reset(PyRefType::Owned, new_dict);
+  dict = Take(new_dict);
   EXPECT_EQ(new_dict, dict.get());
 
-  dict.Reset(PyRefType::Owned, nullptr);
-  EXPECT_EQ(nullptr, dict.get());
-
-  dict.Reset(PyRefType::Owned, PyDict_New());
+  dict = Take(PyDict_New());
   EXPECT_NE(nullptr, dict.get());
   dict.Reset();
   EXPECT_EQ(nullptr, dict.get());
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -55,6 +55,7 @@
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 // Defined in the SWIG source file
 #if PY_MAJOR_VERSION >= 3
@@ -765,19 +766,16 @@
   if (!main_dict.IsValid())
 return m_session_dict;
 
-  PythonObject item = main_dict.GetItemForKey(PythonString(m_dictionary_name));
-  m_session_dict.Reset(PyRefType::Borrowed, item.get());
+  m_session_dict = unwrapIgnoringErrors(
+  As(main_dict.GetItem(m_dictionary_name)));
   return m_session_dict;
 }
 
 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
 return m_sys_module_dict;
-
-  PythonObject sys_module(PyRefType::Borrowed, PyImport_AddModule("sys"));
-  if (sys_module.IsValid())
-m_sys_module_dict.Reset(PyRefType::Borrowed,
-PyModule_GetDict(sys_module.get()));
+  PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
+  m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
@@ -1053,9 +1051,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid()) {
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
   }
 
   if (!locals.IsValid())
@@ -1204,9 +1201,8 @@
   PythonDictionary locals = GetSessionDictionary();
 
   if (!locals.IsValid())
-locals.Reset(
-PyRefType::Owned,
-PyObject_GetAttrString(globals.get(), m_dictionary_name.c_str()));
+locals = unwrapIgnoringErrors(
+As(globals.GetAttribute(m_dictionary_name)));
 
   if (!locals.IsValid())
 locals = globals;
Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -174,6 +174,32 @@
   static auto get(const T &value) { return value.get(); }
 };
 
+/* This is a helper class for functions that wrap python C API functions
+ * that want a null-terminated c string as an argument.   It's suboptimal
+ * for such wrappers to just take a StringRef, because those may not be
+ * null-terminated, so we'd wind up doing a full copy just to pass a
+ * fixed string. Instead, wrappers can just take their arguments as
+ * CStringArg, and callers can pass StringRefs, Twines, strings, or
+ * const char*, and the right thing will happen. */
+class CStringArg {
+  llvm::SmallString<32> storage;
+  const char *cstr;
+
+public:
+  CStringArg(const char *s) { cstr = s; }
+  CStringArg(const std::string &s) { cstr = s.c_str(); }
+
+  template 
+  CStringArg(T x,
+ typename std::enable_if::value,
+ void *>::type = 0) {
+llvm::Twine twine(x);
+llvm::StringRef ref =

[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna created this revision.
lawrence_danna added reviewers: JDevlieghere, clayborg, labath, jingham.
Herald added a subscriber: mgorny.
Herald added a project: LLDB.

With this patch, only the no-argument form of `Reset()` remains in 
PythonDataObjects.   It also deletes PythonExceptionState in favor of 
PythonException, because the only call-site of PythonExceptionState was
also using Reset, so I cleaned up both while I was there.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69214

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonExceptionState.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonExceptionStateTests.cpp
+++ /dev/null
@@ -1,164 +0,0 @@
-//===-- PythonExceptionStateTest.cpp --*- C++
-//-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "gtest/gtest.h"
-
-#include "Plugins/ScriptInterpreter/Python/PythonDataObjects.h"
-#include "Plugins/ScriptInterpreter/Python/PythonExceptionState.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
-#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-
-#include "PythonTestSuite.h"
-
-using namespace lldb_private;
-
-class PythonExceptionStateTest : public PythonTestSuite {
-public:
-protected:
-  void RaiseException() {
-PyErr_SetString(PyExc_RuntimeError, "PythonExceptionStateTest test error");
-  }
-};
-
-TEST_F(PythonExceptionStateTest, TestExceptionStateChecking) {
-  PyErr_Clear();
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  RaiseException();
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAcquisitionSemantics) {
-  PyErr_Clear();
-  PythonExceptionState no_error(false);
-  EXPECT_FALSE(no_error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Discard();
-
-  PyErr_Clear();
-  RaiseException();
-  error.Acquire(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestDiscardSemantics) {
-  PyErr_Clear();
-
-  // Test that discarding an exception does not restore the exception
-  // state even when auto-restore==true is set
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Discard();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-}
-
-TEST_F(PythonExceptionStateTest, TestResetSemantics) {
-  PyErr_Clear();
-
-  // Resetting when auto-restore is true should restore.
-  RaiseException();
-  PythonExceptionState error(true);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error.Reset();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-
-  // Resetting when auto-restore is false should discard.
-  RaiseException();
-  PythonExceptionState error2(false);
-  EXPECT_TRUE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-  error2.Reset();
-  EXPECT_FALSE(error2.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestManualRestoreSemantics) {
-  PyErr_Clear();
-  RaiseException();
-  PythonExceptionState error(false);
-  EXPECT_TRUE(error.IsError());
-  EXPECT_FALSE(PythonExceptionState::HasErrorOccurred());
-
-  error.Restore();
-  EXPECT_FALSE(error.IsError());
-  EXPECT_TRUE(PythonExceptionState::HasErrorOccurred());
-
-  PyErr_Clear();
-}
-
-TEST_F(PythonExceptionStateTest, TestAutoRestoreSemantics) {
-  PyErr_Clear();
-  // Test that

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Cool. I'm glad we could figure this out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68968



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

[Lldb-commits] [PATCH] D69014: [LLDB] bugfix: command script add -f doesn't work for some callables

[Pavel Labath via Phabricator via lldb-commits]

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

I am happy with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69014



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