[Lldb-commits] [PATCH] D75975: [lldb] Copy m_behaves_like_zeroth_frame on stack frame update

2020-03-11 Thread Anton Kolesov via Phabricator via lldb-commits
anton.kolesov created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Field StackFrame::m_behaves_like_zeroth_frame was introduced in commit
[1], however that commit hasn't added a copying of the field to
UpdatePreviousFrameFromCurrentFrame, therefore the value wouldn't change
when updating frames to reflect the current situation.

The particular scenario, where this matters is following. Assume we have
function `main` that invokes function `func1`. We set breakpoint at
`func1` entry and in `main` after the `func1` call, and do not stop at
the `main` entry. Therefore, when debugger stops for the first time,
`func1` is frame#0, while `main` is frame#1, thus
m_behaves_like_zeroth_frame is set to 0 for `main` frame. Execution is
resumed, and stops now in `main`, where it is now frame#0. However while
updating the frame object, m_behaves_like_zeroth_frame remains false.
This field plays an important role when calculating line information for
backtrace: for frame#0, PC is the current line, therefore line
information is retrieved for PC, however for all other frames this is
not the case - calculated PC is a return-PC, i.e. instruction after the
function call line, therefore for those frames LLDB needs to step back
by one instruction. Initial implementation did this strictly for frames
that have index != 0 (and index is updated properly in
`UpdatePreviousFrameFromCurrentFrame`), but m_behaves_like_zeroth_frame
added a capability for middle-of-stack frames to behave in a similar
manner. But because current code now doesn't check frame idx,
m_behaves_like_zeroth_frame must be set to true for frames with 0 index,
not only for frame that behave like one. In the described test case,
after stopping in `main`, LLDB would still consider frame#0 as
non-zeroth, and would subtract instruction from the PC, and would report
previous like as current line.

The error doesn't manifest itself in LLDB interpreter though - it can be
reproduced through LLDB-MI and when using SB API, but not when we
interpreter command "continue" is executed.  Honestly, I didn't fully
understand why it works in interpreter, I did found that bug "fixes"
itself if I enable `DEBUG_STACK_FRAMES` in StackFrameList.cpp, because
that calls `StackFrame::Dump` and that calls
`GetSymbolContext(eSymbolContextEverything)`, which fills the context of
frame on the first breakpoint, therefore it doesn't have to be
recalculated (improperly) on a second frame. However, on first
breakpoint symbol context is calculated for the "call" line, not the
next one, therefore it should be recalculated anyway on a second
breakpoint, and it is done correctly, even though
m_behaves_like_zeroth_frame is still incorrect, as long as
`GetSymbolContext(eSymbolContextEverything)` has been called.

[1] 31e6dbe1c6a6 Fix PC adjustment in StackFrame::GetSymbolContext


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75975

Files:
  lldb/source/Target/StackFrame.cpp
  lldb/test/API/functionalities/unwind/zeroth_frame/Makefile
  lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
  lldb/test/API/functionalities/unwind/zeroth_frame/main.c

Index: lldb/test/API/functionalities/unwind/zeroth_frame/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/zeroth_frame/main.c
@@ -0,0 +1,8 @@
+void func_inner() {
+int a = 1;  // Set breakpoint 1 here
+}
+
+int main() {
+func_inner();
+return 0; // Set breakpoint 2 here
+}
Index: lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/zeroth_frame/TestZerothFrame.py
@@ -0,0 +1,96 @@
+"""
+Test that line information is recalculated properly for a frame when it moves
+from the middle of the backtrace to a zero index.
+
+This is a regression test for a StackFrame bug, where whether frame is zero or
+not depends on an internal field. When LLDB was updating its frame list value
+of the field wasn't copied into existing StackFrame instances, so those
+StackFrame instances, would use an incorrect line entry evaluation logic in
+situations if it was in the middle of the stack frame list (not zeroth), and
+then moved to the top position. The difference in logic is that for zeroth
+frames line entry is returned for program counter, while for other frame
+(except for those that "behave like zeroth") it is for the instruction
+preceding PC, as PC points to the next instruction after function call. When
+the bug is present, when execution stops at the second breakpoint
+SBFrame.GetLineEntry() returns line entry for the previous line, rather than
+the one with a breakpoint. Note that this is specific to
+SBFrame.GetLineEntry(), SBFrame.GetPCAddress().GetLineEntry() would return
+correct entry.
+
+This bug doesn't reproduce through an LLDB interpretator, however it happens
+wh

[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

There's a `target.inherit-env` setting in lldb (which I believe also works 
correctly for remote launches). Could you use that instead of reimplementing 
the feature in vscode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74636



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


[Lldb-commits] [PATCH] D75877: [lldb/Reproducers] Fix replay for process attach workflows

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

A bunch of comments but nothing really major. Maybe it would be nice to put the 
code for yamlification of ProcessInfo into a separate patch?




Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:499-512
+auto error_or_file = MemoryBuffer::getFile(*process_file);
+if (auto err = error_or_file.getError()) {
+  SetError(result, errorCodeToError(err));
+  return false;
+}
+
+ProcessInstanceInfoList infos;

Maybe some kind of a utility function to convert a file to an object?
`template Expected readAsYaml(StringRef filename)` ?



Comment at: lldb/source/Host/macosx/objcxx/Host.mm:596
  ProcessInstanceInfoList &process_infos) {
+  if (llvm::Optional infos =
+  repro::GetReplayProcessInstanceInfoList()) {

This means that every implementation of FindProcesses will need to introduce 
this bolierplate. We should put this into common code somehow. One way to do 
that would be to rename all the platform-specific implementations to something 
like DoFindProcesses, and then implement FindProcesses 
`source/Host/common/Host.cpp` to handle the delegation & reproducer logic.



Comment at: lldb/source/Utility/FileSpec.cpp:543
+raw_ostream &Out) {
+  Out << Val.GetPath();
+}

There's more to FileSpecs than just the path -- they also hold the path syntax 
and the "case-sensitive" bit. Kind of not needed for your current goal, but 
maybe we should add those too while we're here?



Comment at: lldb/source/Utility/ProcessInfo.cpp:400-403
+llvm::StringRef llvm::yaml::MappingTraits::validate(
+IO &io, ProcessInstanceInfo &) {
+  return {};
+}

You don't actually have to provide these functions if they are not going to do 
anything.



Comment at: lldb/source/Utility/ProcessInfo.cpp:417-430
+  static std::unique_ptr>
+  loader = repro::MultiLoader::Create(
+  repro::Reproducer::Instance().GetLoader());
+
+  if (!loader)
+return {};
+

random thought: Would any of this be simpler if this wasn't a "multi" provider 
but rather stored all of the responses as a sequence in a single file?



Comment at: lldb/source/Utility/ProcessInfo.cpp:436
+
+  if (auto err = yin.error())
+return {};

what's the type of this?



Comment at: 
lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py:30
+
+inferior = self.spawnSubprocess(
+self.getBuildArtifact("somewhat_unique_name"))

You still need to do the `wait_for_file_on_target` dance here to ensure that 
`lldb_enable_attach` is executed before we actually attach. One example of that 
is in  `test/API/python_api/hello_world/main.c`.



Comment at: lldb/test/API/functionalities/reproducers/attach/main.cpp:10
+  while (true) {
+std::this_thread::sleep_for(microseconds(1));
+  }

You probably copied this from some existing test, but I'd say this is putting 
unnecessary load on the system. For this use case even a 1-second sleep would 
be perfectly fine.


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

https://reviews.llvm.org/D75877



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


[Lldb-commits] [PATCH] D75979: [lldb] Implement StackFrame::BehavesLikeZerothFrame

2020-03-11 Thread Anton Kolesov via Phabricator via lldb-commits
anton.kolesov created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Commit [1] added a declaration of function-member
StackFrame::BehavesLikeZerothFrame but hasn't added an implementation
for the function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75979

Files:
  lldb/source/Target/StackFrame.cpp


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -1211,6 +1211,10 @@
   return m_stack_frame_kind == StackFrame::Kind::Artificial;
 }
 
+bool StackFrame::BehavesLikeZerothFrame() const {
+  return m_behaves_like_zeroth_frame;
+}
+
 lldb::LanguageType StackFrame::GetLanguage() {
   CompileUnit *cu = GetSymbolContext(eSymbolContextCompUnit).comp_unit;
   if (cu)


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -1211,6 +1211,10 @@
   return m_stack_frame_kind == StackFrame::Kind::Artificial;
 }
 
+bool StackFrame::BehavesLikeZerothFrame() const {
+  return m_behaves_like_zeroth_frame;
+}
+
 lldb::LanguageType StackFrame::GetLanguage() {
   CompileUnit *cu = GetSymbolContext(eSymbolContextCompUnit).comp_unit;
   if (cu)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

2020-03-11 Thread Hans Wennborg via Phabricator via lldb-commits
hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406



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


[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

2020-03-11 Thread Aaron Ballman via Phabricator via lldb-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406



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


[Lldb-commits] [PATCH] D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In D75880#1915999 , @jingham wrote:

>   We use that all over the place in lldb, and I much prefer it. 


I'm afraid that's no longer true. I count 30 instances of `&&` or `||` at the 
beginning of a line versus 2343 instances of it being at the end. One of the 
main advantages of having a tool like clang-format is that you don't have to 
argue about the right way to format something. It may not produce the nicest 
possible layout all the time, but it will be close enough most of the time. And 
it will be consistent...

Overall, I'd just recommend getting into the habit of running clang-format 
before submitting a patch. I noticed a bunch of inconsistencies in the 
formatting of this patch. Right now, I only highlighted those that are 
definitely inferior to what clang-format would produce.




Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:32
+class ThreadPlanStack {
+friend class lldb_private::Thread;
+

this should be indented



Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:105-108
+ const PlanStack &GetStackOfKind(ThreadPlanStack::StackKind kind) const;
+ 
+ PlanStack m_plans; ///< The stack of plans this thread is executing.
+ PlanStack m_completed_plans; ///< Plans that have been completed by this

inconsistent indentation



Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:156-157
+private:
+using PlansList = std::unordered_map;
+PlansList m_plans_list;
+};

here too



Comment at: lldb/source/Target/Process.cpp:1261
+return llvm::make_error(
+llvm::formatv("Invalid option with value '{0}'.", tid),
+llvm::inconvertibleErrorCode());

jingham wrote:
> labath wrote:
> > This error message doesn't really make sense for a function like this. What 
> > option is it talking about?
> > 
> > As the only reason this can fail is because there are no plans for the 
> > given tid, maybe you don't actually need the Expected, and can signal that 
> > with a simple nullptr?
> I originally had this as a pointer that returns NULL when not found, but I 
> thought we were going towards Expected's for everything that might not 
> produce a value.  I'm happier with it as just a pointer, so I put it back to 
> that.
I don't think we want to use Expected everywhere. We definitely trying to avoid 
returning default-constructed objects and checking them with IsValid or 
something. And if I am creating a new class, I try to make it so that it does 
not have an "invalid" state and use Optionals or Expecteds to signal its 
absence. But pointers are existing type with a well known "invalid" value, so I 
don't see a problem with using it. Furthermore, with pointers it is possible to 
express that something is always valid (by using a reference), which is 
something that isn't doable with a class with an IsValid method.

Expecteds are mostly useful when you actually want to return an error from a 
function (instead of a Status + default value combo), for instance when there 
are multiple ways it can fail, and you want to user to know what happened. Or 
if you think that it is dangerous to ignore possible errors from the function 
and you want to make sure the user checks for them. For simple getter-like 
functions, I don't think any of this applies.



Comment at: lldb/source/Target/Thread.cpp:1059-1060
+void Thread::PushPlan(ThreadPlanSP thread_plan_sp) {
+  if (!thread_plan_sp)
+assert("Don't push an empty thread plan.");
 

I meant `assert(thread_plan_sp && "Don't push an empty thread plan.")`



Comment at: lldb/source/Target/ThreadPlanStack.cpp:233-234
+lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlan() const {
+  if (m_plans.size() == 0)
+llvm_unreachable("There will always be a base plan.");
+  return m_plans.back();

assert(!empty)



Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}

jingham wrote:
> labath wrote:
> > This is the usual place where one would add a llvm_unreachable. Some 
> > compilers do not consider the above switch to be fully covered (and if we 
> > are to be fully pedantic, it isn't) and will complain about falling off of 
> > the end of a non-void function.
> I added a default, but what do you mean by "if we are to be fully pedantic, 
> it isn't"?  It's an enum with 3 cases, all of which are listed.  How is that 
> not fully covered?
The first sentence of [[ https://en.cppreference.com/w/cpp/language/enum | 
cppreference enum definition ]] could be helpful here:
```
An enumeration is a distinct type whose value is restricted to a range of 
values (see below for details), which may include several explicitly named 
constants ("enumerators").
```
What exactly is the "range of values" of an enum is a complex topic, but 
suffice to say it usually 

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

2020-03-11 Thread Aaron Ballman via Phabricator via lldb-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from minor nits, this seems reasonable to me, LG!




Comment at: clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:16
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/SetVector.h"
 #include 

Please sort the includes.



Comment at: clang/lib/AST/ASTContext.cpp:1619
 void ASTContext::addedLocalImportDecl(ImportDecl *Import) {
-  assert(!Import->NextLocalImport && "Import declaration already in the 
chain");
+  assert(!Import->getNextLocalImport() && "Import declaration already in the 
chain");
   assert(!Import->isFromASTFile() && "Non-local import declaration");

80-col limit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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


[Lldb-commits] [PATCH] D75979: [lldb] Implement StackFrame::BehavesLikeZerothFrame

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

If it's not used anywhere, maybe we should delete it instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75979



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


[Lldb-commits] [PATCH] D75925: [lldb] reject `.debug_arange` sections with nonzero segment size

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In D75925#1915693 , @clayborg wrote:

> Change looks good, just needs a test. Should be easy to take a simple binary 
> that has a .debug_aranges, and run obj2yaml on it, and tweak the segment size 
> as needed?


In this case I think the cleanest solution would be to write a c++ unit test: 
create a simple debug_aranges header (just hardcode bytes, nothing fancy), pass 
it to DWARFDebugArangesSet::extract, and check the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75925



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


[Lldb-commits] [PATCH] D75761: [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects

2020-03-11 Thread Raphael Isemann via Phabricator via lldb-commits
teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I only have some minor comments but otherwise I think this can land. The title 
could use some updating as "Fix to get the AST we generate for function 
templates to be closer to what clang generates and expects" seems very 
abstract. What about "[lldb] Remove template parameters from 
FunctionTemplateDecl names" or something like that?




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1182
+
+  name = D.getFunctionBaseName(nullptr, &Size);
+}

You can just pass a nullptr instead of `&Size` if you don't use the value.



Comment at: lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py:16
 self.build()
-(_, _, thread, _) = lldbutil.run_to_name_breakpoint(self, "main")
-frame = thread.GetSelectedFrame()
-expr = "foo(42)"
+(self.target, self.process, _, bkpt) = 
lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))

This line can just be `lldbutil.run_to_source_breakpoint(self, '// break here', 
lldb.SBFileSpec("main.cpp"))`, no need to assign self.anything. The SBFileSpec 
constructor is curious as it seems the constructor without the bool is 
deprecated since 2010 (?) but we're using it in many other places (as the bool 
parameter doesn't appear to be documented.)



Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:1
 template
 int foo(T t1) {

labath wrote:
> shafik wrote:
> > labath wrote:
> > > Do we have a test for the spaceship operator?
> > We currently don't support C++2a but when we do we should add a test.
> How is this lack of support "implemented"? Would it make sense to test that 
> we do something reasonable (e.g. ignore it) if we do happen to run into it?
> 
> Given that the new version of this patch doesn't treat the spaceship operator 
> specially, I don't think it needs to/should be a part of this patch, but it 
> is something to think about...
Well the expression evaluator doesn't activate C++20 so we would just not parse 
the `<=>` as the space ship operator in an expression (and therefore not call 
it). We could still get it into our AST in which case we would probably handle 
it as if C++20 was activated (at least on the Clang side, not sure about the 
other parts of LLDB).

I don't think this review is blocked by this though. We first need to add 
proper C++20 support and then we can actually test this. But I'm also not 
opposed to having a test that tries calling <=> and just makes sure that Clang 
handles it and then errors out without crashing anything.



Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:15
+
+int f(int) { return 1; }
+

I wish this function had a more descriptive name. Maybe something like 
`FuncWithOverload` and `g` would be `FuncWithoutOverload`. Or a comment that 
the `f` here is intended to have the same name as `A::f` or something like 
that. Same for the other single-letter function names in here (as they all seem 
to serve a special purpose and are not just generic functions).



Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:42
+
+struct C {};
+

This isn't related to `A::C` from what I understand, so could this have another 
name than `C`?



Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:67
+ h(10) + h(1.) +
+ // variadic function
+ var(1) + var(1, 2); // break here

Can you move the comments what those expressions are testing to 
`TestTemplateFunctions.py`? If the expression fails it would be good to have 
the respective comment next to the expression instead of the copy in the .cpp 
file.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D75761



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


[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In D75929#1916127 , @ikudrin wrote:

> I believe that this patch is more or less compatible with any approach which 
> might be taken. The idea is that there is a set of constants for internal use 
> and functions to translate them to/from external representation and both 
> constants and translation functions might be adjusted when needed. In any 
> case, the general design remains the same.


That's true, but I'm not sure it is really the best solution. This way we will 
have three numbering schemes (four if you count the "index" thingy in llvm-dwp) 
floating around: the "gnu" scheme, the "official" scheme, and the "internal" 
scheme. That's quite a lot to keep in ones head at once.

I'm wondering if if wouldn't be simpler to have two complete enums --  with the 
"gnu" scheme, and one with the "official" scheme -- and then to internally use 
the enum matching the on-disk format currently in use. To insulate the users 
from having to guess the right enum to use, we could add a series of accessors: 
`getLocOffset`, `getMacInfoOffset`, ..., which would use the appropriate enum 
based on the index version (or assert if there is no such constant in the given 
version).

WDYT?

Regardless of the outcome of that, I think it would be good to split this patch 
up and separate the enum shuffling from the new functionality (does this only 
add parsing support for v5 indexes, or is there something more?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929



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


[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

2020-03-11 Thread James Henderson via Phabricator via lldb-commits
jhenderson added inline comments.



Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:5
+#include "gtest/gtest.h"
+#include 
+

jhenderson wrote:
> I think it's normal to put a blank line between standard library headers and 
> other includes.
Ignore this. I was thinking of a different code base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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


[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

2020-03-11 Thread Igor Kudrin via Phabricator via lldb-commits
ikudrin added a comment.

Thanks for the links! What a coincidence...

I believe that this patch is more or less compatible with any approach which 
might be taken. The idea is that there is a set of constants for internal use 
and functions to translate them to/from external representation and both 
constants and translation functions might be adjusted when needed. In any case, 
the general design remains the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929



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


[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

2020-03-11 Thread James Henderson via Phabricator via lldb-commits
jhenderson added inline comments.



Comment at: llvm/include/llvm/Support/ELFAttributes.h:32
+   bool HasTagPrefix = true);
+Optional attrTypeFromString(StringRef Tag, TagNameMap Map);
+

HsiangKai wrote:
> jhenderson wrote:
> > Can this be `Optional`?
> `AttrType` is the enum in `ELFAttrs`. There is also `AttrType` in 
> `ARMBuildAttributes` and `RISCVAttributes`. They all could be implicitly 
> converted to integer. So, I use integer as the common interface between 
> different architecture attributes enum.
Okay, seems reasonable. I wonder if AttrType needs to be specified as having 
`unsigned` underlying tap (in all cases) to be consistent with the 
`TagNameItem::attr` member.



Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:1
+#include "llvm/Support/ELFAttributeParser.h"
+#include "llvm/Support/ARMAttributeParser.h"

Missing licence header.

Test files usually are called *Test.cpp, where * is the base file/class that is 
being tested. It seems like this file is only testing the ARMAttributeParser. 
Should it be called "ARMAttributeParserTest.cpp"



Comment at: llvm/unittests/Support/ELFAttributeParser.cpp:5
+#include "gtest/gtest.h"
+#include 
+

I think it's normal to put a blank line between standard library headers and 
other includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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


[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

2020-03-11 Thread Igor Kudrin via Phabricator via lldb-commits
ikudrin added a comment.

In D75929#1916466 , @labath wrote:

> In D75929#1916127 , @ikudrin wrote:
>
> > I believe that this patch is more or less compatible with any approach 
> > which might be taken. The idea is that there is a set of constants for 
> > internal use and functions to translate them to/from external 
> > representation and both constants and translation functions might be 
> > adjusted when needed. In any case, the general design remains the same.
>
>
> That's true, but I'm not sure it is really the best solution.


Well, I do not pretend this to be a perfect solution, but anything I can 
imagine has its drawbacks. The whole problem comes from overlapping values in 
both standards, and any solution has to fix that somehow.

> This way we will have three numbering schemes (four if you count the "index" 
> thingy in llvm-dwp) floating around: the "gnu" scheme, the "official" scheme, 
> and the "internal" scheme. That's quite a lot to keep in ones head at once.

However, you do not need to worry about all the schemas everywhere. The only 
place they meet is translation functions. Most of the code just use internal 
encoding, which is enough.

> I'm wondering if if wouldn't be simpler to have two complete enums --  with 
> the "gnu" scheme, and one with the "official" scheme -- and then to 
> internally use the enum matching the on-disk format currently in use. To 
> insulate the users from having to guess the right enum to use, we could add a 
> series of accessors: `getLocOffset`, `getMacInfoOffset`, ..., which would use 
> the appropriate enum based on the index version (or assert if there is no 
> such constant in the given version).
> 
> WDYT?

Can't see how the accessors simplify the things. For me, `getLocOffset()` is 
almost the same as `getOffset(DW_SECT_LOC)` (or `getOffset(DW_SECT_GNU_LOC)`). 
That is no more than an agreement. Note that this patch does not have things 
like `DW_SECT_GNU_INFO`. There is only one constant for each section, so their 
usage is quite determined. The only exception is `DW_SECT_MACRO` and 
`DW_SECT_GNU_MACRO`, I just did not want to overcomplicate translation 
functions before receiving the feedback about the approach in general.

> Regardless of the outcome of that, I think it would be good to split this 
> patch up and separate the enum shuffling from the new functionality (does 
> this only add parsing support for v5 indexes, or is there something more?).

I'll prepare a separate review for the refactoring in `llvm-dwp.cpp`. As for 
the new identifiers and all shuffling stuff around, I am not sure it is really 
valuable to separate them because without the parser of v5 indexes they are 
meaningless and just dead code. Anyway, the splitting should wait until we 
decide whether the approach is viable in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929



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


[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

2020-03-11 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

In D75929#1916760 , @ikudrin wrote:

> In D75929#1916466 , @labath wrote:
>
> > That's true, but I'm not sure it is really the best solution.
>
>
> Well, I do not pretend this to be a perfect solution, but anything I can 
> imagine has its drawbacks. The whole problem comes from overlapping values in 
> both standards, and any solution has to fix that somehow.


Can't argue with that. :)

> 
> 
>> This way we will have three numbering schemes (four if you count the "index" 
>> thingy in llvm-dwp) floating around: the "gnu" scheme, the "official" 
>> scheme, and the "internal" scheme. That's quite a lot to keep in ones head 
>> at once.
> 
> However, you do not need to worry about all the schemas everywhere. The only 
> place they meet is translation functions. Most of the code just use internal 
> encoding, which is enough.

Well, that's where the accessor functions would come in. If everyone is calling 
that, then they are the only ones that need to handle the translation. One 
thing that bugs me about the current approach is that there are two 
more-or-less standardized enumerations, but we don't have enum types for either 
of those -- instead we have an enum containing the mangled internal constants.

However, I concede that this is just a matter of presentation, and the 
"internal" enum would be essentially implemented by this set of accessors.

What would you say to something different instead -- we do something similar to 
what was done with location lists, where we "upgrade" the format to the newest 
version during parsing? This is almost what you are doing right now -- the only 
difference is that the "internal" enum would no longer be internal -- it would 
actually match the on-disk format of a v5 index.  This v5 enum would contain 
the official DWARFv5 constants as well as the new extensions we want to 
introduce for mixed 5+4 indices.

This means that if we adopt the currently proposed 5+4 approach (which is 
looking increasingly likely -- if you hadn't posted this patch, I would 
probably be working on it now), there will only be two enums. But until we 
actually start writing files like this, the new extension constants will still 
only be kind of internal, and if there is a change in the mixed index approach 
we can always shuffle things around here.

>> Regardless of the outcome of that, I think it would be good to split this 
>> patch up and separate the enum shuffling from the new functionality (does 
>> this only add parsing support for v5 indexes, or is there something more?).
> 
> I'll prepare a separate review for the refactoring in `llvm-dwp.cpp`. As for 
> the new identifiers and all shuffling stuff around, I am not sure it is 
> really valuable to separate them because without the parser of v5 indexes 
> they are meaningless and just dead code. Anyway, the splitting should wait 
> until we decide whether the approach is viable in general.

Some of the files in this patch are only touched because of the introduction of 
`_GNU` in the DW_SECT constants. It would be clearer if that are done in an NFC 
patch. Also the parsing of a v5 index and making sure that a v5 dwp compile 
unit can find its location lists correctly look like they could be separate 
(you already have separate tests for those two things anyway).

But yea, that can wait until we have consensus on the overall direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75929



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


[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

2020-03-11 Thread Djordje Todorovic via Phabricator via lldb-commits
djtodoro added a comment.

In D73534#1915048 , @mstorsjo wrote:

> This broke compiling for mingw, repro.c:
>
>   a(short);
>   b() { a(1); }
>
>
> `clang -target x86_64-w64-mingw32 -c repro.c -g -O2`, which gives `Assertion 
> '!MI.isMoveImmediate() && "Unexpected MoveImm instruction"' failed.`


Thanks for reporting this! Since this is the case of the `X86::MOV16ri` the 
D75326  will solve this issue.


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

https://reviews.llvm.org/D73534



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


[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

2020-03-11 Thread Djordje Todorovic via Phabricator via lldb-commits
djtodoro added a comment.

In D73534#1916291 , @djtodoro wrote:

> In D73534#1915048 , @mstorsjo wrote:
>
> > This broke compiling for mingw, repro.c:
> >
> >   a(short);
> >   b() { a(1); }
> >
> >
> > `clang -target x86_64-w64-mingw32 -c repro.c -g -O2`, which gives 
> > `Assertion '!MI.isMoveImmediate() && "Unexpected MoveImm instruction"' 
> > failed.`
>
>
> Thanks for reporting this! Since this is the case of the `X86::MOV16ri` the 
> D75326  will solve this issue.


The alternative is D75974 .


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

https://reviews.llvm.org/D73534



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


[Lldb-commits] [PATCH] D76002: [lldb] Add YAML traits for ConstString and FileSpec

2020-03-11 Thread Jonas Devlieghere via Phabricator via lldb-commits
JDevlieghere created this revision.
JDevlieghere added a reviewer: labath.
Herald added a project: LLDB.

Add YAML traits for ConstString and FileSpec so they can be serialized as part 
of ProcessInfo.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D76002

Files:
  lldb/include/lldb/Utility/ConstString.h
  lldb/include/lldb/Utility/FileSpec.h
  lldb/source/Utility/ConstString.cpp
  lldb/source/Utility/FileSpec.cpp
  lldb/unittests/Utility/ConstStringTest.cpp
  lldb/unittests/Utility/FileSpecTest.cpp

Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -420,3 +420,24 @@
   EXPECT_TRUE(Match("", ""));
 
 }
+
+TEST(FileSpecTest, Yaml) {
+  std::string buffer;
+  llvm::raw_string_ostream os(buffer);
+
+  // Serialize.
+  FileSpec fs_windows("F:\\bar", FileSpec::Style::windows);
+  llvm::yaml::Output yout(os);
+  yout << fs_windows;
+  os.flush();
+
+  // Deserialize.
+  FileSpec deserialized;
+  llvm::yaml::Input yin(buffer);
+  yin >> deserialized;
+
+  EXPECT_EQ(deserialized.GetPathStyle(), fs_windows.GetPathStyle());
+  EXPECT_EQ(deserialized.GetFilename(), fs_windows.GetFilename());
+  EXPECT_EQ(deserialized.GetDirectory(), fs_windows.GetDirectory());
+  EXPECT_EQ(deserialized, fs_windows);
+}
Index: lldb/unittests/Utility/ConstStringTest.cpp
===
--- lldb/unittests/Utility/ConstStringTest.cpp
+++ lldb/unittests/Utility/ConstStringTest.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Utility/ConstString.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/YAMLParser.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
@@ -137,3 +138,22 @@
   EXPECT_TRUE(null == static_cast(nullptr));
   EXPECT_TRUE(null != "bar");
 }
+
+TEST(ConstStringTest, YAML) {
+  std::string buffer;
+  llvm::raw_string_ostream os(buffer);
+
+  // Serialize.
+  std::vector strings = {ConstString("foo"), ConstString("bar"),
+  ConstString("")};
+  llvm::yaml::Output yout(os);
+  yout << strings;
+  os.flush();
+
+  // Deserialize.
+  std::vector deserialized;
+  llvm::yaml::Input yin(buffer);
+  yin >> deserialized;
+
+  EXPECT_EQ(strings, deserialized);
+}
Index: lldb/source/Utility/FileSpec.cpp
===
--- lldb/source/Utility/FileSpec.cpp
+++ lldb/source/Utility/FileSpec.cpp
@@ -537,3 +537,17 @@
   if (!file.empty())
 Stream << file;
 }
+
+void llvm::yaml::ScalarEnumerationTraits::enumeration(
+IO &io, FileSpec::Style &value) {
+  io.enumCase(value, "windows", FileSpec::Style::windows);
+  io.enumCase(value, "posix", FileSpec::Style::posix);
+  io.enumCase(value, "native", FileSpec::Style::native);
+}
+
+void llvm::yaml::MappingTraits::mapping(IO &io, FileSpec &f) {
+  io.mapRequired("directory", f.m_directory);
+  io.mapRequired("file", f.m_filename);
+  io.mapRequired("resolved", f.m_is_resolved);
+  io.mapRequired("style", f.m_style);
+}
Index: lldb/source/Utility/ConstString.cpp
===
--- lldb/source/Utility/ConstString.cpp
+++ lldb/source/Utility/ConstString.cpp
@@ -337,3 +337,15 @@
 llvm::StringRef Options) {
   format_provider::format(CS.GetStringRef(), OS, Options);
 }
+
+void llvm::yaml::ScalarTraits::output(const ConstString &Val,
+   void *, raw_ostream &Out) {
+  Out << Val.GetStringRef();
+}
+
+llvm::StringRef
+llvm::yaml::ScalarTraits::input(llvm::StringRef Scalar, void *,
+ ConstString &Val) {
+  Val = ConstString(Scalar);
+  return {};
+}
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -18,6 +18,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/YAMLTraits.h"
 
 #include 
 #include 
@@ -397,6 +398,8 @@
   ConstString GetLastPathComponent() const;
 
 protected:
+  template  friend struct llvm::yaml::MappingTraits;
+
   // Convenience method for setting the file without changing the style.
   void SetFile(llvm::StringRef path);
 
@@ -436,6 +439,16 @@
   static void format(const lldb_private::FileSpec &F, llvm::raw_ostream &Stream,
  StringRef Style);
 };
+
+namespace yaml {
+template <> struct ScalarEnumerationTraits {
+  static void enumeration(IO &io, lldb_private::FileSpec::Style &style);
+};
+
+template <> struct MappingTraits {
+  static void mapping(IO &io, lldb_private::FileSpec &f);
+};
+} // namespace yaml
 } // namespace llvm
 
 #endif // LLDB_UTILITY_FILESPEC_H
Index: lldb/include/lldb/Utility/ConstString.h
===

[Lldb-commits] [PATCH] D75864: Add a decorator option to skip tests based on a default setting

2020-03-11 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment.

In D75864#1914916 , @labath wrote:

> Right, that makes sense, though it makes me cry a bit every time we add a new 
> option to the test decorators...


We call it the Christmas tree approach to testing!


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

https://reviews.llvm.org/D75864



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


[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

2020-03-11 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment.

To avoid bot breakage, I would recommend testing -DLLVM_ENABLE_MODULES=1 stage2 
build works before landing this, as it is notoriously sensitive to header 
reshuffling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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


[Lldb-commits] [lldb] 0396aa4 - Add a decorator option to skip tests based on a default setting.

2020-03-11 Thread Adrian Prantl via lldb-commits

Author: Adrian Prantl
Date: 2020-03-11T10:00:36-07:00
New Revision: 0396aa4c05a4b97101da14b5e813c8ab3a34e9d0

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

LOG: Add a decorator option to skip tests based on a default setting.

This patch allows skipping a test based on a default setting, which is
useful when running the testsuite in different "modes" based on a
default setting. This is a feature I need for the Swift testsuite, but
I think it's generally useful.

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

Added: 
lldb/test/API/sanity/TestSettingSkipping.py

Modified: 
lldb/packages/Python/lldbsuite/test/decorators.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/decorators.py 
b/lldb/packages/Python/lldbsuite/test/decorators.py
index 065b4ec92dac..d2241833f494 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -158,7 +158,8 @@ def _decorateTest(mode,
   debug_info=None,
   swig_version=None, py_version=None,
   macos_version=None,
-  remote=None, dwarf_version=None):
+  remote=None, dwarf_version=None,
+  setting=None):
 def fn(self):
 skip_for_os = _match_decorator_property(
 lldbplatform.translate(oslist), self.getPlatform())
@@ -196,6 +197,8 @@ def fn(self):
 skip_for_dwarf_version = (dwarf_version is None) or (
 _check_expected_version(dwarf_version[0], dwarf_version[1],
 self.getDwarfVersion()))
+skip_for_setting = (setting is None) or (
+setting in configuration.settings)
 
 # For the test to be skipped, all specified (e.g. not None) parameters 
must be True.
 # An unspecified parameter means "any", so those are marked skip by 
default.  And we skip
@@ -210,7 +213,8 @@ def fn(self):
   (py_version, skip_for_py_version, "python version"),
   (macos_version, skip_for_macos_version, "macOS version"),
   (remote, skip_for_remote, "platform locality 
(remote/local)"),
-  (dwarf_version, skip_for_dwarf_version, "dwarf version")]
+  (dwarf_version, skip_for_dwarf_version, "dwarf version"),
+  (setting, skip_for_setting, "setting")]
 reasons = []
 final_skip_result = True
 for this_condition in conditions:
@@ -279,7 +283,8 @@ def skipIf(bugnumber=None,
debug_info=None,
swig_version=None, py_version=None,
macos_version=None,
-   remote=None, dwarf_version=None):
+   remote=None, dwarf_version=None,
+   setting=None):
 return _decorateTest(DecorateMode.Skip,
  bugnumber=bugnumber,
  oslist=oslist, hostoslist=hostoslist,
@@ -288,7 +293,8 @@ def skipIf(bugnumber=None,
  debug_info=debug_info,
  swig_version=swig_version, py_version=py_version,
  macos_version=macos_version,
- remote=remote, dwarf_version=dwarf_version)
+ remote=remote, dwarf_version=dwarf_version,
+ setting=setting)
 
 
 def _skip_for_android(reason, api_levels, archs):

diff  --git a/lldb/test/API/sanity/TestSettingSkipping.py 
b/lldb/test/API/sanity/TestSettingSkipping.py
new file mode 100644
index ..10e7144a0068
--- /dev/null
+++ b/lldb/test/API/sanity/TestSettingSkipping.py
@@ -0,0 +1,29 @@
+"""
+This is a sanity check that verifies that test can be sklipped based on 
settings.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class SettingSkipSanityTestCase(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  NO_DEBUG_INFO_TESTCASE = True
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'no-dynamic-values'))
+  def testSkip(self):
+"""This setting is on by default"""
+self.assertTrue(False, "This test should not run!")
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'run-target'))
+  def testNoMatch(self):
+self.assertTrue(True, "This test should run!")
+
+  @skipIf(setting=('target.i-made-this-one-up', 'true'))
+  def testNotExisting(self):
+self.assertTrue(True, "This test should run!")
+



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


[Lldb-commits] [PATCH] D76004: [lldb] Add YAML traits for ArchSpec and ProcessInstanceInfo

2020-03-11 Thread Jonas Devlieghere via Phabricator via lldb-commits
JDevlieghere created this revision.
JDevlieghere added a reviewer: labath.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
JDevlieghere added a parent revision: D76002: [lldb] Add YAML traits for 
ConstString and FileSpec.

Add YAML traits for ArchSpec and ProcessInstanceInfo so they can be serialized 
for the reproducers.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D76004

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/include/lldb/Utility/ProcessInfo.h
  lldb/source/Utility/ArchSpec.cpp
  lldb/source/Utility/ProcessInfo.cpp
  lldb/unittests/Utility/ArchSpecTest.cpp
  lldb/unittests/Utility/ProcessInstanceInfoTest.cpp

Index: lldb/unittests/Utility/ProcessInstanceInfoTest.cpp
===
--- lldb/unittests/Utility/ProcessInstanceInfoTest.cpp
+++ lldb/unittests/Utility/ProcessInstanceInfoTest.cpp
@@ -108,3 +108,30 @@
   EXPECT_TRUE(match.Matches(info_bar));
   EXPECT_TRUE(match.Matches(info_empty));
 }
+
+TEST(ProcessInstanceInfoMatch, Yaml) {
+  std::string buffer;
+  llvm::raw_string_ostream os(buffer);
+
+  // Serialize.
+  ProcessInstanceInfo info("a.out", ArchSpec("x86_64-pc-linux"), 47);
+  info.SetUserID(1);
+  info.SetEffectiveUserID(2);
+  info.SetGroupID(3);
+  info.SetEffectiveGroupID(4);
+  llvm::yaml::Output yout(os);
+  yout << info;
+  os.flush();
+
+  // Deserialize.
+  ProcessInstanceInfo deserialized;
+  llvm::yaml::Input yin(buffer);
+  yin >> deserialized;
+
+  EXPECT_EQ(deserialized.GetNameAsStringRef(), info.GetNameAsStringRef());
+  EXPECT_EQ(deserialized.GetArchitecture(), info.GetArchitecture());
+  EXPECT_EQ(deserialized.GetUserID(), info.GetUserID());
+  EXPECT_EQ(deserialized.GetGroupID(), info.GetGroupID());
+  EXPECT_EQ(deserialized.GetEffectiveUserID(), info.GetEffectiveUserID());
+  EXPECT_EQ(deserialized.GetEffectiveGroupID(), info.GetEffectiveGroupID());
+}
Index: lldb/unittests/Utility/ArchSpecTest.cpp
===
--- lldb/unittests/Utility/ArchSpecTest.cpp
+++ lldb/unittests/Utility/ArchSpecTest.cpp
@@ -9,8 +9,9 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Utility/ArchSpec.h"
-#include "llvm/BinaryFormat/MachO.h"
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/BinaryFormat/MachO.h"
+#include "llvm/Support/YAMLParser.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -200,14 +201,14 @@
 
 EXPECT_TRUE(A.IsValid());
 EXPECT_TRUE(B.IsValid());
-
+
 EXPECT_EQ(llvm::Triple::ArchType::arm, B.GetTriple().getArch());
 EXPECT_EQ(llvm::Triple::VendorType::UnknownVendor,
   B.GetTriple().getVendor());
 EXPECT_EQ(llvm::Triple::OSType::Linux, B.GetTriple().getOS());
 EXPECT_EQ(llvm::Triple::EnvironmentType::UnknownEnvironment,
   B.GetTriple().getEnvironment());
-
+
 A.MergeFrom(B);
 EXPECT_EQ(llvm::Triple::ArchType::arm, A.GetTriple().getArch());
 EXPECT_EQ(llvm::Triple::VendorType::UnknownVendor,
@@ -406,3 +407,23 @@
 ASSERT_TRUE(D.TripleEnvironmentWasSpecified());
   }
 }
+
+TEST(ArchSpecTest, YAML) {
+  std::string buffer;
+  llvm::raw_string_ostream os(buffer);
+
+  // Serialize.
+  llvm::yaml::Output yout(os);
+  std::vector archs = {ArchSpec("x86_64-pc-linux"),
+ ArchSpec("x86_64-apple-macosx10.12"),
+ ArchSpec("i686-pc-windows")};
+  yout << archs;
+  os.flush();
+
+  // Deserialize.
+  std::vector deserialized;
+  llvm::yaml::Input yin(buffer);
+  yin >> deserialized;
+
+  EXPECT_EQ(archs, deserialized);
+}
Index: lldb/source/Utility/ProcessInfo.cpp
===
--- lldb/source/Utility/ProcessInfo.cpp
+++ lldb/source/Utility/ProcessInfo.cpp
@@ -331,3 +331,21 @@
   m_name_match_type = NameMatch::Ignore;
   m_match_all_users = false;
 }
+
+void llvm::yaml::MappingTraits::mapping(
+IO &io, ProcessInstanceInfo &Info) {
+  io.mapRequired("executable", Info.m_executable);
+  io.mapRequired("arg0", Info.m_arg0);
+  io.mapRequired("arch", Info.m_arch);
+  io.mapRequired("uid", Info.m_uid);
+  io.mapRequired("gid", Info.m_gid);
+  io.mapRequired("pid", Info.m_pid);
+  io.mapRequired("effective-uid", Info.m_euid);
+  io.mapRequired("effective-gid", Info.m_egid);
+  io.mapRequired("parent-pid", Info.m_parent_pid);
+}
+
+void llvm::yaml::MappingTraits::mapping(
+IO &io, ProcessInstanceInfoList &List) {
+  io.mapRequired("processes", List.m_infos);
+}
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -1467,3 +1467,15 @@
   if (!environ_str.empty())
 s << "-" << environ_str;
 }
+
+void llvm::yaml::ScalarTraits::output(const ArchSpec &Val, void *,
+raw_ostream &Out) {
+  Val.DumpTriple(Out);
+}
+
+llvm::StringRef
+llvm::yaml::ScalarTraits::input(ll

[Lldb-commits] [PATCH] D75925: [lldb] reject `.debug_arange` sections with nonzero segment size

2020-03-11 Thread Luke Drummond via Phabricator via lldb-commits
ldrumm updated this revision to Diff 249663.
ldrumm added a comment.

I found similar unittests for other DWARF entries which allow me to check more 
about the parser state, so I went with Pavel's suggestion for the testcase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75925

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp
  lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp


Index: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
@@ -18,6 +18,7 @@
 #include "Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDataExtractor.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
 #include "Plugins/SymbolFile/PDB/SymbolFilePDB.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
@@ -310,3 +311,34 @@
   EXPECT_EQ("abbreviation declaration attribute list not terminated with a "
 "null entry", llvm::toString(std::move(error)));
 }
+
+TEST_F(SymbolFileDWARFTests, ParseArangesNonzeroSegmentSize) {
+  // This `.debug_aranges` table header is a valid 32bit big-endian section
+  // according to the DWARFv5 spec:6.2.1, but contains segment selectors which
+  // are not supported by lldb, and should be gracefully rejected
+  const unsigned char binary_data[] = {
+0, 0, 0, 41, // unit_length (length field not including this field itself)
+0, 2, // DWARF version number (half)
+0, 0, 0, 0, // offset into the .debug_info_table (ignored for the purposes 
of this test
+4, // address size
+1, // segment size
+// alignment for the first tuple which "begins at an offset that is a
+// multiple of the size of a single tuple". Tuples are nine bytes in this 
example.
+0, 0, 0, 0, 0, 0,
+// BEGIN TUPLES
+1, 0, 0, 0, 4, 0, 0, 0, 1, // a 1byte object starting at address 4 in 
segment 1
+0, 0, 0, 0, 4, 0, 0, 0, 1, // a 1byte object starting at address 4 in 
segment 0
+// END TUPLES
+0, 0, 0, 0, 0, 0, 0, 0, 0 // terminator
+  };
+  DWARFDataExtractor data;
+  data.SetData(static_cast(binary_data), sizeof binary_data,
+   lldb::ByteOrder::eByteOrderBig);
+  DWARFDebugArangeSet debug_aranges;
+  offset_t off = 0;
+  llvm::Error error = debug_aranges.extract(data, &off);
+  EXPECT_TRUE(bool(error));
+  EXPECT_EQ("segmented arange entries are not supported",
+llvm::toString(std::move(error)));
+  EXPECT_EQ(off, 12); // Parser should read no further than the segment size
+}
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp
@@ -63,7 +63,8 @@
   // 1 - the version looks good
   // 2 - the address byte size looks plausible
   // 3 - the length seems to make sense
-  // size looks plausible
+  // 4 - size looks plausible
+  // 5 - the arange tuples do not contain a segment field
   if (m_header.version < 2 || m_header.version > 5)
 return llvm::make_error(
 "Invalid arange header version");
@@ -81,6 +82,10 @@
 return llvm::make_error(
 "Invalid arange header length");
 
+  if (m_header.seg_size)
+return llvm::make_error(
+"segmented arange entries are not supported");
+
   // The first tuple following the header in each set begins at an offset
   // that is a multiple of the size of a single tuple (that is, twice the
   // size of an address). The header is padded, if necessary, to the


Index: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
@@ -18,6 +18,7 @@
 #include "Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDataExtractor.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
 #include "Plugins/SymbolFile/PDB/SymbolFilePDB.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
@@ -310,3 +311,34 @@
   EXPECT_EQ("abbreviation declaration attribute list not terminated with a "
 "null entry", llvm::toString(std::move(error)));
 }
+
+TEST_F(SymbolFileDWARFTests, ParseArangesNonzeroSegmentSize) {
+  // This `.debug_aranges` table header is a valid 32bit big-endian section
+  // according to the DWARFv5 spec:6.2.1, but contains segment selectors which
+  // ar

[Lldb-commits] [PATCH] D75864: Add a decorator option to skip tests based on a default setting

2020-03-11 Thread Adrian Prantl via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0396aa4c05a4: Add a decorator option to skip tests based on 
a default setting. (authored by aprantl).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75864

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/test/API/sanity/TestSettingSkipping.py


Index: lldb/test/API/sanity/TestSettingSkipping.py
===
--- /dev/null
+++ lldb/test/API/sanity/TestSettingSkipping.py
@@ -0,0 +1,29 @@
+"""
+This is a sanity check that verifies that test can be sklipped based on 
settings.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class SettingSkipSanityTestCase(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  NO_DEBUG_INFO_TESTCASE = True
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'no-dynamic-values'))
+  def testSkip(self):
+"""This setting is on by default"""
+self.assertTrue(False, "This test should not run!")
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'run-target'))
+  def testNoMatch(self):
+self.assertTrue(True, "This test should run!")
+
+  @skipIf(setting=('target.i-made-this-one-up', 'true'))
+  def testNotExisting(self):
+self.assertTrue(True, "This test should run!")
+
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -158,7 +158,8 @@
   debug_info=None,
   swig_version=None, py_version=None,
   macos_version=None,
-  remote=None, dwarf_version=None):
+  remote=None, dwarf_version=None,
+  setting=None):
 def fn(self):
 skip_for_os = _match_decorator_property(
 lldbplatform.translate(oslist), self.getPlatform())
@@ -196,6 +197,8 @@
 skip_for_dwarf_version = (dwarf_version is None) or (
 _check_expected_version(dwarf_version[0], dwarf_version[1],
 self.getDwarfVersion()))
+skip_for_setting = (setting is None) or (
+setting in configuration.settings)
 
 # For the test to be skipped, all specified (e.g. not None) parameters 
must be True.
 # An unspecified parameter means "any", so those are marked skip by 
default.  And we skip
@@ -210,7 +213,8 @@
   (py_version, skip_for_py_version, "python version"),
   (macos_version, skip_for_macos_version, "macOS version"),
   (remote, skip_for_remote, "platform locality 
(remote/local)"),
-  (dwarf_version, skip_for_dwarf_version, "dwarf version")]
+  (dwarf_version, skip_for_dwarf_version, "dwarf version"),
+  (setting, skip_for_setting, "setting")]
 reasons = []
 final_skip_result = True
 for this_condition in conditions:
@@ -279,7 +283,8 @@
debug_info=None,
swig_version=None, py_version=None,
macos_version=None,
-   remote=None, dwarf_version=None):
+   remote=None, dwarf_version=None,
+   setting=None):
 return _decorateTest(DecorateMode.Skip,
  bugnumber=bugnumber,
  oslist=oslist, hostoslist=hostoslist,
@@ -288,7 +293,8 @@
  debug_info=debug_info,
  swig_version=swig_version, py_version=py_version,
  macos_version=macos_version,
- remote=remote, dwarf_version=dwarf_version)
+ remote=remote, dwarf_version=dwarf_version,
+ setting=setting)
 
 
 def _skip_for_android(reason, api_levels, archs):


Index: lldb/test/API/sanity/TestSettingSkipping.py
===
--- /dev/null
+++ lldb/test/API/sanity/TestSettingSkipping.py
@@ -0,0 +1,29 @@
+"""
+This is a sanity check that verifies that test can be sklipped based on settings.
+"""
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class SettingSkipSanityTestCase(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  NO_DEBUG_INFO_TESTCASE = True
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'no-dynamic-values'))
+  def testSkip(self):
+"""This setting is on by default"""
+self.assertTrue(False, "This test should not run!")
+
+  @skipIf(setting=('target.prefer-dynamic-value', 'run-target'))
+  def testNoMatch(self):
+self.assertTrue(True, "This test should run!")
+
+  @skipIf(setting=('target.i-made-this-one-up', 'true'))
+  

[Lldb-commits] [PATCH] D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID

2020-03-11 Thread Jim Ingham via Phabricator via lldb-commits
jingham updated this revision to Diff 249674.
jingham added a comment.

more reformatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880

Files:
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStack.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Process.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Target/ThreadList.cpp
  lldb/source/Target/ThreadPlan.cpp
  lldb/source/Target/ThreadPlanStack.cpp

Index: lldb/source/Target/ThreadPlanStack.cpp
===
--- /dev/null
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -0,0 +1,370 @@
+//===-- ThreadPlanStack.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 "lldb/Target/ThreadPlanStack.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadPlan.h"
+#include "lldb/Utility/Log.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+static void PrintPlanElement(Stream *s, const ThreadPlanSP &plan,
+ lldb::DescriptionLevel desc_level,
+ int32_t elem_idx) {
+  s->IndentMore();
+  s->Indent();
+  s->Printf("Element %d: ", elem_idx);
+  plan->GetDescription(s, desc_level);
+  s->EOL();
+  s->IndentLess();
+}
+
+void ThreadPlanStack::DumpThreadPlans(Stream *s,
+  lldb::DescriptionLevel desc_level,
+  bool include_internal) const {
+
+  uint32_t stack_size;
+
+  s->IndentMore();
+  s->Indent();
+  s->Printf("Active plan stack:\n");
+  int32_t print_idx = 0;
+  for (auto plan : m_plans) {
+PrintPlanElement(s, plan, desc_level, print_idx++);
+  }
+
+  if (AnyCompletedPlans()) {
+print_idx = 0;
+s->Indent();
+s->Printf("Completed Plan Stack:\n");
+for (auto plan : m_completed_plans)
+  PrintPlanElement(s, plan, desc_level, print_idx++);
+  }
+
+  if (AnyDiscardedPlans()) {
+print_idx = 0;
+s->Indent();
+s->Printf("Discarded Plan Stack:\n");
+for (auto plan : m_discarded_plans)
+  PrintPlanElement(s, plan, desc_level, print_idx++);
+  }
+
+  s->IndentLess();
+}
+
+size_t ThreadPlanStack::CheckpointCompletedPlans() {
+  m_completed_plan_checkpoint++;
+  m_completed_plan_store.insert(
+  std::make_pair(m_completed_plan_checkpoint, m_completed_plans));
+  return m_completed_plan_checkpoint;
+}
+
+void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t checkpoint) {
+  auto result = m_completed_plan_store.find(checkpoint);
+  assert(result != m_completed_plan_store.end() &&
+ "Asked for a checkpoint that didn't exist");
+  m_completed_plans.swap((*result).second);
+  m_completed_plan_store.erase(result);
+}
+
+void ThreadPlanStack::DiscardCompletedPlanCheckpoint(size_t checkpoint) {
+  m_completed_plan_store.erase(checkpoint);
+}
+
+void ThreadPlanStack::ThreadDestroyed(Thread *thread) {
+  // Tell the plan stacks that this thread is going away:
+  for (ThreadPlanSP plan : m_plans)
+plan->ThreadDestroyed();
+
+  for (ThreadPlanSP plan : m_discarded_plans)
+plan->ThreadDestroyed();
+
+  for (ThreadPlanSP plan : m_completed_plans)
+plan->ThreadDestroyed();
+
+  // Now clear the current plan stacks:
+  m_plans.clear();
+  m_discarded_plans.clear();
+  m_completed_plans.clear();
+
+  // Push a ThreadPlanNull on the plan stack.  That way we can continue
+  // assuming that the plan stack is never empty, but if somebody errantly asks
+  // questions of a destroyed thread without checking first whether it is
+  // destroyed, they won't crash.
+  if (thread != nullptr) {
+lldb::ThreadPlanSP null_plan_sp(new ThreadPlanNull(*thread));
+m_plans.push_back(null_plan_sp);
+  }
+}
+
+void ThreadPlanStack::EnableTracer(bool value, bool single_stepping) {
+  for (ThreadPlanSP plan : m_plans) {
+if (plan->GetThreadPlanTracer()) {
+  plan->GetThreadPlanTracer()->EnableTracing(value);
+  plan->GetThreadPlanTracer()->EnableSingleStep(single_stepping);
+}
+  }
+}
+
+void ThreadPlanStack::SetTracer(lldb::ThreadPlanTracerSP &tracer_sp) {
+  for (ThreadPlanSP plan : m_plans)
+plan->SetThreadPlanTracer(tracer_sp);
+}
+
+void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP new_plan_sp) {
+  // If the thread plan doesn't already have a tracer, give it its parent's
+  // tracer:
+  // The first plan has to be a base plan:
+  assert(m_plans.size() > 0 ||
+ new_plan_sp->IsBasePlan() && "Zeroth plan must be a base plan");
+
+  if (!n

[Lldb-commits] [PATCH] D75562: Add an opque payload field to lldb::Type (NFC).

2020-03-11 Thread Shafik Yaghmour via Phabricator via lldb-commits
shafik added a comment.

I want to see how you end up resolving the comments on payload being a plain 
integer type in D75626  before looking at this 
again. Maybe it makes more sense to use a type, the use is pretty clever but 
perhaps makes for opaque code in some places.


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

https://reviews.llvm.org/D75562



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


[Lldb-commits] [PATCH] D76009: [lldb/Target] Initialize new targets environment variables from target.env-vars

2020-03-11 Thread Frederic Riss via Phabricator via lldb-commits
friss created this revision.
friss added reviewers: jingham, labath.
Herald added a project: LLDB.

The TargetProperties constructor invokes a series of callbacks to
prime the properties from the default ones. The one callback in
charge of updating the inferior environment was commented out
because it crashed.

The reason for the crash is that TargetProperties is a parent class
of Target and the callbacks were invoked using a Target that was
not fully initialized. This patch moves the initial callback
invocations to a separate function that can be called at the end
the Target constructor, thus preventing the crash.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

The added test checks that the LaunchInfo object returned by
the target has been primed with the values from the settings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76009

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/settings/TestSettings.py

Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -218,6 +218,15 @@
 self.addTearDownHook(
 lambda: self.runCmd("settings clear target.env-vars"))
 
+launch_info = self.dbg.GetTargetAtIndex(0).GetLaunchInfo()
+found_env_var = False
+for i in range(0, launch_info.GetNumEnvironmentEntries()):
+if launch_info.GetEnvironmentEntryAtIndex(i) == "MY_ENV_VAR=YES":
+found_env_var = True
+break
+self.assertTrue(found_env_var,
+"MY_ENV_VAR was not set in LunchInfo object")
+
 self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()),
 RUN_SUCCEEDED)
 
@@ -238,15 +247,6 @@
 """Test that the host env vars are passed to the launched process."""
 self.build()
 
-exe = self.getBuildArtifact("a.out")
-self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
-
-# By default, inherit-env is 'true'.
-self.expect(
-'settings show target.inherit-env',
-"Default inherit-env is 'true'",
-startstr="target.inherit-env (boolean) = true")
-
 # Set some host environment variables now.
 os.environ["MY_HOST_ENV_VAR1"] = "VAR1"
 os.environ["MY_HOST_ENV_VAR2"] = "VAR2"
@@ -256,6 +256,15 @@
 os.environ.pop("MY_HOST_ENV_VAR1")
 os.environ.pop("MY_HOST_ENV_VAR2")
 
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# By default, inherit-env is 'true'.
+self.expect(
+'settings show target.inherit-env',
+"Default inherit-env is 'true'",
+startstr="target.inherit-env (boolean) = true")
+
 self.addTearDownHook(unset_env_variables)
 self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()),
 RUN_SUCCEEDED)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -81,6 +81,19 @@
   return class_name;
 }
 
+void TargetProperties::RunCallbacks() {
+  // Update m_launch_info once it was created
+  Arg0ValueChangedCallback();
+  RunArgsValueChangedCallback();
+  EnvVarsValueChangedCallback();
+  InputPathValueChangedCallback();
+  OutputPathValueChangedCallback();
+  ErrorPathValueChangedCallback();
+  DetachOnErrorValueChangedCallback();
+  DisableASLRValueChangedCallback();
+  DisableSTDIOValueChangedCallback();
+}
+
 Target::Target(Debugger &debugger, const ArchSpec &target_arch,
const lldb::PlatformSP &platform_sp, bool is_dummy_target)
 : TargetProperties(this),
@@ -113,6 +126,8 @@
  target_arch.GetArchitectureName(),
  target_arch.GetTriple().getTriple().c_str());
   }
+
+  RunCallbacks();
 }
 
 Target::~Target() {
@@ -3468,18 +3483,6 @@
 ConstString("Experimental settings - setting these won't produce "
 "errors if the setting is not present."),
 true, m_experimental_properties_up->GetValueProperties());
-
-// Update m_launch_info once it was created
-Arg0ValueChangedCallback();
-RunArgsValueChangedCallback();
-// EnvVarsValueChangedCallback(); // FIXME: cause segfault in
-// Target::GetPlatform()
-InputPathValueChangedCallback();
-OutputPathValueChangedCallback();
-ErrorPathValueChangedCallback();
-DetachOnErrorValueChangedCallback();
-DisableASLRValueChangedCallback();
-DisableSTDIOValueChangedCallback();
   } else {
  

[Lldb-commits] [PATCH] D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID

2020-03-11 Thread Jim Ingham via Phabricator via lldb-commits
jingham marked an inline comment as done.
jingham added inline comments.



Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}

labath wrote:
> jingham wrote:
> > labath wrote:
> > > This is the usual place where one would add a llvm_unreachable. Some 
> > > compilers do not consider the above switch to be fully covered (and if we 
> > > are to be fully pedantic, it isn't) and will complain about falling off 
> > > of the end of a non-void function.
> > I added a default, but what do you mean by "if we are to be fully pedantic, 
> > it isn't"?  It's an enum with 3 cases, all of which are listed.  How is 
> > that not fully covered?
> The first sentence of [[ https://en.cppreference.com/w/cpp/language/enum | 
> cppreference enum definition ]] could be helpful here:
> ```
> An enumeration is a distinct type whose value is restricted to a range of 
> values (see below for details), which may include several explicitly named 
> constants ("enumerators").
> ```
> What exactly is the "range of values" of an enum is a complex topic, but 
> suffice to say it usually includes more than the named constants one has 
> mentioned in the enum definition (imagine bitfield enums).
> Clang takes the "common sense" approach and assumes that if one does a switch 
> over an enum, he expects it to always have one of the named constants. Gcc 
> follows a stricter interpretation and concludes that there are execution 
> flows which do not return a value from this function and issues a warning. 
> Which behavior is more correct is debatable...
> 
> Unfortunately adding a default case is not the right solution here (though I 
> can see how that could be understood from my comment -- sorry), because clang 
> will then warn you about putting a default statement in what it considers to 
> be a fully covered switch. The right solution is to put the llvm_unreachable 
> call *after* the switch statement, which is the layout that will make 
> everyone happy. You don't need (or shouldn't) put the return statement 
> because the compilers know that the macro terminates the progream.
> 
> I'm sorry that this turned into such a lesson, but since you were using the 
> macro in an atypical way, I wanted to illustrate what is the more common use 
> of it.
The "may" in that sentence is apparently the crucial bit, but it didn't explain 
why an enum specified only by named constants might have other possible values. 
 I read a little further on, but didn't get to the part where it explains that. 
 Seems to me in this case the compiler shouldn't have to to guess about the 
extent of the valid values of this enum and then be conservative or not in 
handling the results of its guess.  I don't see the point of an enum if a 
legally constructed enum value (not cast from some random value) isn't 
exhausted by the constants used to define it.  That just seems weird.

But arguing with C++ compiler diagnostics isn't a productive use of time, and I 
feel like too deep an understanding of the C++ type system's mysteries is not 
good for one's mental stability.  So I'm more than content to cargo cult this 
one...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880



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


[Lldb-commits] [PATCH] D76011: Add a verification mechanism to CompilerType.

2020-03-11 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl created this revision.
aprantl added reviewers: jingham, davide, labath, teemperor.

Badly-written code can combine an unrelated TypeSystem and opaque type
pointer into a CompilerType. This is particularly an issue in
swift-lldb. This patch adds an assertion mechanism that catches these
kinds of mistakes early. Because this is an assertion-only code path
there is not cost for release builds.


https://reviews.llvm.org/D76011

Files:
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  lldb/source/Symbol/TypeSystem.cpp

Index: lldb/source/Symbol/TypeSystem.cpp
===
--- lldb/source/Symbol/TypeSystem.cpp
+++ lldb/source/Symbol/TypeSystem.cpp
@@ -70,6 +70,10 @@
   return CreateInstanceHelper(language, nullptr, target);
 }
 
+#ifndef NDEBUG
+bool TypeSystem::Verify(lldb::opaque_compiler_type_t type) { return true; }
+#endif
+
 bool TypeSystem::IsAnonymousType(lldb::opaque_compiler_type_t type) {
   return false;
 }
Index: lldb/source/Symbol/CompilerType.cpp
===
--- lldb/source/Symbol/CompilerType.cpp
+++ lldb/source/Symbol/CompilerType.cpp
@@ -865,6 +865,12 @@
   return false;
 }
 
+#ifndef NDEBUG
+bool CompilerType::Verify() const {
+  return !IsValid() || m_type_system->Verify(m_type);
+}
+#endif
+
 bool lldb_private::operator==(const lldb_private::CompilerType &lhs,
   const lldb_private::CompilerType &rhs) {
   return lhs.GetTypeSystem() == rhs.GetTypeSystem() &&
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -495,6 +495,10 @@
 
   // Tests
 
+#ifndef NDEBUG
+  bool Verify(lldb::opaque_compiler_type_t type) override;
+#endif
+  
   bool IsArrayType(lldb::opaque_compiler_type_t type,
CompilerType *element_type, uint64_t *size,
bool *is_incomplete) override;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2532,6 +2532,12 @@
 
 // Tests
 
+#ifndef NDEBUG
+bool TypeSystemClang::Verify(lldb::opaque_compiler_type_t type) {
+  return !type || llvm::isa(GetQualType(type).getTypePtr());
+}
+#endif
+
 bool TypeSystemClang::IsAggregateType(lldb::opaque_compiler_type_t type) {
   clang::QualType qual_type(RemoveWrappingTypes(GetCanonicalQualType(type)));
 
Index: lldb/include/lldb/Symbol/TypeSystem.h
===
--- lldb/include/lldb/Symbol/TypeSystem.h
+++ lldb/include/lldb/Symbol/TypeSystem.h
@@ -129,6 +129,11 @@
   void *other_opaque_decl_ctx) = 0;
 
   // Tests
+#ifndef NDEBUG
+  /// Verify the integrity of the type to catch CompilerTypes that mix
+  /// and match invalid TypeSystem/Opaque type pairs.
+  virtual bool Verify(lldb::opaque_compiler_type_t type) = 0;
+#endif
 
   virtual bool IsArrayType(lldb::opaque_compiler_type_t type,
CompilerType *element_type, uint64_t *size,
Index: lldb/include/lldb/Symbol/CompilerType.h
===
--- lldb/include/lldb/Symbol/CompilerType.h
+++ lldb/include/lldb/Symbol/CompilerType.h
@@ -39,7 +39,9 @@
   ///
   /// \see lldb_private::TypeSystemClang::GetType(clang::QualType)
   CompilerType(TypeSystem *type_system, lldb::opaque_compiler_type_t type)
-  : m_type(type), m_type_system(type_system) {}
+  : m_type(type), m_type_system(type_system) {
+assert(Verify() && "verification failed");
+  }
 
   CompilerType(const CompilerType &rhs)
   : m_type(rhs.m_type), m_type_system(rhs.m_type_system) {}
@@ -380,6 +382,13 @@
   }
 
 private:
+#ifndef NDEBUG
+  /// If the type is valid, ask the TypeSystem to verify the integrity
+  /// of the type to catch CompilerTypes that mix and match invalid
+  /// TypeSystem/Opaque type pairs.
+  bool Verify() const;
+#endif
+
   lldb::opaque_compiler_type_t m_type = nullptr;
   TypeSystem *m_type_system = nullptr;
 };
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D76011: Add a verification mechanism to CompilerType.

2020-03-11 Thread Jim Ingham via Phabricator via lldb-commits
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

Seems fine to me.


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

https://reviews.llvm.org/D76011



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


[Lldb-commits] [lldb] ea96037 - Add a verification mechanism to CompilerType.

2020-03-11 Thread Adrian Prantl via lldb-commits

Author: Adrian Prantl
Date: 2020-03-11T12:43:32-07:00
New Revision: ea960371861acad11b7018a5e280ae7a41ab9c02

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

LOG: Add a verification mechanism to CompilerType.

Badly-written code can combine an unrelated TypeSystem and opaque type
pointer into a CompilerType. This is particularly an issue in
swift-lldb. This patch adds an assertion mechanism that catches these
kinds of mistakes early. Because this is an assertion-only code path
there is not cost for release builds.

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

Added: 


Modified: 
lldb/include/lldb/Symbol/CompilerType.h
lldb/include/lldb/Symbol/TypeSystem.h
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/source/Symbol/CompilerType.cpp
lldb/source/Symbol/TypeSystem.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/CompilerType.h 
b/lldb/include/lldb/Symbol/CompilerType.h
index 0d536e090d20..0d1b00a1b26c 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -39,7 +39,9 @@ class CompilerType {
   ///
   /// \see lldb_private::TypeSystemClang::GetType(clang::QualType)
   CompilerType(TypeSystem *type_system, lldb::opaque_compiler_type_t type)
-  : m_type(type), m_type_system(type_system) {}
+  : m_type(type), m_type_system(type_system) {
+assert(Verify() && "verification failed");
+  }
 
   CompilerType(const CompilerType &rhs)
   : m_type(rhs.m_type), m_type_system(rhs.m_type_system) {}
@@ -380,6 +382,13 @@ class CompilerType {
   }
 
 private:
+#ifndef NDEBUG
+  /// If the type is valid, ask the TypeSystem to verify the integrity
+  /// of the type to catch CompilerTypes that mix and match invalid
+  /// TypeSystem/Opaque type pairs.
+  bool Verify() const;
+#endif
+
   lldb::opaque_compiler_type_t m_type = nullptr;
   TypeSystem *m_type_system = nullptr;
 };

diff  --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index 37c87a4bd025..a84b9a1c441c 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -129,6 +129,11 @@ class TypeSystem : public PluginInterface {
   void *other_opaque_decl_ctx) = 0;
 
   // Tests
+#ifndef NDEBUG
+  /// Verify the integrity of the type to catch CompilerTypes that mix
+  /// and match invalid TypeSystem/Opaque type pairs.
+  virtual bool Verify(lldb::opaque_compiler_type_t type) = 0;
+#endif
 
   virtual bool IsArrayType(lldb::opaque_compiler_type_t type,
CompilerType *element_type, uint64_t *size,

diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 47f1a852289a..42655ad6cc24 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2532,6 +2532,12 @@ ConvertAccessTypeToObjCIvarAccessControl(AccessType 
access) {
 
 // Tests
 
+#ifndef NDEBUG
+bool TypeSystemClang::Verify(lldb::opaque_compiler_type_t type) {
+  return !type || llvm::isa(GetQualType(type).getTypePtr());
+}
+#endif
+
 bool TypeSystemClang::IsAggregateType(lldb::opaque_compiler_type_t type) {
   clang::QualType qual_type(RemoveWrappingTypes(GetCanonicalQualType(type)));
 

diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 75e749f5783f..e22635dce9d0 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -495,6 +495,10 @@ class TypeSystemClang : public TypeSystem {
 
   // Tests
 
+#ifndef NDEBUG
+  bool Verify(lldb::opaque_compiler_type_t type) override;
+#endif
+  
   bool IsArrayType(lldb::opaque_compiler_type_t type,
CompilerType *element_type, uint64_t *size,
bool *is_incomplete) override;

diff  --git a/lldb/source/Symbol/CompilerType.cpp 
b/lldb/source/Symbol/CompilerType.cpp
index 1507f2c3121b..6c150988a012 100644
--- a/lldb/source/Symbol/CompilerType.cpp
+++ b/lldb/source/Symbol/CompilerType.cpp
@@ -865,6 +865,12 @@ bool CompilerType::GetValueAsScalar(const 
lldb_private::DataExtractor &data,
   return false;
 }
 
+#ifndef NDEBUG
+bool CompilerType::Verify() const {
+  return !IsValid() || m_type_system->Verify(m_type);
+}
+#endif
+
 bool lldb_private::operator==(const lldb_private::CompilerType &lhs,
   const lldb_private::CompilerType &rhs) {
   return lhs.GetTypeSystem() == rhs.GetTypeSystem() &&

diff  --git a/lldb/source/Symbol/TypeSystem.cpp 
b/lldb/source/Symbol/TypeSy

[Lldb-commits] [PATCH] D76011: Add a verification mechanism to CompilerType.

2020-03-11 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl closed this revision.
aprantl added a comment.

ea960371861acad11b7018a5e280ae7a41ab9c02 



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

https://reviews.llvm.org/D76011



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


[Lldb-commits] [lldb] c915cb9 - Avoid including Module.h from ExternalASTSource.h

2020-03-11 Thread Reid Kleckner via lldb-commits

Author: Reid Kleckner
Date: 2020-03-11T13:37:41-07:00
New Revision: c915cb957dc37275ce1ca1a0b993239c82f12692

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

LOG: Avoid including Module.h from ExternalASTSource.h

Module.h takes 86ms to parse, mostly parsing the class itself. Avoid it
if possible. ASTContext.h depends on ExternalASTSource.h.

A few NFC changes were needed to make this possible:

- Move ASTSourceDescriptor to Module.h. This needs Module to be
  complete, and seems more related to modules and AST files than
  external AST sources.
- Move "import complete" bit from Module* pointer int pair to
  NextLocalImport pointer. Required because PointerIntPair
  requires Module to be complete, and now it may not be.

Reviewed By: aaron.ballman, hans

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

Added: 


Modified: 
clang/include/clang/AST/ASTContext.h
clang/include/clang/AST/Decl.h
clang/include/clang/AST/ExternalASTSource.h
clang/include/clang/Basic/Module.h
clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
clang/lib/AST/ASTContext.cpp
clang/lib/AST/Decl.cpp
clang/lib/AST/ExternalASTSource.cpp
clang/lib/AST/ItaniumMangle.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Basic/Module.cpp
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDebugInfo.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Removed: 




diff  --git a/clang/include/clang/AST/ASTContext.h 
b/clang/include/clang/AST/ASTContext.h
index 92e5921fa375..75ab911d2459 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -121,6 +121,7 @@ class Preprocessor;
 class Stmt;
 class StoredDeclsMap;
 class TargetAttr;
+class TargetInfo;
 class TemplateDecl;
 class TemplateParameterList;
 class TemplateTemplateParmDecl;
@@ -881,7 +882,7 @@ class ASTContext : public RefCountedBase {
   void addedLocalImportDecl(ImportDecl *Import);
 
   static ImportDecl *getNextLocalImport(ImportDecl *Import) {
-return Import->NextLocalImport;
+return Import->getNextLocalImport();
   }
 
   using import_range = llvm::iterator_range;
@@ -909,13 +910,7 @@ class ASTContext : public RefCountedBase {
 
   /// Get the additional modules in which the definition \p Def has
   /// been merged.
-  ArrayRef getModulesWithMergedDefinition(const NamedDecl *Def) {
-auto MergedIt =
-MergedDefModules.find(cast(Def->getCanonicalDecl()));
-if (MergedIt == MergedDefModules.end())
-  return None;
-return MergedIt->second;
-  }
+  ArrayRef getModulesWithMergedDefinition(const NamedDecl *Def);
 
   /// Add a declaration to the list of declarations that are initialized
   /// for a module. This will typically be a global variable (with internal

diff  --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index b31cbab50222..bc7676534175 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -4340,17 +4340,18 @@ class ImportDecl final : public Decl,
   friend class ASTReader;
   friend TrailingObjects;
 
-  /// The imported module, along with a bit that indicates whether
-  /// we have source-location information for each identifier in the module
-  /// name.
-  ///
-  /// When the bit is false, we only have a single source location for the
-  /// end of the import declaration.
-  llvm::PointerIntPair ImportedAndComplete;
+  /// The imported module.
+  Module *ImportedModule = nullptr;
 
   /// The next import in the list of imports local to the translation
   /// unit being parsed (not loaded from an AST file).
-  ImportDecl *NextLocalImport = nullptr;
+  ///
+  /// Includes a bit that indicates whether we have source-location information
+  /// for each identifier in the module name.
+  ///
+  /// When the bit is false, we only have a single source location for the
+  /// end of the import declaration.
+  llvm::PointerIntPair NextLocalImportAndComplete;
 
   ImportDecl(DeclContext *DC, SourceLocation StartLoc, Module *Imported,
  ArrayRef IdentifierLocs);
@@ -4360,6 +4361,20 @@ class ImportDecl final : public Decl,
 
   ImportDecl(EmptyShell Empty) : Decl(Import, Empty) {}
 
+  bool isImportComplete() const { return NextLocalImportAndComplete.getInt(); }
+
+  void setImportComplete(bool C) { NextLocalImportAndComplete.setInt(C); }
+
+  /// The next import in the list of imports local to the translation
+  /// unit being parsed (not loaded from an AST file).
+  ImportDecl *getNextLocalImport() const {
+return NextLocalImportAndComplete.getPointer();
+  }
+
+  void setNextLocalImport(ImportDecl *Im

[Lldb-commits] [lldb] e08464f - Avoid including FileManager.h from SourceManager.h

2020-03-11 Thread Reid Kleckner via lldb-commits

Author: Reid Kleckner
Date: 2020-03-11T13:53:12-07:00
New Revision: e08464fb450456881733c885267b32dc7339cf11

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

LOG: Avoid including FileManager.h from SourceManager.h

Most clients of SourceManager.h need to do things like turning source
locations into file & line number pairs, but this doesn't require
bringing in FileManager.h and LLVM's FS headers.

The main code change here is to sink SM::createFileID into the cpp file.
I reason that this is not performance critical because it doesn't happen
on the diagnostic path, it happens along the paths of macro expansion
(could be hot) and new includes (less hot).

Saves some includes:
309 -
/usr/local/google/home/rnk/llvm-project/clang/include/clang/Basic/FileManager.h
272 -
/usr/local/google/home/rnk/llvm-project/clang/include/clang/Basic/FileSystemOptions.h
271 -
/usr/local/google/home/rnk/llvm-project/llvm/include/llvm/Support/VirtualFileSystem.h
267 -
/usr/local/google/home/rnk/llvm-project/llvm/include/llvm/Support/FileSystem.h
266 -
/usr/local/google/home/rnk/llvm-project/llvm/include/llvm/Support/Chrono.h

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

Added: 


Modified: 
clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
clang-tools-extra/clangd/Format.cpp
clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/include/clang/Basic/SourceManager.h
clang/include/clang/Frontend/CompilerInstance.h
clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
clang/include/clang/Lex/DirectoryLookup.h
clang/include/clang/Lex/ModuleMap.h
clang/include/clang/Lex/PPCallbacks.h
clang/lib/AST/ExternalASTSource.cpp
clang/lib/AST/MicrosoftMangle.cpp
clang/lib/Basic/SanitizerBlacklist.cpp
clang/lib/Basic/SourceManager.cpp
clang/lib/Basic/XRayLists.cpp
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CoverageMappingGen.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Frontend/PrecompiledPreamble.cpp
clang/lib/Index/CommentToXML.cpp
clang/lib/Index/USRGeneration.cpp
clang/lib/Lex/ModuleMap.cpp
clang/lib/Lex/PPCallbacks.cpp
clang/lib/Lex/PPLexerChange.cpp
clang/lib/Parse/Parser.cpp
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
clang/tools/clang-import-test/clang-import-test.cpp
clang/tools/clang-refactor/TestSupport.cpp
clang/tools/libclang/CXSourceLocation.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
clang/unittests/Frontend/CompilerInstanceTest.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Removed: 




diff  --git 
a/clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp 
b/clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
index ed1bc2f490ae..0f6ebbf2e23b 100644
--- a/clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
+++ b/clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
@@ -10,6 +10,7 @@
 #include "HeaderMapCollector.h"
 #include "PathConfig.h"
 #include "SymbolInfo.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/MacroInfo.h"

diff  --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp 
b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index 483177454527..7e5501ccc60a 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "ExpandModularHeadersPPCallbacks.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTReader.h"

diff  --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h 
b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
index 30c8236e7f91..fe1b00b4680a 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
@@ -13,6 +13,13 @@
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseSet.h"
 
+namespace llvm {
+namespace vfs {
+class OverlayFileSystem;
+class InMemoryFil

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

2020-03-11 Thread Reid Kleckner via Phabricator via lldb-commits
rnk added a comment.

Thanks!

In D75784#1917181 , @aprantl wrote:

> To avoid bot breakage, I would recommend testing -DLLVM_ENABLE_MODULES=1 
> stage2 build works before landing this, as it is notoriously sensitive to 
> header reshuffling.


Good idea, I confirmed this passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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


[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

2020-03-11 Thread Reid Kleckner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
rnk marked an inline comment as done.
Closed by commit rGc915cb957dc3: Avoid including Module.h from 
ExternalASTSource.h (authored by rnk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -40,6 +40,10 @@
 class DWARFASTParserClang;
 class PDBASTParser;
 
+namespace clang {
+class FileManager;
+}
+
 namespace lldb_private {
 
 class ClangASTMetadata;
Index: lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 #define LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 
+#include "clang/Basic/Module.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Sema/Sema.h"
@@ -71,7 +72,7 @@
 return m_Source->getModule(ID);
   }
 
-  llvm::Optional
+  llvm::Optional
   getSourceDescriptor(unsigned ID) override {
 return m_Source->getSourceDescriptor(ID);
   }
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1968,8 +1968,8 @@
 
 void ASTDeclReader::VisitImportDecl(ImportDecl *D) {
   VisitDecl(D);
-  D->ImportedAndComplete.setPointer(readModule());
-  D->ImportedAndComplete.setInt(Record.readInt());
+  D->ImportedModule = readModule();
+  D->setImportComplete(Record.readInt());
   auto *StoredLocs = D->getTrailingObjects();
   for (unsigned I = 0, N = Record.back(); I != N; ++I)
 StoredLocs[I] = readSourceLocation();
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8491,10 +8491,10 @@
   return (I - PCHModules.end()) << 1;
 }
 
-llvm::Optional
+llvm::Optional
 ASTReader::getSourceDescriptor(unsigned ID) {
   if (const Module *M = getSubmodule(ID))
-return ExternalASTSource::ASTSourceDescriptor(*M);
+return ASTSourceDescriptor(*M);
 
   // If there is only a single PCH, return it instead.
   // Chained PCH are not supported.
@@ -8503,8 +8503,8 @@
 ModuleFile &MF = ModuleMgr.getPrimaryModule();
 StringRef ModuleName = llvm::sys::path::filename(MF.OriginalSourceFileName);
 StringRef FileName = llvm::sys::path::filename(MF.FileName);
-return ASTReader::ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
-  MF.Signature);
+return ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
+   MF.Signature);
   }
   return None;
 }
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -20,6 +20,7 @@
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeOrdering.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
@@ -60,7 +61,7 @@
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
-  ExternalASTSource::ASTSourceDescriptor PCHDescriptor;
+  ASTSourceDescriptor PCHDescriptor;
   SourceLocation CurLoc;
   llvm::MDNode *CurInlinedAt = nullptr;
   llvm::DIType *VTablePtrType = nullptr;
@@ -379,9 +380,7 @@
   /// When generating debug information for a clang module or
   /// precompiled header, this module map will be used to determine
   /// the module of origin of each Decl.
-  void setPCHDescriptor(ExternalASTSource::ASTSourceDescriptor PCH) {
-PCHDescriptor = PCH;
-  }
+  void setPCHDescriptor(ASTS

[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

2020-03-11 Thread Reid Kleckner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe08464fb4504: Avoid including FileManager.h from 
SourceManager.h (authored by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D75406?vs=247479&id=249752#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406

Files:
  clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
  clang-tools-extra/clangd/Format.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/ModuleMap.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Basic/SanitizerBlacklist.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/XRayLists.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPCallbacks.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/libclang/CXSourceLocation.cpp
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/Frontend/CompilerInstanceTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Index: lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
===
--- lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
+++ lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Utility/AnsiTerminal.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringSet.h"
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -9,6 +9,7 @@
 #include "ClangExpressionSourceCode.h"
 
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
Index: clang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- clang/unittests/Frontend/CompilerInstanceTest.cpp
+++ clang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "llvm/Support/FileSystem.h"
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -8,6 +8,7 @@
 
 #include 
 
+#include "clang/Basic/FileManager.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
Index: clang/tools/libclang/CXSourceLocation.cpp
===
--- clang/tools/libclang/CXSourceLocation.cpp
+++ clang/tools/libclang/CXSourceLocation.cpp
@@ -10,13 +10,14 @@
 //
 //===--===//
 
-#include "clang/Frontend/ASTUnit.h"
+#include "CXSourceLocation.h"
 #include "CIndexer.h"
 #include "CLog.h"
 #include "CXLoadedDiagnostic.h"
-#include "CXSourceLocation.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Format.h"
 
Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSuppo

[Lldb-commits] [PATCH] D75877: [lldb/Reproducers] Fix replay for process attach workflows

2020-03-11 Thread Jonas Devlieghere via Phabricator via lldb-commits
JDevlieghere marked 7 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/source/Utility/ProcessInfo.cpp:417-430
+  static std::unique_ptr>
+  loader = repro::MultiLoader::Create(
+  repro::Reproducer::Instance().GetLoader());
+
+  if (!loader)
+return {};
+

labath wrote:
> random thought: Would any of this be simpler if this wasn't a "multi" 
> provider but rather stored all of the responses as a sequence in a single 
> file?
Maybe/Probably? I'm not sure. But even if it were a bit simpler, I think it's 
better to reuse the existing multi-provider for consistency. 


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

https://reviews.llvm.org/D75877



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


[Lldb-commits] [PATCH] D75925: [lldb] reject `.debug_arange` sections with nonzero segment size

2020-03-11 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added a comment.

lgtm once all formatting issues are taken care of. Pavel?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75925



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


[Lldb-commits] [PATCH] D75877: [lldb/Reproducers] Fix replay for process attach workflows

2020-03-11 Thread Jonas Devlieghere via Phabricator via lldb-commits
JDevlieghere updated this revision to Diff 249764.
JDevlieghere added a comment.

Address code review feedback.


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

https://reviews.llvm.org/D75877

Files:
  lldb/include/lldb/Host/Host.h
  lldb/include/lldb/Utility/ProcessInfo.h
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/linux/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Host/netbsd/Host.cpp
  lldb/source/Host/openbsd/Host.cpp
  lldb/source/Host/windows/Host.cpp
  lldb/source/Utility/ProcessInfo.cpp
  lldb/test/API/functionalities/reproducers/attach/Makefile
  lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
  lldb/test/API/functionalities/reproducers/attach/main.cpp

Index: lldb/test/API/functionalities/reproducers/attach/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/reproducers/attach/main.cpp
@@ -0,0 +1,24 @@
+#include 
+#include 
+#include 
+
+using std::chrono::seconds;
+
+int main(int argc, char const *argv[]) {
+  lldb_enable_attach();
+
+  // Create the synchronization token.
+  FILE *f;
+  if (f = fopen(argv[1], "wx")) {
+fputs("\n", f);
+fflush(f);
+fclose(f);
+  } else
+return 1;
+
+  while (true) {
+std::this_thread::sleep_for(seconds(1));
+  }
+
+  return 0;
+}
Index: lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
===
--- /dev/null
+++ lldb/test/API/functionalities/reproducers/attach/TestReproducerAttach.py
@@ -0,0 +1,82 @@
+"""
+Test reproducer attach.
+"""
+
+import lldb
+import tempfile
+from lldbsuite.test import lldbtest_config
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CreateAfterAttachTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+@skipIfFreeBSD
+@skipIfNetBSD
+@skipIfWindows
+@skipIfRemote
+@skipIfiOSSimulator
+def test_create_after_attach_with_fork(self):
+"""Test thread creation after process attach."""
+exe = '%s_%d' % (self.testMethodName, os.getpid())
+token = exe + '.token'
+if not lldb.remote_platform:
+token = self.getBuildArtifact(token)
+if os.path.exists(token):
+os.remove(token)
+
+self.build(dictionary={'EXE': exe})
+self.addTearDownHook(self.cleanupSubprocesses)
+
+reproducer_patch = tempfile.NamedTemporaryFile().name
+
+inferior = self.spawnSubprocess(self.getBuildArtifact(exe), [token])
+pid = inferior.pid
+
+lldbutil.wait_for_file_on_target(self, token)
+
+# Use Popen because pexpect is overkill and spawnSubprocess is
+# asynchronous.
+capture = subprocess.Popen([
+lldbtest_config.lldbExec, '-b', '--capture', '--capture-path',
+reproducer_patch, '-o', 'proc att -n {}'.format(exe), '-o',
+'reproducer generate'
+],
+   stdin=subprocess.PIPE,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.PIPE)
+outs, errs = capture.communicate()
+self.assertTrue('Process {} stopped'.format(pid) in outs)
+self.assertTrue('Reproducer written' in outs)
+
+# Check that replay works.
+replay = subprocess.Popen(
+[lldbtest_config.lldbExec, '-replay', reproducer_patch],
+stdin=subprocess.PIPE,
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE)
+outs, errs = replay.communicate()
+self.assertTrue('Process {} stopped'.format(pid) in outs)
+
+# Check that reproducer dump works for process info.
+replay = subprocess.Popen([
+lldbtest_config.lldbExec, '-b', '-o',
+'reproducer dump -f {} -p process'.format(reproducer_patch)
+],
+  stdin=subprocess.PIPE,
+  stdout=subprocess.PIPE,
+  stderr=subprocess.PIPE)
+outs, errs = replay.communicate()
+self.assertTrue('name = {}'.format(exe))
+self.assertTrue('pid = {}'.format(pid))
+
+# Remove the reproducer but don't complain in case the directory was
+# never created.
+try:
+shutil.rmtree(reproducer_patch)
+except OSError:
+pass
Index: lldb/test/API/functionalities/reproducers/attach/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/reproducers/attach/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules
Index: lldb/source/Utility/ProcessInfo.cpp
===

[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

2020-03-11 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added a comment.

In D74636#1916356 , @labath wrote:

> There's a `target.inherit-env` setting in lldb (which I believe also works 
> correctly for remote launches). Could you use that instead of reimplementing 
> the feature in vscode?


The real question is if we want launching to fail when the user tries to enable 
this for remote targets? If we don't care if it fails, then we can use 
"target.inherit-env", but I kind of like that it does fail as it clearly 
conveys to the user that their environment won't be passed along instead of 
just ignoring the request. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74636



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


[Lldb-commits] [lldb] 213aea4 - Remove unused Endian.h includes, NFC

2020-03-11 Thread Reid Kleckner via lldb-commits

Author: Reid Kleckner
Date: 2020-03-11T15:45:34-07:00
New Revision: 213aea4c5836934771eb97eb97e4c964053a8596

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

LOG: Remove unused Endian.h includes, NFC

Mainly avoids including Host.h everywhere:

$ diff -u <(sort thedeps-before.txt) <(sort thedeps-after.txt) \
| grep '^[-+] ' | sort | uniq -c | sort -nr
   3141 - 
/usr/local/google/home/rnk/llvm-project/llvm/include/llvm/Support/Host.h

Added: 


Modified: 
clang/lib/Driver/Distro.cpp
clang/lib/Driver/Driver.cpp
clang/lib/Driver/ToolChains/Cuda.cpp
clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
clang/tools/driver/cc1gen_reproducer_main.cpp
clang/unittests/AST/StructuralEquivalenceTest.cpp
clang/unittests/CodeGen/TBAAMetadataTest.cpp
clang/unittests/Driver/DistroTest.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
lld/ELF/DriverUtils.cpp
lld/MinGW/Driver.cpp
lld/wasm/Driver.cpp
lldb/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h
llvm/include/llvm/Support/Endian.h
llvm/tools/llvm-ar/llvm-ar.cpp
llvm/tools/llvm-exegesis/lib/LlvmState.cpp
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
llvm/unittests/Support/CommandLineTest.cpp

Removed: 




diff  --git a/clang/lib/Driver/Distro.cpp b/clang/lib/Driver/Distro.cpp
index 06707fefc9d0..acfde7d8bd0b 100644
--- a/clang/lib/Driver/Distro.cpp
+++ b/clang/lib/Driver/Distro.cpp
@@ -11,9 +11,10 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/ADT/Triple.h"
 
 using namespace clang::driver;
 using namespace clang;

diff  --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 642b00b57fb6..37761893f137 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -38,8 +38,8 @@
 #include "ToolChains/NaCl.h"
 #include "ToolChains/NetBSD.h"
 #include "ToolChains/OpenBSD.h"
-#include "ToolChains/PS4CPU.h"
 #include "ToolChains/PPCLinux.h"
+#include "ToolChains/PS4CPU.h"
 #include "ToolChains/RISCVToolchain.h"
 #include "ToolChains/Solaris.h"
 #include "ToolChains/TCE.h"
@@ -71,6 +71,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"

diff  --git a/clang/lib/Driver/ToolChains/Cuda.cpp 
b/clang/lib/Driver/ToolChains/Cuda.cpp
index d6050925cd9e..01c5a9175da2 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -18,6 +18,7 @@
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"

diff  --git a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp 
b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
index 99298316718b..f1ab2aed54c0 100644
--- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -12,6 +12,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"

diff  --git a/clang/tools/driver/cc1gen_reproducer_main.cpp 
b/clang/tools/driver/cc1gen_reproducer_main.cpp
index 4aadab7301bc..472055ee2170 100644
--- a/clang/tools/driver/cc1gen_reproducer_main.cpp
+++ b/clang/tools/driver/cc1gen_reproducer_main.cpp
@@ -18,6 +18,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/YAMLTraits.h"

diff  --git a/clang/unittests/AST/StructuralEquivalenceTest.cpp 
b/clang/unittests/AST/StructuralEquivalenceTest.cpp
index 8e467527eadb..493b847fa5fa 100644
--- a/clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ b/clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -1,8 +1,9 @@
 #include "clang/AST/ASTContext.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/AST/ASTStructuralEquivalence.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Host.h"
 
 #include "Language.h"
 #include 

[Lldb-commits] [lldb] ae73ab6 - Update debugserver test for new ostype names

2020-03-11 Thread Adrian Prantl via lldb-commits

Author: Adrian Prantl
Date: 2020-03-11T15:50:03-07:00
New Revision: ae73ab64b66d1889b447303d432f4d217d222def

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

LOG: Update debugserver test for new ostype names

Added: 


Modified: 
lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py

Removed: 




diff  --git a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py 
b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
index 86b54dd3e8e5..2a2d174afeeb 100644
--- a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -105,18 +105,19 @@ def check_simulator_ostype(self, sdk, platform, 
arch='x86_64'):
 @skipIfRemote
 def test_simulator_ostype_ios(self):
 self.check_simulator_ostype(sdk='iphonesimulator',
-platform='ios')
+platform='iossimulator')
 
 @apple_simulator_test('appletv')
 @debugserver_test
 @skipIfRemote
 def test_simulator_ostype_tvos(self):
 self.check_simulator_ostype(sdk='appletvsimulator',
-platform='tvos')
+platform='tvossimulator')
 
 @apple_simulator_test('watch')
 @debugserver_test
 @skipIfRemote
 def test_simulator_ostype_watchos(self):
 self.check_simulator_ostype(sdk='watchsimulator',
-platform='watchos', arch='i386')
+platform='watchossimulator',
+arch='i386')



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


[Lldb-commits] [lldb] cd4c1ad - Add newly-missing include

2020-03-11 Thread Adrian Prantl via lldb-commits

Author: Adrian Prantl
Date: 2020-03-11T19:30:53-07:00
New Revision: cd4c1adabeae8ea2939ee2d00d9d57aba3767960

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

LOG: Add newly-missing include

Added: 


Modified: 
lldb/unittests/Host/HostInfoTest.cpp

Removed: 




diff  --git a/lldb/unittests/Host/HostInfoTest.cpp 
b/lldb/unittests/Host/HostInfoTest.cpp
index 1b31fde8580f..ed4b7b5d39c0 100644
--- a/lldb/unittests/Host/HostInfoTest.cpp
+++ b/lldb/unittests/Host/HostInfoTest.cpp
@@ -11,6 +11,7 @@
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/lldb-defines.h"
+#include "llvm/Support/Host.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;



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


[Lldb-commits] [lldb] 5161194 - Revert "Update debugserver test for new ostype names"

2020-03-11 Thread Adrian Prantl via lldb-commits

Author: Adrian Prantl
Date: 2020-03-11T21:32:12-07:00
New Revision: 5161194fad8cdd0b496c63c410855290c5e5190b

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

LOG: Revert "Update debugserver test for new ostype names"

I accidentally commited this while cherry-picking commits out of my
reflog.

This reverts commit ae73ab64b66d1889b447303d432f4d217d222def.

Added: 


Modified: 
lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py

Removed: 




diff  --git a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py 
b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
index 2a2d174afeeb..86b54dd3e8e5 100644
--- a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -105,19 +105,18 @@ def check_simulator_ostype(self, sdk, platform, 
arch='x86_64'):
 @skipIfRemote
 def test_simulator_ostype_ios(self):
 self.check_simulator_ostype(sdk='iphonesimulator',
-platform='iossimulator')
+platform='ios')
 
 @apple_simulator_test('appletv')
 @debugserver_test
 @skipIfRemote
 def test_simulator_ostype_tvos(self):
 self.check_simulator_ostype(sdk='appletvsimulator',
-platform='tvossimulator')
+platform='tvos')
 
 @apple_simulator_test('watch')
 @debugserver_test
 @skipIfRemote
 def test_simulator_ostype_watchos(self):
 self.check_simulator_ostype(sdk='watchsimulator',
-platform='watchossimulator',
-arch='i386')
+platform='watchos', arch='i386')



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


[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

2020-03-11 Thread walter erquinigo via Phabricator via lldb-commits
wallace added a comment.

Regarding implementation:

The target.inherit-env setting is only effectively used by CommandObjectProcess 
when launching the target from the command line. lldb-vscode is not following 
the same codepath and not using that property.

What about exposing the Platform's environment in the API and merging 
SBPlatform->GetEnvironment() with the launch.json's environment if 
inheritEnvironment is True? That would be very similar to what is doing 
CommandObjectProcess

-

If the user creates the target on their own using custom launchCommands, I 
imagine these commands would go through CommandObjectProcess, and if 
target.inherit-env is also set to true, then this would work also for remote 
cases.

Is that right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74636



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


[Lldb-commits] [PATCH] D76045: [lldb/API] Make Launch(Simple) use args and env from target properties

2020-03-11 Thread Frederic Riss via Phabricator via lldb-commits
friss created this revision.
friss added reviewers: jingham, labath.
Herald added projects: LLDB, libc++abi.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++abi.
friss updated this revision to Diff 249834.
friss added a comment.
friss removed a reviewer: libc++abi.
friss removed a project: libc++abi.
friss removed a subscriber: libcxx-commits.
friss added a parent revision: D76009: [lldb/Target] Initialize new targets 
environment variables from target.env-vars.

Remove unrelated change commited by accident


When no arguments or environment is provided to SBTarget::LaunchSimple,
make it use the values surrently set in the target properties. You can
get the current behavior back by passing an empty array instead.

It seems like using the target defaults is a much more intuitive
behavior for those APIs. It's unllikely that anyone passed NULL/None to
this API after having set properties in order to explicitely ignore them.

One direct application of this change is within the testsuite. We have
plenty of tests calling LaunchSimple and passing None as environment.
If you passed --inferior-env to dotest.py to, for example, set
(DY)LD_LIBRARY_PATH, it wouldn't be taken into account.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76045

Files:
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBTarget.cpp
  lldb/test/API/commands/settings/TestSettings.py

Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -204,10 +204,15 @@
 
 @skipIfDarwinEmbedded   #  debugserver on ios etc can't write files
 def test_run_args_and_env_vars(self):
+self.do_test_run_args_and_env_vars(use_launchsimple=False)
+
+@skipIfDarwinEmbedded   #  debugserver on ios etc can't write files
+def test_launchsimple_args_and_env_vars(self):
+self.do_test_run_args_and_env_vars(use_launchsimple=True)
+
+def do_test_run_args_and_env_vars(self, use_launchsimple):
 """Test that run-args and env-vars are passed to the launched process."""
 self.build()
-exe = self.getBuildArtifact("a.out")
-self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
 # Set the run-args and the env-vars.
 # And add hooks to restore the settings during tearDown().
@@ -218,7 +223,11 @@
 self.addTearDownHook(
 lambda: self.runCmd("settings clear target.env-vars"))
 
-launch_info = self.dbg.GetTargetAtIndex(0).GetLaunchInfo()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+target = self.dbg.GetTargetAtIndex(0)
+launch_info = target.GetLaunchInfo()
 found_env_var = False
 for i in range(0, launch_info.GetNumEnvironmentEntries()):
 if launch_info.GetEnvironmentEntryAtIndex(i) == "MY_ENV_VAR=YES":
@@ -227,7 +236,12 @@
 self.assertTrue(found_env_var,
 "MY_ENV_VAR was not set in LunchInfo object")
 
-self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()),
+wd = self.get_process_working_directory()
+if use_launchsimple:
+process = target.LaunchSimple(None, None, wd)
+self.assertTrue(process)
+else:
+self.runCmd("process launch --working-dir '{0}'".format(wd),
 RUN_SUCCEEDED)
 
 # Read the output file produced by running the program.
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -371,10 +371,19 @@
 Module *exe_module = target_sp->GetExecutableModulePointer();
 if (exe_module)
   launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec(), true);
-if (argv)
+if (argv) {
   launch_info.GetArguments().AppendArguments(argv);
-if (envp)
+} else {
+  auto default_launch_info = target_sp->GetProcessLaunchInfo();
+  launch_info.GetArguments().AppendArguments(
+  default_launch_info.GetArguments());
+}
+if (envp) {
   launch_info.GetEnvironment() = Environment(envp);
+} else {
+  auto default_launch_info = target_sp->GetProcessLaunchInfo();
+  launch_info.GetEnvironment() = default_launch_info.GetEnvironment();
+}
 
 if (listener.IsValid())
   launch_info.SetListener(listener.GetSP());
Index: lldb/include/lldb/API/SBTarget.h
===
--- lldb/include/lldb/API/SBTarget.h
+++ lldb/include/lldb/API/SBTarget.h
@@ -127,7 +127,9 @@
   /// The argument array.
   ///
   /// \param[in] envp
-  /// The environment array.
+  /// The environment array. If this isn't provided, the default
+  /// environment values (provided thro

[Lldb-commits] [PATCH] D76045: [lldb/API] Make Launch(Simple) use args and env from target properties

2020-03-11 Thread Frederic Riss via Phabricator via lldb-commits
friss updated this revision to Diff 249834.
friss added a comment.

Remove unrelated change commited by accident


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76045

Files:
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBTarget.cpp
  lldb/test/API/commands/settings/TestSettings.py

Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -204,10 +204,15 @@
 
 @skipIfDarwinEmbedded   #  debugserver on ios etc can't write files
 def test_run_args_and_env_vars(self):
+self.do_test_run_args_and_env_vars(use_launchsimple=False)
+
+@skipIfDarwinEmbedded   #  debugserver on ios etc can't write files
+def test_launchsimple_args_and_env_vars(self):
+self.do_test_run_args_and_env_vars(use_launchsimple=True)
+
+def do_test_run_args_and_env_vars(self, use_launchsimple):
 """Test that run-args and env-vars are passed to the launched process."""
 self.build()
-exe = self.getBuildArtifact("a.out")
-self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
 # Set the run-args and the env-vars.
 # And add hooks to restore the settings during tearDown().
@@ -218,7 +223,11 @@
 self.addTearDownHook(
 lambda: self.runCmd("settings clear target.env-vars"))
 
-launch_info = self.dbg.GetTargetAtIndex(0).GetLaunchInfo()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+target = self.dbg.GetTargetAtIndex(0)
+launch_info = target.GetLaunchInfo()
 found_env_var = False
 for i in range(0, launch_info.GetNumEnvironmentEntries()):
 if launch_info.GetEnvironmentEntryAtIndex(i) == "MY_ENV_VAR=YES":
@@ -227,7 +236,12 @@
 self.assertTrue(found_env_var,
 "MY_ENV_VAR was not set in LunchInfo object")
 
-self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()),
+wd = self.get_process_working_directory()
+if use_launchsimple:
+process = target.LaunchSimple(None, None, wd)
+self.assertTrue(process)
+else:
+self.runCmd("process launch --working-dir '{0}'".format(wd),
 RUN_SUCCEEDED)
 
 # Read the output file produced by running the program.
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -371,10 +371,19 @@
 Module *exe_module = target_sp->GetExecutableModulePointer();
 if (exe_module)
   launch_info.SetExecutableFile(exe_module->GetPlatformFileSpec(), true);
-if (argv)
+if (argv) {
   launch_info.GetArguments().AppendArguments(argv);
-if (envp)
+} else {
+  auto default_launch_info = target_sp->GetProcessLaunchInfo();
+  launch_info.GetArguments().AppendArguments(
+  default_launch_info.GetArguments());
+}
+if (envp) {
   launch_info.GetEnvironment() = Environment(envp);
+} else {
+  auto default_launch_info = target_sp->GetProcessLaunchInfo();
+  launch_info.GetEnvironment() = default_launch_info.GetEnvironment();
+}
 
 if (listener.IsValid())
   launch_info.SetListener(listener.GetSP());
Index: lldb/include/lldb/API/SBTarget.h
===
--- lldb/include/lldb/API/SBTarget.h
+++ lldb/include/lldb/API/SBTarget.h
@@ -127,7 +127,9 @@
   /// The argument array.
   ///
   /// \param[in] envp
-  /// The environment array.
+  /// The environment array. If this isn't provided, the default
+  /// environment values (provided through `settings set
+  /// target.env-vars`) will be used.
   ///
   /// \param[in] stdin_path
   /// The path to use when re-directing the STDIN of the new
@@ -175,7 +177,9 @@
   /// The argument array.
   ///
   /// \param[in] envp
-  /// The environment array.
+  /// The environment array. If this isn't provided, the default
+  /// environment values (provided through `settings set
+  /// target.env-vars`) will be used.
   ///
   /// \param[in] working_directory
   /// The working directory to have the child process run in
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits