[Lldb-commits] [PATCH] D52376: [API][Swig] Overloaded functions for SBTarget

[Jim Ingham via Phabricator via lldb-commits]

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

This patch changes the SB API.  We don't break binary compatibility with the SB 
API's, and especially not for something like 
GetInstructions/GetInstructionsWithFlavor, which are functions somebody out 
there is sure to be using.  See:

https://lldb.llvm.org/SB-api-coding-rules.html

I didn't know you could list multiple signatures in one type map, that part is 
cool!

But we don't allow removing SB API's, so the part where you remove a function 
and replace it with a defaulted argument one or straight up remove an API can't 
go in as is.  If you want to add a cleaner function, that's fine.

BTW, Sean isn't working on lldb anymore, so assigning him as a reviewer isn't 
going to do much good :-(


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52376



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

[Lldb-commits] [PATCH] D52376: [Swig] Merge typemaps with same bodies

[Jim Ingham via Phabricator via lldb-commits]

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

Nice, thanks!


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52376



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

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I agree with Zachary, converting to NativeProcess is enough of a project that 
we should not block useful fixes to the current ProcessWindows plugin on it.  
OTOH, it is a good project - somebody should add it to the Projects page.


https://reviews.llvm.org/D52618



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

[Lldb-commits] [PATCH] D52651: Add functionality to export settings

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

To Raphael's points:

Argument completion handlers are set by the command object implementing 
HandleArgumentCompletion and dispatching to the 
CommandCompletions::eDiskFileCompletion.  An example of this is in 
CommandObjectProcessLaunch.  Note, arguments currently have a kind (as do 
options) but for historical reasons, those kinds don't automatically bind to 
completers.  Be nice to finish that bit of the design at some point...  But 
till then, this way though boilerplate is straightforward.

The StreamFile constructor doesn't handle ~.  Maybe it should?  Or you could 
make a FileSpec, they do handle ~ completion, and there's a StreamFile 
constructor that takes a FileSpec.

Breakpoints use "breakpoint write" and "breakpoint read" to save themselves.  
Maybe it would be clearer if we use the same word for saving breakpoints and 
saving settings?

I also wonder if it would be good to add a "settings {import, read?}" command 
to go along with the settings {export,write}.  It is a little odd to do 
"settings export" to export the settings, and "command source" to read them 
back in.  That's relying on the accident that you are exporting the settings as 
commands, which (a) would be better not to tie ourselves to and (b) users 
shouldn't have to know that...

It also might be nice to have the output file be a -f option and then I could 
do:

(lldb) settings export -f foo.cmds

to export all settings and:

(lldb) settings export -f foo.cmds target

to export all the target commands, etc...  That might be more flexible for 
ordinary use.  If you do that, the easiest way to specify a completer is by 
using OptionGroupFile.




Comment at: source/Commands/CommandObjectSettings.cpp:213
+}
+
 // Split the raw command into var_name and value pair.

"settings clear" also clears the settings value.  I'm not sure I'm in favor of 
having two ways of doing this, especially when the second one is undocumented.  
Presumably this isn't intrinsic to exporting settings, so it should be done as 
a separate patch anyway.



Comment at: source/Commands/CommandObjectSettings.cpp:331
+var_name_arg.arg_repetition = eArgRepeatOptional;
+
+// There is only one variant this argument could be; put it into the

Shouldn't this be eArgRepeatPlain?  You don't do anything in the case where 
there's no filename argument.

BTW, I don't think these argument repeat counts are actually enforced.  They 
are used to write out the help strings, but not to check incoming arguments for 
commands so far as I can see.  The development of these and the argument kind 
specifications stalled when the person who originally worked on them left, and 
is waiting for somebody to pick them back up again.



Comment at: source/Commands/CommandObjectSettings.cpp:356
+// Open file for dumping the exported settings.
+const std::string export_path = args.GetArgumentAtIndex(0);
+const uint32_t options = File::eOpenOptionWrite |

You don't know that you actually got an argument.  You should check that here.


https://reviews.llvm.org/D52651



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The only thing that worries me about this fix is that it's addressing a problem 
that's a pretty easy mistake to make.  Is there ever a case that you would want 
to resolve a DWARF expression in the context of a .o file module?  If not, is 
there some way we can make this happen automatically.  This fix seems a bit ad 
hoc.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52772: [Settings] Make "settings set" without a value equivalent to "settings clear"

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Would it be possible for the exporter to notice empty settings and write 
"settings clear" instead?  I'm worried that if you have a complicated setting, 
and the you do:

(lldb) settings set target.some-complex-setting

and decide you are wrong, you don't want to change the complex setting, then 
you have to know to delete the text - hitting a return is actually destructive.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52772



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

[Lldb-commits] [PATCH] D52678: DWARFExpression: Resolve file addresses in the linked module

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I was thinking more like if it's possible to figure out that a given module has 
a debug_map parent, "SetModule" could automatically redirect to the that 
module.  Then all clients have to do is SetModule with the module they have to 
hand and it would get the right thing.  That's why I asked if there was ever a 
case where we'd want to run a DWARF expression using the .o file module when 
there's a parent debug_map.

But maybe that's overthinking it.


https://reviews.llvm.org/D52678



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

[Lldb-commits] [PATCH] D52788: Add EchoCommentCommands to CommandInterpreterRunOptions in addition to the existing EchoCommands and expose both as interpreter settings.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The StopOnCrash logic is slightly different.  Because the outer if is 
"GetStopOnCrash()" you will only set stop on crash if it is set at this level 
and was set in the previously pushed flag set.  That forces StopOnCrash to be 
set from the top all the way down.

For StopOnError, the outermost if checks if the option is set, so you will 
inherit the previous flag set's behavior if the option is unset at this level.

StopOnCrash is used for the --batch mode of the lldb driver, so it has to 
propagate to any newly sourced sets of commands for it to be useful.  It does 
need this different kind of setting.


https://reviews.llvm.org/D52788



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

[Lldb-commits] [PATCH] D52772: [Settings] Make "settings set" without a value equivalent to "settings clear"

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I'm thinking of the scenario where you type:

(lldb) settings set target.run-args

and then decide you didn't want to change the run-args after all.  Before this 
change hitting return used to be a way to dismiss the settings operation, and 
that actually seems a pretty natural thing to do - especially given that there 
is a "clear" operation.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52772



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

[Lldb-commits] [PATCH] D52772: [Settings] Make "settings set" without a value equivalent to "settings clear"

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

And more, I just like operations to be explicit and not have overloads like 
"settings set property" == "settings clear property".


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52772



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

[Lldb-commits] [PATCH] D52851: Adding support to step into the callable wrapped by libc++ std::function

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Just a couple of trivial requests, mostly about comments...




Comment at: 
packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:27-28
+
+@add_test_categories(["libc++"])
+def test(self):
+"""Test that std::function as defined by libc++ is correctly printed 
by LLDB"""

davide wrote:
> Is this affected by the debug info variant used? (i.e. 
> dSYM/gmodules/DWARF/dwo). 
> If not, this could be a `NO_DEBUG_INFO` test
I don't think this test will be sensitive to those differences, so this 
probably can be a NO_DEBUG_INFO test.



Comment at: 
packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:45-61
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+break_in_main = target.BreakpointCreateBySourceRegex('// Set break 
point at this line.', self.main_source_spec)
+self.assertTrue(break_in_main, VALID_BREAKPOINT)
+
+self.process = target.LaunchSimple(

Most of this setup (excepting the line_number calls) can be done in one line 
using lldbutils.run_to_source_breakpoint.



Comment at: 
packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:64
+self.thread.StepInto()
+self.assertEqual( 
self.thread.GetFrameAtIndex(0).GetLineEntry().GetLine(), self.source_foo_line ) 
;
+self.process.Continue()

Can you also check the source file?  The way libcxx does inlining nowadays, all 
these std::function calls introduce lots of inlining so the chance that you got 
to the right line but in an inlined function is not zero...



Comment at: source/Target/CPPLanguageRuntime.cpp:295-301
+  // Handling the case where we are attempting to step into std::function.
+  // Currently due to the the:
+  //
+  //target.process.thread.step-avoid-regexp
+  //
+  // setting we will currenly step right out of standard library code.
+  //

I'm not sure this comment is helpful.  You want to do the trampoline detection 
regardless of the value of the regexp.  And we really aren't violating that 
contract, since we don't intent to STOP on step into std::function...



Comment at: source/Target/CPPLanguageRuntime.cpp:329
+  return ret_plan_sp;
+} else {
+  ret_plan_sp.reset(new ThreadPlanStepInRange(thread, range_of_curr_func,

You need to explain what this step in is about.  Otherwise this is pretty 
mysterious.


https://reviews.llvm.org/D52851



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

[Lldb-commits] [PATCH] D53010: Add an alias "var" for "frame var" and "vo" for "frame var -O"

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
Herald added subscribers: lldb-commits, abidh.

"expression" is a hugely inefficient way to get the value of a local variable.  
There are a few cases where "frame variable" and "expression" will produce 
different results on the same expression (e.g. "foo->bar" when foo has an -> 
operator overload).  But in general, if you are printing a local var, "frame 
var" is way more efficient.  I've been trying to teach people to use 'frame 
var' more when it is appropriate, and it was suggested that adding a top-level 
alias for it might make the command easier to find and remember.

This patch adds two aliases:

var - frame var
vo  - frame var -O

The latter is for our ObjC friends who often see the object description, rather 
than the object's ivars, as the primary source of information about the object.

I also added some caveats to the frame var help page.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53010

Files:
  source/Commands/CommandObjectFrame.cpp
  source/Interpreter/CommandInterpreter.cpp


Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -386,6 +386,13 @@
   if (cmd_obj_sp) {
 AddAlias("rbreak", cmd_obj_sp, "--func-regex %1");
   }
+
+  cmd_obj_sp = GetCommandSPExact("frame variable", false);
+  if (cmd_obj_sp) {
+AddAlias("var", cmd_obj_sp);
+AddAlias("vo", cmd_obj_sp, "--object-description");
+  }
+  
 }
 
 void CommandInterpreter::Clear() {
Index: source/Commands/CommandObjectFrame.cpp
===
--- source/Commands/CommandObjectFrame.cpp
+++ source/Commands/CommandObjectFrame.cpp
@@ -427,7 +427,17 @@
 "arguments and local variables in scope. Names of argument, "
 "local, file static and file global variables can be specified. "
 "Children of aggregate variables can be specified such as "
-"'var->child.x'.",
+"'var->child.x'.  The -> and [] operators in 'frame variable' do "
+"not invoke operator overloads if they exist, but directly access "
+"the specified element.  If you want to trigger operator overloads 
"
+"use the expression command to print the variable instead."
+"\nIt is worth noting that except for overloaded "
+"operators, when printing local variables 'expr local_var' and "
+"'frame var local_var' produce the same "
+"results.  However, 'frame variable' is more efficient, since it "
+"uses debug information and memory reads directly, rather than "
+"parsing and evaluating an expression, which may even involve "
+"JITing and running code in the target program.",
 nullptr, eCommandRequiresFrame | eCommandTryTargetAPILock |
  eCommandProcessMustBeLaunched |
  eCommandProcessMustBePaused | 
eCommandRequiresProcess),


Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -386,6 +386,13 @@
   if (cmd_obj_sp) {
 AddAlias("rbreak", cmd_obj_sp, "--func-regex %1");
   }
+
+  cmd_obj_sp = GetCommandSPExact("frame variable", false);
+  if (cmd_obj_sp) {
+AddAlias("var", cmd_obj_sp);
+AddAlias("vo", cmd_obj_sp, "--object-description");
+  }
+  
 }
 
 void CommandInterpreter::Clear() {
Index: source/Commands/CommandObjectFrame.cpp
===
--- source/Commands/CommandObjectFrame.cpp
+++ source/Commands/CommandObjectFrame.cpp
@@ -427,7 +427,17 @@
 "arguments and local variables in scope. Names of argument, "
 "local, file static and file global variables can be specified. "
 "Children of aggregate variables can be specified such as "
-"'var->child.x'.",
+"'var->child.x'.  The -> and [] operators in 'frame variable' do "
+"not invoke operator overloads if they exist, but directly access "
+"the specified element.  If you want to trigger operator overloads "
+"use the expression command to print the variable instead."
+"\nIt is worth noting that except for overloaded "
+"operators, when printing local variables 'expr local_var' and "
+"'frame var local_var' produce the same "
+"results.  However, 'frame variable' is more efficient, since it "
+"uses debug information and memory reads directly, rather than "
+"parsing and evaluating an expression, which may even involve "
+"JITing and running code in the target program.",
 nullptr, eCommandRequiresFrame | eCommandTryTargetAPILock |
   

[Lldb-commits] [PATCH] D53010: Add an alias "var" for "frame var" and "vo" for "frame var -O"

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344102: Add "var" and "vo" aliases for 
"frame variable" and "frame variable -O". (authored by 
jingham, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53010?vs=168742&id=168927#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53010

Files:
  lldb/trunk/source/Commands/CommandObjectFrame.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp


Index: lldb/trunk/source/Commands/CommandObjectFrame.cpp
===
--- lldb/trunk/source/Commands/CommandObjectFrame.cpp
+++ lldb/trunk/source/Commands/CommandObjectFrame.cpp
@@ -427,7 +427,17 @@
 "arguments and local variables in scope. Names of argument, "
 "local, file static and file global variables can be specified. "
 "Children of aggregate variables can be specified such as "
-"'var->child.x'.",
+"'var->child.x'.  The -> and [] operators in 'frame variable' do "
+"not invoke operator overloads if they exist, but directly access "
+"the specified element.  If you want to trigger operator overloads 
"
+"use the expression command to print the variable instead."
+"\nIt is worth noting that except for overloaded "
+"operators, when printing local variables 'expr local_var' and "
+"'frame var local_var' produce the same "
+"results.  However, 'frame variable' is more efficient, since it "
+"uses debug information and memory reads directly, rather than "
+"parsing and evaluating an expression, which may even involve "
+"JITing and running code in the target program.",
 nullptr, eCommandRequiresFrame | eCommandTryTargetAPILock |
  eCommandProcessMustBeLaunched |
  eCommandProcessMustBePaused | 
eCommandRequiresProcess),
Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -423,6 +423,13 @@
   if (cmd_obj_sp) {
 AddAlias("rbreak", cmd_obj_sp, "--func-regex %1");
   }
+
+  cmd_obj_sp = GetCommandSPExact("frame variable", false);
+  if (cmd_obj_sp) {
+AddAlias("var", cmd_obj_sp);
+AddAlias("vo", cmd_obj_sp, "--object-description");
+  }
+  
 }
 
 void CommandInterpreter::Clear() {


Index: lldb/trunk/source/Commands/CommandObjectFrame.cpp
===
--- lldb/trunk/source/Commands/CommandObjectFrame.cpp
+++ lldb/trunk/source/Commands/CommandObjectFrame.cpp
@@ -427,7 +427,17 @@
 "arguments and local variables in scope. Names of argument, "
 "local, file static and file global variables can be specified. "
 "Children of aggregate variables can be specified such as "
-"'var->child.x'.",
+"'var->child.x'.  The -> and [] operators in 'frame variable' do "
+"not invoke operator overloads if they exist, but directly access "
+"the specified element.  If you want to trigger operator overloads "
+"use the expression command to print the variable instead."
+"\nIt is worth noting that except for overloaded "
+"operators, when printing local variables 'expr local_var' and "
+"'frame var local_var' produce the same "
+"results.  However, 'frame variable' is more efficient, since it "
+"uses debug information and memory reads directly, rather than "
+"parsing and evaluating an expression, which may even involve "
+"JITing and running code in the target program.",
 nullptr, eCommandRequiresFrame | eCommandTryTargetAPILock |
  eCommandProcessMustBeLaunched |
  eCommandProcessMustBePaused | eCommandRequiresProcess),
Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -423,6 +423,13 @@
   if (cmd_obj_sp) {
 AddAlias("rbreak", cmd_obj_sp, "--func-regex %1");
   }
+
+  cmd_obj_sp = GetCommandSPExact("frame variable", false);
+  if (cmd_obj_sp) {
+AddAlias("var", cmd_obj_sp);
+AddAlias("vo", cmd_obj_sp, "--object-description");
+  }
+  
 }
 
 void CommandInterpreter::Clear() {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51934: [target] Change target create's behavior wrt loading dependent files.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

IIRC, the test is disabled on Linux not because of problems with building the 
test executables, but because the Linux port doesn't currently load the 
dependencies of an binary when it loads the binary.  So the test was irrelevant 
on Linux, as was the feature.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51934



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

[Lldb-commits] [PATCH] D52851: Adding support to step into the callable wrapped by libc++ std::function

[Jim Ingham via Phabricator via lldb-commits]

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

Couple more lines you can delete from the test case, and I think you should 
make this a debug-variant insensitive test.  Do that and this is good.




Comment at: 
packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:29-30
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), 
CURRENT_EXECUTABLE_SET)
+
+exe = self.getBuildArtifact("a.out")

You don't need these two lines anymore.  You never use the exe variable, and 
the target you make with this "file" command is never used (you correctly use 
the one returned by run_to_source_breakpoint.)

Also, this doesn't seem like a debug variant sensitive test, so you really 
should make this a NO_DEBUG_INFO test.  We're trying to do that wherever it 
makes sense just to keep down the number of tests we run.


https://reviews.llvm.org/D52851



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

[Lldb-commits] [PATCH] D52851: Adding support to step into the callable wrapped by libc++ std::function

[Jim Ingham via Phabricator via lldb-commits]

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

This looks great, thanks for making this work, it will be SO helpful for 
debugging std::function uses.


https://reviews.llvm.org/D52851



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

[Lldb-commits] [PATCH] D53208: [lldbsuite] Fix the mac version decorator to work on non-mac platforms

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

We do compose the decorators in a bunch of places (like Shafik's usage here).  
That will work more naturally if the categories that the decorators assert are 
as decoupled as possible.  So the statement about macos version should only 
skip the test if the os is macos, and make no comment about other os's.  That 
decouples the macos_version test from the oslist test, and allows something 
like the construct in the tests that Shafik wrote, which seems a pretty natural 
way to express "if on macOS, it has to be version > x, otherwise it has to be 
clang > y".


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53208



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

[Lldb-commits] [PATCH] D51934: [target] Change target create's behavior wrt loading dependent files.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I wouldn't be surprised if Jonas isn't familiar enough with the Windows port to 
answer the question.  But if you have a running lldb you can tell easily.  Just 
do:

lldb SomeBinaryThatLoadsSharedLibraries
(lldb) image list

Do you get only SomeBinaryThatLoadsSharedLibraries, or do you get a list of the 
dependencies of the binary as well?  If the former, then this test won't test 
anything for you.  If the latter, then this test will test something real, and 
we should figure out how to get the test to build on Windows.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51934



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

[Lldb-commits] [PATCH] D53208: [lldbsuite] Fix the mac version decorator to work on non-mac platforms

[Jim Ingham via Phabricator via lldb-commits]

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

That seems more convenient to me, it makes the mac version only relevant if you 
are on a mac.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53208



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

[Lldb-commits] [PATCH] D53309: Return a useful "Error" for an expression that completes but produces no result

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
Herald added subscribers: lldb-commits, abidh, JDevlieghere.

When you run an expression like:

(lldb) expr int $x = 10

the expression has no result.  The ValueObject (and then SBValue) you get back 
from the expression signals that by putting an error indicating this in the 
Status object returned by SBValue::GetError().  Unfortunately, this error was 
not terribly helpful.  The error number was a private constant that only 
UserExpression knew about, and the error string was unset.  This meant you 
couldn't really trust the result of SBValue.GetError().Success() when you ran 
an expression.

This commit adds an eExpressionProducedNoResult constant to the 
ExpressionResults enum, and used that and an appropriate string in the error 
object.  So now you can usefully tell the difference between an expression that 
produces no result and one that failed.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53309

Files:
  include/lldb/Expression/UserExpression.h
  include/lldb/lldb-enumerations.h
  packages/Python/lldbsuite/test/expression_command/no-result/Makefile
  packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
  packages/Python/lldbsuite/test/expression_command/no-result/main.c
  source/Commands/CommandObjectExpression.cpp
  source/Expression/REPL.cpp
  source/Expression/UserExpression.cpp
  
source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

Index: packages/Python/lldbsuite/test/expression_command/no-result/main.c
===
--- packages/Python/lldbsuite/test/expression_command/no-result/main.c
+++ packages/Python/lldbsuite/test/expression_command/no-result/main.c
@@ -0,0 +1,9 @@
+#include 
+
+int
+main()
+{
+  int test_var = 10;
+  printf ("Set a breakpoint here: %d.\n", test_var);
+  return 0;
+}
Index: packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
===
--- packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
+++ packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
@@ -0,0 +1,45 @@
+"""
+Test that an expression that returns no result returns a sensible error.
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestExprNoResult(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_no_result(self):
+"""Run an expression that has no result, check the error."""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.sample_test()
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def sample_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+result = frame.EvaluateExpression("int $x = 10")
+# No result expressions are considered to fail:
+self.assertTrue(result.GetError().Fail(), "An expression with no result is a failure.")
+# But the reason should be eExpressionProducedNoResult
+self.assertEqual(result.GetError().GetError(), lldb.eExpressionProducedNoResult, 
+ "But the right kind of failure")
Index: packages/Python/lldbsuite/test/expression_command/no-result/Makefile
===
--- packages/Python/lldbsuite/test/expression_command/no-result/Makefile
+++ packages/Python/lldbsuite/test/expression_command/no-result/Makefile
@@ -0,0 +1,6 @@
+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS += -std=c99
+
+include $(LEVEL)/Makefile.rules
Index: source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
===
--- source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
+++ source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
@@ -1758,7 +1758,7 @@
   if (!expr_result->GetError().Success()) {
 Status err = expr_result->GetError();
 // Expression returned is void, so this is actually a success
-if (err.GetError() == UserExpression::kNoResult) {
+if (err.GetError() == lldb::eExpressionProducedNoResult) {
   if (log)
 log->Printf("%s - expression returned void.", __FUNCTION__);
 
Index: source/Expression/UserExpression.cpp
===
--- source/Expression/UserExpr

[Lldb-commits] [PATCH] D53309: Return a useful "Error" for an expression that completes but produces no result

[Jim Ingham via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB344647: Return a named error in the result object of an 
expression with no result (authored by jingham, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53309?vs=169778&id=169899#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53309

Files:
  include/lldb/Expression/UserExpression.h
  include/lldb/lldb-enumerations.h
  packages/Python/lldbsuite/test/expression_command/no-result/Makefile
  packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
  packages/Python/lldbsuite/test/expression_command/no-result/main.c
  source/Commands/CommandObjectExpression.cpp
  source/Expression/ExpressionSourceCode.cpp
  source/Expression/REPL.cpp
  source/Expression/UserExpression.cpp
  
source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

Index: include/lldb/Expression/UserExpression.h
===
--- include/lldb/Expression/UserExpression.h
+++ include/lldb/Expression/UserExpression.h
@@ -288,10 +288,6 @@
uint32_t line_offset = 0, std::string *fixed_expression = nullptr,
lldb::ModuleSP *jit_module_sp_ptr = nullptr);
 
-  static const Status::ValueType kNoResult =
-  0x1001; ///< ValueObject::GetError() returns this if there is no result
-  /// from the expression.
-
   const char *GetFixedText() {
 if (m_fixed_text.empty())
   return nullptr;
Index: include/lldb/lldb-enumerations.h
===
--- include/lldb/lldb-enumerations.h
+++ include/lldb/lldb-enumerations.h
@@ -254,7 +254,8 @@
   eExpressionHitBreakpoint,
   eExpressionTimedOut,
   eExpressionResultUnavailable,
-  eExpressionStoppedForDebug
+  eExpressionStoppedForDebug,
+  eExpressionProducedNoResult
 };
 
 enum SearchDepth {
Index: packages/Python/lldbsuite/test/expression_command/no-result/main.c
===
--- packages/Python/lldbsuite/test/expression_command/no-result/main.c
+++ packages/Python/lldbsuite/test/expression_command/no-result/main.c
@@ -0,0 +1,9 @@
+#include 
+
+int
+main()
+{
+  int test_var = 10;
+  printf ("Set a breakpoint here: %d.\n", test_var);
+  return 0;
+}
Index: packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
===
--- packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
+++ packages/Python/lldbsuite/test/expression_command/no-result/TestNoResult.py
@@ -0,0 +1,45 @@
+"""
+Test that an expression that returns no result returns a sensible error.
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestExprNoResult(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_no_result(self):
+"""Run an expression that has no result, check the error."""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.sample_test()
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def sample_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+result = frame.EvaluateExpression("int $x = 10")
+# No result expressions are considered to fail:
+self.assertTrue(result.GetError().Fail(), "An expression with no result is a failure.")
+# But the reason should be eExpressionProducedNoResult
+self.assertEqual(result.GetError().GetError(), lldb.eExpressionProducedNoResult, 
+ "But the right kind of failure")
Index: packages/Python/lldbsuite/test/expression_command/no-result/Makefile
===
--- packages/Python/lldbsuite/test/expression_command/no-result/Makefile
+++ packages/Python/lldbsuite/test/expression_command/no-result/Makefile
@@ -0,0 +1,6 @@
+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS += -std=c99
+
+include $(LEVEL)/Makefile.rules
Index: source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
===
--- source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
+++ source/Plugins/LanguageRuntime/RenderScript/RenderScriptRun

[Lldb-commits] [PATCH] D53361: [API] Extend the `SBThreadPlan` interface

[Jim Ingham via Phabricator via lldb-commits]

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

And tick the checkbox...


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53361



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

[Lldb-commits] [PATCH] D53361: [API] Extend the `SBThreadPlan` interface

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Presumably you are doing this so one scripted thread plan can use another one?  
This is the right way to do that, so this is great.  Not sure why I didn't 
think of it.

BTW, looks like I added this without any tests.  Totally my bad, this was a 
weekend experiment and was inert if not used, so I got the bits in and then 
wrote a bug on myself to write tests - demonstrating again why that's 
inadvisable...

If you are playing around with this and have a moment to formalize what you are 
doing into a test or two, I'd be grateful...


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53361



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a reviewer: clayborg.
jingham added a comment.

This seems like the sort of thing Greg should have a look at as well.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D52953: [lldb-mi] Implement -gdb-set breakpoint pending on/off

[Jim Ingham via Phabricator via lldb-commits]

jingham added a reviewer: apolyakov.
jingham added a comment.

This looks okay to me, there's a trivial comment nit...  But Alexander has been 
doing the most work on MI recently, he might want to give this a look-over.


Repository:
  rL LLVM

https://reviews.llvm.org/D52953



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

[Lldb-commits] [PATCH] D53361: [API] Extend the `SBThreadPlan` interface

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This should be easy to test with the python testsuite (lldbtest.)  Start with 
the sample_test in packages/Python/lldbsuite/test - don't use the inline one, 
that won't be flexible enough.  Then you can just make a scripted thread plan 
that just pushes a "step over" or "step in" plan, and step with it and make 
sure it landed where it should.  Then to test your addition, make another 
scripted plan that calls your trivial stepping one, and make sure that stops 
where expected as well.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53361



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

[Lldb-commits] [PATCH] D53530: Fix (and improve) the support for C99 variable length array types

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This seems good to me, but Greg is more expert than I so I'd wait on his okay.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1901-1907
+dwarf->GetTypeList()->Insert(type_sp);
+// Cache the type if it isn't context-specific.
+auto &cache = dwarf->GetDIEToType();
+if (exe_ctx.GetFrameSP())
+  cache.erase(die.GetDIE());
+else
+  cache[die.GetDIE()] = type_sp.get();

You have to do this twice, maybe there should be a method to add the results to 
the DIE to Type map?


https://reviews.llvm.org/D53530



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

It would be great not to have to use comments to express what these values 
mean.  OTOH, having to put in static casts whenever you want to or values 
together would be a pain, so if there's no way to do it without magic, I'm a 
little less enthused...

But on the magic: It looks like BitMaskEnum.h pulls in MathTypes.h which pulls 
in Compiler.h which then pulls in llvm-config.h.  You are supposed to be able 
to build tools on top of lldb with just the headers that go in LLDB.framework, 
you are not required to have the full source tree for an lldb build.  I don't 
want to impose that restriction without very good reason, and fixing this wart 
doesn't rise to that level.  Anyway, so if we want to include BitMaskEnum.h we 
would be required to ship a bunch of llvm headers (including some build 
produced ones IIUC).  I don't think that's a very good idea.

It also looks to me like the value of the largest item + 1 gets baked into the 
operator overloading code.  What would happen if we decided to add an element 
to the enum?


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

That would be great!


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

That looks good to me, though you should wait for Greg to weigh in.  He might 
notice something I missed.


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D44603: [lldb] Introduce StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This is good.  The addition of the "info" command will be helpful for people 
trying to debug their recognizers.  It's okay to add multiple -s and -n's later 
- though the fact that you don't allow "apply to all frames" may make us want 
the ability to provide more than one option sooner rather than later...

Sorry for this thought coming late, but I worry a little bit about the fact 
that the class name is the recognizers identity, particularly for deleting.  I 
might have a good recognizer class that I want to add to one library, then 
later in the session want to apply it to another library as well.  The second 
addition will mean that the name now exists twice but with different shared 
libraries, and then since I only provide the name to "delete" I can't tell 
which one I've deleted.

It also makes the API's a little odd, since the name of the recognizer is 
important to the commands for handling it, but it isn't clear from the API's 
that I have to provide a unique name for them.

I think it would be better to have the RecognizerManager keep an ID for each 
recognizer, and have list print and delete take the index.

Also, it might be convenient to have "frame recognizer delete" with no 
arguments query "Do you want to delete all recognizers" and then delete them.  
That's the way "break delete" works so it's an accepted pattern in lldb.  This 
could be done as a follow-on, however.




Comment at: 
packages/Python/lldbsuite/test/functionalities/frame-recognizer/TestFrameRecognizer.py:54
+
+self.runCmd("b foo")
+self.runCmd("r")

Would you mind using:

lldbutil.run_break_set_by_symbol(self, "foo") 

instead of this runCmd.  The lldbutil routine will check that the breakpoint 
got set and error out here if it didn't.  That will reduce the head-scratching 
if for some reason we fail to set the breakpoint and then the test falls over 
downstream.




Comment at: 
packages/Python/lldbsuite/test/functionalities/frame-recognizer/TestFrameRecognizer.py:91
+"""
+self.runCmd("b bar")
+self.runCmd("c")

Also here.


https://reviews.llvm.org/D44603



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

[Lldb-commits] [PATCH] D53361: [API] Extend the `SBThreadPlan` interface

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

That looks right.  Thanks for adding the test!


https://reviews.llvm.org/D53361



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

[Lldb-commits] [PATCH] D52772: [Settings] Make "settings set" without a value equivalent to "settings clear"

[Jim Ingham via Phabricator via lldb-commits]

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

That looks fine.


https://reviews.llvm.org/D52772



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

[Lldb-commits] [PATCH] D53656: Adding formatters for libc++ std::u16string and std::u32string

[Jim Ingham via Phabricator via lldb-commits]

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

This looks reasonable to me.


https://reviews.llvm.org/D53656



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I worry that your patch changes the behavior when you add the type_class 
explicitly in the lookup (i.e. look up "struct Struct" not "Struct".  That 
should work...

Note, this doesn't currently work in type lookup:

  (lldb) type lookup "struct Foo"
  no type was found matching '"struct Foo"'

which I'll have to fix (grr...), but does work correctly in the SB API's:

  (lldb) script
  Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
  >>> module = lldb.target.modules[0]
  >>> print module
  (x86_64) /tmp/structs
  >>> result_list = module.FindTypes("struct Foo")
  >>> print result_list.GetSize()
  1
  >>> print result_list.GetTypeAtIndex(0)
  struct Foo {
  int First;
  }

Other than that this looks correct to me.




Comment at: lldb/source/Core/Module.cpp:1012
+TypeClass type_class = eTypeClassAny;
+Type::GetTypeScopeAndBasename(type_name, type_scope, type_basename,
+  type_class);

GetTypeScopeAndBasename's behavior is not well documented.  It has a bool 
return which should mean some kind of failure.  The code you are replacing 
checks for type_basename.empty() and the type_class not being set, and falls 
back on the input name if it is.  You unconditionally use type_basename.  

The old code's test doesn't actually accord with the current behavior of 
GetTypeScopeAndBasename, which will only leave basename empty if the input name 
was empty, so far as I can see.  So I don't think your version is wrong, but it 
is right only by accident.  If we are going to rely on this behavior then it's 
probably a good idea to document it in the definition of 
GetTypeScopeAndBasename.  Or go back to checking whether type_basename is 
empty...

Also, by not calling GetTypeScopeAndBasename before you call FindTypes_Impl, 
your version of the code would pass it "struct Foo" if that's what the user 
typed, whereas the old code would pass "Foo" (the struct would get stripped by 
GetTypeScopeAndBasename).  I'm not sure whether that matters, did you try 'type 
lookup "struct Struct"' or something like that?  It doesn't look like you do 
that in your test cases.

Also your "exact" would call "struct ::Foo" not exact, whereas the old code 
would call that exact.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53677: Fix a bug PlatformDarwin::SDKSupportsModule

[Jim Ingham via Phabricator via lldb-commits]

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

Looks good to me.


https://reviews.llvm.org/D53677



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

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

This looks good to me.

Looking at the addition of Type::ConsumeTypeClass makes it really clear that 
both this function and Type::GetTypeScopeAndBasename need to dispatch to the 
CompilerSystem to do this work.  The actual code is way too C-family-ish to be 
in the generic Type.cpp.  That will require passing in a language because I 
don't think you know enough from just the symfile and name to know which 
language the user was looking up types for.

But this change doesn't make it harder to get that right, so fixing it doesn't 
need to be part of this patch.

So far as I can tell, you can't do an O(1) lookup of an exact name in DWARF 
from the dwarf_names table (or the proceeding apple tables).  The tables are 
organized by base name (or really whatever in the DW_AT_name attribute of the 
die).  So you will always have to pull the base name out, find all the base 
name matches and then run up the parent dies to build up the fully qualified 
name.  Each name entry has the parent DIE, so building up the full name is 
pretty efficient.  But still, the best you can do 
O(number_of_occurances_of_name) and knowing the name is exact doesn't help the 
search speed.  If I'm right about that (Greg will surely know) then we should 
remove the FIXME comment in SymbolFileDWARF since it refers to an unfixable fix.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Jim Ingham via Phabricator via lldb-commits]

jingham added subscribers: clayborg, zturner.
jingham added a comment.

So far as I can tell, this patch will make lookup of exact types faster for 
PDB, but because of the way DWARF debug_names tables are constructed, I don't 
think there's any way we can do the same thing for DWARF.

But unless I'm misunderstanding the patch, this doesn't change correctness of 
the lookup (except for fixing "type lookup 'struct Foo'").  Did I miss 
something?

Jim


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Ah, right...  Too many patches (a good problem!)

The standard as I read it says that the name entry points into the general 
string table, but doesn't specify which entry it points to.  However, the 
current DWARF debug_info doesn't ever emit a string for the fully qualified 
name, so you would have to construct it specially to have an exact name to 
refer to.  I also couldn't see anything that said specifically whether the 
DW_AT_name for a type has to be the full name or a base name, it just says:

If a name has been given to the structure, union, or class in the source 
program, then the corresponding structure type, union type, or class type entry 
has a DW_AT_name attribute whose value is a null-terminated string containing 
the type name.

But it would bloat the name tables to use the full name, and since you can 
reconstruct it from the context it's not needed...  So I've only seen base 
names in the name entry for types in the debug_info.

Anyway, current clang for -gdwarf-5 produces:

  Bucket 1 [
Name 2 {
  Hash: 0xB887389
  String: 0x00c3 "Foo"
  Entry @ 0x92 {
Abbrev: 0x39
Tag: DW_TAG_namespace
DW_IDX_die_offset: 0x004c
  }
}
  ]
  Bucket 2 [
Name 3 {
  Hash: 0xB8860BA
  String: 0x00c7 "Bar"
  Entry @ 0x9b {
Abbrev: 0x13
Tag: DW_TAG_structure_type
DW_IDX_die_offset: 0x004e
  }
}
Name 4 {
  Hash: 0x7C9A7F6A
  String: 0x00b5 "main"
  Entry @ 0xa4 {
Abbrev: 0x2E
Tag: DW_TAG_subprogram
DW_IDX_die_offset: 0x0026
  }
}

For:

namespace Foo
{

  struct Bar
  {
int First;
  };

}

int
main() 
{

  Foo::Bar mine = {10};
  return mine.First;

}

Jim


https://reviews.llvm.org/D53597



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

So the current approach relies on the ability to sniff the name to determine 
the context in which the user intends to find it.  It does (and always did) use 
the presence of an initial "::" to tell whether a symbol is exact.  That's 
obviously also inappropriate for a generic Type method.  But OTOH, there needs 
to be a funnel point where you take in a string you know nothing about (from 
the user either in type lookup or in an expression) and find it as best you 
can.  I don't think we want to force command line users to say "type lookup 
--exact " so we've got to figure it out internally.

Internally, it might be helpful to do some initial analysis of the input type 
string, and then dispatch it to an exact or inexact search.  But given I don't 
think you can get away without a FindTypes that takes a string that you don't 
know whether it is exact or not, I don't feel strongly about that.

We should abstract "is exact" and ask the various type systems to handle that 
request, and we also need to abstract "remove type class" and again ask the 
various type systems to handle that.  That seems to me the main ugliness that 
this patch reveals.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Is there anything PDB specific about the test you've added?  If not, it might 
be good to use this as a generic SymbolFile test.


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

It seemed to me like Greg thought you were changing the behavior of lookups, 
which this patch doesn't do, it just makes it more efficient.  I don't know if 
that alters his objections or not.

The Module and higher layer of FindTypes calls are awkward.  For instance 
Module::FindTypes takes an "exact_match" parameter, but it doesn't actually 
mean "exact match".  If exact_match is passed in as true, then it does mean 
exactly match the name.  But if it is false it means "check if this name is 
fully qualified and if so make it an exact match otherwise do a partial match".

The header comment for the function is wrong, too.  It says:

  /// @param[in] exact_match
  /// If \b true, \a type_name is fully qualified and must match
  /// exactly. If \b false, \a type_name is a partially qualified
  /// name where the leading namespaces or classes can be
  /// omitted to make finding types that a user may type
  /// easier.

So we should change the variable name (or even just the docs) at this level to 
better reflect what the parameter actually does.  You still need to turn off 
the guessing because we don't have a 100% certain way to tell whether a type 
name is fully qualified, and the alternative of prepending a "::" to force the 
guessing algorithm's hand is gross (and very C++ specific).  But with that 
proviso, I don't think passing this as a parameter is any better/worse than 
having Module:FindTypesExact and Module::FindTypesAutoDetect or some better 
name that I can't think up right now.

But I don't think you need the third "FindTypesPartial" that forces a partial 
match is useful.  I don't see a use for "this name really looks fully qualified 
but I want you to treat it as a partial name".  So expressing this as a bool 
parameter seems fine to me.

OTOH, I don't think it is desirable to have different versions of the Symfile 
and lower calls for Exact and not Exact, passing the exact_match parameter 
through there is fine IMO.  After all, in all these calls exact_match means 
exactly what it says, you are requiring an exact match.  So you don't gain 
clarity by having different function names.

BTW, you should remove the O(1) FIXME comment so you don't send some future 
eager contributor on a wild goose chase to figure out how to do this thing you 
can't do in DWARF.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Well, what's really going on is that I'm not familiar enough with lit to know 
that it doesn't have the ability to run different commands to produce the input 
file...  But as you guessed, my point is that you have written a bunch of tests 
that would be valuable to test against any symfile format, so it is a shame to 
only run them against one format.  OTOH, if that's not possible right now, I'm 
content to wait for some enhancements to lit that make it possible.


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Could you also use Vedant's new FileCheck dotest test class?  That should allow 
you to write the tests exactly as you are, but use the dotest mechanism to 
build and run the example.


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

dotest tests don't require a process.  Presumably dotest knows how to build 
windows targeted PDB debug flavor files (to go along with dwarf/dsym/etc.).  So 
it would be straightforward to make a test that had your input sources, built 
and made a target out of it and then poked at static variables and their types. 
 That would straightaway run with all the different symbol file formats we 
support.

That was why using Vedant's FileCheck thing made sense to me.  You would use 
that to specify the test cases, since you like that way of writing tests and 
anyway already have them written out in that form, but use the dotest machinery 
to build it for whatever symfile format and target architecture/s the person 
who was running the test dialed up.  But if you are interesting in also getting 
this to work with the straight FileCheck style test, your time is your own...

BTW, I would use dotest tests specifically for the kind of thing you are doing 
here because then you can test against the SBType and SBTypeMembers from the 
debug info you've ingested, which would give you bit and byte offsets and sizes 
for free.  But if your differing tastes end up getting you to add that info to 
"type lookup" - which we really should do - then I guess we win either way...


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The first of the commands you showed presents info we definitely should add to 
type lookup.  I actually have a bug around to do that, but it hasn't risen to 
the top of my queue because it's trivial to do with the SB API's so every time 
I've needed that info I get it from script.  Selfish me...

The second command is done in lldb with "memory read -t TYPE", and you can also 
use the "-c COUNT" argument to treat the memory as an array of COUNT elements 
of the TYPE:

  (lldb) fr v myFoo
  (foo *) myFoo = 0x00010040
  (lldb) memory read -t foo 0x00010040
  (foo) 0x10040 = {
First = 0
Second = 0
  }
  (lldb) memory read -t foo 0x00010040 -c 10
  (foo) 0x10040 = {
First = 0
Second = 0
  }
  (foo) 0x10048 = {
First = 1
Second = 10
  }
  (foo) 0x100400010 = {
First = 2
Second = 20
  }

if you want to also see the types of all the subelements add the -T flag, and 
if you want to see all the memory locations of the subelements, add the -L flag:

  (lldb) memory read -t foo 0x00010040 -c 10 -T -L
  0x00010040: (foo) 0x10040 = {
  0x00010040:   (int) First = 0
  0x00010044:   (int) Second = 0
  }
  0x00010048: (foo) 0x10048 = {
  0x00010048:   (int) First = 1
  0x0001004c:   (int) Second = 10
  }

BTW, the latter two flags have the same meaning pretty much wherever we print 
values (expression, frame var...)


https://reviews.llvm.org/D53731



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

[Lldb-commits] [PATCH] D53761: [Target] Do not skip a stop on a breakpoint if a plan was completed

[Jim Ingham via Phabricator via lldb-commits]

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

This seems safe, but you certainly want to add a comment explaining why you are 
doing this.

We find the expression breakpoint by calling ObjectFile::GetEntryPointAddress 
on the main executable.  So you should be able to test this everywhere by 
making that function result available somehow and then setting a user 
breakpoint there.

If you were making a dotest test I'd suggest adding 
SBTarget::GetEntryPointAddress and then use that.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53761



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

[Lldb-commits] [PATCH] D53761: [Target] Do not skip a stop on a breakpoint if a plan was completed

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM


https://reviews.llvm.org/D53761



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Exact matches aren't a C++ specific thing.  An exact match is useful for mixed 
C/C++ since you might want to say Foo and not Bar::Foo.  IIRC a couple of the 
places where exact was dialed up explicitly in FindTypes were for C types.   
But since C and ObjC allow multiple types with the same name, that's one way 
you might see multiple matches for "exact match".  Moreover C+ used to be fuzzy 
about whether non-exported classes from different linkage uses had to follow 
the ODR.  I haven't followed whether this got nailed down but it still seems 
Quixotic to expect you could enforce this, since you are asking two totally 
unrelated library vendors to avoid each other's names for private classes...  
So having several classes with the same name is still a possibility IRL with 
C++.

Why we care is that if somebody runs the command:

(lldb) expression (::SomeClass *) 0x12345

We're going to try our best to make that work.  If SomeClass is not visible in 
the module/comp_unit/function in which we are currently stopped, then we will 
go looking far and wide for SomeClass.  If we find one result somewhere out in 
the world, the expression will succeed, but if we find two results which are 
different, then we want to give an error about ambiguous type.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

That depends on the definition of "fully qualified name".  If you can ensure 
that it means "name of C++ class" - or other ODR enforcing type system, then 
you could make that assumption.  In C you are free to redefine types on a 
per-function basis if you so desire; and sadly some interface generators 
(including MIG the one for generating Mach message handlers) do just this...)  
So you would need to see a way to restrict the inputs to this function.  That 
doesn't seem like it would be straightforward to me.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

You could also get multiple matches if you had classes in anonymous namespaces 
with the same name in multiple compile units.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Yes, that usage is exactly the sort of use I was talking about.  When we get an 
ObjC object and want to find its dynamic type, we read the type name by 
following the object's ISA pointer to the Class object.  Then we go to look up 
the type from that name.  We know we want an exact match to the name we found, 
but there's nothing that says the same module can't have an C++ class with the 
same name as an ObjC class.  So that code needs to get all the types found, and 
sort through them for the one that is an ObjC interface type, and discard all 
the others.  You will need to support that behavior somehow.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Yes.  You can't put them in the same CompileUnit because declaring variables of 
the type would be ambiguous (though all other references would be okay since 
ObjC method dispatch looks totally different from C++ method calling).  But 
there's nothing keeping the same module from having a C++ and an ObjC class of 
the same name.


https://reviews.llvm.org/D53662



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

[Lldb-commits] [PATCH] D44603: [lldb] Introduce StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM.  Thanks for working on this!


https://reviews.llvm.org/D44603



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

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

There were a bunch of other tests testing wchar_t handling, all in:

lang/cpp/wchar_t

as well as some tests in:

functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py

Those tests weren't failed (except for the latter test, and that only for 
android/gmodules.  Do you know why those tests were passing when the 
functionality was broken?  Maybe they need fixing to be more accurate?


https://reviews.llvm.org/D53989



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

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

So the deal is that we were relying on a summary formatter to print wchar_t 
before, and now you have a format option that handles them as well?  Do we need 
both? Maybe the summary also handles wchar_t * strings?

As an aside, for reasons that are not entirely clear to me most of the data 
formatter code is #ifndef LLDB_DISABLE_PYTHON.  That shouldn't be true, C++ 
based formatters (which all the built-in formatters are) should be able to work 
in the absence of Python.  Figuring out why this is true is on my list of 
things to investigate some spare afternoon...


https://reviews.llvm.org/D53989



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

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

It doesn't seem unreasonable to want to build lldb for smaller systems that 
don't have Python available, and in fact we do that internally at Apple. 
Actually, it DOES seem a little unreasonable to me because after all you can 
just run the debugserver/lldb-server and connect remotely.  But I have not to 
date been able to convince the folks who have to work on said systems that they 
don't really want an on device lldb.  Until I can win that argument - which I 
project happening only just shy of never - I'd rather not  lose the ability to 
build lldb without Python.


https://reviews.llvm.org/D53989



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

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Note also, the vast majority of the uses of LLDB_DISABLE_PYTHON are related to 
the requirement that we have Python to use any of the data formatter 
facilities.  Those uses shouldn't be necessary.  All the scripted interpreters 
should go through the generic ScriptInterpreter interface.  There's a 
ScriptInterpreterNone that should stand in for the Python interpreter in every 
use except directly managing the Python interpreter.  So there's no structural 
reason why we should need LLDB_DISABLE_PYTHON for anything but the 
initializers.  We should just be able to not build the *Python.cpp files and 
use the define only to not initialize the Python script interpreter.  Something 
got balled up in how the data formatters were implemented that this isn't true, 
IMHO.

If somebody wants to spend time looking at this, figuring how to untangle this 
would serve your purposes and also get the architecture back right at the same 
time.


https://reviews.llvm.org/D53989



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

[Lldb-commits] [PATCH] D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit")

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added a reviewer: aprantl.
Herald added a subscriber: lldb-commits.

Sometimes you want to make sure that the target doesn't run at all when running 
an expression, and you'd rather the expression fail if it would have run the 
target.  You can do this with the "expression" command by passing "--allow-jit 
0" but there's no way to do that with SBExpressionOptions.  This patch adds 
that setting.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54056

Files:
  include/lldb/API/SBExpressionOptions.h
  packages/Python/lldbsuite/test/expression_command/dont_allow_jit/Makefile
  
packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
  packages/Python/lldbsuite/test/expression_command/dont_allow_jit/main.c
  scripts/interface/SBExpressionOptions.i
  source/API/SBExpressionOptions.cpp

Index: source/API/SBExpressionOptions.cpp
===
--- source/API/SBExpressionOptions.cpp
+++ source/API/SBExpressionOptions.cpp
@@ -159,6 +159,15 @@
 : m_opaque_ap->default_execution_policy);
 }
 
+bool SBExpressionOptions::GetAllowJIT() {
+  return m_opaque_ap->GetExecutionPolicy() != eExecutionPolicyNever;
+}
+
+void SBExpressionOptions::SetAllowJIT(bool allow) {
+  m_opaque_ap->SetExecutionPolicy(allow ? m_opaque_ap->default_execution_policy
+: eExecutionPolicyNever);
+}
+
 EvaluateExpressionOptions *SBExpressionOptions::get() const {
   return m_opaque_ap.get();
 }
Index: scripts/interface/SBExpressionOptions.i
===
--- scripts/interface/SBExpressionOptions.i
+++ scripts/interface/SBExpressionOptions.i
@@ -132,6 +132,14 @@
 
 void
 SetTopLevel(bool b = true);
+  
+%feature("docstring", "Gets whether to JIT an expression if it cannot be interpreted.") GetAllowJIT;
+bool
+GetAllowJIT();
+  
+%feature("docstring", "Sets whether to JIT an expression if it cannot be interpreted.") SetAllowJIT;
+void
+SetAllowJIT(bool allow);
 
 protected:
 
Index: packages/Python/lldbsuite/test/expression_command/dont_allow_jit/main.c
===
--- packages/Python/lldbsuite/test/expression_command/dont_allow_jit/main.c
+++ packages/Python/lldbsuite/test/expression_command/dont_allow_jit/main.c
@@ -0,0 +1,15 @@
+#include 
+
+int
+call_me(int input)
+{
+  return printf("I was called: %d.\n", input);
+}
+
+int
+main()
+{
+  int test_var = 10;
+  printf ("Set a breakpoint here: %d.\n", test_var);
+  return call_me(100);
+}
Index: packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
===
--- packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
+++ packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
@@ -0,0 +1,94 @@
+"""
+Test that --allow-jit=false does disallow JITting:
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+class TestAllowJIT(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr21765")
+def test_allow_jit_expr_command(self):
+"""Test the --allow-jit command line flag"""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.expr_cmd_test()
+
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr21765")
+def test_allow_jit_options(self):
+"""Test the SetAllowJIT SBExpressionOption setting"""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.expr_options_test()
+
+
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def expr_cmd_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+
+# First make sure we can call the function with 
+interp = self.dbg.GetCommandInterpreter()
+self.expect("expr --allow-jit 1 -- call_me(10)",
+substrs = ["(int) $", "= 18"])
+# Now make sure it fails with the "can't IR interpret message" if allow-jit is false:
+self.expect("expr --allow-jit 0 -- call_me(10)",
+error=True,
+substrs = ["Can't run the expression locally"])
+
+def expr_

[Lldb-commits] [PATCH] D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit")

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

--jit wouldn't describe what the flag actually does.  Currently allow-jit and 
the SB Setting I added for it set the execution policy to 
eExecutionPolicyWhenNeeded, not eExecutionPolicyAlways.  So this really does 
just allow the JIT to be used, it doesn't force it.

If we wanted to add the ability to force use of the JIT always, we could either 
add another flag, or make an enum setting that had {never, always, when 
needed}.  But I can't see why you would want that, except maybe to work around 
IR interpreter bugs. I'd rather not add options just for working around bugs if 
I can help it.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54056



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

[Lldb-commits] [PATCH] D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit")

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346053: Add an SBExpressionOptions setting mirroring the 
"exec" command's --allow-jit. (authored by jingham, committed by 
).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54056?vs=172454&id=172465#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54056

Files:
  lldb/trunk/include/lldb/API/SBExpressionOptions.h
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/main.c
  lldb/trunk/scripts/interface/SBExpressionOptions.i
  lldb/trunk/source/API/SBExpressionOptions.cpp

Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/Makefile
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/Makefile
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/Makefile
@@ -0,0 +1,6 @@
+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS += -std=c99
+
+include $(LEVEL)/Makefile.rules
Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/dont_allow_jit/TestSampleTest.py
@@ -0,0 +1,94 @@
+"""
+Test that --allow-jit=false does disallow JITting:
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+class TestAllowJIT(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr21765")
+def test_allow_jit_expr_command(self):
+"""Test the --allow-jit command line flag"""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.expr_cmd_test()
+
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr21765")
+def test_allow_jit_options(self):
+"""Test the SetAllowJIT SBExpressionOption setting"""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.expr_options_test()
+
+
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def expr_cmd_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+
+# First make sure we can call the function with 
+interp = self.dbg.GetCommandInterpreter()
+self.expect("expr --allow-jit 1 -- call_me(10)",
+substrs = ["(int) $", "= 18"])
+# Now make sure it fails with the "can't IR interpret message" if allow-jit is false:
+self.expect("expr --allow-jit 0 -- call_me(10)",
+error=True,
+substrs = ["Can't run the expression locally"])
+
+def expr_options_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+frame = thread.GetFrameAtIndex(0)
+
+# First make sure we can call the function with the default option set. 
+options = lldb.SBExpressionOptions()
+# Check that the default is to allow JIT:
+self.assertEqual(options.GetAllowJIT(), True, "Default is true")
+
+# Now use the options:
+result = frame.EvaluateExpression("call_me(10)", options)
+self.assertTrue(result.GetError().Success(), "expression succeeded")
+self.assertEqual(result.GetValueAsSigned(), 18, "got the right value.")
+
+# Now disallow JIT and make sure it fails:
+options.SetAllowJIT(False)
+# Check that we got the right value:
+self.assertEqual(options.GetAllowJIT(), False, "Got False after setting to False")
+
+# Again use it and ensure we fail:
+result = frame.EvaluateExpression("call_me(10)", options)
+self.assertTrue(result.GetError().Fail(), "expression failed with no JIT")
+self.assertTrue("Can't run the expression locally" in result.GetError().GetCString(), "Got right error")
+
+# Finally set the allow JIT value back to tr

[Lldb-commits] [PATCH] D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit")

[Jim Ingham via Phabricator via lldb-commits]

jingham added a subscriber: aprantl.
jingham added a comment.

Thanks for doing this!  Why do we care about the file name of the test, 
shouldn't we be using the test class name for everything (that one I did 
remember to change...)

Jim


Repository:
  rL LLVM

https://reviews.llvm.org/D54056



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

[Lldb-commits] [PATCH] D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

[Jim Ingham via Phabricator via lldb-commits]

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

Looking at the extant API's we do seem to prefer "const CompilerType &" over 
"const CompilerType"  so it seems more consistent to choose that.  Other than 
that, this looks fine to me.




Comment at: include/lldb/Symbol/ClangASTContext.h:909
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-  lldb::opaque_compiler_type_t type,
-  const CompilerType &enumerator_qual_type, const Declaration &decl,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration &decl, const char *name,
+  int64_t enum_value, uint32_t enum_value_bit_size);

clayborg wrote:
> teemperor wrote:
> > Can we pass `enum_type` as const ref?
> CompilerType instances are two pointers. Pretty cheap to copy.
We're not entirely consistent, but a quick glance through headers show us 
almost always choosing to pass "const CompilerType &" over "const CompilerType".



Comment at: 
packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py:42
+## integral is not implicitly convertible to a scoped enum
+value = frame.EvaluateExpression("1 == Foo::FooBar")
+self.assertTrue(value.IsValid())

davide wrote:
> This clearly looks like can be an inline test (and probably would make the 
> thing more readable, IMHO).
The form of test to use is up to the test writer, seems to me.  This test is 
not at all hard to read.


https://reviews.llvm.org/D54003



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

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

[Jim Ingham via Phabricator via lldb-commits]

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

This is pretty good, but in all the places where some plan tries to find a 
sub-plan to do its job, you are losing the text of the error when that job 
fails.  So the controlling plan can't present a good error message.  You need 
to hold onto the Status object and return that, either as the error from 
ValidatePlan if it happens when the plan is getting created, or in the plan's 
stop description so that StopInfoThreadPlan::GetDescription can print it 
properly.




Comment at: include/lldb/Target/Thread.h:643
   //--
-  virtual lldb::ThreadPlanSP QueueFundamentalPlan(bool abort_other_plans);
+  virtual lldb::ThreadPlanSP QueueFundamentalPlan(Status &status,
+  bool abort_other_plans);

You need to change the HeaderDoc to describe the status parameter.  Looks like 
you have to do this for all the QueueThreadPlan... functions.



Comment at: include/lldb/Target/ThreadPlanBase.h:55
   friend lldb::ThreadPlanSP
-  Thread::QueueFundamentalPlan(bool abort_other_plans);
+  Thread::QueueFundamentalPlan(Status &status, bool abort_other_plans);
 

I don't think it is useful to pass an error in this case.  The "Fundamental 
plan" just fields unhandled responses from other plans, and queuing it can 
never fail.  



Comment at: include/lldb/Target/ThreadPlanRunToAddress.h:66
  // using to stop us at m_address.
+  bool m_could_not_resolve_hw_bp;
 

Looks like you are adding this to most of the plans.  Would it make sense to 
add it to the ThreadPlan base class?  This is just an error flag, so it would 
stay false except is a derived plan wants to set it.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py:33-43
+exe = self.getBuildArtifact("a.out")
+target = self.dbg.CreateTarget(exe)
+
+breakpoint = target.BreakpointCreateByLocation("main.c", 1)
+
+self.runCmd("run")
+

This can all be done with:

(target, process, stepping_thread) = 
lldbutil.run_to_line_breakpoint(SBFileSpec("main.c"), 1)




Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py:47
+
+self.expect("thread step-in")
+self.expect("thread step-in", error=True)

Can you do this using "SBThread.StepInto" and check the error return in the 
case where it fails?  Since we are treating the possibility of step's failing, 
we need to make sure that the SB API's return errors everywhere and that the 
errors look right.  So it would be good to test that.

Ditto for the other stepping tests.



Comment at: source/API/SBThread.cpp:733
 new_plan_sp = thread->QueueThreadPlanForStepSingleInstruction(
-false, abort_other_plans, stop_other_threads);
+new_plan_status, false, abort_other_plans, stop_other_threads);
   }

Shouldn't new_plan_status get reflected in the "error" parameter passed into 
SBThread::StepInto?



Comment at: source/API/SBThread.cpp:767
+  new_plan_status, abort_other_plans, NULL, false, stop_other_threads,
+  eVoteYes, eVoteNoOpinion, 0, avoid_no_debug));
 

Same here.  If new_plan_status comes back with an error, we probably don't want 
to call ResumeNewPlan, and we want to report the error from queueing the plan.



Comment at: source/API/SBThread.cpp:818
 
+  Status new_plan_status;
   ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForStepOut(

Same comment here.



Comment at: source/API/SBThread.cpp:847
   Thread *thread = exe_ctx.GetThreadPtr();
-  ThreadPlanSP new_plan_sp(
-  thread->QueueThreadPlanForStepSingleInstruction(step_over, true, true));
+  Status new_plan_status;
+  ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForStepSingleInstruction(

And here.



Comment at: source/API/SBThread.cpp:881
 
+  Status new_plan_status;
   ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForRunToAddress(

And here.



Comment at: source/API/SBThreadPlan.cpp:155
 start_address->CalculateSymbolContext(&sc);
+Status plan_status;
 return SBThreadPlan(

Can you add a variant of these calls that takes an SBError &?  The scripted 
thread plans will need to have a way to report this error when they try to 
queue a plan and it fails.  And below as well.



Comment at: source/Commands/CommandObjectThread.cpp:802
 } else {
+  result.SetError(new_plan_status);
   result.AppendError("Could

[Lldb-commits] [PATCH] D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

[Jim Ingham via Phabricator via lldb-commits]

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

Looks good to me.


https://reviews.llvm.org/D54003



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

You were probably speaking loosely, but to be clear, the code you are changing 
doesn't get used for expressions - i.e. the expression command - unless I'm 
missing something.

This little mini-parser is for doing things like:

(lldb) frame variable foo.bar

We don't use clang or the expression parser to comprehend foo.bar, we use 
GetValuesForVariableExpressionPath instead.  But 'frame variable' 'target 
variable' etc. which use this are fairly limited in what they need to support.  
They just needs to handle variable references - either local variables or 
static variables, and references to children of those variables.  No calls, no 
casts, no types, etc.

So I don't see that the ability to handle template definition markers like 
 is important.  I don't see how that would show up in a variable name or 
the name of one of its children.


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Those seem legit things to try to capture, though a little esoteric.  Since 
"frame variable" and "target variable" didn't support these constructs before 
you should certainly add some tests for that.

The frame variable parser also supports:

(lldb) frame variable foo[0]

where foo is anything that can produce "vector" like children (e.g. 
std::vector's).  Will your change work with that?


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

We do also handle [], though it isn't obvious to me after a quick glance where 
that gets done.  This is a little cheesy because the name of the child that we 
are finding with [0] is actually "[0]", so you just have to be careful not to 
consume that when you consume the variable name.

I don't remember any other special aggregate names like this.


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D54454: Be more permissive in what we consider a variable name.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

A dotest test can be xfailed if the debug format is not PDB, right?  At least 
we can xfail them for all the DWARF variants so it should be possible to do 
that for PDB as well.  So you should be able to write a test for this and then 
just xfail it till the DWARF parser can be fixed.

About the parsing rules... The name of the children of arrays or array like 
entities is, for example, "[0]".  So the virtue of the current approach is that 
we grab a "legal" identifier, and then the next bit we grab will be the name of 
the child or synthetic child - if there are any.   But we don't actually have 
to know what the name of the child is, it can be anything so long as it doesn't 
start with a legal identifier first character.

Your suggest approach will bless useable aggregate child names.  That's 
probably okay.  These child names should look like the way you would access the 
element in the languages we support - and this code is really C-ish right now, 
so just allowing the C/C++ style element accessors isn't a huge restriction.

BTW, I was just using "frame variable" as a example, target variable also 
supports []:

  (lldb) source list -l 1
 1  int g_vec[3] = {10, 20, 30};
 2  
 3  int main() {
 4return g_vec[0] + g_vec[1]; 
 5  }
  (lldb) target var g_vec[0]
  (int) g_vec[0] = 10


https://reviews.llvm.org/D54454



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

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: clayborg, jasonmolenda.
Herald added subscribers: lldb-commits, abidh, atanasyan, kbarton, javed.absar, 
nemanjai.

For reasons that are unclear to me, when the ABIXXX::CreateInstance function is 
called to make a new ABI for a given process and ArchSpec, we would only make 
the plugin once, with the initial process and architecture.  Then if we got 
called with a different process, we would check to see if we already made one 
and if we did, hand back the one we had made - even though that was for a 
different process.

If there were ever anything per-process that effected the ABI plugin's behavior 
(for instance if it relied on a Process property) you could very well use the 
wrong processes setting.  Even worse, since the ABI's hold onto a process 
through a weak pointer, if the initial process had gone away, you would not be 
able to get to any process at all, and silently fall back on some default 
behavior.

This caching goes back to prehistoric days in lldb, but I can't think of any 
reason why we would do this.  It seems clearly wrong, and ABI plugins are 
really cheap to make - they pretty much just copy the process SP to a weak 
pointer and that's about all.  So this also seems like an unnecessary 
optimization.

Greg or Jason, do you remember why we did this?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460

Files:
  source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
  source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Index: source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
===
--- source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
+++ source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
@@ -1091,11 +1091,8 @@
 
 ABISP
 ABISysV_x86_64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::x86_64) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_x86_64(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_x86_64(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
===
--- source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
+++ source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
@@ -202,11 +202,8 @@
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_s390x(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
===
--- source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
+++ source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
@@ -220,11 +220,8 @@
 
 ABISP
 ABISysV_ppc::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::ppc) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_ppc(process_sp));
-return g_abi_sp;
+ return ABISP(new ABISysV_ppc(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
===
--- source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
+++ source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips64) ||
   (arch_type == llvm::Triple::mips64el)) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_mips64(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_mips64(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
===
--- source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
+++ source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) |

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346775: Since ABI's now hold a process WP, they should 
be handed (authored by jingham, committed by ).
Herald added subscribers: llvm-commits, jrtc27.

Changed prior to commit:
  https://reviews.llvm.org/D54460?vs=173802&id=173877#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54460

Files:
  lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  lldb/trunk/source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  lldb/trunk/source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
  lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Index: lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
@@ -199,12 +199,9 @@
 
 ABISP
 ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
 if (arch.GetTriple().getArch() == llvm::Triple::x86) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_i386(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_i386(process_sp));
 }
   }
   return ABISP();
Index: lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
@@ -1016,11 +1016,8 @@
 
 ABISP
 ABISysV_hexagon::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::hexagon) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_hexagon(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_hexagon(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
@@ -1667,15 +1667,12 @@
 
 ABISP
 ABISysV_arm64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
 if (arch_type == llvm::Triple::aarch64) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_arm64(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_arm64(process_sp));
 }
   }
 
Index: lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) ||
   (arch_type == llvm::Triple::mipsel)) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_mips(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_mips(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
@@ -202,11 +202,8 @@
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_s390x(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
===
--- lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
+++ lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
@@ -1323,16 +1323,13 @@
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, 

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB346775: Since ABI's now hold a process WP, they 
should be handed (authored by jingham, committed by ).

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460

Files:
  source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
  source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Index: source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
===
--- source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
+++ source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
@@ -199,12 +199,9 @@
 
 ABISP
 ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
 if (arch.GetTriple().getArch() == llvm::Triple::x86) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_i386(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_i386(process_sp));
 }
   }
   return ABISP();
Index: source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
===
--- source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
+++ source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
@@ -1016,11 +1016,8 @@
 
 ABISP
 ABISysV_hexagon::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::hexagon) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_hexagon(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_hexagon(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
===
--- source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
+++ source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
@@ -1667,15 +1667,12 @@
 
 ABISP
 ABISysV_arm64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
 if (arch_type == llvm::Triple::aarch64) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_arm64(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_arm64(process_sp));
 }
   }
 
Index: source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
===
--- source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
+++ source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) ||
   (arch_type == llvm::Triple::mipsel)) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_mips(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_mips(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
===
--- source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
+++ source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
@@ -202,11 +202,8 @@
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_s390x(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
===
--- source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
+++ source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
@@ -1323,16 +1323,13 @@
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
 if ((arch_type == llvm::Triple::arm) ||
 (arch_type == llvm::Triple::thumb)) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABIMacOSX_arm(process_sp));
-  return g_abi_sp;
+  return A

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Okay, I think I'll keep what Jason did and just make them per-process.  As Greg 
said, they are very cheap to make, since they just take a Process SP and get 
the weak pointer from it.  None of them have any other state.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



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

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

As for testing, Jason was careful to use a WP, so nothing crashes.  The 
process's SP doesn't resolve and then you get the default setting of any 
process specific setting that uses the ABI.  Since there aren't any of those 
yet - this currently has no observable consequences.  But there will be soon, 
and we can use that to test that we are getting this process's settings easily 
once that gets checked in.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



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

[Lldb-commits] [PATCH] D54444: [CMake] Use extended llvm_codesign to pass entitlements for lldb-server

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

We do use lldb-server, though only in the platform mode, for remote testsuite 
running.  The gdbserver mode doesn't do anything for a macOS hosted 
lldb-server, so lldb-server should not require any entitlements.  But we do use 
it.


https://reviews.llvm.org/D5



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

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

[Jim Ingham via Phabricator via lldb-commits]

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

The lldb API's parameters are ordered input first than output.  Pretty much all 
the API's that take a Status as a parameter take it as the last parameter.  So 
it looks weird to have the Status &error first in the QueueThreadPlan... API's. 
 This pattern gets annoying when you have default parameters, so it's okay to 
put the out parameters before the default parameters (though default parameters 
are also not so great, so removing them is also okay...)

Other than that this looked fine.


https://reviews.llvm.org/D54221



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

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

[Jim Ingham via Phabricator via lldb-commits]

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

Looks good to me.


https://reviews.llvm.org/D54221



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I didn't read the patch in detail yet, these are just meta-comments:

It looks like the libOption approach (or this implementation of it) is missing 
the notion of option groups.  That's what you see in the first section of the 
lldb --help printout in the current version.  That's not that big a deal for 
the lldb driver command, as it doesn't have all that many disparate groups of 
options.  But Davide mentioned using this for the lldb command line as well, 
where we do use this to help auto-validate complex commands in a fair number of 
commands.

One thing that is missing from the SB API's was some way of defining the 
arguments and options for a command.  Because of this, Python based commands 
can't take part in the completions features and auto-help generation that 
"real" commands had.  If we had such a thing, then the driver could use the 
same mechanism to define itself as a command, and then just run itself at 
startup.  That would centralize all the argument parsing that we do regardless 
of how it is backed.


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

You have to gather all the -O's and -S's and -Q's and add them to the list of 
code that gets sourced before the file is loaded in the order in which you find 
them.  There can be more than one of each of these and they can be interspersed 
anywhere among the other command options.  Ditto for the -o's, -s's and -q's 
(except they get run after the file).  I don't know how complicated a set of 
tests I wrote when I originally added these, so you might want to check that 
all these cases are covered.


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D43886: [lldb] Add GetCurrentException APIs to SBThread, add frame recognizer for objc_exception_throw for Obj-C runtimes

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The exception breakpoints know that libobjc.B.dylib`objc_exception_throw is 
where you stop for exception breakpoints, and now the ObjCExceptionRecognizer 
knows the same thing.  It always makes me nervous when two different places 
have the same hard-coded string.  Can we add an API to ObjCLanguageRuntime to 
get the FileSpec for the module and a function name for the exception function, 
and then have both CreateExceptionResolver and RegisterObjCExceptionRecognizer 
use that?  Actually it would be better to just add this to the LanguageRuntime, 
because then we could use it for the C++ CreateExceptionResolver and later 
(when one of us gets to it) for RegisterCPlusPlusExceptionRecognizer as well.  
But since C++ can throw in a couple of places, it will need to return a vector 
of name+library pairs.

Other than this niggle, this looks great.


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

https://reviews.llvm.org/D43886



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

[Lldb-commits] [PATCH] D43886: [lldb] Add GetCurrentException APIs to SBThread, add frame recognizer for objc_exception_throw for Obj-C runtimes

[Jim Ingham via Phabricator via lldb-commits]

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

This isn't an external API, so we can generalize it when we get around to 
extending the Recognizers to C++ exceptions.  This is fine for now.

Thanks for doing this!


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

https://reviews.llvm.org/D43886



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

[Lldb-commits] [PATCH] D54843: [Expr] Check the language before ignoring Objective C keywords

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

I think the ideal behavior would be "if we don't have debug info for the frame, 
choose ObjC++".  But if you are in frame with debug info, obey that frame's 
language (except that we have to use C++ because the expression parser uses C++ 
references to play some of the games it plays.)  The reasoning behind that is 
because in the first case, you won' have local names and are calling out to 
global objects and having the widest language available is the most convenient. 
 If we did it that way,  we could also make the user's choice of language 
always hold (except again you can't use "class" anywhere because of the use of 
C++...).

But I don't see an easy way to implement that behavior.  You'd have to plumb 
through another bit - has debug info - which is orthogonal to the language, but 
have that affect the language choice.

However, this change is strictly better for non-objc programs, and preserves 
the same behavior as currently holds when there is ObjC.  So I think this 
change is fine for now.


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

https://reviews.llvm.org/D54843



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Jim Ingham via Phabricator via lldb-commits]

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

This is a little bit odd, but it does make it easy to call methods on a value 
you got from FindVariable without having to cons up an expression.  That seems 
worthwhile.

It would be good to add a little more explanation of what this is doing in the 
docs: "in the context of the object" is a little vague.

You should add some tests where the operation doesn't make sense, for instance 
what if the SBValue is a scalar?  Or what if it is an expression result so it's 
not in memory.  You have some error handling in the patch but your test doesn't 
exercise it.  Be good to make sure that actually works.

You also only test the case where the value is a struct, can you test a pointer 
to a struct and an ObjC class?  They both should work but you don't have tests 
for them.

As an implementation detail, the fact that lldb has to get local variable 
lookup right by textually injecting the local variables declarations into the 
expression is a bug, these should be provided on demand from the ASTSource.  
But that doesn't work correctly at present - the AST source gets asked AFTER 
looking in the class context which is to late - and we have do the local 
variable insertion trick instead.  So if we ever fix the bug in clang lookup, 
your implementation will break, and you will have to interfere in that lookup 
instead.  But that's for the future.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55318



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

[Lldb-commits] [PATCH] D44072: [lldb] Retrieve currently handled Obj-C exception via __cxa_current_exception_type and add GetCurrentExceptionBacktrace SB ABI

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This looks good.  Can you add two more tests:

1. Can you add a step to your tests that shows fetching the current exception 
object from a thread that doesn't have an exception does the right thing (i.e. 
nothing).
2. Since C++ and ObjC exceptions use the same mechanism can you test that at 
abort after an uncaught C++ exception, getting the exception for the thread 
also does nothing (i.e. doesn't crash or produce some mangled object).


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

https://reviews.llvm.org/D44072



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: jasonmolenda, aprantl.
Herald added subscribers: lldb-commits, abidh.

The test "TestExec.py" has been on and off flakey on the GreenDragon MacOS bots 
for a while now.  I'm trying to fix that.  It adds a lot of noise to the bots.

For background:

The job of detecting an exec used to be handled by the DynamicLoaderMacOSX 
plugin.  But when dyld switched over to a remote API approach rather than data 
structures lldb inspects, that job became hard.  About the only clue remaining 
for the new dyld API loader (DynamicLoaderMacOS) is whether the program is back 
at dyld_start when it gets a SIGTRAP.  But if dyld has moved in the course of 
the exec then the new dyld_start address is not the one we knew it as before 
the exec, so this detection will fail.  To make this work on the lldb side, 
we'd have to refresh the shared library state every time we got a SIGTRAP.  But 
that's expensive.

Fortunately, newer debugservers figure this out on their end and annotate the 
stop packet with "reason:exec".  So there's no need to do it on the lldb side.  
But because we didn't want to deal with code signing on the bots, they 
sometimes run older debugservers.  And indeed, when Adrian and I were 
investigating the failures by looking at the packet log we only saw failures 
when the stop reason didn't include reason:exec.

Also fortunately, in order to support other tools that haven't updated to the 
new dyld API's, for now dyld is still maintaining the data structures lldb used 
to look at.

So this patch adds back the check for the dyld info data structure moving that 
the DynamicLoaderMacOSX used.  For debugservers that send reason:exec, this 
code won't ever get run since ProcessGDBRemote will immediately turn the 
SIGTRAP into an exec.  And it will work correctly with older debugservers until 
dyld actually removes this structure.  At that point, the info address will be 
LLDB_INVALID_ADDRESS, and this code won't help anymore.  But presumably by then 
we won't have any debugservers we care about that don't send "reason:exec".


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55399

Files:
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,7 @@
   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -79,7 +79,8 @@
 //--
 DynamicLoaderMacOS::DynamicLoaderMacOS(Process *process)
 : DynamicLoaderDarwin(process), m_image_infos_stop_id(UINT32_MAX),
-  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex() {}
+  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex()
+  , m_maybe_image_infos_address(LLDB_INVALID_ADDRESS) {}
 
 //--
 // Destructor
@@ -95,16 +96,26 @@
   if (m_process) {
 // If we are stopped after an exec, we will have only one thread...
 if (m_process->GetThreadList().GetSize() == 1) {
-  // See if we are stopped at '_dyld_start'
-  ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
-  if (thread_sp) {
-lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
-if (frame_sp) {
-  const Symbol *symbol =
-  frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
-  if (symbol) {
-if (symbol->GetName() == ConstString("_dyld_start"))
-  did_exec = true;
+  // Maybe we still have an image infos address around?  If so see
+  // if that has changed, and if so we have exec'ed:
+  if (m_maybe_image_infos_address != LLDB_INVALID_ADDRESS) {
+lldb::addr_t  image_infos_address = m_process->GetImageInfoAddress();
+if (image_infos_address != m_maybe_image_infos_address)
+  did_exec = true;
+  }
+
+  if (!did_exec) {
+// See if we are stopped at '_dyld_start'
+ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
+if (thread_sp) {
+  lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
+  if (frame_sp) {
+const Symbol *symbol =
+frame_sp->GetSymbolContext(eSymbolContextSymbol).

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 177085.
jingham marked an inline comment as done.
jingham added a comment.

Added a comment and removed an errant space.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399

Files:
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,12 @@
   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address; // If dyld is still maintaining the
+// all_image_infos address, store 
it
+// here so we can use it to detect
+// exec's when talking to
+// debugservers that don't support
+// the "reason:exec" annotation.
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -79,7 +79,8 @@
 //--
 DynamicLoaderMacOS::DynamicLoaderMacOS(Process *process)
 : DynamicLoaderDarwin(process), m_image_infos_stop_id(UINT32_MAX),
-  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex() {}
+  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex(),
+  m_maybe_image_infos_address(LLDB_INVALID_ADDRESS) {}
 
 //--
 // Destructor
@@ -95,16 +96,26 @@
   if (m_process) {
 // If we are stopped after an exec, we will have only one thread...
 if (m_process->GetThreadList().GetSize() == 1) {
-  // See if we are stopped at '_dyld_start'
-  ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
-  if (thread_sp) {
-lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
-if (frame_sp) {
-  const Symbol *symbol =
-  frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
-  if (symbol) {
-if (symbol->GetName() == ConstString("_dyld_start"))
-  did_exec = true;
+  // Maybe we still have an image infos address around?  If so see
+  // if that has changed, and if so we have exec'ed:
+  if (m_maybe_image_infos_address != LLDB_INVALID_ADDRESS) {
+lldb::addr_t image_infos_address = m_process->GetImageInfoAddress();
+if (image_infos_address != m_maybe_image_infos_address)
+  did_exec = true;
+  }
+
+  if (!did_exec) {
+// See if we are stopped at '_dyld_start'
+ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
+if (thread_sp) {
+  lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
+  if (frame_sp) {
+const Symbol *symbol =
+frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
+if (symbol) {
+  if (symbol->GetName() == ConstString("_dyld_start"))
+did_exec = true;
+}
   }
 }
   }
@@ -180,6 +191,7 @@
   }
 
   m_dyld_image_infos_stop_id = m_process->GetStopID();
+  m_maybe_image_infos_address = m_process->GetImageInfoAddress();
 }
 
 bool DynamicLoaderMacOS::NeedToDoInitialImageFetch() { return true; }


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,12 @@
   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address; // If dyld is still maintaining the
+// all_image_infos address, store it
+// here so we can use it to detect
+// exec's when talking to
+// debugservers that don't support
+// the "reason:exec" annotation.
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
=

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 2 inline comments as done.
jingham added inline comments.



Comment at: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:83
+  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex()
+  , m_maybe_image_infos_address(LLDB_INVALID_ADDRESS) {}
 

aprantl wrote:
> Why not just write C++11-style `= LLDB_INVALID_ADDRESS` in the declaration?
I think it's confusing to have some declaration initialization and some 
constructor initialization, and I didn't want to change this class over to all 
initialization as it would add noise to the patch.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: include/lldb/API/SBValue.h:310-312
+  lldb::SBValue EvaluateExpression(const char *expr) const;
+  lldb::SBValue EvaluateExpression(const char *expr,
+   const SBExpressionOptions &options) const;

zturner wrote:
> Can you use `StringRef` instead of `const char *` here?
This is an SB API.  We don't use StringRef's or any other llvm data types in 
the SB API's.


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

https://reviews.llvm.org/D55318



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: source/API/SBValue.cpp:1367-1371
+  if (log)
+log->Printf("SBValue(%p)::EvaluateExpression (expr=\"%s\") => SBValue(%p) "
+"(execution result=%d)",
+static_cast(value_sp.get()), expr,
+static_cast(res_val_sp.get()), expr_res);

zturner wrote:
> If you decide to make that change, note that the macros call `Format`, not 
> `Printf`, so in this case you would have to change your format string to 
> something like:
> 
> ```
> LLDB_LOG(log, "SBValue({0}::EvaluateExpression (expr=\"{1}\") => SBValue({2}) 
> (execution result = {3})",
> value_sp.get(), expr, res_val_sp.get(), expr_res);
> ```
> 
> BTW, I would discourage logging pointer values, as it makes log output 
> non-deterministic and doesn't often help when reading log files.  That said, 
> it wouldn't be the first time we logged pointer values.
Logging the pointer value of something long-lived can often be really helpful 
when you are actually debugging lldb, since you can notice some oddity by 
looking at the log and then get directly to the object that was logged.  
Logging the values of short-lived pointers is not so useful.  I think this is 
more of the latter case, and maybe logging the incoming value_sp's name and 
whether the result was successful might be more useful.


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

https://reviews.llvm.org/D55318



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:104
+if (image_infos_address != m_maybe_image_infos_address)
+  did_exec = true;
+  }

jasonmolenda wrote:
> Do you want to copy the image_infos_address value into 
> m_maybe_image_infos_address so we can detect the next exec?
You don't need to. If we've exec'ed we have to call DoInitialImageFetch again - 
since the world has changed - which resets the value.  I don't want to do it 
twice, but I can put in a comment explaining why if that would help.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:104
+if (image_infos_address != m_maybe_image_infos_address)
+  did_exec = true;
+  }

jingham wrote:
> jasonmolenda wrote:
> > Do you want to copy the image_infos_address value into 
> > m_maybe_image_infos_address so we can detect the next exec?
> You don't need to. If we've exec'ed we have to call DoInitialImageFetch again 
> - since the world has changed - which resets the value.  I don't want to do 
> it twice, but I can put in a comment explaining why if that would help.
Eh, that's dumb, it's just a single assign, so why not.  I added that in the 
committed version.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB348559: Handle detecting exec for DynamicLoaderMacOS with 
older debugservers (authored by jingham, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55399?vs=177085&id=177087#toc

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399

Files:
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,12 @@
   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address; // If dyld is still maintaining the
+// all_image_infos address, store 
it
+// here so we can use it to detect
+// exec's when talking to
+// debugservers that don't support
+// the "reason:exec" annotation.
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -79,7 +79,8 @@
 //--
 DynamicLoaderMacOS::DynamicLoaderMacOS(Process *process)
 : DynamicLoaderDarwin(process), m_image_infos_stop_id(UINT32_MAX),
-  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex() {}
+  m_break_id(LLDB_INVALID_BREAK_ID), m_mutex(),
+  m_maybe_image_infos_address(LLDB_INVALID_ADDRESS) {}
 
 //--
 // Destructor
@@ -95,16 +96,31 @@
   if (m_process) {
 // If we are stopped after an exec, we will have only one thread...
 if (m_process->GetThreadList().GetSize() == 1) {
-  // See if we are stopped at '_dyld_start'
-  ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
-  if (thread_sp) {
-lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
-if (frame_sp) {
-  const Symbol *symbol =
-  frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
-  if (symbol) {
-if (symbol->GetName() == ConstString("_dyld_start"))
-  did_exec = true;
+  // Maybe we still have an image infos address around?  If so see
+  // if that has changed, and if so we have exec'ed.
+  if (m_maybe_image_infos_address != LLDB_INVALID_ADDRESS) {
+lldb::addr_t image_infos_address = m_process->GetImageInfoAddress();
+if (image_infos_address != m_maybe_image_infos_address) {
+  // We don't really have to reset this here, since we are going to
+  // call DoInitialImageFetch right away to handle the exec.  But in
+  // case anybody looks at it in the meantime, it can't hurt.
+  m_maybe_image_infos_address = image_infos_address;
+  did_exec = true;
+}
+  }
+
+  if (!did_exec) {
+// See if we are stopped at '_dyld_start'
+ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
+if (thread_sp) {
+  lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
+  if (frame_sp) {
+const Symbol *symbol =
+frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
+if (symbol) {
+  if (symbol->GetName() == ConstString("_dyld_start"))
+did_exec = true;
+}
   }
 }
   }
@@ -180,6 +196,7 @@
   }
 
   m_dyld_image_infos_stop_id = m_process->GetStopID();
+  m_maybe_image_infos_address = m_process->GetImageInfoAddress();
 }
 
 bool DynamicLoaderMacOS::NeedToDoInitialImageFetch() { return true; }


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,12 @@
   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address; // If dyld is still maintaining the
+// all_image_infos address, store it
+

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D55356#1327280 , @clayborg wrote:

> In D55356#1327242 , @labath wrote:
>
> > In D55356#1327224 , @clayborg 
> > wrote:
> >
> > > In D55356#1327099 , @labath 
> > > wrote:
> > >
> > > > Actually, this now causes an lldb-mi test to fail, but it's not clear 
> > > > to me if the problem is in the test, or this patch. This issue happens 
> > > > when lldb-mi is printing the "library loaded" message after a module 
> > > > gets added to a not-yet-running target. It tries to print the load 
> > > > address by first getting the base address and then converting that to a 
> > > > load address.
> > > >
> > > > Before this patch, that would always fail, because well.. ELF and 
> > > > PECOFF had this function unimplemented, and for MachO the base address 
> > > > was section-relative, and so it wasn't resolved to a load address 
> > > > without the section being loaded. However, with this patch, in the ELF 
> > > > (and presumably PECOFF) case, the load address is not section-relative 
> > > > and so the `GetLoadAddress` function happily returns the address.
> > > >
> > > > Is this the expected behavior here? (i.e., 
> > > > object_file->GetLoadAddress().GetLoadAddress(target) returning a valid 
> > > > value even though the target is not running)
> > >
> > >
> > > Not unless someone has manually set the section load address in the test?
> >
> >
> > The test is not setting the load address manually. This simply falls out of 
> > how `Address::GetLoadAddress`  is implemented:
> >
> >   addr_t Address::GetLoadAddress(Target *target) const {
> > SectionSP section_sp(GetSection());
> > if (section_sp) {
> >   ...
> > } else {
> >   // We don't have a section so the offset is the load address
> >   return m_offset;
> > }
> >   }
> >
> >
> > So, where's the bug here? It's not clear to me how to change 
> > `Address::GetLoadAddress` to do something different than what it is doing 
> > now.
>
>
> So this is a bug really. When there is no section, we should specify what the 
> m_offset is using lldb_private::AddressType in maybe a new ivar name 
> "m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And 
> the above code would become:
>
>  
>
> addr_t Address::GetLoadAddress(Target *target) const {
>
>   SectionSP section_sp(GetSection());
>   if (section_sp) {
> ...
>   } else if (m_offset_type == eAddressTypeLoad) {
> // We don't have a section so the offset is the load address
> return m_offset;
>   }
>
> }
>
>   We just need to be careful and see if we can not make lldb_private::Address 
> get any bigger byte size wise if we can at all avoid it.


I must be missing something.  If there's a file around so that we can express 
this address relative to the file, why would it ever not be expressed as a 
section + offset?  If there's not a file around, then what does it mean to say 
this address ie eAddressTypeFile but we don't know the file?


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

https://reviews.llvm.org/D55356



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

[Lldb-commits] [PATCH] D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This change looks good to me too.

I don't think this will fix the parameter/local variable lookup inversion that 
we've been hacking around. The problem comes because to make an JITted object 
we can easily call we make a wrapper function with a simple argument list - 
that makes running it easy but means it doesn't share the original function's 
context.   Then we smuggle all the local Decls into the wrapper function using 
the FindExternalLexicalDecls from our ClangASTSource.  Unfortunately, that 
lookup gets consulted after the namespace/class lookup is done.  So names in 
the current class or namespace context shadow local variables/arguments.

This will clean up the AST Context's we make but sadly I don't think it will 
affect the expression parser name lookup issues.  If you wanted to test that 
proposition, you can do:

settings set target.experimental.inject-local-vars false

then try some tests where a local variable or parameter name shadows an ivar or 
type name in the current context.  That will find the wrong variable when this 
is false.  I'm sure there are also tests for this, but I don't remember where 
they are off-hand.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55571



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

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added a reviewer: clayborg.
Herald added subscribers: lldb-commits, abidh.

Because of the way we use weak pointers in Process, it is not safe to just 
destroy a fully live Process object.  For instance, because the Thread holds a 
ProcessWP, if the Process gets destroyed as a result of its SharedPointer count 
going to 0, the thread can't get back to it's Process anymore (the WP->SP 
conversion fails), and since it gets to the Target from the Process, it can't 
get its target either.

This is structurally weak, but we've worked around it so far by making sure we 
call Finalize on the Process before we allow it to be destroyed.  Finalize 
clears out all of the objects Process owns, and then the eventual destruction 
can be just reclamation of memory.

However, we shot ourselves in the foot in Target::Launch by storing away a 
ProcessWP for the previous process owned by the target, then setting our 
m_process_sp to the new process THEN reconstituting the ProcessWP and calling 
Finalize on it.  But if Target was the last holder of the old ProcessSP, then 
when we set m_process_sp to the new process, that would trigger the destruction 
of the old Process BEFORE finalizing it.  Tearing down the Process before 
finalizing it doesn't always crash, it depends on what work the process was 
doing at the time.  But sometimes it does crash.

This patch avoids this problem by first finalizing the old process, THEN 
resetting the shared pointer.  That way we know Finalize will happen before 
destruction.

This is a NFC commit, it only fixes a fairly obscure crash.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55631

Files:
  source/Target/Target.cpp


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2868,8 +2868,6 @@
 ProcessWP process_wp;
 if (m_process_sp)
   process_wp = m_process_sp;
-m_process_sp =
-GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
 // Cleanup the old process since someone might still have a strong
 // reference to this process and we would like to allow it to cleanup as
@@ -2880,6 +2878,10 @@
 ProcessSP old_process_sp(process_wp.lock());
 if (old_process_sp)
   old_process_sp->Finalize();
+
+m_process_sp =
+GetPlatform()->DebugProcess(launch_info, debugger, this, error);
+
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2868,8 +2868,6 @@
 ProcessWP process_wp;
 if (m_process_sp)
   process_wp = m_process_sp;
-m_process_sp =
-GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
 // Cleanup the old process since someone might still have a strong
 // reference to this process and we would like to allow it to cleanup as
@@ -2880,6 +2878,10 @@
 ProcessSP old_process_sp(process_wp.lock());
 if (old_process_sp)
   old_process_sp->Finalize();
+
+m_process_sp =
+GetPlatform()->DebugProcess(launch_info, debugger, this, error);
+
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Yeah, I'll simplify the logic after this change.

I've had mysterious crashes that I've never been able to reproduce for quite 
some time now.  Things like we go to tear down a process, the thread is 
deleting its thread plans, a thread plan goes to remove one of it's breakpoints 
and crashes because Thread::GetTarget returns a bogus target.  Then a little 
while ago I got one test in the test suite to crash this way (on my machine for 
about a day and only when I debugged it lightly...)  I figured out that the 
problem was that the Thread relies on the ProcessWP -> ProcessSP to get the 
target.  I am pretty sure this is an infrequent and racy crash because 
generally somebody else is holding onto a shared pointer to the process, so 
m_process_sp is not the last reference.  But if it is, the ThreadPlans can no 
longer get to the target.

I thought a bit about having the thread plans handle GetTarget being fallible, 
but they really do have to have a way to get to the Target when being deleted 
or they will leave stray random breakpoints in the target that will be inserted 
on rerun.  That would be no good.

This change patches over the problem for now.  It will make sure that Finalize 
gets called before destruction.

But longer term we either need to require Finalize before destruction, or we 
need to make a more robust way for Threads to get back to their Targets.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55631



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

[Lldb-commits] [PATCH] D55608: Make crashlog.py work or binaries with spaces in their names

[Jim Ingham via Phabricator via lldb-commits]

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

The frame regex will get confused if you had a binary called "foo 0x1"  Then we 
would treat "foo" as the binary name, 0x1 as the pc, and the rest would be the 
function name.  I don't see how to avoid that altogether.  Maybe we could 
insist that the address have at least 8 numbers in it?  Then your binary would 
have to be called "foo 0x123456789" before we get confused, at which point my 
caring level has dropped pretty low.

I'm not sure about the image regex.  In the part where we are grabbing version 
numbers, you've replaced:

([^<]+)

with

([0-9a-zA-Z_]+)

In crashlogs, I see lines like:

  0x10b60b000 -0x10f707fff  com.apple.LLDB.framework (1.1000.11.38.2 - 
1000.11.38.2) <96E36F5C-1A83-39A1-8713-5FDD9701C3F1> 
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/LLDB

In the first form "([^<]+)" will grab "(1.1000.11.38.2 - 1000.11.38.2) ".  I 
presume we just discard this bit.  But ([0-9a-zA-Z_]+)  will stop at the 
first"(" for the version number, won't it?


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

https://reviews.llvm.org/D55608



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

[Lldb-commits] [PATCH] D55559: Remove the Disassembly benchmarks.

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D9



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

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 178306.
jingham added a comment.

This is better.  Instead of just finalizing, I call DeleteCurrentProcess.  All 
the plugins that implement DebugProcess actually call DeleteCurrentProcess, so 
this just makes sure it happens before we drop our (potentially last) reference 
to the process sp.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55631

Files:
  source/Target/Target.cpp


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2864,22 +2864,15 @@
   log->Printf("Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
-// Get a weak pointer to the previous process if we have one
-ProcessWP process_wp;
-if (m_process_sp)
-  process_wp = m_process_sp;
+// If there was a previous process, delete it before we make the new one.
+// One subtle point, we delete the process before we release the reference
+// to m_process_sp.  That way even if we are the last owner, the process
+// will get Finalized before it gets destroyed.
+DeleteCurrentProcess();
+
 m_process_sp =
 GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
-// Cleanup the old process since someone might still have a strong
-// reference to this process and we would like to allow it to cleanup as
-// much as it can without the object being destroyed. We try to lock the
-// shared pointer and if that works, then someone else still has a strong
-// reference to the process.
-
-ProcessSP old_process_sp(process_wp.lock());
-if (old_process_sp)
-  old_process_sp->Finalize();
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2864,22 +2864,15 @@
   log->Printf("Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
-// Get a weak pointer to the previous process if we have one
-ProcessWP process_wp;
-if (m_process_sp)
-  process_wp = m_process_sp;
+// If there was a previous process, delete it before we make the new one.
+// One subtle point, we delete the process before we release the reference
+// to m_process_sp.  That way even if we are the last owner, the process
+// will get Finalized before it gets destroyed.
+DeleteCurrentProcess();
+
 m_process_sp =
 GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
-// Cleanup the old process since someone might still have a strong
-// reference to this process and we would like to allow it to cleanup as
-// much as it can without the object being destroyed. We try to lock the
-// shared pointer and if that works, then someone else still has a strong
-// reference to the process.
-
-ProcessSP old_process_sp(process_wp.lock());
-if (old_process_sp)
-  old_process_sp->Finalize();
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349435: Call DeleteCurrentProcess before we replace the old 
process. (authored by jingham, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55631?vs=178306&id=178575#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55631

Files:
  lldb/trunk/source/Target/Target.cpp


Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -2864,22 +2864,15 @@
   log->Printf("Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
-// Get a weak pointer to the previous process if we have one
-ProcessWP process_wp;
-if (m_process_sp)
-  process_wp = m_process_sp;
+// If there was a previous process, delete it before we make the new one.
+// One subtle point, we delete the process before we release the reference
+// to m_process_sp.  That way even if we are the last owner, the process
+// will get Finalized before it gets destroyed.
+DeleteCurrentProcess();
+
 m_process_sp =
 GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
-// Cleanup the old process since someone might still have a strong
-// reference to this process and we would like to allow it to cleanup as
-// much as it can without the object being destroyed. We try to lock the
-// shared pointer and if that works, then someone else still has a strong
-// reference to the process.
-
-ProcessSP old_process_sp(process_wp.lock());
-if (old_process_sp)
-  old_process_sp->Finalize();
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "


Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -2864,22 +2864,15 @@
   log->Printf("Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
-// Get a weak pointer to the previous process if we have one
-ProcessWP process_wp;
-if (m_process_sp)
-  process_wp = m_process_sp;
+// If there was a previous process, delete it before we make the new one.
+// One subtle point, we delete the process before we release the reference
+// to m_process_sp.  That way even if we are the last owner, the process
+// will get Finalized before it gets destroyed.
+DeleteCurrentProcess();
+
 m_process_sp =
 GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
-// Cleanup the old process since someone might still have a strong
-// reference to this process and we would like to allow it to cleanup as
-// much as it can without the object being destroyed. We try to lock the
-// shared pointer and if that works, then someone else still has a strong
-// reference to the process.
-
-ProcessSP old_process_sp(process_wp.lock());
-if (old_process_sp)
-  old_process_sp->Finalize();
   } else {
 if (log)
   log->Printf("Target::%s the platform doesn't know how to debug a "
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits