[Lldb-commits] [PATCH] D159315: [lldb] Add OperatingSystem base class to the lldb python module

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I hope that some comments are helpful! The documentation that you added is 
tremendously helpful -- I know how documentation is sometimes a thankless task, 
so I will say what everyone is thinking: "THANK YOU".




Comment at: lldb/examples/python/templates/operating_system.py:11
+"""
+Class that provides data for an instance of a LLDB 'OperatingSystemPython' 
plug-in class
+





Comment at: lldb/examples/python/templates/operating_system.py:27
+- queue : thread dispatch queue name (optional key/value pair)
+- state : thred state (mandatory, set to 'stopped' for now)
+- core : the index of the core (lldb) thread that this OS Thread should 
shadow





Comment at: lldb/examples/python/templates/operating_system.py:31
+Possible values include:
+'breakpoint' if the thread is stopped at a breakpoint
+'none' thread is just stopped because the process is stopped





Comment at: lldb/examples/python/templates/operating_system.py:32
+'breakpoint' if the thread is stopped at a breakpoint
+'none' thread is just stopped because the process is stopped
+'trace' the thread just single stepped





Comment at: lldb/examples/python/templates/operating_system.py:33
+'none' thread is just stopped because the process is stopped
+'trace' the thread just single stepped
+The usual value for this while threads are in memory is 'none'





Comment at: lldb/examples/python/templates/operating_system.py:73
+def get_thread_info(self):
+"""Get the list of operating system threads. This method get called
+automatically every time the process stops and it needs to update its





Comment at: lldb/examples/python/templates/operating_system.py:80
+containing at least for each entry, the thread id, it's name,
+queue, state, stop reason. It can also contain a 
`register_data_addr`
+The list can be empty.

I am sorry but I do not quite understand the meaning of this description. It is 
probably because I am dense, but maybe you can look at rewording it slightly?



Comment at: lldb/examples/python/templates/operating_system.py:89
+id. This method is called when unwinding the stack of one of the
+operating system thread.
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159315

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

[Lldb-commits] [PATCH] D129338: Tell the user which pathname was invalid...

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:2717
   if (module_file.GetDirectory().IsEmpty()) {
 error.SetErrorString("invalid directory name");
 return false;

Sorry for dropping in unsolicited, but would this be another error that we 
would want to add more specificity for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129338

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

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Not that my opinion means much, but the changes look good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131294

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

According to https://en.cppreference.com/w/cpp/language/operator_precedence, I 
would read the left operand of the `&&` as 

1. The `==` has higher precedence than `||` so, `b = (lbound == one_compl64)`
2. `b || one_cmpl32`

which does not seem like what the original author intended. I absolutely think 
that the fix is correct, but I just wanted to get everyone's feedback on 
whether this seems like more than just a "suspicious bitwise expression" (and 
more like a "mistaken bitwise expression").

All that said, I could be completely, 100% wrong. And, if I am, feel free to 
ignore me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

DavidSpickett wrote:
> hawkinsw wrote:
> > According to 
> > https://en.cppreference.com/w/cpp/language/operator_precedence, I would 
> > read the left operand of the `&&` as 
> > 
> > 1. The `==` has higher precedence than `||` so, `b = (lbound == 
> > one_compl64)`
> > 2. `b || one_cmpl32`
> > 
> > which does not seem like what the original author intended. I absolutely 
> > think that the fix is correct, but I just wanted to get everyone's feedback 
> > on whether this seems like more than just a "suspicious bitwise expression" 
> > (and more like a "mistaken bitwise expression").
> > 
> > All that said, I could be completely, 100% wrong. And, if I am, feel free 
> > to ignore me!
> The corrected code also makes sense given that MPX is some kind of memory 
> protection across ranges.
> 
> If `((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0)` is true 
> then upper bound < lower bound making an invalid range. Which is what I'd 
> expect for some default/uninitialised state (especially if zero size ranges 
> are allowed, so upper == 0 and lower == 0 couldn't be used).
@DavidSpickett I think that you and I are saying the same thing, right? We are 
both saying that the corrected code looks much "better" than the original?

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D131983: [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;

fixathon wrote:
> If getAsInteger() fails we trigger the error path.
> If getAsInteger() succeeds, but the fetched value (side-effect) is negative 
> we trigger the error path.
> 
> I'm not 100% happy with my own code here..
> getAsInteger() somewhat counter-intuitively returns *true* on failure, and 
> there's reliance on the short-circuit evaluation sequence-points for correct 
> evaluation when the 1st part is setting value via side-effect, and the second 
> validates it. This makes the expression somewhat hard to reason about.
> 
> Also mutating **m_count** directly may result in a non-atomic transition to 
> its new valid state in case when getAsInteger() fetches negative input, which 
> is less than perfect.
> 
> That said, this code is just parsing command options, in a single-threaded 
> manner, so that shouldn't matter.
I am not sure if this helps or not, but ...

`getAsInteger` is the exact interface that the LLVM command-line parsing 
library uses for parsing numeric options. I looked through the codebase for 
other places that do the same type of operation to see if they use a different 
technique and could not find anything. 

To address your non-atomic comment, we could introduce a temporary? Given your 
description of how this code will be used (single-threaded), that seems like 
overkill? 

I am no expert and I hope that being part of the discussion is helpful. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [PATCH] D131983: [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;

Hope this doesn't screw up Phabricator, but I just wanted to confirm with 
@clayborg that, yes, that is specified: 

http://eel.is/c++draft/expr.log.and


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [PATCH] D121800: [LLDB] Change enumaration to enumeration

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: clayborg, zequanwu, labath.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Change enumaration to enumeration in code handling LLDB help output.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121800

Files:
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/Options.td


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -863,7 +863,7 @@
 
 let Command = "target dependents" in {
   def dependents_no_dependents : Option<"no-dependents", "d">, Group<1>,
-OptionalEnumArg<"Value", "OptionEnumValues(g_dependents_enumaration)">,
+OptionalEnumArg<"Value", "OptionEnumValues(g_dependents_enumeration)">,
 Desc<"Whether or not to load dependents when creating a target. If the "
  "option is not specified, the value is implicitly 'default'. If the "
  "option is specified but without a value, the value is implicitly "
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -137,7 +137,7 @@
 
 // Note that the negation in the argument name causes a slightly confusing
 // mapping of the enum values.
-static constexpr OptionEnumValueElement g_dependents_enumaration[] = {
+static constexpr OptionEnumValueElement g_dependents_enumeration[] = {
 {
 eLoadDependentsDefault,
 "default",


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -863,7 +863,7 @@
 
 let Command = "target dependents" in {
   def dependents_no_dependents : Option<"no-dependents", "d">, Group<1>,
-OptionalEnumArg<"Value", "OptionEnumValues(g_dependents_enumaration)">,
+OptionalEnumArg<"Value", "OptionEnumValues(g_dependents_enumeration)">,
 Desc<"Whether or not to load dependents when creating a target. If the "
  "option is not specified, the value is implicitly 'default'. If the "
  "option is specified but without a value, the value is implicitly "
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -137,7 +137,7 @@
 
 // Note that the negation in the argument name causes a slightly confusing
 // mapping of the enum values.
-static constexpr OptionEnumValueElement g_dependents_enumaration[] = {
+static constexpr OptionEnumValueElement g_dependents_enumeration[] = {
 {
 eLoadDependentsDefault,
 "default",
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121802: [LLDB] Fix typos in LLDB help output.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: clayborg, zequanwu, DavidSpickett.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Correct a few spelling errors and typos in the LLDB help output.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121802

Files:
  lldb/source/Commands/Options.td


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -138,17 +138,17 @@
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
 Desc<"Set the breakpoint by function name.  Can be repeated multiple times 
"
-"to makeone breakpoint for multiple names">;
+"to make one breakpoint for multiple names.">;
   def breakpoint_set_source_regexp_function :
 Option<"source-regexp-function", "X">, Group<9>, Arg<"FunctionName">,
 Completion<"Symbol">,
 Desc<"When used with '-p' limits the source regex to source contained in "
-"the namedfunctions.  Can be repeated multiple times.">;
+"the named functions.  Can be repeated multiple times.">;
   def breakpoint_set_fullname : Option<"fullname", "F">, Group<4>,
 Arg<"FullName">, Required, Completion<"Symbol">,
 Desc<"Set the breakpoint by fully qualified function names. For C++ this "
 "means namespaces and all arguments, and for Objective-C this means a full 
"
-"functionprototype with class and selector.  Can be repeated multiple 
times"
+"function prototype with class and selector.  Can be repeated multiple 
times"
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Required,
@@ -163,8 +163,8 @@
   def breakpoint_set_basename : Option<"basename", "b">, Group<8>,
 Arg<"FunctionName">, Required, Completion<"Symbol">,
 Desc<"Set the breakpoint by function basename (C++ namespaces and 
arguments"
-" will beignored).  Can be repeated multiple times to make one breakpoint "
-"for multiplesymbols.">;
+" will be ignored).  Can be repeated multiple times to make one breakpoint 
"
+"for multiple symbols.">;
   def breakpoint_set_source_pattern_regexp :
 Option<"source-pattern-regexp", "p">, Group<9>, Arg<"RegularExpression">,
 Required, Desc<"Set the breakpoint by specifying a regular expression 
which"
@@ -335,7 +335,7 @@
 Arg<"AddressOrExpression">,
 Desc<"Disassemble function containing this address.">;
   def disassemble_options_force : Option<"force", "\\x01">, 
Groups<[2,3,4,5,7]>,
-Desc<"Force dissasembly of large functions.">;
+Desc<"Force disassembly of large functions.">;
 }
 
 let Command = "expression" in {
@@ -410,7 +410,7 @@
 Desc<"If true, only apply this recognizer to frames whose PC currently 
points to the "
 "first instruction of the specified function. If false, the recognizer "
 "will always be applied, regardless of the current position within the 
specified function. The "
-"implementor should keep in mind that some features, e.g. accessing 
function argument "
+"implementer should keep in mind that some features, e.g. accessing 
function argument "
 "values via $arg, are not guaranteed to work reliably in this case, so 
extra care must "
 "be taken to make the recognizer operate correctly. Defaults to true.">;
 }


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -138,17 +138,17 @@
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
 Desc<"Set the breakpoint by function name.  Can be repeated multiple times "
-"to makeone breakpoint for multiple names">;
+"to make one breakpoint for multiple names.">;
   def breakpoint_set_source_regexp_function :
 Option<"source-regexp-function", "X">, Group<9>, Arg<"FunctionName">,
 Completion<"Symbol">,
 Desc<"When used with '-p' limits the source regex to source contained in "
-"the namedfunctions.  Can be repeated multiple times.">;
+"the named functions.  Can be repeated multiple times.">;
   def breakpoint_set_fullname : Option<"fullname", "F">, Group<4>,
 Arg<"FullName">, Required, Completion<"Symbol">,
 Desc<"Set the breakpoint by fully qualified function names. For C++ this "
 "means namespaces and all arguments, and for Objective-C this means a full "
-"functionprototype with class and selector.  Can be repeated multiple times"
+"function prototype with class and selector.  Can be repeated multiple times"
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Requir

[Lldb-commits] [PATCH] D121802: [LLDB] Fix typos in LLDB help output.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.
Herald added a subscriber: JDevlieghere.

I hope I didn't spam you all with asking for a review. It seems you are 
recently committers to this file. I am a long-time user, first-time 
contributor. I hope I am following best practices.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121802

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

[Lldb-commits] [PATCH] D121800: [LLDB] Change enumaration to enumeration

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D121800#3385982 , @labath wrote:

> Looks good. I presume you need someone to commit this for you (?) What's the 
> name&email I can use for commit attribution?

Thanks for the quick review!! So glad to be able to help. I do need someone to 
commit for me. You can use the info associated with this account:

Will Hawkins
wh...@obs.cr

Thanks again for such a quick review and all your work on LLDB!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121800

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

[Lldb-commits] [PATCH] D121802: [LLDB] Fix typos in LLDB help output.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D121802#3386012 , @labath wrote:

> I think you're doing everything right. Thanks for the fixes. I can commit 
> this together with the other patch.

Thanks again for being so welcoming. Definitely encourages me to continue 
offering patches. I've use phabricator before (when I worked at Mozilla) and 
was scared of the overhead. But, the tools have definitely improved and the 
process was painless. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121802

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

[Lldb-commits] [PATCH] D124338: [lldb] Update online help text (consistency, typo)

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: JDevlieghere, teemperor.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Update the online help text for `breakpoint set` to be
consistent with respect to the spelling of Objective-C
and fix a few space-related typos.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124338

Files:
  lldb/source/Commands/Options.td


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -95,7 +95,7 @@
   def breakpoint_modify_command : Option<"command", "C">, Group<4>,
 Arg<"Command">,
 Desc<"A command to run when the breakpoint is hit, can be provided more "
-"than once, the commands will get run in order left to right.">;
+"than once, the commands will be run in left-to-right order.">;
 }
 
 let Command = "breakpoint dummy" in {
@@ -132,8 +132,8 @@
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
-"Again, this will allow lldb to track that offset on subsequent reloads. "
-" The module need not have been loaded at the time you specify this "
+"Again, this will allow lldb to track that offset on subsequent reloads.  "
+"The module need not have been loaded at the time you specify this "
 "breakpoint, and will get resolved when the module is loaded.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
@@ -152,8 +152,8 @@
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Required,
-Desc<"Set the breakpoint by ObjC selector name. Can be repeated multiple "
-"times to make one breakpoint for multiple Selectors.">;
+Desc<"Set the breakpoint by Objective-C selector name. Can be repeated "
+"multiple times to make one breakpoint for multiple Selectors.">;
   def breakpoint_set_method : Option<"method", "M">, Group<6>, Arg<"Method">,
 Required, Desc<"Set the breakpoint by C++ method names.  Can be repeated "
 "multiple times to make one breakpoint for multiple methods.">;


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -95,7 +95,7 @@
   def breakpoint_modify_command : Option<"command", "C">, Group<4>,
 Arg<"Command">,
 Desc<"A command to run when the breakpoint is hit, can be provided more "
-"than once, the commands will get run in order left to right.">;
+"than once, the commands will be run in left-to-right order.">;
 }
 
 let Command = "breakpoint dummy" in {
@@ -132,8 +132,8 @@
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
-"Again, this will allow lldb to track that offset on subsequent reloads. "
-" The module need not have been loaded at the time you specify this "
+"Again, this will allow lldb to track that offset on subsequent reloads.  "
+"The module need not have been loaded at the time you specify this "
 "breakpoint, and will get resolved when the module is loaded.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
@@ -152,8 +152,8 @@
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Required,
-Desc<"Set the breakpoint by ObjC selector name. Can be repeated multiple "
-"times to make one breakpoint for multiple Selectors.">;
+Desc<"Set the breakpoint by Objective-C selector name. Can be repeated "
+"multiple times to make one breakpoint for multiple Selectors.">;
   def breakpoint_set_method : Option<"method", "M">, Group<6>, Arg<"Method">,
 Required, Desc<"Set the breakpoint by C++ method names.  Can be repeated "
 "multiple times to make one breakpoint for multiple methods.">;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124338: [lldb] Update online help text (consistency, typo)

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I hope that I didn't SPAM you, @teemperor and @JDevlieghere ! Thanks for all 
that you do for the LLVM community!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124338

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

[Lldb-commits] [PATCH] D124338: [lldb] Update online help text (consistency, typo)

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 425032.
hawkinsw added a comment.

Updating thanks to @jdevlieghere feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124338

Files:
  lldb/source/Commands/Options.td


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -95,7 +95,7 @@
   def breakpoint_modify_command : Option<"command", "C">, Group<4>,
 Arg<"Command">,
 Desc<"A command to run when the breakpoint is hit, can be provided more "
-"than once, the commands will get run in order left to right.">;
+"than once, the commands will be run in left-to-right order.">;
 }
 
 let Command = "breakpoint dummy" in {
@@ -132,8 +132,8 @@
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
-"Again, this will allow lldb to track that offset on subsequent reloads. "
-" The module need not have been loaded at the time you specify this "
+"Again, this will allow lldb to track that offset on subsequent reloads.  "
+"The module need not have been loaded at the time you specify this "
 "breakpoint, and will get resolved when the module is loaded.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
@@ -152,8 +152,8 @@
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Required,
-Desc<"Set the breakpoint by ObjC selector name. Can be repeated multiple "
-"times to make one breakpoint for multiple Selectors.">;
+Desc<"Set the breakpoint by Objective-C selector name.  Can be repeated "
+"multiple times to make one breakpoint for multiple Selectors.">;
   def breakpoint_set_method : Option<"method", "M">, Group<6>, Arg<"Method">,
 Required, Desc<"Set the breakpoint by C++ method names.  Can be repeated "
 "multiple times to make one breakpoint for multiple methods.">;


Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -95,7 +95,7 @@
   def breakpoint_modify_command : Option<"command", "C">, Group<4>,
 Arg<"Command">,
 Desc<"A command to run when the breakpoint is hit, can be provided more "
-"than once, the commands will get run in order left to right.">;
+"than once, the commands will be run in left-to-right order.">;
 }
 
 let Command = "breakpoint dummy" in {
@@ -132,8 +132,8 @@
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
-"Again, this will allow lldb to track that offset on subsequent reloads. "
-" The module need not have been loaded at the time you specify this "
+"Again, this will allow lldb to track that offset on subsequent reloads.  "
+"The module need not have been loaded at the time you specify this "
 "breakpoint, and will get resolved when the module is loaded.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
@@ -152,8 +152,8 @@
 " to make one breakpoint for multiple names.">;
   def breakpoint_set_selector : Option<"selector", "S">, Group<5>,
 Arg<"Selector">, Required,
-Desc<"Set the breakpoint by ObjC selector name. Can be repeated multiple "
-"times to make one breakpoint for multiple Selectors.">;
+Desc<"Set the breakpoint by Objective-C selector name.  Can be repeated "
+"multiple times to make one breakpoint for multiple Selectors.">;
   def breakpoint_set_method : Option<"method", "M">, Group<6>, Arg<"Method">,
 Required, Desc<"Set the breakpoint by C++ method names.  Can be repeated "
 "multiple times to make one breakpoint for multiple methods.">;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124338: [lldb] Update online help text (consistency, typo)

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Added that extra space, as requested! And, yes, I will need someone to land 
this one for me. I am going to ask for commit access, but so far have not! 
Thanks for your help, @JDevlieghere !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124338

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: tatyana-krasnukha, jingham.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When setting an address breakpoint using a non-section address
in lldb before having ever run the program, the binary itself
is not considered a module. As a result, the breakpoint is
unresolved (and never gets resolved subsequently).

This patch changes that behavior: as a last resort, the binary
is considered as a module when resolving a non-section address
breakpoint.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124731

Files:
  lldb/source/Breakpoint/BreakpointResolverAddress.cpp


Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-if (module_sp) {
+if (m_module_filespec) {
+  // ... and we're given a module, see if we can find the
+  // appropriate module loaded in the binary, and fix up
+  // m_addr to use that.
+  ModuleSpec module_spec(m_module_filespec);
+  containing_module_sp =
+  target.GetImages().FindFirstModule(module_spec);
+} else {
+  // ... and we're not given a module, see if the offset is
+  // somewhere in the executable module. If it is, then we'll
+  // fix up m_addr to use that.
+  containing_module_sp = target.GetExecutableModule();
+}
+if (containing_module_sp) {
   Address tmp_address;
-  if (module_sp->ResolveFileAddress(m_addr.GetOffset(), tmp_address))
+  if (containing_module_sp->ResolveFileAddress(m_addr.GetOffset(),
+   tmp_address))
 m_addr = tmp_address;
 }
   }


Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-if (module_sp) {
+if (m_module_filespec) {
+  // ... and we're given a module, see if we can find the
+  // appropriate module loaded in the binary, and fix up
+  // m_addr to use that.
+  ModuleSpec module_spec(m_module_filespec);
+  containing_module_sp =
+  target.GetImages().FindFirstModule(module_spec);
+} else {
+  // ... and we're not given a module, see if the offset is
+  // somewhere in the executable module. If it is, then we'll
+  // fix up m_addr to use that.
+  containing_module_sp = target.GetExecutableModule();
+}
+if (containing_module_sp) {
   Address tmp_address;
-  if (module_sp->ResolveFileAddress(m_addr.GetOffset(), tmp_address))
+  if (containing_module_sp->ResolveFileAddress(m_addr.GetOffset(),
+   tmp_address))
 m_addr = tmp_address;
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

First, a thank you from a long-time `gdb` user who has finally seen the light 
and working to switch to `lldb`! The work that you all do to build an awesome, 
modern debugger is amazing.

As for this patch, I have committed a few typo fixes in the past, but this is 
my first code submission. I hope that I am following proper procedures and that 
this is a useful improvement.

Please let me know if I have done anything wrong and/or how to make this 
submission better. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124922: [lldb] Inject commands into log output when directed to file

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: teemperor, JDevlieghere, labath.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When logging for the lldb channel is enabled for any category and the
channel output is being directed to a file, inject a copy of every
command entered. This change will make it easier to contextualize the
debugging output with any commands that the user entered.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124922

Files:
  lldb/include/lldb/Utility/Log.h
  lldb/source/Commands/CommandObjectLog.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp


Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1838,11 +1838,17 @@
   std::string original_command_string(command_line);
 
   Log *log = GetLog(LLDBLog::Commands);
+  Log *lldb_log = GetChannelLogWhenAnyCategoryEnabled();
   llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
command_line);
 
   LLDB_LOGF(log, "Processing command: %s", command_line);
   LLDB_SCOPED_TIMERF("Processing command: %s.", command_line);
+  // If there is logging enabled for any category in the lldb channel that
+  // is being directed to a file, log every command. This functionality will
+  // make it easier for the user reading a log file after the fact to correlate
+  // the debugging output with the commands that may have caused it.
+  LLDB_LOGF_FILE_ONLY(lldb_log, "(lldb) %s\n", command_line);
 
   if (WasInterrupted()) {
 result.AppendError("interrupted");
Index: lldb/source/Commands/CommandObjectLog.cpp
===
--- lldb/source/Commands/CommandObjectLog.cpp
+++ lldb/source/Commands/CommandObjectLog.cpp
@@ -88,6 +88,7 @@
   case 'f':
 log_file.SetFile(option_arg, FileSpec::Style::native);
 FileSystem::Instance().Resolve(log_file);
+log_options |= LLDB_LOG_OPTION_LOG_TO_FILE;
 break;
   case 't':
 log_options |= LLDB_LOG_OPTION_THREADSAFE;
Index: lldb/include/lldb/Utility/Log.h
===
--- lldb/include/lldb/Utility/Log.h
+++ lldb/include/lldb/Utility/Log.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,6 +42,7 @@
 #define LLDB_LOG_OPTION_BACKTRACE (1U << 7)
 #define LLDB_LOG_OPTION_APPEND (1U << 8)
 #define LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION (1U << 9)
+#define LLDB_LOG_OPTION_LOG_TO_FILE (1U << 10)
 
 // Logging Functions
 namespace lldb_private {
@@ -238,6 +240,18 @@
   return LogChannelFor().GetLog(Log::MaskType(mask));
 }
 
+/// Retrieve the Log object for a channel when any of its categories are
+/// enabled.
+///
+/// Returns a valid Log object if any of the categories for a channel are
+/// enabled. Otherwise, returns nullptr.
+template  Log *GetChannelLogWhenAnyCategoryEnabled() {
+  static_assert(
+  std::is_same>::value, "");
+  Log::MaskType any_category_flag = std::numeric_limits::max();
+  return LogChannelFor().GetLog(any_category_flag);
+}
+
 } // namespace lldb_private
 
 /// The LLDB_LOG* macros defined below are the way to emit log messages.
@@ -274,6 +288,15 @@
   log_private->Printf(__VA_ARGS__);
\
   } while (0)
 
+// Write a message to the log only if that log is sending its output to a file.
+#define LLDB_LOGF_FILE_ONLY(log, ...)  
\
+  do { 
\
+::lldb_private::Log *log_private = (log);  
\
+if (log_private && 
\
+log_private->GetOptions().Test(LLDB_LOG_OPTION_LOG_TO_FILE))   
\
+  log_private->Printf(__VA_ARGS__);
\
+  } while (0)
+
 #define LLDB_LOGV(log, ...)
\
   do { 
\
 ::lldb_private::Log *log_private = (log);  
\


Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1838,11 +1838,17 @@
   std::string original_command_string(command_line);
 
   Log *log = GetLog(LLDBLog::Commands);
+  Log *lldb_log = GetChannelLogWhenAnyCategoryEnabled();
   llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
command_line);
 
   LLDB_LOGF(log, "Processing c

[Lldb-commits] [PATCH] D124922: [lldb] Inject commands into log output when directed to file

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

As always, thank you for the work that you all do on lldb! An amazing tool! It 
is so much fun to be able to help and I //hope// that this is a helpful 
addition. Please let me know whether this is something that you find useful and 
whether or not there are changes that I can make to get this patch acceptable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124922

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

[Lldb-commits] [PATCH] D124922: [lldb] Inject commands into log output when directed to file

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124922#3491190 , @labath wrote:

> Just my opinion, but I wouldn't like to complicate the logging architecture 
> with file-only log statements and cross-channel dependencies. If you want to 
> cross-reference some other logging channel with the executed commands then 
> I'd say you should enable both channels. We can definitely talk about making 
> the command feature more prominently in the log stream.

Thanks! I am just trying to be helpful!

You are suggesting that the user would always add `commands` to what you 
`enable` in order to have this feature? Ie,

`log enable -f lldb.out lldb break commands`

The only reason that would be less than ideal for me (personally) is that there 
is lots of other output that comes along with `commands` that I might not 
necessarily want.

Again, just trying to be helpful! Again, I am happy to do whatever the group 
consensus is!!

Just happy to participate!
Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124922

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

[Lldb-commits] [PATCH] D124922: [lldb] Inject commands into log output when directed to file

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124922#3491214 , @labath wrote:

> Yeah, I guess that what I am saying.
>
> For me the `commands` channel gives 6 lines of output for a command. Compared 
> to what some of our other logging channels do, that's nothing.

Totally understand.

Whatever the consensus is, I am happy to oblige. I just have been having so 
much fun working with LLDB. While I have your ear, are there any other pain 
points where I could contribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124922

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

[Lldb-commits] [PATCH] D124922: [lldb] Inject commands into log output when directed to file

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124922#3491267 , @labath wrote:

> Heh, well.. there definitely are, though a lot of things that come to mind 
> right now are not really suitable for a first-time contributor. Is there any 
> specific are that you would like to know (or that you know already)?
>
> One of the things that I ran into recently, but hadn't had time to fix is the 
> relocation processing code in `ObjectFileELF::ApplyRelocations`. It's 
> currently fairly messy, and mixes relocations of different architectures 
> (which generally doesn't work as different constants mean different things 
> depending on the architecture. If you could refactor the function such that 
> it has a clear split between architectures, and still allows for code sharing 
> for relocations that mean the same thing on each architecture, then that 
> would be greatly appreciated.
>
> You can try that out by compiling a .o file for some non-x86 architecture and 
> checking whether lldb can read the debug info without asserting.

Thanks!!

If there is interest in pursuing this patch, I will keep it open!

I have committed several typo changes in the past and I am a fairly seasoned 
developer. In fact, I have another outstanding patch waiting for review.

I will definitely look in to what you suggest. I hope that you will see some 
submissions forthcoming. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124922

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124731#3494609 , @jingham wrote:

> This seems like a reasonable fallback, and the implementation looks fine.
> You need to add a test case setting an address breakpoint in the executable 
> and making sure that works.  Should be easy, set a line breakpoint in the 
> main executable, get the resolved address and set an address breakpoint with 
> that and make sure that breakpoint got a location.
> You should also add some words to the "break set" command help saying that 
> this is the fallback behavior.

Thank you for the feedback! I will make those changes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 427791.
hawkinsw added a comment.

Updated patch including responses to helpful feedback from @jingham.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

Files:
  lldb/source/Breakpoint/BreakpointResolverAddress.cpp


Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-if (module_sp) {
+if (m_module_filespec) {
+  // ... and we're given a module, see if we can find the
+  // appropriate module loaded in the binary, and fix up
+  // m_addr to use that.
+  ModuleSpec module_spec(m_module_filespec);
+  containing_module_sp =
+  target.GetImages().FindFirstModule(module_spec);
+} else {
+  // ... and we're not given a module, see if the offset is
+  // somewhere in the executable module. If it is, then we'll
+  // fix up m_addr to use that.
+  containing_module_sp = target.GetExecutableModule();
+}
+if (containing_module_sp) {
   Address tmp_address;
-  if (module_sp->ResolveFileAddress(m_addr.GetOffset(), tmp_address))
+  if (containing_module_sp->ResolveFileAddress(m_addr.GetOffset(),
+   tmp_address))
 m_addr = tmp_address;
 }
   }


Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-if (module_sp) {
+if (m_module_filespec) {
+  // ... and we're given a module, see if we can find the
+  // appropriate module loaded in the binary, and fix up
+  // m_addr to use that.
+  ModuleSpec module_spec(m_module_filespec);
+  containing_module_sp =
+  target.GetImages().FindFirstModule(module_spec);
+} else {
+  // ... and we're not given a module, see if the offset is
+  // somewhere in the executable module. If it is, then we'll
+  // fix up m_addr to use that.
+  containing_module_sp = target.GetExecutableModule();
+}
+if (containing_module_sp) {
   Address tmp_address;
-  if (module_sp->ResolveFileAddress(m_addr.GetOffset(), tmp_address))
+  if (containing_module_sp->ResolveFileAddress(m_addr.GetOffset(),
+   tmp_address))
 m_addr = tmp_address;
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 427794.
hawkinsw added a comment.

(for real) Updating the patch based on helpful feedback from @jingham.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

Files:
  lldb/source/Breakpoint/BreakpointResolverAddress.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
  
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
  lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c

Index: lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
@@ -0,0 +1,6 @@
+int function(int a) { return a; }
+
+int main() {
+  int f = function(10);
+  return f;
+}
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
@@ -0,0 +1,36 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def get_address_from_symbol(self, symbol):
+target = lldbutil.run_to_breakpoint_make_target(self, "a.out", True)
+bp = target.BreakpointCreateByName(symbol, None)
+address = bp.GetLocationAtIndex(0).GetAddress().GetFileAddress()
+return address
+
+def test_set_address_no_module(self):
+self.build()
+
+main_address = self.get_address_from_symbol("main")
+
+tgt = lldbutil.run_to_breakpoint_make_target(self)
+dbg = tgt.GetDebugger()
+
+dbg.HandleCommand(f"break set -a {main_address:#x}")
+self.assertTrue(tgt.GetNumBreakpoints() == 1)
+
+bp = tgt.GetBreakpointAtIndex(0)
+self.assertTrue(bp != None)
+
+_, _, thread, _ = lldbutil.run_to_breakpoint_do_run(self, tgt, bp)
+self.assertTrue(thread.GetNumFrames() >= 1)
+
+thread_pc = thread.GetFrameAtIndex(0).GetPCAddress()
+self.assertTrue(thread_pc != None)
+
+self.assertTrue(main_address==thread_pc.GetFileAddress())
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := inferior.c
+
+include Makefile.rules
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -128,13 +128,15 @@
 Arg<"AddressOrExpression">, Required,
 Desc<"Set the breakpoint at the specified address.  If the address maps "
 "uniquely to a particular binary, then the address will be converted to "
-"a \"file\"address, so that the breakpoint will track that binary+offset "
+"a \"file\" address, so that the breakpoint will track that binary+offset "
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
 "Again, this will allow lldb to track that offset on subsequent reloads.  "
 "The module need not have been loaded at the time you specify this "
-"breakpoint, and will get resolved when the module is loaded.">;
+"breakpoint, and will get resolved when the module is loaded.  If no "
+"module is specified, the binary being debugged is considered as a "
+"fallback.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
 Desc<"Set the breakpoint by function name.  Can be repeated multiple times "
Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-ModuleSP module_sp = targe

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124731#3498195 , @hawkinsw wrote:

> (for real) Updating the patch based on helpful feedback from @jingham.

I used to know better how to make sure that phabricator notified people that 
the commit was updated based on their feedback, but I've since forgotten! 
Sorry! I hope that you didn't get spammed @jingham. Thanks again for the 
helpful review! It was fun to get to learn the unit-testing system. I hope that 
the test is acceptable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D125148: Add an example command that runs to one of a set of breakpoints

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Just trying to be helpful!




Comment at: lldb/examples/python/cont_to_bkpt.py:26
+target = exe_ctx.target
+if  not exe_ctx.target.IsValid():
+result.SetError("Need a valid target")

Reuse `target` here rather than `exe_ctx.target`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125148

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Sorry to bother you all (@jingham, @JDevlieghere  and @labath), but I just 
wondered if there was anything else that I could do to spruce this up! I want 
to make sure that it meets everyone's expectations! It's been awesome to work 
on this submission!

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D125434: Make a more convenient way to allow Darwin users to ignore certain Mach Exceptions

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I hope that those minor edits help at least a little!




Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSXProperties.td:14
+ "(e.g. 'EXC_BAD_ACCESS|EXC_BAD_INSTRUCTION'). "
+ "lldb will instead stop on the BSD signal the exception was converted"
+ " into, if there is one.">;





Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSXProperties.td:15
+ "lldb will instead stop on the BSD signal the exception was converted"
+ " into, if there is one.">;
+}




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125434

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Hello @labath and @JDevlieghere -- I hope that your Friday is going well! I'd 
love to make any other changes to this if necessary! Thanks for everything!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw marked 5 inline comments as done.
hawkinsw added inline comments.



Comment at: 
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py:11
+def get_address_from_symbol(self, symbol):
+target = lldbutil.run_to_breakpoint_make_target(self, "a.out", True)
+bp = target.BreakpointCreateByName(symbol, None)

JDevlieghere wrote:
> Can we reuse this target? If so you can store it as `self.target` and reuse 
> it from `test_set_address_no_module`.
Thanks for the suggestion! I *wish* that we could reuse it (I know that 
instantiating multiple targets per test will increase the runtime), but I would 
prefer (?) to have them separate to guarantee the test is doing what we want. 

If we reused the target from this function when we ran the actual test, it 
would already have `a.out` loaded and, as a result, not trigger the actual 
behavior we want to test (setting a breakpoint when the program's "modules" 
have not already been loaded.). 

Does that make sense? Seem reasonable? 



Comment at: 
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py:25
+dbg.HandleCommand(f"break set -a {main_address:#x}")
+self.assertTrue(tgt.GetNumBreakpoints() == 1)
+

JDevlieghere wrote:
> Our test harness has a bunch of helper functions that generate better error 
> messages if the assertion fails. For example, here you could use 
> `assertEqual(1, tgt.GetNumBreakpoints())`. If the assertion fails, it will 
> print something like:
> 
> > tgt.GetNumBreakpoints() was expected to be 1 but was 2
> 
> which is much more informative than 
> 
> ? tgt.GetNumBreakpoints() == 1 was expected to be True but was False
Great suggestion! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 429411.
hawkinsw added a comment.

Updating the patchset according to the helpful feedback from @jdevlieghere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

Files:
  lldb/source/Breakpoint/BreakpointResolverAddress.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
  
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
  lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c

Index: lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
@@ -0,0 +1,6 @@
+int function(int a) { return a; }
+
+int main() {
+  int f = function(10);
+  return f;
+}
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
@@ -0,0 +1,36 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def get_address_from_symbol(self, symbol):
+target = lldbutil.run_to_breakpoint_make_target(self, "a.out", True)
+bp = target.BreakpointCreateByName(symbol, None)
+address = bp.GetLocationAtIndex(0).GetAddress().GetFileAddress()
+return address
+
+def test_set_address_no_module(self):
+self.build()
+
+main_address = self.get_address_from_symbol("main")
+
+target = lldbutil.run_to_breakpoint_make_target(self)
+debugger = target.GetDebugger()
+
+debugger.HandleCommand(f"break set -a {main_address:#x}")
+self.assertTrue(target.GetNumBreakpoints() == 1)
+
+bp = target.GetBreakpointAtIndex(0)
+self.assertTrue(bp != None)
+
+_, _, thread, _ = lldbutil.run_to_breakpoint_do_run(self, target, bp)
+self.assertGreaterEqual(thread.GetNumFrames(), 1)
+
+thread_pc = thread.GetFrameAtIndex(0).GetPCAddress()
+self.assertNotEqual(thread_pc, None)
+
+self.assertEquals(main_address, thread_pc.GetFileAddress())
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := inferior.c
+
+include Makefile.rules
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -128,13 +128,15 @@
 Arg<"AddressOrExpression">, Required,
 Desc<"Set the breakpoint at the specified address.  If the address maps "
 "uniquely to a particular binary, then the address will be converted to "
-"a \"file\"address, so that the breakpoint will track that binary+offset "
+"a \"file\" address, so that the breakpoint will track that binary+offset "
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
 "Again, this will allow lldb to track that offset on subsequent reloads.  "
 "The module need not have been loaded at the time you specify this "
-"breakpoint, and will get resolved when the module is loaded.">;
+"breakpoint, and will get resolved when the module is loaded.  If no "
+"module is specified, the binary being debugged is considered as a "
+"fallback.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
 Desc<"Set the breakpoint by function name.  Can be repeated multiple times "
Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filesp

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

@JDevlieghere Thanks again for the feedback. I updated the patchset according 
to your comments. I left open one of your suggestions to get your feedback.

Thanks again!
Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Just wanted to see if there was anything else I could do to help make this 
patch acceptable, @JDevlieghere and @jingham . Thanks again for all your help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 430781.
hawkinsw added a comment.

Stupidly missed a few of the places where bare assertions were used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

Files:
  lldb/source/Breakpoint/BreakpointResolverAddress.cpp
  lldb/source/Commands/Options.td
  lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
  
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
  lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c

Index: lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/inferior.c
@@ -0,0 +1,6 @@
+int function(int a) { return a; }
+
+int main() {
+  int f = function(10);
+  return f;
+}
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
@@ -0,0 +1,36 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def get_address_from_symbol(self, symbol):
+target = lldbutil.run_to_breakpoint_make_target(self, "a.out", True)
+bp = target.BreakpointCreateByName(symbol, None)
+address = bp.GetLocationAtIndex(0).GetAddress().GetFileAddress()
+return address
+
+def test_set_address_no_module(self):
+self.build()
+
+main_address = self.get_address_from_symbol("main")
+
+target = lldbutil.run_to_breakpoint_make_target(self)
+debugger = target.GetDebugger()
+
+debugger.HandleCommand(f"break set -a {main_address:#x}")
+self.assertEquals(target.GetNumBreakpoints(), 1)
+
+bp = target.GetBreakpointAtIndex(0)
+self.assertIsNotNone(bp)
+
+_, _, thread, _ = lldbutil.run_to_breakpoint_do_run(self, target, bp)
+self.assertGreaterEqual(thread.GetNumFrames(), 1)
+
+thread_pc = thread.GetFrameAtIndex(0).GetPCAddress()
+self.assertNotEqual(thread_pc, None)
+
+self.assertEquals(main_address, thread_pc.GetFileAddress())
Index: lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/breakpoint/set/address-nomodule/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := inferior.c
+
+include Makefile.rules
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -128,13 +128,15 @@
 Arg<"AddressOrExpression">, Required,
 Desc<"Set the breakpoint at the specified address.  If the address maps "
 "uniquely to a particular binary, then the address will be converted to "
-"a \"file\"address, so that the breakpoint will track that binary+offset "
+"a \"file\" address, so that the breakpoint will track that binary+offset "
 "no matter where the binary eventually loads.  Alternately, if you also "
 "specify the module - with the -s option - then the address will be "
 "treated as a file address in that module, and resolved accordingly.  "
 "Again, this will allow lldb to track that offset on subsequent reloads.  "
 "The module need not have been loaded at the time you specify this "
-"breakpoint, and will get resolved when the module is loaded.">;
+"breakpoint, and will get resolved when the module is loaded.  If no "
+"module is specified, the binary being debugged is considered as a "
+"fallback.">;
   def breakpoint_set_name : Option<"name", "n">, Group<3>, Arg<"FunctionName">,
 Completion<"Symbol">, Required,
 Desc<"Set the breakpoint by function name.  Can be repeated multiple times "
Index: lldb/source/Breakpoint/BreakpointResolverAddress.cpp
===
--- lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ lldb/source/Breakpoint/BreakpointResolverAddress.cpp
@@ -121,16 +121,27 @@
 
   if (filter.AddressPasses(m_addr)) {
 if (breakpoint.GetNumLocations() == 0) {
-  // If the address is just an offset, and we're given a module, see if we
-  // can find the appropriate module loaded in the binary, and fix up
-  // m_addr to use that.
-  if (!m_addr.IsSectionOffset() && m_module_filespec) {
+  // If the address is just an offset ...
+  if (!m_addr.IsSectionOffset()) {
+ModuleSP containing_module_sp = nullptr;
 Target &target = breakpoint.GetTarget();
-ModuleSpec module_spec(m_module_filespec);
-

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

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

@JDevlieghere So sorry! I cannot believe I missed those. Thanks for putting up 
with me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D124731#3528269 , @JDevlieghere 
wrote:

> Please let me know if you need someone to land this on your behalf.

Unfortunately, I do! Sorry! I have not yet completed the committer process. I 
still feel too new. Thanks for the mentorship on this patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I am going to investigate why the buildbot failed. When I ran the tests locally 
everything ran fine. I am terribly, terribly sorry!

@JDevlieghere -- If you could help me debug the failure, or point me to the 
documentation on how to read buildbot output, that would be great!

Sorry again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I realize the reason for the failure. It would appear that there are tests that 
are otherwise affected by the change in the behavior introduced here. I will 
take care of updating these tests as quickly as possible. I am sorry for the 
trouble, @JDevlieghere !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw created this revision.
hawkinsw added reviewers: teemperor, JDevlieghere.
Herald added a project: All.
hawkinsw requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

After changing the "fallback" behavior when a user sets a breakpoint
without specifying a module the bad-address-breakpoint test case failed
incorrectly. This patch updates that test case in order to more
thoroughly discover an illegal address and use that as the means for
testing whether a breakpoint set at an illegal address fails to resolve.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126109

Files:
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py


Index: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,25 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
+
+
+illegal_address = 0x0
 error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+ptr = process.ReadPointerFromMemory(illegal_address, error)
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
 
 if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:


Index: lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,25 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
+
+
+illegal_address = 0x0
 error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+ptr = process.ReadPointerFromMemory(illegal_address, error)
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
 
 if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py:43
+if region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
 

DavidSpickett wrote:
> DavidSpickett wrote:
> > I'm confused as to what the logic is here now.
> > 
> > You:
> > * read from address 0x0 to fill in error
> > * walk the memory regions until the highest one, making the end of that the 
> > illegal address
> > * assume that the error value from reading 0x0 is the same as you'd get 
> > from this new illegal address
> > ...
> > 
> > Or am I missing something and the ptr was just left in accidentally.
> > 
> > (I would suggest you could jump to the last region in the list straight 
> > away but I don't think we actually require them to be sorted, plus 
> > sometimes you get multiple regions with the same base)
> > 
> (same base but different end addresses I mean)
Hello! Thank you for the review!

I was initializing `illegal_address` to 0 as a way to, well, initialize the 
value. I suppose that I could have initialized it to `None` and then used that 
a special value in the loop. In fact, now that I think about it, that's the 
tact that I will take in a second version of this patch. 

But, yes, you basically have the rest correct: I walk through all the memory 
regions and find the highest address. Then, when I have that address (which I 
know is not in a valid memory region), I will use that as the address to set 
for the illegal breakpoint.

Does that make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py:43
+if region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
 

hawkinsw wrote:
> DavidSpickett wrote:
> > DavidSpickett wrote:
> > > I'm confused as to what the logic is here now.
> > > 
> > > You:
> > > * read from address 0x0 to fill in error
> > > * walk the memory regions until the highest one, making the end of that 
> > > the illegal address
> > > * assume that the error value from reading 0x0 is the same as you'd get 
> > > from this new illegal address
> > > ...
> > > 
> > > Or am I missing something and the ptr was just left in accidentally.
> > > 
> > > (I would suggest you could jump to the last region in the list straight 
> > > away but I don't think we actually require them to be sorted, plus 
> > > sometimes you get multiple regions with the same base)
> > > 
> > (same base but different end addresses I mean)
> Hello! Thank you for the review!
> 
> I was initializing `illegal_address` to 0 as a way to, well, initialize the 
> value. I suppose that I could have initialized it to `None` and then used 
> that a special value in the loop. In fact, now that I think about it, that's 
> the tact that I will take in a second version of this patch. 
> 
> But, yes, you basically have the rest correct: I walk through all the memory 
> regions and find the highest address. Then, when I have that address (which I 
> know is not in a valid memory region), I will use that as the address to set 
> for the illegal breakpoint.
> 
> Does that make sense?
And, woah, I now realize what you meant! So sorry!

Yes, `ptr` was leftover. I am so sorry!

Standby for the next version of this patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 431427.
hawkinsw added a comment.

Updating based on DavidSpickett helpful feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

Files:
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py


Index: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,26 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
-error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
 
-if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# illegal_address will hold (optionally) an address that, if
+# used as a breakpoint, will generate an unresolved breakpoint.
+illegal_address = None
+
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
+
+if illegal_address != None:
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:


Index: lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,26 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
-error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
 
-if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# illegal_address will hold (optionally) an address that, if
+# used as a breakpoint, will generate an unresolved breakpoint.
+illegal_address = None
+
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
+
+if illegal_address != None:
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

@DavidSpickett Thank you for the review! I hope that this 2nd version of the 
patch addresses your helpful comments!

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

@DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the 
proper protocol for you to review again? I am sorry to ask but I don't know the 
right procedure and don't want to do the wrong thing!




Comment at: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py:42
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:

DavidSpickett wrote:
> General rule is to use `is None` instead of `== None`, though the result is 
> the same here.
Yes! I always make that error!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw updated this revision to Diff 432192.
hawkinsw added a comment.

Updating the revision to consider feedback (!= None vs is not None)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

Files:
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py


Index: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,26 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
-error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
 
-if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# illegal_address will hold (optionally) an address that, if
+# used as a breakpoint, will generate an unresolved breakpoint.
+illegal_address = None
+
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
+
+if illegal_address is not None:
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:


Index: lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
===
--- lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
+++ lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py
@@ -27,14 +27,26 @@
   "Set a breakpoint here",
   lldb.SBFileSpec("main.c"))
 
-# Now see if we can read from 0.  If I can't do that, I don't
-# have a good way to know what an illegal address is...
-error = lldb.SBError()
 
-ptr = process.ReadPointerFromMemory(0x0, error)
 
-if not error.Success():
-bkpt = target.BreakpointCreateByAddress(0x0)
+# illegal_address will hold (optionally) an address that, if
+# used as a breakpoint, will generate an unresolved breakpoint.
+illegal_address = None
+
+# Walk through all the memory regions in the process and
+# find an address that is invalid.
+regions = process.GetMemoryRegions()
+for region_idx in range(regions.GetSize()):
+region = lldb.SBMemoryRegionInfo()
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:
+illegal_address = region.GetRegionEnd()
+
+if illegal_address is not None:
+# Now, set a breakpoint at the address we know is illegal.
+bkpt = target.BreakpointCreateByAddress(illegal_address)
+# Verify that breakpoint is not resolved.
 for bp_loc in bkpt:
 self.assertEquals(bp_loc.IsResolved(), False)
 else:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D126109#3535931 , @JDevlieghere 
wrote:

> In D126109#3534522 , @hawkinsw 
> wrote:
>
>> @DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the 
>> proper protocol for you to review again? I am sorry to ask but I don't know 
>> the right procedure and don't want to do the wrong thing!
>
> If the patch is accepted with a comment you can land the patch with the 
> comment addressed. Unless you disagree in which case you can continue 
> discussing in the code review.

Thanks @JDevlieghere ! I have updated the patch according to his feedback and I 
am running a full test on my desktop. I think that this is ready to land but I 
still haven't gotten myself in order and requested committer access so I will 
need your help again.

Thanks again for your patience and help getting me acclimated! Contributing is 
lots of fun!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw reopened this revision.
hawkinsw added a comment.

In D124731#3530692 , @omjavaid wrote:

> I have reverted this temporarily as It broke LLDB API test 
> TestBadAddressBreakpoints.py

@omjavaid We have updated that test so that this should no longer cause a 
problem. I am sorry to say that I don't know the proper procedure for reopening 
this. Should I open a separate request? Thank you for your help!

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731

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

[Lldb-commits] [PATCH] D126240: [lldb] Tighten the scope of a couple of locks in DataFormatters.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/DataFormatters/FormatManager.cpp:595
+  {
+std::lock_guard guard(m_language_categories_mutex);
+m_language_categories_map[lang_type] =

Forgive me if I am speaking out of turn, but do we need to check again here for 
whether `lang_type` is in the map? In other words, it seems possible that (in 
some zany situation) while we are doing the memory allocation (line 593) 
someone else has come along and added that category since we released the lock 
on it in 592. 

I hope that this is helpful. Please (again) forgive me if I am speaking out of 
turn.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126240

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

[Lldb-commits] [PATCH] D126240: [lldb] Tighten the scope of a couple of locks in DataFormatters.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/DataFormatters/FormatManager.cpp:595
+  {
+std::lock_guard guard(m_language_categories_mutex);
+m_language_categories_map[lang_type] =

jgorbe wrote:
> hawkinsw wrote:
> > Forgive me if I am speaking out of turn, but do we need to check again here 
> > for whether `lang_type` is in the map? In other words, it seems possible 
> > that (in some zany situation) while we are doing the memory allocation 
> > (line 593) someone else has come along and added that category since we 
> > released the lock on it in 592. 
> > 
> > I hope that this is helpful. Please (again) forgive me if I am speaking out 
> > of turn.
> Not at all! Thank you for your comment! :)
> 
> You are absolutely right, I had overlooked this case. Another annoying 
> consequence is that in this case the racing threads would create multiple 
> `LanguageCategory` objects. The `LanguageCategory` constructor calls `Enable` 
> which in turn ends up calling the `Changed` method of the listener, so even 
> if we check the map before insertion I think we'd still see the listener 
> receiving multiple notifications that the category was enabled, which is not 
> ideal.
> 
> I don't know how to properly fix this. I'll give it some more thought.
Glad that it was helpful! I will keep an eye on this PR and be ready to help 
again if you can find a way out of that tricky conundrum!! :-)

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126240

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

[Lldb-commits] [PATCH] D126240: [lldb] Tighten the scope of a couple of locks in DataFormatters.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

In D126240#3564913 , @jgorbe wrote:

> I've been experimenting a little bit and it seems that I can avoid the 
> deadlocks I was seeing by applying only the modification to 
> `TypeCategoryMap::Add`. This would avoid the problem that @hawkinsw pointed 
> out in my change to `FormatManager::GetCategoryForLanguage`. However, the 
> following problem could still happen with the change to 
> `TypeCategoryMap::Add`:
>
> 1. Thread A acquires the lock, runs `m_map[key1] = value1`, and releases the 
> lock
> 2. Thread B acquires the lock, runs `m_map[key2] = value2`, and releases the 
> lock
> 3. Thread A runs `listener->Changed()`
> 4. Thread B runs `listener->Changed()`
>
> So we would have two changes, the listener would be called twice, but both 
> listener calls would see the same state with the two changes already applied, 
> instead of observing each change individually. I'll keep thinking to see if I 
> can fix this elsewhere.

I am *so* sorry I haven't had a chance to come back to work on this change 
until now, but I did have one thought that I will investigate this PM. I will 
keep you posted if I find anything. Again, I am sorry for the radio silence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126240

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

[Lldb-commits] [PATCH] D133045: Partial fix for handling backticks in commands and aliases

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Sorry if these comments are not helpful! Everything looks great!




Comment at: lldb/source/Interpreter/Options.cpp:1026
+} else
+  option_to_insert = CommandInterpreter::g_no_argument;
+

Question: Could we drop the final `else` if we initialized `option_to_insert` 
to `CommandInterpreter::g_no_argument`? Just curious. 



Comment at: lldb/test/API/commands/command/backticks/TestBackticksInAlias.py:16
+def test_backticks_in_alias(self):
+"""Test that an alias can contain active backtraces."""
+self.build()

Nit: Typo. backtraces => backticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133045

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

[Lldb-commits] [PATCH] D133240: [Formatters][NFCI] Replace 'is_regex' arguments with an enum.

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/include/lldb/DataFormatters/FormattersContainer.h:46
   /// just matching by comparing with m_type_name string.
-  bool m_is_regex;
+  lldb::FormatterMatchType m_match_type;
 

I am really intrigued by this entire patchset and just wanted to drop by to 
help, if I could. I hope that I am not upsetting you by commenting on something 
so trivial, but it seems like we would want to change the comment for this data 
member while we are at it?

Again, I think this work is great and I hope I am helping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133240

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

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

I hope that these comments are helpful. If they are not, please feel free to 
tell me to stop! I appreciated learning from reading through your discussion 
with @labath !




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:265
+:param dereference: A ValueCheck for the SBValue returned by the
+`Derefence` function.
 """

nit: typo `Derefence` => `Dereference`



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:19
 
-static ValueObjectSP GetCoroFramePtrFromHandle(ValueObject &valobj) {
-  ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
+static uint64_t GetCoroFramePtrFromHandle(ValueObjectSP valobj_sp) {
   if (!valobj_sp)

Do we want to return a `lldb::addr_t`? I know that they are likely the same, 
but do we want it for consistency?

I see (below) several instances where we are passing around "addresses" and 
they are plain `uint64_t` so I am obviously being unhelpful. Sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815

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

[Lldb-commits] [PATCH] D133038: Add SBDebugger::GetSetting()/GetSettings() public APIs

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py:25
+src_dir = self.getSourceDir()
+self.runCmd('settings set target.source-map . "%s"' % src_dir)
+

Sorry if this comment is not helpful, but I was wondering ...

Could we use `source_map_setting_path` here? That would make future changes 
easier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038

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

[Lldb-commits] [PATCH] D133038: Add SBDebugger::GetSetting() public API

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py:25
+src_dir = self.getSourceDir()
+self.runCmd('settings set target.source-map . "%s"' % src_dir)
+

yinghuitan wrote:
> hawkinsw wrote:
> > Sorry if this comment is not helpful, but I was wondering ...
> > 
> > Could we use `source_map_setting_path` here? That would make future changes 
> > easier?
> @hawkinsw, sorry, almost missed your comment. There is no single value for 
> source map path but a pair of original/replacement. In this case, original is 
> "." while the replacement is "src_dir" so I think src_dir makes sense here. 
No problem! I am sorry I miscommunicated! I just meant, could we write 
something like

```
self.runCmd('settings set %s . "%s"' % (source_map_setting_path, src_dir))
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038

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

[Lldb-commits] [PATCH] D132734: [lldb] Fix member access in GetExpressionPath

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/Core/ValueObject.cpp:1947
-  // name ([%d]) to the expression path
-  if (m_flags.m_is_array_item_for_pointer &&
-  epformat == eGetExpressionPathFormatHonorPointers)

I am *absolutely* not an expert here but I've spent several hours trying to 
debug why this incredibly thorough, well-written patch does not handle the case 
in `TestArray.py`. It would seem to me that this is the check that we forgot to 
bring over in to the new logic. I was attempting to be able to have an updated 
version of the patch by tonight, but didn't get further than just debugging. I 
will continue to work on it -- I am sure that you gurus have other, more 
important things to work on. 

That said, I am just trying to help so if I am totally off base, please let me 
know!

Sincerely,
Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132734

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

[Lldb-commits] [PATCH] D132734: [lldb] Fix member access in GetExpressionPath

[Will Hawkins via Phabricator via lldb-commits]

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

I updated this revision with the following change and I *think* that things are 
happy again:

  diff --git a/lldb/source/Core/ValueObject.cpp 
b/lldb/source/Core/ValueObject.cpp
  index 226a2c3f690f..1f939c6fa2cd 100644
  --- a/lldb/source/Core/ValueObject.cpp
  +++ b/lldb/source/Core/ValueObject.cpp
  @@ -2004,6 +2004,11 @@ void ValueObject::GetExpressionPath(Stream &s,
 s.PutChar(')');
 }
   
  +  if (m_flags.m_is_array_item_for_pointer &&
  +  epformat == eGetExpressionPathFormatHonorPointers) {
  +s.PutCString(m_name.GetStringRef());
  +  }
  +
 if (print_obj_access)
   s.PutChar('.');
 if (print_ptr_access)

Let me know if that is helpful.

Sincerely,
Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132734

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

[Lldb-commits] [PATCH] D136650: Add a check for TypeSystem use-after-free problems

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

There are *so* many reasons that I love this patch.

First and foremost: I am a professor of computer science at University of 
Cincinnati and teaching Programming Languages this semester and I am about to 
teach *this very technique* for protecting against use-after-free errors. 
Having seen this patch I now have a great example of where it is used in real 
life! So, thank you!

Second, I have left some comments but please take what I said with a grain of 
salt. I hope that it is helpful and not a waste of time.

Thank you!
Will




Comment at: lldb/source/Symbol/CompilerType.cpp:35
+  bool unused;
+  if (GetTypeSystemGraveyard().Lookup(m_type_system, unused)) {
+lldbassert(false && "CompilerType belongs to deleted Typesystem");

I am sorry if this is obvious, but is `CompilerType` used in a multithreaded 
environment? So, is there a possibility that we could pass the check on line 32 
but become invalid by the use of `m_type_system` here and fall victim to an 
attempt (in `Lookup`, perhaps?) to dereference a `NULL` pointer? Again, I am 
sorry if that is a stupid question!



Comment at: lldb/source/Symbol/CompilerType.cpp:36
+  if (GetTypeSystemGraveyard().Lookup(m_type_system, unused)) {
+lldbassert(false && "CompilerType belongs to deleted Typesystem");
+return false;

I know that this is a nit, but should we write `TypeSystem`?



Comment at: lldb/source/Symbol/TypeSystem.cpp:22
+  // Intentionally leaked to avoid C++ destructor ordering issue.
+  static TypeSystemSet *g_typesystem_graveyard = new TypeSystemSet();
+  return *g_typesystem_graveyard;

I spent *way* too long researching to make sure that this initialization is 
thread safe and wanted to drop a link here in case anyone is wondering the same 
thing:

[C++ standard: stmt.dcl para 3, sentence 
3](http://eel.is/c++draft/stmt.dcl#3.sentence-3)


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

https://reviews.llvm.org/D136650

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

[Lldb-commits] [PATCH] D132734: [lldb] Fix member access in GetExpressionPath

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

If you would like, I'd be happy to reopen this patch as a separate issue if 
that's the better way to handle it! Sorry for the SPAM!

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132734

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

[Lldb-commits] [PATCH] D132734: [lldb] Fix member access in GetExpressionPath

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Mr. Sabolčec,

Thank you for the kind response -- I hope my work was helpful! It was lots of 
fun to dig in to this part of the codebase!

Sincerely,
Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132734

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

[Lldb-commits] [PATCH] D136935: [lldb][CPlusPlus] Introduce CPlusPlusLanguage::MethodName::GetReturnType

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp:85
   // Function pointers
-  {"string (*f(vector&&))(float)", "", "f", "(vector&&)", "",
-   "f"},
-  {"void (*&std::_Any_data::_M_access())()", "std::_Any_data",
-   "_M_access", "()", "",
+  {"string (*f(vector&&))(float)", "string", "", "f",
+   "(vector&&)", "", "f"},

labath wrote:
> I believe the return type here should be something like `string (*)(float)` 
> (a pointer to a function that takes a float and returns a string).
I read this "nice" function pointer declaration as:

declare `f` as a function that takes a `vector` by r-value reference that, 
in turn, returns a pointer to a function taking a `float` by value that returns 
a `string`. 

Upon writing that out, I realize that I am basically saying the same thing as 
@labath. Sorry! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136935

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/API/SBTraceCursor.cpp:131
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;
+}

jj10306 wrote:
> open to suggestions on the best way to indicate that the current item doesn't 
> have a timestamp associated with it
Could we use `llvm::Optional`? Sorry if that is a silly suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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

[Lldb-commits] [PATCH] D137645: [trace] Add `SBTraceCursor::GetWallClockTime` API

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/source/API/SBTraceCursor.cpp:127-131
+double SBTraceCursor::GetWallClockTime() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime();
+  return maybe_wall_clock_time ? *maybe_wall_clock_time : -1.0;

wallace wrote:
> we don't have optionals in the SB bridge, so you could do the following
> 
>   bool SBTraceCursor::GetWallClockTime(double &time) {
> if (const auto &maybe_wall_clock_time = m_opaque_sp->GetWallClockTime()) {
>   time = *maybe_wall_clock_time;
>   return true;
> }
> return false;
>   }
Not that it matters, but *that* was going to be my other suggestion! @wallace 
is smarter than I am!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137645

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