[Lldb-commits] [lldb] abddb83 - [lldb] Fix type of --apply-fixits (NFC)

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2023-03-23T22:49:29-07:00
New Revision: abddb8359895a2040a3439850f5c8c9c61123947

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

LOG: [lldb] Fix type of --apply-fixits (NFC)

Added: 


Modified: 
lldb/source/Commands/Options.td

Removed: 




diff  --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index f11c95e5660e2..ea917f78841bb 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -371,7 +371,7 @@ let Command = "expression" in {
 Arg<"Language">, Desc<"Specifies the Language to use when parsing the "
 "expression.  If not set the target.language setting is used.">;
   def expression_options_apply_fixits : Option<"apply-fixits", "X">,
-Groups<[1,2]>, Arg<"Language">, Desc<"If true, simple fix-it hints will be 
"
+Groups<[1,2]>, Arg<"Boolean">, Desc<"If true, simple fix-it hints will be "
 "automatically applied to the expression.">;
   def expression_options_description_verbosity :
 Option<"description-verbosity", "v">, Group<1>,



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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

@jgorbe thank you for letting me know! It's not intended that the names of 
child values are not printed. I have a fix ready for review here D146783 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D146783: [lldb] Add ability to hide the root name of a value

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: aprantl, jingham, jgorbe.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When printing a value, allow the root value's name to be elided, without 
omiting the
names of child values.

At the API level, this adds `SetHideRootName()`, which joins the existing
`SetHideName()` function.

This functionality is used by `dwim-print` and `expression`.

Fixes an issue identified by @jgorbe in https://reviews.llvm.org/D145609.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146783

Files:
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py
  lldb/test/API/commands/dwim-print/main.c

Index: lldb/test/API/commands/dwim-print/main.c
===
--- lldb/test/API/commands/dwim-print/main.c
+++ lldb/test/API/commands/dwim-print/main.c
@@ -1,3 +1,10 @@
+struct Structure {
+  int number;
+};
+
 int main(int argc, char **argv) {
+  struct Structure s;
+  s.number = 30;
+  // break here
   return 0;
 }
Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -114,3 +114,11 @@
 error_msg = "error: 'dwim-print' takes a variable or expression"
 self.expect(f"dwim-print", error=True, startstr=error_msg)
 self.expect(f"dwim-print -- ", error=True, startstr=error_msg)
+
+def test_nested_values(self):
+"""Test dwim-print with nested values (structs, etc)."""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.c"))
+self.runCmd("settings set auto-one-line-summaries false")
+self._expect_cmd(f"dwim-print s", "frame variable")
+self._expect_cmd(f"dwim-print (struct Structure)s", "expression")
Index: lldb/source/DataFormatters/ValueObjectPrinter.cpp
===
--- lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -275,7 +275,7 @@
 
   StreamString varName;
 
-  if (!m_options.m_hide_name) {
+  if (ShowName()) {
 if (m_options.m_flat_output)
   m_valobj->GetExpressionPath(varName);
 else
@@ -314,7 +314,7 @@
   m_stream->Printf("(%s) ", typeName.GetData());
 if (!varName.Empty())
   m_stream->Printf("%s =", varName.GetData());
-else if (!m_options.m_hide_name)
+else if (ShowName())
   m_stream->Printf(" =");
   }
 }
@@ -437,7 +437,7 @@
 if (m_options.m_hide_pointer_value &&
 IsPointerValue(m_valobj->GetCompilerType())) {
 } else {
-  if (!m_options.m_hide_name)
+  if (ShowName())
 m_stream->PutChar(' ');
   m_stream->PutCString(m_value);
   value_printed = true;
@@ -459,7 +459,7 @@
 // let's avoid the overly verbose no description error for a nil thing
 if (m_options.m_use_objc && !IsNil() && !IsUninitialized() &&
 (!m_options.m_pointer_as_array)) {
-  if (!m_options.m_hide_value || !m_options.m_hide_name)
+  if (!m_options.m_hide_value || ShowName())
 m_stream->Printf(" ");
   const char *object_desc = nullptr;
   if (value_printed || summary_printed)
@@ -565,8 +565,14 @@
 if (ShouldPrintValueObject())
   m_stream->EOL();
   } else {
-if (ShouldPrintValueObject())
-  m_stream->PutCString(IsRef() ? ": {\n" : " {\n");
+if (ShouldPrintValueObject()) {
+  if (IsRef()) {
+m_stream->PutCString(": ");
+  } else if (ShowName()) {
+m_stream->PutChar(' ');
+  }
+  m_stream->PutCString("{\n");
+}
 m_stream->IndentMore();
   }
 }
@@ -819,3 +825,9 @@
 bool ValueObjectPrinter::HasReachedMaximumDepth() {
   return m_curr_depth >= m_options.m_max_depth;
 }
+
+bool ValueObjectPrinter::ShowName() const {
+  if (m_curr_depth == 0)
+return !m_options.m_hide_root_name && !m_options.m_hide_name;
+  return !m_options.m_hide_name;
+}
Index: lldb/source/DataFormatters/DumpValueObjectOptions.cpp
===
--- lldb/source/DataFormatters/DumpValueObjectOptions.cpp
+++ lldb/source/DataFormatters/DumpValueObjectOptions.cpp
@@ -19,10 +19,10 @@
   m_decl_printing_helper(), m_pointer_as_array(), m_use_synthetic(true),
   m_scope_already_checked(false), m_flat_output(false), m_ignore_cap(false),
   m_show_types(false), m_show_locatio

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe added a comment.

By the way, I just bisected another problem to this commit: I'm seeing in some 
cases that prettyprinted values with synthetic children don't show child names 
when using `dwim-print`. So print used to show:

  (lldb) p c
  (MyClass) $0 = MyClass object with children: {
[0] = 0x6004 "hello"
  }
  (lldb) p m
  (MyMap) $1 = MyMap object with children: {
["my_key"] = 0x6011 "my_value"
  }

but now it shows

  (lldb) p c
  (MyClass)  MyClass object with children: {
0x6004 "hello"
  }
  (lldb) p m
  (MyMap)  MyMap object with children: {
0x6011 "my_value"
  }

I don't have a repro test case to share right away but I though I'd give you a 
heads-up in case the problem is easy to find from the description. Or maybe 
it's the intended behavior? I get the same behavior with `expr 
--persistent-result off -- m` so I can't tell if it's a bug or if it's just the 
expected behavior when disabling persistent results. If it's the latter, that's 
a strong argument for me to unalias `print` and restore the old behavior in the 
lldbinit file we ship to our internal users.

I'll work on a small shareable test case tomorrow anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

mib wrote:
> kastiglione wrote:
> > mib wrote:
> > > @kastiglione may be you have a better idea how to handle `symbol + 
> > > offset` where ` + offset` might be optional.
> > symbol is always present?
> Technically, in the past the `offs` (symbol + offset) was optional:
> ```
> r'(?: +(.*))?'
> ```
> So I guess `symbol` could be missing.
Breaking it down, there is:

1. a symbol (maybe)
2. the literal text " + " followed by a number (also maybe)

I'll start start with 2:

```
 \+ (\d+)
```

For the symbol, `.*`, it should instead have at least a length of 1. I'd use 
`+` instead of `*`. And, it shouldn't be _any_ character. At the very least it 
should be non-space, which is `\S`.

To combine these at a high level it looks like:

```
(?:()(?:)?)?
```

Filling in these two with symbol='\S+' and offset=' \+ (\d+)', it becomes:

```
(?:(\S+)(?: \+ (\d+))?)?
```

Here's some python real session that minimally exercises this code:

```
% python
>>> import re
>>> pat = re.compile(r"(?:(\S+)(?: \+ (\d+))?)?")
>>> pat.match("func + 123").groups()
('func', '123')
>>> pat.match("func").groups()
('func', None)
>>> pat.match("").groups()
(None, None)
>>> 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D146779: Fix handling of backticks in CommandObjectParsed commands

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1754-1760
+if (error.Success()) {
+  command.erase(start_backtick, end_backtick - start_backtick + 1);
+  command.insert(start_backtick, std::string(expr_str));
+  pos = start_backtick + expr_str.size();
+  continue;
+} else
+  break;

You could avoid a level of indentation and a continue if you invert the 
condition. 



Comment at: lldb/source/Interpreter/CommandObject.cpp:729-738
 for (auto entry : llvm::enumerate(cmd_args.entries())) {
   if (!entry.value().ref().empty() && entry.value().GetQuoteChar() == '`') 
{
-cmd_args.ReplaceArgumentAtIndex(
-entry.index(),
-
m_interpreter.ProcessEmbeddedScriptCommands(entry.value().c_str()));
+// We have to put the backtick back in place for PreprocessCommand.
+std::string opt_string = entry.value().c_str();
+Status error;
+error = m_interpreter.PreprocessToken(opt_string);
+if (error.Success())

There's a lot of `entry.value()` repetition here, might be worth extracting a 
temporary value here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146779

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

[Lldb-commits] [PATCH] D146779: Fix handling of backticks in CommandObjectParsed commands

[Jim Ingham via Phabricator via lldb-commits]

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

A while back I reworked the way that backticks are handled to make backticks in 
aliases work, but I made two bad assumptions and broke the handling for parsed 
commands.  The bad assumptions were:

1. There was testing for backticks in regular commands (turns out the extant 
tests were all for raw commands)
2. That the extant function that was supposed to handle backtick options did 
the thing you do with backtick options

2 is me not reading closely enough.  The function called was 
m_interpreter.ProcessEmbeddedScriptCommands so I really should have seen this 
wasn't doing the right thing.  Apparently at some point we were going to add 
the ability to invoke a script command and insert its text in place of the 
option.  That's not a bad idea, but it was never actually implemented, and 
anyway, you can't use backticks as the demarcation - that's already taken for 
expression substitution.

This patch repairs that breakage, and adds some more tests for backticks in raw 
commands, in the arguments of parsed commands and in the option values for 
parsed commands.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146779

Files:
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/test/API/commands/command/backticks/TestBackticksInAlias.py

Index: lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
===
--- lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
+++ lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
@@ -29,4 +29,53 @@
 interp.HandleCommand("_test-argv-parray-cmd", result)
 self.assertFalse(result.Succeeded(), "CommandAlias::Desugar currently fails if a alias substitutes %N arguments in another alias")
 
+def test_backticks_in_parsed_cmd_argument(self):
+""" break list is a parsed command, use a variable for the breakpoint number
+and make sure that and the direct use of the ID get the same result. """
+self.build()
+target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+# Make a second breakpoint so that if the backtick part -> nothing we'll print too much:
+# It doesn't need to resolve to anything.
+dummy_bkpt = target.BreakpointCreateByName("dont_really_care_if_this_exists")
+
+bkpt_id = bkpt.GetID()
+self.runCmd(f"expr int $number = {bkpt_id}")
+direct_result = lldb.SBCommandReturnObject()
+backtick_result = lldb.SBCommandReturnObject()
+interp = self.dbg.GetCommandInterpreter()
+interp.HandleCommand(f"break list {bkpt_id}", direct_result)
+self.assertTrue(direct_result.Succeeded(), "Break list with id works")
+interp.HandleCommand("break list `$number`", backtick_result)
+self.assertTrue(direct_result.Succeeded(), "Break list with backtick works")
+self.assertEqual(direct_result.GetOutput(), backtick_result.GetOutput(), "Output is the same")
+
+def test_backticks_in_parsed_cmd_option(self):
+# The script interpreter is a raw command, so try that one:
+self.build()
+target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+
+self.runCmd(f"expr int $number = 2")
+direct_result = lldb.SBCommandReturnObject()
+backtick_result = lldb.SBCommandReturnObject()
+interp = self.dbg.GetCommandInterpreter()
+interp.HandleCommand(f"memory read --count 2 argv", direct_result)
+self.assertTrue(direct_result.Succeeded(), "memory read with direct count works")
+interp.HandleCommand("memory read --count `$number` argv", backtick_result)
+self.assertTrue(direct_result.Succeeded(), "memory read with backtick works")
+self.assertEqual(direct_result.GetOutput(), backtick_result.GetOutput(), "Output is the same")
+
+def test_backticks_in_raw_cmd(self):
+# The script interpreter is a raw command, so try that one:
+self.build()
+target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+argc_valobj = thread.frames[0].FindVariable("argc")
+self.assertTrue(argc_valobj.GetError().Success(), "Made argc valobj")
+argc_value = argc_valobj.GetValueAsUnsigned(0)
+self.assertNotEqual(argc_value, 0, "Got a value for argc")
+result = lldb.SBCommandReturnObject()
+interp = self.dbg.GetCommandInterpreter()
+interp.HandleCommand(f"script {argc_value} - `argc`", result)
+ 

[Lldb-commits] [PATCH] D146152: Add __lldb_init_module_with_target for use when sourcing modules for a target

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/bindings/python/python-wrapper.swig:1031-1048
+  // First call the target initializer:
+  if (target) {
+python_function_name_string += ".__lldb_init_module_with_target";
+python_function_name = python_function_name_string.c_str();
+
+auto pfunc = PythonObject::ResolveNameWithDictionary(
+  python_function_name, dict);

jingham wrote:
> jingham wrote:
> > bulbazord wrote:
> > > JDevlieghere wrote:
> > > > I'm surprised we might call both. The way I expected this to work is 
> > > > that if `__lldb_init_module_with_target` is defined, that's what we use 
> > > > if wee have a target and otherwise fall back to `__lldb_init_module` 
> > > > assuming it's defined.
> > > > 
> > > > What's the benefit of calling both? Do you expect the implementation to 
> > > > be different? Or do you think it's more likely that the implementations 
> > > > will be similar, just one having access to the target? 
> > > I can see the advantage to calling both: The use of 
> > > `__lldb_init_module_with_target` can be used to set up things specific to 
> > > your target and `__lldb_init_module` can be used for more general things. 
> > > That being said, you should be able to do everything with 
> > > `__lldb_init_module_with_target` since if you have a target, you should 
> > > have a debugger too.
> > > 
> > > Whatever we choose to go with, the behavior should be explicitly 
> > > documented here: https://lldb.llvm.org/use/python-reference.html 
> > > (`llvm-project/lldb/docs/use/python-reference.rst`). We already document 
> > > one, we should be documenting this one and the expected behavior when 
> > > both are present.
> > How you would use this depends on how you expect your python module to be 
> > used.  
> > 
> > 1) If your python module is only going to be sourced from a Scripting 
> > Resource in a Module, then you could just implement 
> > `__lldb_init_module_with_target`.  
> > 
> > 2) If your python module is imported through `command script import`, then 
> > it will have to implement `__lldb_init_module` since it's really never 
> > initialized in the presence of a specific target, so I wouldn't have a 
> > valid target to pass in.  I don't want to pass in something like the 
> > "currently selected target" in this case just so there's something you can 
> > get a debugger from, that kinda lying...
> > 
> > 3) It's only if you want this python module to be used in either mode, or 
> > if you need to support lldb's that have and don't have this new API, that 
> > you need to have both API's.  In that case, it seemed more useful to let 
> > the user separate the "I'm being initialized as a particular target's 
> > scripting resource" part of the initialization from the more general part.
> I will add docs once we've decided the right way to do this.
I feel like this may lead to some confusion. `__lldb_init_module_with_target` 
makes sense as the thing that runs when you source the python file from dSYM 
and `__lldb_init_module` makes sense as the thing that runs when you do 
`command script import`. Implicitly running `__lldb_init_module` when you are 
imported as a scripting resource in a module would mean there's no way to get 
different behavior depending on which thing ran. Specifically, 
`__lldb_init_module` is always guaranteed to run. If a user wanted both to run, 
I think it might be better as an explicit thing. Like 
`__lldb_init_module_with_target` can call `__lldb_init_module`... I'm not sure, 
what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146152

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

[Lldb-commits] [lldb] ccc2f36 - Android.rules: remove mips* rules

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2023-03-23T15:58:43-07:00
New Revision: ccc2f362db352df8991f493d8a05bdf99eaeea4b

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

LOG: Android.rules: remove mips* rules

They have been obsoleted for a long time and D146565 recently removed
Clang support.

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/make/Android.rules

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/make/Android.rules 
b/lldb/packages/Python/lldbsuite/test/make/Android.rules
index 32f786aa34756..cd7d8ae74d6bf 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Android.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Android.rules
@@ -24,14 +24,6 @@ else ifeq "$(ARCH)" "i386"
SYSROOT_ARCH := x86
STL_ARCH := x86
TRIPLE := i686-none-linux-android
-else ifeq "$(ARCH)" "mips64r6"
-   SYSROOT_ARCH := mips64
-   STL_ARCH := mips64
-   TRIPLE := mips64el-none-linux-android
-else ifeq "$(ARCH)" "mips32"
-   SYSROOT_ARCH := mips
-   STL_ARCH := mips
-   TRIPLE := mipsel-none-linux-android
 else
SYSROOT_ARCH := $(ARCH)
STL_ARCH := $(ARCH)



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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:540
 
+image = self.get_used_image(image_id)
 frame_offset = int(json_frame['imageOffset'])

bulbazord wrote:
> is `image` not the same as `json_image` from a few lines above this?
+1



Comment at: lldb/examples/python/crashlog.py:606-611
+if len(frame_match.groups()) == 4:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs) = frame_match.groups()
+else:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs, frame_symbol, frame_offset) = 
frame_match.groups()

bulbazord wrote:
> What is the difference between `frame_ofs` and `frame_offset`?
`frame_ofs` is actually the `frame symbol name`. I guess I should rename this 
for more clarity.

Some stack frame might have a symbol name and a offset, other might know have a 
symbol name.

Now that I'm thinking about this, I'm not sure if a crash report could be 
already symbolicated (i.e. `symbol:line:column`). In that case I think it will 
be pretty tricky to get the symbol address from the source info. 



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

kastiglione wrote:
> mib wrote:
> > @kastiglione may be you have a better idea how to handle `symbol + offset` 
> > where ` + offset` might be optional.
> symbol is always present?
Technically, in the past the `offs` (symbol + offset) was optional:
```
r'(?: +(.*))?'
```
So I guess `symbol` could be missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Jez Ng via Phabricator via lldb-commits]

int3 added a comment.

I'm trying to add similar support to lld-macho, hence the question :)




Comment at: lld/COFF/LTO.cpp:183
+  [&](size_t task, const Twine &moduleName) {
+buf[task].first = moduleName.str();
 return std::make_unique(

Any reason why this doesn't instead store the module name in the 
`file_names[task]` vector that the cache callback uses? Are the task IDs not 
unique across each run of `ltoObj`, regardless of whether we end up using the 
cache or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/examples/python/crashlog.py:540
 
+image = self.get_used_image(image_id)
 frame_offset = int(json_frame['imageOffset'])

is `image` not the same as `json_image` from a few lines above this?



Comment at: lldb/examples/python/crashlog.py:552-557
+self.symbol_data[image_uuid]["symbols"].append({
+"name": json_frame['symbol'],
+"type": "code",
+"size": 0,
+"address": pc,
+})

mib wrote:
> Same problem here I guess: IIUC if `pc = image['base'] + frame_offset`, then 
> I believe `frame_offset` is not the address of the beginning of the function, 
> but rather an address in the middle of the function (either at a callsite, or 
> at the crash site).
Yeah so `pc` is definitely not the right address of the symbol. It should be 
`pc - frame_offset` right?



Comment at: lldb/examples/python/crashlog.py:606-611
+if len(frame_match.groups()) == 4:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs) = frame_match.groups()
+else:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs, frame_symbol, frame_offset) = 
frame_match.groups()

What is the difference between `frame_ofs` and `frame_offset`?



Comment at: lldb/examples/python/crashlog.py:1175-1176
+if crashlog_parser.__class__ == JSONCrashLogParser:
+u = uuid.UUID(image)
+module_spec.SetUUIDFromString(u.hex)
+else:

Are these indented too much?



Comment at: lldb/examples/python/crashlog.py:1194-1195
+if not result.Succeeded():
+raise InteractiveCrashLogException("couldn't import crash report \
+   inlined symbols for %s (%s)" %
+   (module.file.basename,

Won't this string look like `"couldn't import crash report  
  inlined symbols for %s (%s)"`? You could probably 
just do string concatenation instead. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

mib wrote:
> @kastiglione may be you have a better idea how to handle `symbol + offset` 
> where ` + offset` might be optional.
symbol is always present?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:552-557
+self.symbol_data[image_uuid]["symbols"].append({
+"name": json_frame['symbol'],
+"type": "code",
+"size": 0,
+"address": pc,
+})

Same problem here I guess: IIUC if `pc = image['base'] + frame_offset`, then I 
believe `frame_offset` is not the address of the beginning of the function, but 
rather an address in the middle of the function (either at a callsite, or at 
the crash site).



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

@kastiglione may be you have a better idea how to handle `symbol + offset` 
where ` + offset` might be optional.



Comment at: lldb/examples/python/crashlog.py:880
+"size": 0,
+"address": frame_addr,
+})

I'm not sure if I should subtract the offset from the `frame_addr` so the 
symbol is at the right address.

Currently, the symbol name is `symbol + offset`, so I didn't have to do 
anything subtraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Med Ismail Bennani via Phabricator via lldb-commits]

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

Sometimes, crash reports come with inlined symbols. These provide the
exact stacktrace from the user binary.

However, when investigating a crash, it's very likely that the images related
to the crashed thread are not available on the debugging user system or
that the versions don't match. This causes interactive crashlog to show
a degraded backtrace in lldb.

This patch aims to address that issue, by parsing the inlined symbols
from the crash report and load them into lldb's target.

To do, we rely on the new SymbolFileJSON plugin. This patch also changes
a few things:

- Add `SBModuleSpec::SetUUIDFromString` with a typemap.
- Add an overload to `SBTarget::FindModule(const SBModuleSpec&)`.
- Update the thread parsing method in the JSONCrashLogParser to extract the 
symbol name and address for each stack frame.
- Update the thread and image parsing methods in the TextCrashLogParser to 
refine the regural expressions. We now have new capture group for the symbol 
name and the offset.

So now, when parsing the crash report, we build a data structure
containing all the symbol information for each stackframe. Then, after
launching the scripted process for interactive mode, we write a JSON
symbol file for each module, only containing the symbols that it contains.

Finally, we load the json symbol file into lldb, before showing the user
the process status and backtrace.

rdar://97345586

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146765

Files:
  lldb/bindings/python/python-typemaps.swig
  lldb/examples/python/crashlog.py
  lldb/include/lldb/API/SBModuleSpec.h
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBModuleSpec.cpp
  lldb/source/API/SBTarget.cpp

Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -1555,6 +1555,19 @@
   return sb_module;
 }
 
+SBModule SBTarget::FindModule(const SBModuleSpec &sb_module_spec) {
+  LLDB_INSTRUMENT_VA(this, sb_module_spec);
+
+  SBModule sb_module;
+  TargetSP target_sp(GetSP());
+  if (target_sp && sb_module_spec.IsValid()) {
+// The module list is thread safe, no need to lock
+sb_module.SetSP(
+target_sp->GetImages().FindFirstModule(*sb_module_spec.m_opaque_up));
+  }
+  return sb_module;
+}
+
 SBSymbolContextList SBTarget::FindCompileUnits(const SBFileSpec &sb_file_spec) {
   LLDB_INSTRUMENT_VA(this, sb_file_spec);
 
Index: lldb/source/API/SBModuleSpec.cpp
===
--- lldb/source/API/SBModuleSpec.cpp
+++ lldb/source/API/SBModuleSpec.cpp
@@ -132,6 +132,12 @@
   return m_opaque_up->GetUUID().GetBytes().size();
 }
 
+bool SBModuleSpec::SetUUIDFromString(const char *uuid, size_t uuid_len) {
+  LLDB_INSTRUMENT_VA(this, uuid, uuid_len)
+  m_opaque_up->GetUUID().SetFromStringRef(llvm::StringRef(uuid, uuid_len));
+  return m_opaque_up->GetUUID().IsValid();
+}
+
 bool SBModuleSpec::SetUUIDBytes(const uint8_t *uuid, size_t uuid_len) {
   LLDB_INSTRUMENT_VA(this, uuid, uuid_len)
   m_opaque_up->GetUUID() = UUID(uuid, uuid_len);
Index: lldb/include/lldb/API/SBTarget.h
===
--- lldb/include/lldb/API/SBTarget.h
+++ lldb/include/lldb/API/SBTarget.h
@@ -305,6 +305,8 @@
 
   lldb::SBModule FindModule(const lldb::SBFileSpec &file_spec);
 
+  lldb::SBModule FindModule(const lldb::SBModuleSpec &module_spec);
+
   /// Find compile units related to *this target and passed source
   /// file.
   ///
Index: lldb/include/lldb/API/SBModuleSpec.h
===
--- lldb/include/lldb/API/SBModuleSpec.h
+++ lldb/include/lldb/API/SBModuleSpec.h
@@ -75,6 +75,8 @@
 
   size_t GetUUIDLength();
 
+  bool SetUUIDFromString(const char *uuid, size_t uuid_len);
+
   bool SetUUIDBytes(const uint8_t *uuid, size_t uuid_len);
 
   bool GetDescription(lldb::SBStream &description);
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -40,6 +40,7 @@
 import string
 import subprocess
 import sys
+import tempfile
 import threading
 import time
 import uuid
@@ -431,6 +432,7 @@
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.symbol_data = {}
 
 @abc.abstractmethod
 def parse(self):
@@ -535,9 +537,24 @@
 if ident not in self.crashlog.idents:
 self.crashlog.idents.append(ident)
 
+image = self.get_used_image(image_id)
 frame_offset 

[Lldb-commits] [lldb] 088da8a - [lldb][NFC] makeArrayRef -> ArrayRef

[Arthur Eubanks via lldb-commits]

Author: Arthur Eubanks
Date: 2023-03-23T14:05:06-07:00
New Revision: 088da8a0e57a461f3be4b554f28c4419418c097c

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

LOG: [lldb][NFC] makeArrayRef -> ArrayRef

makeArrayRef is deprecated.

Added: 


Modified: 
lldb/source/Commands/CommandOptionsProcessAttach.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandOptionsProcessAttach.cpp 
b/lldb/source/Commands/CommandOptionsProcessAttach.cpp
index f9bd92938fa1c..d3d864dfe0255 100644
--- a/lldb/source/Commands/CommandOptionsProcessAttach.cpp
+++ b/lldb/source/Commands/CommandOptionsProcessAttach.cpp
@@ -72,5 +72,5 @@ Status CommandOptionsProcessAttach::SetOptionValue(
 }
 
 llvm::ArrayRef CommandOptionsProcessAttach::GetDefinitions() 
{
-  return llvm::makeArrayRef(g_process_attach_options);
+  return llvm::ArrayRef(g_process_attach_options);
 }



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

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd557384b43d3: [LLDB] Fix for D139955 Summary: (authored by 
ayermolo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s

Index: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
@@ -0,0 +1,317 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: lldb-test symbols %t &> %t.txt
+# RUN: cat %t.txt | FileCheck %s
+
+# Tests that error is printed correctly when DW_AT_low_pc value is
+# greater then a range entry.
+
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error message
+
+
+
+# Test was manually modified to change DW_TAG_lexical_block
+# to use DW_AT_ranges, and value lower then DW_AT_low_pc value
+# in DW_TAG_subprogram
+# static int foo(bool b) {
+#   if (b) {
+#int food = 1;
+# return food;
+#   }
+#   return 0;
+# }
+# int main() {
+#   return foo(true);
+# }
+	.text
+	.file	"main.cpp"
+	.section	.text.main,"ax",@progbits
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "base-lower-then-range-entry" "main.cpp"
+	.loc	1 8 0   # main.cpp:8:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	subq	$16, %rsp
+	movl	$0, -4(%rbp)
+.Ltmp0:
+	.loc	1 9 10 prologue_end # main.cpp:9:10
+	movl	$1, %edi
+	callq	_ZL3foob
+	.loc	1 9 3 epilogue_begin is_stmt 0  # main.cpp:9:3
+	addq	$16, %rsp
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.text._ZL3foob,"ax",@progbits
+	.p2align	4, 0x90 # -- Begin function _ZL3foob
+	.type	_ZL3foob,@function
+_ZL3foob:   # @_ZL3foob
+.Lfunc_begin1:
+	.loc	1 1 0 is_stmt 1 # main.cpp:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	movb	%dil, %al
+	andb	$1, %al
+	movb	%al, -5(%rbp)
+.Ltmp2:
+	.loc	1 2 7 prologue_end  # main.cpp:2:7
+	testb	$1, -5(%rbp)
+	je	.LBB1_2
+# %bb.1:# %if.then
+.Ltmp3:
+	.loc	1 3 8   # main.cpp:3:8
+	movl	$1, -12(%rbp)
+	.loc	1 4 12  # main.cpp:4:12
+	movl	-12(%rbp), %eax
+	.loc	1 4 5 is_stmt 0 # main.cpp:4:5
+	movl	%eax, -4(%rbp)
+	jmp	.LBB1_3
+.Ltmp4:
+.LBB1_2:# %if.end
+	.loc	1 6 3 is_stmt 1 # main.cpp:6:3
+	movl	$0, -4(%rbp)
+.LBB1_3:# %return
+	.loc	1 7 1   # main.cpp:7:1
+	movl	-4(%rbp), %eax
+	.loc	1 7 1 epilogue_begin is_stmt 0  # main.cpp:7:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp5:
+.Lfunc_end1:
+	.size	_ZL3foob, .Lfunc_end1-_ZL3foob
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	0   

[Lldb-commits] [lldb] d557384 - [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via lldb-commits]

Author: Alexander Yermolovich
Date: 2023-03-23T14:03:42-07:00
New Revision: d557384b43d32700ed09b08564a4f7823061d999

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

LOG: [LLDB] Fix for D139955 Summary:

Fixing a small typo.

Reviewed By: clayborg

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

Added: 
lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 99a0152eaf6e6..c6873a5b7a09a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1319,7 +1319,7 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(
  range.GetByteSize()));
   else {
 GetObjectFile()->GetModule()->ReportError(
-"{0x:+8}: adding range [{1:x16}-{2:x16}) which has a base "
+"{0:x8}: adding range [{1:x16}-{2:x16}) which has a base "
 "that is less than the function's low PC {3:x16}. Please file "
 "a bug and attach the file at the "
 "start of this error message",

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s 
b/lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
new file mode 100644
index 0..e3cc84db12652
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
@@ -0,0 +1,317 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: lldb-test symbols %t &> %t.txt
+# RUN: cat %t.txt | FileCheck %s
+
+# Tests that error is printed correctly when DW_AT_low_pc value is
+# greater then a range entry.
+
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 
0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error 
message
+
+
+
+# Test was manually modified to change DW_TAG_lexical_block
+# to use DW_AT_ranges, and value lower then DW_AT_low_pc value
+# in DW_TAG_subprogram
+# static int foo(bool b) {
+#   if (b) {
+#int food = 1;
+# return food;
+#   }
+#   return 0;
+# }
+# int main() {
+#   return foo(true);
+# }
+   .text
+   .file   "main.cpp"
+   .section.text.main,"ax",@progbits
+   .globl  main# -- Begin function main
+   .p2align4, 0x90
+   .type   main,@function
+main:   # @main
+.Lfunc_begin0:
+   .file   1 "base-lower-then-range-entry" "main.cpp"
+   .loc1 8 0   # main.cpp:8:0
+   .cfi_startproc
+# %bb.0:# %entry
+   pushq   %rbp
+   .cfi_def_cfa_offset 16
+   .cfi_offset %rbp, -16
+   movq%rsp, %rbp
+   .cfi_def_cfa_register %rbp
+   subq$16, %rsp
+   movl$0, -4(%rbp)
+.Ltmp0:
+   .loc1 9 10 prologue_end # main.cpp:9:10
+   movl$1, %edi
+   callq   _ZL3foob
+   .loc1 9 3 epilogue_begin is_stmt 0  # main.cpp:9:3
+   addq$16, %rsp
+   popq%rbp
+   .cfi_def_cfa %rsp, 8
+   retq
+.Ltmp1:
+.Lfunc_end0:
+   .size   main, .Lfunc_end0-main
+   .cfi_endproc
+# -- End function
+   .section.text._ZL3foob,"ax",@progbits
+   .p2align4, 0x90 # -- Begin function 
_ZL3foob
+   .type   _ZL3foob,@function
+_ZL3foob:   # @_ZL3foob
+.Lfunc_begin1:
+   .loc1 1 0 is_stmt 1 # main.cpp:1:0
+   .cfi_startproc
+# %bb.0:# %entry
+   pushq   %rbp
+   .cfi_def_cfa_offset 16
+   .cfi_offset %rbp, -16
+   movq%rsp, %rbp
+   .cfi_def_cfa_register %rbp
+   movb%dil, %al
+   andb$1, %al
+   movb%al, -5(%rbp)
+.Ltmp2:
+   .loc1 2 7 prologue_end  # main.cpp:2:7
+   testb   $1, -5(%rbp)
+   je  .LBB1_2
+# %bb.1:# %if.then
+.Ltmp3:
+   .loc1 3 8   # main.cpp:3:8
+   movl$1, -12(%rbp)
+   .loc1 4 12  # main.cpp:4:12
+   movl-12(%rbp), %eax
+   .loc1 4 5 is_stmt 0 # main.cpp:4:5
+   movl%eax, -4(%rbp)
+   jmp .LBB1_3
+.Ltmp4:
+.LBB1_2:# %if.end
+   .loc1 6 3 is_stmt 1 # main.cpp:6:3
+   movl$0, -4(%rbp)
+.LB

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo updated this revision to Diff 507855.
ayermolo added a comment.

fixed up refs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s

Index: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
@@ -0,0 +1,317 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: lldb-test symbols %t &> %t.txt
+# RUN: cat %t.txt | FileCheck %s
+
+# Tests that error is printed correctly when DW_AT_low_pc value is
+# greater then a range entry.
+
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error message
+
+
+
+# Test was manually modified to change DW_TAG_lexical_block
+# to use DW_AT_ranges, and value lower then DW_AT_low_pc value
+# in DW_TAG_subprogram
+# static int foo(bool b) {
+#   if (b) {
+#int food = 1;
+# return food;
+#   }
+#   return 0;
+# }
+# int main() {
+#   return foo(true);
+# }
+	.text
+	.file	"main.cpp"
+	.section	.text.main,"ax",@progbits
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "base-lower-then-range-entry" "main.cpp"
+	.loc	1 8 0   # main.cpp:8:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	subq	$16, %rsp
+	movl	$0, -4(%rbp)
+.Ltmp0:
+	.loc	1 9 10 prologue_end # main.cpp:9:10
+	movl	$1, %edi
+	callq	_ZL3foob
+	.loc	1 9 3 epilogue_begin is_stmt 0  # main.cpp:9:3
+	addq	$16, %rsp
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.text._ZL3foob,"ax",@progbits
+	.p2align	4, 0x90 # -- Begin function _ZL3foob
+	.type	_ZL3foob,@function
+_ZL3foob:   # @_ZL3foob
+.Lfunc_begin1:
+	.loc	1 1 0 is_stmt 1 # main.cpp:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	movb	%dil, %al
+	andb	$1, %al
+	movb	%al, -5(%rbp)
+.Ltmp2:
+	.loc	1 2 7 prologue_end  # main.cpp:2:7
+	testb	$1, -5(%rbp)
+	je	.LBB1_2
+# %bb.1:# %if.then
+.Ltmp3:
+	.loc	1 3 8   # main.cpp:3:8
+	movl	$1, -12(%rbp)
+	.loc	1 4 12  # main.cpp:4:12
+	movl	-12(%rbp), %eax
+	.loc	1 4 5 is_stmt 0 # main.cpp:4:5
+	movl	%eax, -4(%rbp)
+	jmp	.LBB1_3
+.Ltmp4:
+.LBB1_2:# %if.end
+	.loc	1 6 3 is_stmt 1 # main.cpp:6:3
+	movl	$0, -4(%rbp)
+.LBB1_3:# %return
+	.loc	1 7 1   # main.cpp:7:1
+	movl	-4(%rbp), %eax
+	.loc	1 7 1 epilogue_begin is_stmt 0  # main.cpp:7:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp5:
+.Lfunc_end1:
+	.size	_ZL3foob, .Lfunc_end1-_ZL3foob
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	0   # DW_CHILDREN_no
+	.byte	17

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo added a comment.

In D146659#4217557 , @clayborg wrote:

> Must be relocations causing the stuff to not match up to the llvm-dwarfdump 
> output.

Looked through llvm-dwarfdump code. Yep. since relocation exists, it just 
returns an addend which is 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

Is there any more feedback on this patch? After all, this adds test coverage 
for existing functionality (with the new GNUstep runtime plugin from D146154 
 that is ready to land).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146058

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

[Lldb-commits] [lldb] 0c5cee7 - [lldb-server] Use Platform plugin corresponding to the host

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-03-23T12:59:25-07:00
New Revision: 0c5cee779929f840f4f286c5894a01f583ee7b4a

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

LOG: [lldb-server] Use Platform plugin corresponding to the host

In ee232506b870ce5282cc4da5ca493d41d361feb3 I moved UnixSignal
initialization from lldbTarget to the various platform plugins. This
inadvertently broke lldb-server because lldb-server doesn't use
Platform plugins. lldb-server still needs to be able to create a
UnixSignals object for the host platform so we can add the relevant
platform plugin to lldb-server to make sure we always have a
HostPlatform.

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

Added: 


Modified: 
lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/SystemInitializerLLGS.cpp

Removed: 




diff  --git 
a/lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py 
b/lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
index b63a09d047024..172c00eb59dc2 100644
--- a/lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
+++ b/lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
@@ -63,7 +63,9 @@ def inferior_crashing(self):
 # The exact stop reason depends on the platform
 if self.platformIsDarwin():
 stop_reason = 'stop reason = EXC_BAD_ACCESS'
-elif self.getPlatform() == "linux" or self.getPlatform() == "freebsd":
+elif self.getPlatform() == "linux":
+stop_reason = 'stop reason = signal SIGSEGV: address not mapped to 
object'
+elif self.getPlatform() == "freebsd":
 stop_reason = 'stop reason = signal SIGSEGV'
 else:
 stop_reason = 'stop reason = invalid address'

diff  --git a/lldb/tools/lldb-server/CMakeLists.txt 
b/lldb/tools/lldb-server/CMakeLists.txt
index 67103e87a1d4a..56da4c8b56807 100644
--- a/lldb/tools/lldb-server/CMakeLists.txt
+++ b/lldb/tools/lldb-server/CMakeLists.txt
@@ -7,20 +7,29 @@ set(LLDB_PLUGINS)
 
 if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND LLDB_PLUGINS lldbPluginProcessLinux)
+  if (CMAKE_SYSTEM_NAME MATCHES "Linux")
+list(APPEND LLDB_PLUGINS lldbPluginPlatformLinux)
+  else()
+list(APPEND LLDB_PLUGINS lldbPluginPlatformAndroid)
+  endif()
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessFreeBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformFreeBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessNetBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformNetBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileMachO)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformMacOSX)
 elseif(CMAKE_SYSTEM_NAME MATCHES "Windows")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFilePECOFF)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformWindows)
 else()
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileELF)
 endif()

diff  --git a/lldb/tools/lldb-server/SystemInitializerLLGS.cpp 
b/lldb/tools/lldb-server/SystemInitializerLLGS.cpp
index 4233252a84dfc..1909ea4dc7984 100644
--- a/lldb/tools/lldb-server/SystemInitializerLLGS.cpp
+++ b/lldb/tools/lldb-server/SystemInitializerLLGS.cpp
@@ -11,12 +11,29 @@
 #if defined(__APPLE__)
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 using HostObjectFile = ObjectFileMachO;
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+using HostPlatform = lldb_private::PlatformMacOSX;
 #elif defined(_WIN32)
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
 using HostObjectFile = ObjectFilePECOFF;
+#include "Plugins/Platform/Windows/PlatformWindows.h"
+using HostPlatform = lldb_private::PlatformWindows;
 #else
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 using HostObjectFile = ObjectFileELF;
+#if defined(__ANDROID__)
+#include "Plugins/Platform/Android/PlatformAndroid.h"
+using HostPlatform = lldb_private::platform_android::PlatformAndroid;
+#elif defined(__FreeBSD__)
+#include "Plugins/Platform/FreeBSD/PlatformFreeBSD.h"
+using HostPlatform = lldb_private::platform_freebsd::PlatformFreeBSD;
+#elif defined(__linux__)
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+using HostPlatform = lldb_private::platform_linux::PlatformLinux;
+#elif defined(__NetBSD__)
+#include "Plugins/Platform/NetBSD/PlatformNetBSD.h"
+using HostPlatform = lldb_private::platform_netbsd::PlatformNetBSD;
+#endif
 #endif
 
 #if defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64)
@@ -58,6 +75,7 @@ llvm::Error SystemInitializerLLGS::Initialize() {
 return e;
 
   HostObjectFile::Initialize();
+  HostPlatform::Initialize();
 
 #if 

[Lldb-commits] [PATCH] D146668: [lldb-server] Use Platform plugin corresponding to the host

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c5cee779929: [lldb-server] Use Platform plugin 
corresponding to the host (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146668

Files:
  lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-server/SystemInitializerLLGS.cpp


Index: lldb/tools/lldb-server/SystemInitializerLLGS.cpp
===
--- lldb/tools/lldb-server/SystemInitializerLLGS.cpp
+++ lldb/tools/lldb-server/SystemInitializerLLGS.cpp
@@ -11,12 +11,29 @@
 #if defined(__APPLE__)
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 using HostObjectFile = ObjectFileMachO;
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+using HostPlatform = lldb_private::PlatformMacOSX;
 #elif defined(_WIN32)
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
 using HostObjectFile = ObjectFilePECOFF;
+#include "Plugins/Platform/Windows/PlatformWindows.h"
+using HostPlatform = lldb_private::PlatformWindows;
 #else
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 using HostObjectFile = ObjectFileELF;
+#if defined(__ANDROID__)
+#include "Plugins/Platform/Android/PlatformAndroid.h"
+using HostPlatform = lldb_private::platform_android::PlatformAndroid;
+#elif defined(__FreeBSD__)
+#include "Plugins/Platform/FreeBSD/PlatformFreeBSD.h"
+using HostPlatform = lldb_private::platform_freebsd::PlatformFreeBSD;
+#elif defined(__linux__)
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+using HostPlatform = lldb_private::platform_linux::PlatformLinux;
+#elif defined(__NetBSD__)
+#include "Plugins/Platform/NetBSD/PlatformNetBSD.h"
+using HostPlatform = lldb_private::platform_netbsd::PlatformNetBSD;
+#endif
 #endif
 
 #if defined(__arm64__) || defined(__aarch64__) || defined(_M_ARM64)
@@ -58,6 +75,7 @@
 return e;
 
   HostObjectFile::Initialize();
+  HostPlatform::Initialize();
 
 #if defined(LLDB_TARGET_ARM) || defined(LLDB_TARGET_ARM64)
   EmulateInstructionARM::Initialize();
@@ -80,6 +98,7 @@
 
 void SystemInitializerLLGS::Terminate() {
   HostObjectFile::Terminate();
+  HostPlatform::Terminate();
 
 #if defined(LLDB_TARGET_ARM) || defined(LLDB_TARGET_ARM64)
   EmulateInstructionARM::Terminate();
Index: lldb/tools/lldb-server/CMakeLists.txt
===
--- lldb/tools/lldb-server/CMakeLists.txt
+++ lldb/tools/lldb-server/CMakeLists.txt
@@ -7,20 +7,29 @@
 
 if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND LLDB_PLUGINS lldbPluginProcessLinux)
+  if (CMAKE_SYSTEM_NAME MATCHES "Linux")
+list(APPEND LLDB_PLUGINS lldbPluginPlatformLinux)
+  else()
+list(APPEND LLDB_PLUGINS lldbPluginPlatformAndroid)
+  endif()
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessFreeBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformFreeBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessNetBSD)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformNetBSD)
 endif()
 
 if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileMachO)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformMacOSX)
 elseif(CMAKE_SYSTEM_NAME MATCHES "Windows")
   list(APPEND LLDB_PLUGINS lldbPluginObjectFilePECOFF)
+  list(APPEND LLDB_PLUGINS lldbPluginPlatformWindows)
 else()
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileELF)
 endif()
Index: lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
===
--- lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
+++ lldb/test/API/functionalities/inferior-crashing/TestInferiorCrashing.py
@@ -63,7 +63,9 @@
 # The exact stop reason depends on the platform
 if self.platformIsDarwin():
 stop_reason = 'stop reason = EXC_BAD_ACCESS'
-elif self.getPlatform() == "linux" or self.getPlatform() == "freebsd":
+elif self.getPlatform() == "linux":
+stop_reason = 'stop reason = signal SIGSEGV: address not mapped to 
object'
+elif self.getPlatform() == "freebsd":
 stop_reason = 'stop reason = signal SIGSEGV'
 else:
 stop_reason = 'stop reason = invalid address'


Index: lldb/tools/lldb-server/SystemInitializerLLGS.cpp
===
--- lldb/tools/lldb-server/SystemInitializerLLGS.cpp
+++ lldb/tools/lldb-server/SystemInitializerLLGS.cpp
@@ -11,12 +11,29 @@
 #if defined(__APPLE__)
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 using HostObjectFile = ObjectFileMachO;
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+using HostPlatform = lldb_private::PlatformMacOSX;
 #elif defined(_WIN32)
 #include "Plugi

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Greg Clayton via Phabricator via lldb-commits]

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

Must be relocations causing the stuff to not match up to the llvm-dwarfdump 
output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe added a comment.

> Are you using persistent results? If not, how much effort is it to either 1) 
> change the tools/code that examine the output to not look for $\d+, or 2) use 
> a custom print/p alias? Honest question.

I'm not using them myself, and it's not much effort to fix the problems I've 
seen because of this. It just seemed weird to have a user-facing behavior 
change like this being committed without a rationale given.

I assumed switching to `dwim-print` would be transparent for users, and I'm not 
sure how a user that relies on these convenience variables is supposed to 
discover what happened when `print` stops producing them.

> I'd be curious to learn about some example you have in mind here.

We have a python test harness that drives a debugger session that we mainly use 
for prettyprinter tests. Then we have tests that look like:

  self.ContinueTo('StopHereForDebugger')
  self.TestPrintOutputMatches('my_variable', 'Cord of length 4: "foo\\n"')

when I ported this from gdb I couldn't find an equivalent to gdb's `output` 
command (that prints just the value), so I reached for `print` which printed 
`(Type) $0 = value` instead, and kept everything after the ' = '. It wasn't 
even using the persistent values, but the change in the output format broke it.

I also found a failure in a one-off test that worked as a minimal sanity check 
for debug builds: build a debug binary, then run the debugger on it and try to 
print some variables in order to verify that debug builds do have debug info 
that the debugger can use (i.e. debug info is not completely missing, or 
corrupted). This is a shell script that basically does:

  OUTPUT=$("$LLDB" -b -s "$SCRIPT_FILE" "$TEST_PROGRAM")

and then used the conveniently sequentially-numbered persistent results to grep 
the output:

  # Check that `argc` is 1 and has type `int`.
  (echo "$OUTPUT" | grep "(int).*[$]0 = 1") || die

I know they're not the most robust tests in the world, and as I said, it was 
not much effort to fix (they're already fixed!). But it still feels like a 
user-facing regression, and the consistency argument can go both ways: you 
could similarly argue that `dwim-print` should put the result of `frame 
variable` into a convenience variable to keep it consistent with the cases 
where the expression evaluator is used. So the real argument is that most users 
don't need it and it shouldn't be enabled by default. Which is a fine argument 
to make (although it's sad that we don't have data to support it one way or the 
other), but I had to ask to know because the commit description didn't mention 
any reasons at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:11
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 
0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error 
message

clayborg wrote:
> clayborg wrote:
> > Shouldn't the low PC of the function be zero?
> ```
> 0x0043:   DW_TAG_subprogram
> DW_AT_low_pc  (0x0001)
> DW_AT_high_pc (0x002e)
> DW_AT_frame_base  (DW_OP_reg6 RBP)
> DW_AT_linkage_name("_ZL3foob")
> DW_AT_name("foo")
> DW_AT_decl_file   
> ("base-lower-then-range-entry/base-lower-then-range-entry/main.cpp")
> DW_AT_decl_line   (1)
> DW_AT_type(0x008b)
> 
> 0x0060: DW_TAG_formal_parameter
>   DW_AT_location  (DW_OP_fbreg -5)
>   DW_AT_name  ("b")
>   DW_AT_decl_file 
> ("base-lower-then-range-entry/base-lower-then-range-entry/main.cpp")
>   DW_AT_decl_line (1)
>   DW_AT_type  (0x0092)
> 
> 0x006e: DW_TAG_lexical_block
>   DW_AT_ranges(0x
>  [0x, 0x001f)
>  [0x, 0x002d))
> ```
> 
> Seems like we have a bug in the code that prints the error message?
Stepped through the code 0x21 is what gets extracted in 
DWARFDebugInfoEntry::GetDIENamesAndRanges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D146679: [lldb] Add support for the DW_AT_trampoline attribute with mangled names

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:253
 
+  bool GetEnableTrampolineSupport() const;
+

JDevlieghere wrote:
> What does trampoline "support" mean? Could this be named something more 
> descriptive? From the description of the patch it sounds like this guards 
> whether you step through trampolines, so maybe something like 
> `GetEnableStepThroughTrampolines` or something? 
Yeah I agree the name is pretty generic. This guards both stepping and setting 
breakpoints on trampolines (by name). We could potentially have two different 
settings (one for stepping on trampolines, one for breakpoints), but at the 
same time I don't think there are many users who would want to change only of 
or the other. I'll add a doxygen comment here with the explanation of what this 
does, but I'd be happy to change the setting name if anyone has a better 
suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146679

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

[Lldb-commits] [PATCH] D146679: [lldb] Add support for the DW_AT_trampoline attribute with mangled names

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 updated this revision to Diff 507841.
augusto2112 marked 14 inline comments as done.
augusto2112 added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146679

Files:
  lldb/include/lldb/Symbol/Function.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/ThreadPlanRunToAddress.h
  lldb/include/lldb/Target/ThreadPlanStepThrough.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Symbol/Function.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td
  lldb/source/Target/ThreadPlanRunToAddress.cpp
  lldb/source/Target/ThreadPlanShouldStopHere.cpp
  lldb/source/Target/ThreadPlanStepThrough.cpp
  lldb/test/API/lang/c/trampoline_stepping/Makefile
  lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
  lldb/test/API/lang/c/trampoline_stepping/main.c

Index: lldb/test/API/lang/c/trampoline_stepping/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/main.c
@@ -0,0 +1,41 @@
+void foo(void) {}
+
+__attribute__((__trampoline__("foo")))
+void bar(void) {
+  foo();
+}
+
+__attribute__((__trampoline__("bar")))
+void baz(void) {
+  bar();
+}
+
+__attribute__((__trampoline__("bar")))
+void doesnt_call_trampoline(void) {
+  int a = 2;
+  int b = 3;
+  int c = a + b;
+}
+
+
+void direct_trampoline_call(void) {
+  bar(); // Break here for direct 
+  bar();
+}
+
+void chained_trampoline_call(void) {
+  baz(); // Break here for chained
+  baz();
+}
+
+void unused_target(void) {
+  doesnt_call_trampoline(); // Break here for unused
+}
+
+int main(void) {
+  direct_trampoline_call();
+  chained_trampoline_call();
+  unused_target();
+  return 0;
+}
+
Index: lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
===
--- /dev/null
+++ lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
@@ -0,0 +1,80 @@
+"""Test that stepping in/out of trampolines works as expected.
+"""
+
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestTrampoline(TestBase):
+def setup(self, bkpt_str):
+self.build()
+
+_, _, thread, _ = lldbutil.run_to_source_breakpoint(
+self, bkpt_str, lldb.SBFileSpec('main.c'))
+return thread
+
+def test_direct_call(self):
+thread = self.setup('Break here for direct')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping in will take us directly to the trampoline target.
+thread.StepInto()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('foo', name)
+
+# Check that stepping out takes us back to the trampoline caller.
+thread.StepOut()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+# Check that stepping over the end of the trampoline target 
+# takes us back to the trampoline caller.
+thread.StepInto()
+thread.StepOver()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('direct_trampoline_call', name)
+
+
+def test_chained_call(self):
+thread = self.setup('Break here for chained')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('chained_trampoline_call', name)
+
+# Check that stepping in will take us directly to the trampoline target.
+thread.StepInto()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('foo', name)
+
+# Check that stepping out takes us back to the trampoline caller.
+thread.StepOut()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('chained_trampoline_call', name)
+
+# Check that stepping over the end of the trampoline target 
+# takes us back to the trampoline caller.
+thread.StepInto()
+thread.StepOver()
+name = thread.frames[0].GetFunctionName()
+self.assertIn('chained_trampoline_call', name)
+
+def test_unused_target(self):
+thread = self.setup('Break here for unused')
+
+# Sanity check that we start out in the correct function.
+name = thread.frames[0].GetFunctionName()
+self.assertIn('un

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

> Oh, so in the cases where `dwim-print` delegates to frame variable you can't 
> then refer to that result in another `dwim-print` command/other place where 
> you'd like to reference a previously printed value?

If the variable is in scope, you can use the variable. If it's not in scope, 
can you tell me about the workflows that you'd use it in?

> That seems like a significant regression over existing print functionality 
> and compared to gdb. At least for me that'd be annoying/inconvenient to have 
> to think about which kind of printing I'm doing (& have to explicitly use the 
> less stable full expression based printing) when I want to reuse a value.

Our expectation is that for users such as yourself, the solution is to 
customize the `p` alias, or alternatively create a separate alias, like `pr` 
(print w/ result). Does that seem reasonable? My plan is to use `e` for the 
cases where I want persistent results, since `expression` still defaults to 
persisting result variables.

> Got data on that? I'd /guess/ I use it maybe in single digit % of my prints, 
> but probably at least 1%, though no idea if that's representative - even if 
> it is, the inconsistency feels counter to the goals of dwim-print & the 
> convenience provided by gdb's consistent value handling.

Re data: only anecdotal. I expect that those of us who work on lldb represent 
the biggest users of persistent results, but how much is that? You estimate 1%. 
I use it less than that, maybe one out of a thousand (0.01%), but probably 
less. @aprantl has mentioned he's more likely to hit the up arrow and edit a 
previous command than use persistent results, which I can see being true for 
most users.

It's not that the ability is taken away, only that the default is being set to 
match the majority of cases – the 99+% of prints don't need a persistent result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> The rationale is: dwim-print doesn't always use expression evaluation, it 
> prefers to use frame variable where possible. In the future it could be 
> expanded, for example to print register as well. Because dwim-print doesn't 
> always use expression, there isn't always a persistent result.

Oh, so in the cases where `dwim-print` delegates to `frame variable` you can't 
then refer to that result in another `dwim-print` command/other place where 
you'd like to reference a previously printed value?

That seems like a significant regression over existing print functionality and 
compared to gdb. At least for me that'd be annoying/inconvenient to have to 
think about which kind of printing I'm doing (& have to explicitly use the less 
stable full expression based printing) when I want to reuse a value.

> To make dwim-print output consistent, and because it's presumed most users 
> don't use persistent results

Got data on that? I'd /guess/ I use it maybe in single digit % of my prints, 
but probably at least 1%, though no idea if that's representative - even if it 
is, the inconsistency feels counter to the goals of dwim-print & the 
convenience provided by gdb's consistent value handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

FYI: This might be because we are using a .o file and relocations are being 
applied internally!?




Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:11
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 
0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error 
message

clayborg wrote:
> Shouldn't the low PC of the function be zero?
```
0x0043:   DW_TAG_subprogram
DW_AT_low_pc(0x0001)
DW_AT_high_pc   (0x002e)
DW_AT_frame_base(DW_OP_reg6 RBP)
DW_AT_linkage_name  ("_ZL3foob")
DW_AT_name  ("foo")
DW_AT_decl_file 
("base-lower-then-range-entry/base-lower-then-range-entry/main.cpp")
DW_AT_decl_line (1)
DW_AT_type  (0x008b)

0x0060: DW_TAG_formal_parameter
  DW_AT_location(DW_OP_fbreg -5)
  DW_AT_name("b")
  DW_AT_decl_file   
("base-lower-then-range-entry/base-lower-then-range-entry/main.cpp")
  DW_AT_decl_line   (1)
  DW_AT_type(0x0092)

0x006e: DW_TAG_lexical_block
  DW_AT_ranges  (0x
 [0x, 0x001f)
 [0x, 0x002d))
```

Seems like we have a bug in the code that prints the error message?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:11
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 
0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error 
message

Shouldn't the low PC of the function be zero?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2640
   if (!D || !D->isCompleteDefinition())
-return FwdDecl;
+return {FwdDecl, nullptr};
 

aprantl wrote:
> I'm curious if this works if we encounter a forward declaration, early exit 
> here, then encounter a use of the type, and then the definition, since 
> completeClass effectively also ignores the preferred name?
Good point. If we ever take this branch we won't end up emitting the preferred 
name. For my `-gmodules` test cases this works out fine because the modules 
that contain the instantiations would see the preferred name. But I can indeed 
construct a non-modules test case where we end up using a forward declared 
structure where the preferred name gets ignored here. Not sure we can do much 
here for such cases.

The alternative I considered where we create some sort of 
`PreferredNameCache[TagType*] => DIDerivedType` and use it when replacing 
forward declarations in `finalize()` doesn't work for the normal case because 
we don't have any forward declarations to replace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145803

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

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo updated this revision to Diff 507812.
ayermolo added a comment.

updated test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s

Index: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s
@@ -0,0 +1,317 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: lldb-test symbols %t &> %t.txt
+# RUN: cat %t.txt | FileCheck %s
+
+# Tests that error is printed correctly when DW_AT_low_pc value is
+# greater then a range entry.
+
+# CHECK: 0x006e: adding range [0x-0x001f)
+# CHECK-SAME: which has a base that is less than the function's low PC 0x0021.
+# CHECK-SAME: Please file a bug and attach the file at the start of this error message
+
+
+
+# Test was manually modified to change DW_TAG_lexical_block
+# to use DW_AT_ranges, and value lower then DW_AT_low_pc value
+# in DW_TAG_subprogram
+# static int foo(bool b) {
+#   if (b) {
+#int food = 1;
+# return food;
+#   }
+#   return 0;
+# }
+# int main() {
+#   return foo(true);
+# }
+	.text
+	.file	"main.cpp"
+	.section	.text.main,"ax",@progbits
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "base-lower-then-range-entry" "main.cpp"
+	.loc	1 8 0   # main.cpp:8:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	subq	$16, %rsp
+	movl	$0, -4(%rbp)
+.Ltmp0:
+	.loc	1 9 10 prologue_end # main.cpp:9:10
+	movl	$1, %edi
+	callq	_ZL3foob
+	.loc	1 9 3 epilogue_begin is_stmt 0  # main.cpp:9:3
+	addq	$16, %rsp
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.text._ZL3foob,"ax",@progbits
+	.p2align	4, 0x90 # -- Begin function _ZL3foob
+	.type	_ZL3foob,@function
+_ZL3foob:   # @_ZL3foob
+.Lfunc_begin1:
+	.loc	1 1 0 is_stmt 1 # main.cpp:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	movb	%dil, %al
+	andb	$1, %al
+	movb	%al, -5(%rbp)
+.Ltmp2:
+	.loc	1 2 7 prologue_end  # main.cpp:2:7
+	testb	$1, -5(%rbp)
+	je	.LBB1_2
+# %bb.1:# %if.then
+.Ltmp3:
+	.loc	1 3 8   # main.cpp:3:8
+	movl	$1, -12(%rbp)
+	.loc	1 4 12  # main.cpp:4:12
+	movl	-12(%rbp), %eax
+	.loc	1 4 5 is_stmt 0 # main.cpp:4:5
+	movl	%eax, -4(%rbp)
+	jmp	.LBB1_3
+.Ltmp4:
+.LBB1_2:# %if.end
+	.loc	1 6 3 is_stmt 1 # main.cpp:6:3
+	movl	$0, -4(%rbp)
+.LBB1_3:# %return
+	.loc	1 7 1   # main.cpp:7:1
+	movl	-4(%rbp), %eax
+	.loc	1 7 1 epilogue_begin is_stmt 0  # main.cpp:7:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp5:
+.Lfunc_end1:
+	.size	_ZL3foob, .Lfunc_end1-_ZL3foob
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	0   # DW_CHILDREN_no
+	.byte	17 

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Fix the error message to check for the exact error message and this is good to 
go.




Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:10
+
+# CHECK: which has a base that is less than the function's low PC
+

might be good to verify that the correct DIE offset of the block ID is. in the 
error message. If this code hadn't assert'ed we still might have gotten a 
string output, but we need to verify the data is correct. If you run it 
manually once you can see the exact error message we will need to check for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146659

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> breaks basically anything that examines debugger output

I'd be curious to learn about some example you have in mind here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

> Also `--persistent-result on` can't be passed to print, it only works for me 
> if I run specifically `dwim-print`. Is that intended?

The intension is that for users who want persistent results enabled can do so 
by customizing their `p` or `print` alias, like so:

  command unalias print
  command alias print dwim-print --persistent-result on --



> What was the rationale for this change? It changes the output format of a 
> common command (given that `print` is now an alias for `dwim-print`) and it 
> breaks basically anything that examines debugger output.

Are you using persistent results? If not, how much effort is it to either 1) 
change the tools/code that examine the output to not look for `$\d+`, or 2) use 
a custom `print`/`p` alias? Honest question.

The rationale is: `dwim-print` doesn't always use expression evaluation, it 
prefers to use `frame variable` where possible. In the future it could be 
expanded, for example to print register as well. Because `dwim-print` doesn't 
always use `expression`, there isn't always a persistent result. To make 
`dwim-print` output consistent, and because it's presumed most users don't use 
persistent results, we changed `dwim-print` to default to no persistent 
results. By consistent output, I mean that if a user runs `print someVar` and 
then follows that by running `print someVar.dump()`, it could be confusing if 
the first command doesn't have a persistent result, but the second one does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe added a comment.

What was the rationale for this change? It changes the output format of a 
common command (given that `print` is now an alias for `dwim-print`) and it 
breaks basically anything that examines debugger output.

Also `--persistent-result on` can't be passed to `print`, it only works for me 
if I run specifically `dwim-print`. Is that intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

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

[Lldb-commits] [PATCH] D146679: [lldb] Add support for the DW_AT_trampoline attribute with mangled names

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Symbol/Function.h:664
 
+  llvm::StringRef m_trampoline_target;
+

I'd like to see a doxygen comment (1) explaining what the trampoline target is 
(i.e. according to the summary the mangled name of the trampoline target)  and 
also why it's safe to store a StringRef. I didn't trace the lifetime but it 
seems like the StringRef is coming from a SymbolContext. Is the lifetime of the 
SC guaranteed to exceed that of the function object? If not, should this store 
an owning std::string?



Comment at: lldb/include/lldb/Target/Target.h:253
 
+  bool GetEnableTrampolineSupport() const;
+

What does trampoline "support" mean? Could this be named something more 
descriptive? From the description of the patch it sounds like this guards 
whether you step through trampolines, so maybe something like 
`GetEnableStepThroughTrampolines` or something? 



Comment at: lldb/source/Core/Module.cpp:779-783
+  bool is_trampoline = false;
+  if (Target::GetGlobalProperties().GetEnableTrampolineSupport() &&
+  sc.function) {
+is_trampoline = !sc.function->IsTrampoline();
+  }

+1 on what Alex said. Also I would either write

```bool is_trampoline = 
Target::GetGlobalProperties().GetEnableTrampolineSupport() && sc.function && 
!sc.function->IsTrampoline()```

or

```
  if (Target::GetGlobalProperties().GetEnableTrampolineSupport()) 
is_trampoline = sc.function && !sc.function->IsTrampoline();
```

if you really wanted to split it up



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2443
 
+auto *trampoline_target = die.GetTrampolineTargetName();
+

The return type of `GetTrampolineTargetName` isn't obvious from the name so I 
don't think this is a good place to use `auto` (in accordance with the LLVM 
style guide)



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:208-211
+  if (IsValid())
+return m_die->GetTrampolineTargetName(m_cu);
+  else
+return nullptr;

No else-after-return. I would write:

```
return IsValid() ? m_die->GetTrampolineTargetName(m_cu) : nullptr;
```




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:211
+  else
+return nullptr;
+}

nvm, I see this is done for consistency with the existing code... 



Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:217
+  images.FindSymbolsWithNameAndType(symbol_name, eSymbolTypeCode, 
code_symbols);
+  size_t num_code_symbols = code_symbols.GetSize();
+

This is more of a preference but I would make this `const` to avoid 
accidentally modifying this value. 



Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:221-222
+for (uint32_t i = 0; i < num_code_symbols; i++) {
+  SymbolContext context;
+  AddressRange addr_range;
+  if (code_symbols.GetContextAtIndex(i, context)) {

Not sure how hot this is, but would it be worthwhile moving this out of the 
loop and reusing the same object for every iteration?



Comment at: lldb/source/Target/ThreadPlanRunToAddress.cpp:226-232
+if (log) {
+  addr_t load_addr =
+  addr_range.GetBaseAddress().GetLoadAddress(target_sp.get());
+
+  LLDB_LOGF(log, "Found a trampoline target symbol at 0x%" PRIx64 ".",
+load_addr);
+}

Nit: I would just inline this. I'm sure the compiler will optimize this for 
you, but knowing the macro has the same `if(log)` check, I try avoid wrapping 
it in `if(log)`. 



Comment at: lldb/source/Target/ThreadPlanStepThrough.cpp:126
+  TargetSP target_sp(thread.CalculateTarget());
+  StackFrame *current_frame = thread.GetStackFrameAtIndex(0).get();
+  const SymbolContext ¤t_context =

Should we check that the current frame is not null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146679

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc640a146c4ca: [lldb] Explicitly set libcxx paths when 
USE_SYSTEM_STDLIB is provided (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] c640a14 - [lldb] Explicitly set libcxx paths when USE_SYSTEM_STDLIB is provided

[Felipe de Azevedo Piovezan via lldb-commits]

Author: Felipe de Azevedo Piovezan
Date: 2023-03-23T11:53:13-04:00
New Revision: c640a146c4caa3cca559e308e2e7ecc78c45140d

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

LOG: [lldb] Explicitly set libcxx paths when USE_SYSTEM_STDLIB is provided

For tests marked as "USE_SYSTEM_STDLIB", the expectation is that the
system's standard library should be used. However, the implementation of
this flag is such that we simply don't pass _any_ libcxxx-related flags
to Clang; in turn, Clang will use its defaults.

For a Clang/Libcxx pair compiled together, Clang defaults to:
1. The headers of the sibling libcxx.
2. The libraries of the system.

This mismatch is actually a bug in the driver; once fixed, however, (2)
would point to the sibling libcxx as well, which is _not_ what test
authors intended with the USE_SYSTEM_STDLIB flag.

As such, this patch explicitly sets a path to the system's libraries.
This change is done only in Apple platforms so that we can test this
works in this case first.

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/make/Makefile.rules

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules 
b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
index 25c4d88763326..4c225ed360be5 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@ ifeq (1,$(USE_LIBCPP))
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)



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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507757.
fdeazeve added a comment.

Fix typo in error msg


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system libcxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507756.
fdeazeve added a comment.

Improve error message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system liccxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT must be set on Darwin to use the system liccxx")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM




Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:434
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT should have been set")
+endif




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [lldb] 5193c4a - [lldb][AArch64] Fix run-qemu.sh when only MTE is enabled.

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-03-23T13:48:44Z
New Revision: 5193c4a8b38c3e61c862d5badf1cace7c26324f7

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

LOG: [lldb][AArch64] Fix run-qemu.sh when only MTE is enabled.

SVE and MTE both require a CPU with that feature before
you can use the other options, but we only added the "max"
cpu when SVE was enabled too.

Added: 


Modified: 
lldb/scripts/lldb-test-qemu/run-qemu.sh

Removed: 




diff  --git a/lldb/scripts/lldb-test-qemu/run-qemu.sh 
b/lldb/scripts/lldb-test-qemu/run-qemu.sh
old mode 100644
new mode 100755
index 339b8d955e613..d11711c10e772
--- a/lldb/scripts/lldb-test-qemu/run-qemu.sh
+++ b/lldb/scripts/lldb-test-qemu/run-qemu.sh
@@ -109,8 +109,12 @@ elif [[ "$ARCH" == "arm64" ]]; then
   QEMU_SVE_MAX_VQ=4
   QEMU_CPU="cortex-a53"
 
+  if [[ $SVE ]] || [[ $MTE ]]; then
+QEMU_CPU="max"
+  fi
+
   if [[ $SVE ]]; then
-QEMU_CPU="max,sve-max-vq=$QEMU_SVE_MAX_VQ"
+QEMU_CPU="$QEMU_CPU,sve-max-vq=$QEMU_SVE_MAX_VQ"
   fi
   if [[ $MTE ]]; then
 QEMU_MACHINE="$QEMU_MACHINE,mte=on"



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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 507711.
fdeazeve added a comment.

Fix typos in commit message.
Add hard error in case SDKROOT is not set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT should have been set")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,16 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+ifeq "$(SDKROOT)" ""
+ $(error "SDKROOT should have been set")
+endif
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:434
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif

fdeazeve wrote:
> Michael137 wrote:
> > Should we add a hard error if we get here with an empty `SDKROOT`?
> I thought about this, but at the start of this file we have this:
> 
> ```
> # Handle SDKROOT on Darwin
> #--
> 
> ifeq "$(OS)" "Darwin"
> ifeq "$(SDKROOT)" ""
>   # We haven't otherwise set the SDKROOT, so set it now to macosx
>   SDKROOT := $(shell xcrun --sdk macosx --show-sdk-path)
> endif
> endif
> ```
> 
> So it would be a bit redundant...
> But I think you're right that we could have an error in case someone, 
> someday, changes the logic above.
Yea don't feel too strongly about it, since as you say, the check is at the top 
of the file already


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:434
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif

Michael137 wrote:
> Should we add a hard error if we get here with an empty `SDKROOT`?
I thought about this, but at the start of this file we have this:

```
# Handle SDKROOT on Darwin
#--

ifeq "$(OS)" "Darwin"
ifeq "$(SDKROOT)" ""
# We haven't otherwise set the SDKROOT, so set it now to macosx
SDKROOT := $(shell xcrun --sdk macosx --show-sdk-path)
endif
endif
```

So it would be a bit redundant...
But I think you're right that we could have an error in case someone, someday, 
changes the logic above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Typo in commit message and description:
`USE_SYSTEM_LIBCXX`  should be `USE_SYSTEM_STDLIB`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:434
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif

Should we add a hard error if we get here with an empty `SDKROOT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Michael Buch via Phabricator via lldb-commits]

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

Makes sense to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146714

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

[Lldb-commits] [PATCH] D146714: [lldb] Explicitly set libcxx paths when USE_SYSTEM_LIBCXX is provided

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve created this revision.
Herald added a subscriber: mikhail.ramalho.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

For tests marked as "USE_SYSTEM_LIBCXX", the expectation is that the
system's standard library should be used. However, the implementation of
this flag is such that we simply don't pass _any_ libcxxx-related flags
to Clang; in turn, Clang will use its defaults.

For a Clang/Libcxx pair compiled together, Clang defaults to:

1. The headers of the sibling libcxx.
2. The libraries of the system.

This mismatch is actually a bug in the driver; once fixed, however, (2)
would point to the sibling libcxx as well, which is _not_ what test
authors intended with the USE_SYSTEM_LIBCXX flag.

As such, this patch explicitly sets a path to the system's libraries.
This change is done only in Apple platforms so that we can test this
works in this case first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146714

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,13 @@
endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem 
$(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -428,6 +428,13 @@
 	endif
 endif
 
+ifeq (1, $(USE_SYSTEM_STDLIB))
+ifeq "$(OS)" "Darwin"
+CXXFLAGS += -nostdlib++ -nostdinc++ -cxx-isystem $(SDKROOT)/usr/include/c++/v1
+LDFLAGS += -L$(SDKROOT)/usr/lib -Wl,-rpath,$(SDKROOT)/usr/lib -lc++
+endif
+endif
+
 # If no explicit request was made, but we have paths to a custom libcxx, use
 # them.
 ifeq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP), $(USE_SYSTEM_STDLIB)),)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> So the better solution may be along the lines of a register formatting plugin 
> that depends on TypeSystemClang.

Based on discussion here and on another patch, I'm going to try Adrian's 
suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

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

[Lldb-commits] [PATCH] D146553: [lldb][CMake] Enforce not linking against plugin libs in core libs

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> I suppose it just depends on what you're trying to do, which I realize may 
> not be the most helpful advice but I hope that with enough examples it 
> becomes at least a little more clear how things in lldb are currently done...

No it makes sense, I think I have an idea how to do it. Certainly the 
philosophy behind keeping them separated like this is clear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146553

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

Re: [Lldb-commits] [PATCH] D146553: [lldb][CMake] Enforce not linking against plugin libs in core libs

[David Spickett via lldb-commits]

> But the command interpreter doesn't "depend" on these plugin commands, it
just vends them.

Right, I'm conflating depending on as in linking to or specifically asking
for a certain plugin, vs. making use of the information that category of
plugins could provide if they are present. Commands don't just crash if
there's no plugin for a certain thing, they just say ok we don't know
anything at this time.

> You start with something of a particular plugin type you want to work
with - a path to a binary or a the language type you got from a StackFrame
- which is managed by a particular plugin class, and you first ask the
manager for that plugin type "find me the plugin that handles my entity",
then call the generic API's on that plugin.  You shouldn't dial up
particular plugins or call anything but the plugin interface methods on
them.  If you have to do something that's specific to a particular plugin
implementation, that needs to be done inside the plugin, and vended in some
neutral way.

Understood. I should take what I want to do and make an API for that task.
Then the "how" goes in a plugin.

On Wed, 22 Mar 2023 at 20:16, Jim Ingham  wrote:

> That general facility is why we think of plugins as something more than
> just implementations.  Along with the "on load" actions, they also have the
> job of detecting "Am I the right implementation for this binary, this
> language, this OS version, etc".   That also informs the proper way to use
> them.  You start with something of a particular plugin type you want to
> work with - a path to a binary or a the language type you got from a
> StackFrame - which is managed by a particular plugin class, and you first
> ask the manager for that plugin type "find me the plugin that handles my
> entity", then call the generic API's on that plugin.  You shouldn't dial up
> particular plugins or call anything but the plugin interface methods on
> them.  If you have to do something that's specific to a particular plugin
> implementation, that needs to be done inside the plugin, and vended in some
> neutral way.
>
> Jim
>
> > On Mar 22, 2023, at 11:33 AM, Jim Ingham via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > David,
> >
> > I think you were also thinking about things like the `language
> cplusplus` commands or the `settings set plugin.***` settings.  That isn't
> a case of generic code depending on specific plugin implementations.
> Rather that's a general feature of the plugin loader, it queries the plugin
> to see if it has any commands or settings to add to the command interpreter
> when loaded.  But the command interpreter doesn't "depend" on these plugin
> commands, it just vends them.
> >
> > Jim
> >
> >> On Mar 22, 2023, at 10:26 AM, Alex Langford via Phabricator via
> lldb-commits  wrote:
> >>
> >> bulbazord added a comment.
> >>
> >> In D146553#4212417 ,
> @DavidSpickett wrote:
> >>
> >>> Is this policy documented anywhere? Perhaps you can update one of the
> design pages if it isn't already there.
> >>>
> >>> Because I am a bit confused about it. Some parts of lldb use plugins
> and those parts are called by commands through a few other layers. However
> commands can't link to the plugins directly, but some of them do show
> information that further down came from a plugin. I'm wondering what the
> best practices are (and some examples would be great!).
> >>
> >> This policy is not really documented anywhere. I've been working
> towards making the non-plugins not depend on any plugins for a few years
> now (along with some other folks, some of whom are no longer working on
> LLDB). As for updating one of the design pages, I think I'll update this
> one: https://lldb.llvm.org/design/overview.html
> >> Currently, we are down to maybe 3-4 places where we're using plugins in
> non-plugin contexts and I have a few plans to remove those dependencies.
> They're a bit challenging to remove so I'll be moving a bit more carefully.
> >>
> >> I can understand the confusion. Navigating LLDB's design is quite
> challenging because a lot of it is kind of ad-hoc and grew organically
> while some transitions from one thing to another weren't always completed
> 100%. I personally think that the name "plugin" is a bit misleading because
> they're not really things you can add/remove at runtime. You can't really
> even add/remove them at CMake configure time (though there was some
> interest in doing this in the past). I think a name better than "plugin"
> would be "implementation" because that's what the plugins are: specific
> implementations of more general debugger concepts (e.g. ABI support,
> Platforms, Languages and LanguageRuntimes, ObjectFiles, etc).
> >>
> >> As for best practices, I suppose that depends on exactly which plugin
> you're using. If the goal is just to get some information from a relevant
> plugin, you can look at the way the Language plugins are designed. The
> Language class in `lldb/source/Target