[Lldb-commits] [lldb] 662c1c2 - [LLDB][ARM] Remove unused LoadPseudoRegistersFromFrame function

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-08-16T08:28:50Z
New Revision: 662c1c28813b4dd0cf8775b9acb950ded9705cae

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

LOG: [LLDB][ARM] Remove unused LoadPseudoRegistersFromFrame function

https://reviews.llvm.org/D131658 identified a bug in this and
turns out it's not used anywhere.

Reviewed By: JDevlieghere, clayborg

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

Added: 


Modified: 
lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h

Removed: 




diff  --git a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp 
b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
index 4bfff9277f08..1bb96fc8a2f7 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -26,43 +26,6 @@ EmulationStateARM::EmulationStateARM() : m_vfp_regs(), 
m_memory() {
 
 EmulationStateARM::~EmulationStateARM() = default;
 
-bool EmulationStateARM::LoadPseudoRegistersFromFrame(StackFrame &frame) {
-  RegisterContext *reg_ctx = frame.GetRegisterContext().get();
-  bool success = true;
-  uint32_t reg_num;
-
-  for (int i = dwarf_r0; i < dwarf_r0 + 17; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-RegisterValue reg_value;
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  m_gpr[i - dwarf_r0] = reg_value.GetAsUInt32();
-} else
-  success = false;
-  }
-
-  for (int i = dwarf_d0; i < dwarf_d0 + 32; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-RegisterValue reg_value;
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  uint64_t value = reg_value.GetAsUInt64();
-  uint32_t idx = i - dwarf_d0;
-  if (idx < 16) {
-m_vfp_regs.s_regs[idx * 2] = (uint32_t)value;
-m_vfp_regs.s_regs[idx * 2 + 1] = (uint32_t)(value >> 32);
-  } else
-m_vfp_regs.d_regs[idx - 16] = value;
-} else
-  success = false;
-  }
-
-  return success;
-}
-
 bool EmulationStateARM::StorePseudoRegisterValue(uint32_t reg_num,
  uint64_t value) {
   if (reg_num <= dwarf_cpsr)

diff  --git a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h 
b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
index bc885dab9ac7..bf322f5b1a91 100644
--- a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -32,8 +32,6 @@ class EmulationStateARM {
 
   void ClearPseudoMemory();
 
-  bool LoadPseudoRegistersFromFrame(lldb_private::StackFrame &frame);
-
   bool LoadStateFromDictionary(lldb_private::OptionValueDictionary *test_data);
 
   bool CompareState(EmulationStateARM &other_state,



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

[Lldb-commits] [PATCH] D131664: [LLDB][ARM] Remove unused LoadPseudoRegistersFromFrame function

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG662c1c28813b: [LLDB][ARM] Remove unused 
LoadPseudoRegistersFromFrame function (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131664

Files:
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
  lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h


Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -32,8 +32,6 @@
 
   void ClearPseudoMemory();
 
-  bool LoadPseudoRegistersFromFrame(lldb_private::StackFrame &frame);
-
   bool LoadStateFromDictionary(lldb_private::OptionValueDictionary *test_data);
 
   bool CompareState(EmulationStateARM &other_state,
Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -26,43 +26,6 @@
 
 EmulationStateARM::~EmulationStateARM() = default;
 
-bool EmulationStateARM::LoadPseudoRegistersFromFrame(StackFrame &frame) {
-  RegisterContext *reg_ctx = frame.GetRegisterContext().get();
-  bool success = true;
-  uint32_t reg_num;
-
-  for (int i = dwarf_r0; i < dwarf_r0 + 17; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-RegisterValue reg_value;
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  m_gpr[i - dwarf_r0] = reg_value.GetAsUInt32();
-} else
-  success = false;
-  }
-
-  for (int i = dwarf_d0; i < dwarf_d0 + 32; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-RegisterValue reg_value;
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  uint64_t value = reg_value.GetAsUInt64();
-  uint32_t idx = i - dwarf_d0;
-  if (idx < 16) {
-m_vfp_regs.s_regs[idx * 2] = (uint32_t)value;
-m_vfp_regs.s_regs[idx * 2 + 1] = (uint32_t)(value >> 32);
-  } else
-m_vfp_regs.d_regs[idx - 16] = value;
-} else
-  success = false;
-  }
-
-  return success;
-}
-
 bool EmulationStateARM::StorePseudoRegisterValue(uint32_t reg_num,
  uint64_t value) {
   if (reg_num <= dwarf_cpsr)


Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.h
@@ -32,8 +32,6 @@
 
   void ClearPseudoMemory();
 
-  bool LoadPseudoRegistersFromFrame(lldb_private::StackFrame &frame);
-
   bool LoadStateFromDictionary(lldb_private::OptionValueDictionary *test_data);
 
   bool CompareState(EmulationStateARM &other_state,
Index: lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
===
--- lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
+++ lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp
@@ -26,43 +26,6 @@
 
 EmulationStateARM::~EmulationStateARM() = default;
 
-bool EmulationStateARM::LoadPseudoRegistersFromFrame(StackFrame &frame) {
-  RegisterContext *reg_ctx = frame.GetRegisterContext().get();
-  bool success = true;
-  uint32_t reg_num;
-
-  for (int i = dwarf_r0; i < dwarf_r0 + 17; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-RegisterValue reg_value;
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  m_gpr[i - dwarf_r0] = reg_value.GetAsUInt32();
-} else
-  success = false;
-  }
-
-  for (int i = dwarf_d0; i < dwarf_d0 + 32; ++i) {
-reg_num =
-reg_ctx->ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, i);
-RegisterValue reg_value;
-const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoAtIndex(reg_num);
-
-if (reg_ctx->ReadRegister(reg_info, reg_value)) {
-  uint64_t value = reg_value.GetAsUInt64();
-  uint32_t idx = i - dwarf_d0;
-  if (idx < 16) {
-m_vfp_regs.s_regs[idx * 2] = (uint32_t)value;
-m_vfp_regs.s_regs[idx * 2 + 1] = (uint32_t)(value >> 32);
-  } else
-m_vfp_regs.d_regs[idx - 16] = value;
-} else
-  success = false;
-  }
-
-  return success;
-}
-
 bool EmulationStateARM::StorePseudoRegisterValue(uint32_t reg_num,
  uint64_t value) {
   if (reg_num <= dwarf_

[Lldb-commits] [PATCH] D51387: Allow Template argument accessors to automatically unwrap parameter packs

[Michael Buch via Phabricator via lldb-commits]

Michael137 accepted this revision.
Michael137 added a comment.

LGTM

Just had some minor stylistic/documentation suggestions




Comment at: lldb/source/API/SBType.cpp:576
 
 lldb::TemplateArgumentKind SBType::GetTemplateArgumentKind(uint32_t idx) {
   LLDB_INSTRUMENT_VA(this, idx);

Minor: this could be an opportunity to add documentation for this API in 
`SBType.h`, mentioning that parameter packs are always expanded



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7122
+  if (expand_pack && num_args) {
+auto &pack = template_arg_list[num_args - 1];
+if (pack.getKind() == clang::TemplateArgument::Pack)





Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7167
+   size_t idx, bool expand_pack) {
+  const auto &args = decl->getTemplateArgs();
+

Do we use `args.size() - 1` enough times throughout this function that it 
warrants something like `const auto lastIdx = args.size() - 1;`?



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7172
+return nullptr;
+
+  if (idx < args.size() - 1)

In my opinion this series of checks is subtle enough to warrant some comments. 
But not a blocker



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7175
+return &args[idx];
+
+  if (!expand_pack ||





Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7179
+return idx >= args.size() ? nullptr : &args[idx];
+
+  const auto &pack = args[args.size() - 1];





Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7181
+  const auto &pack = args[args.size() - 1];
+  return &pack.pack_elements()[idx - (args.size() - 1)];
+}

Do we want a defensive check on 'idx' here? Something along the lines of
```
const auto pack_idx  = idx - (args.size() - 1);
const auto pack_size = pack.pack_size();
assert (pack_idx < pack_size && "Parameter pack index out-of-bounds");
if (pack_idx >= pack_size)
  return nullptr;
```



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:325
 bool IsValid() const {
   // Having a pack name but no packed args doesn't make sense, so mark
   // these template parameters as invalid.

Minor: this comment applies to the condition on line 330 now. Could move it 
down?


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

https://reviews.llvm.org/D51387

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

[Lldb-commits] [PATCH] D131335: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131335#3722595 , @Michael137 
wrote:

> As @labath mentioned, we do force clang to preserve the linkage name via 
> `asm()`, but only for class member functions. This was added in 
> `675767a5910d2ec77ef8b51c78fe312cf9022896` (https://reviews.llvm.org/D40283) 
> to also support `abi_tag`!

Yes, that's the patch I was alluding to. I'm sorry I was too lazy to look it up.

> But that didn't cover templates functions:
>
>   Use the DWARF linkage name when importing C++ methods.
>   When importing C++ methods into clang AST nodes from the DWARF symbol
>   table, preserve the DW_AT_linkage_name and use it as the linker
>   ("asm") name for the symbol.
>   
>   Concretely, this enables `expression` to call into names that use the
>   GNU `abi_tag` extension
>
> I tried adding an `AsmLabelAttr` to the `FunctionDecl`s we create when 
> parsing DWARF and it does fix the ABI-tag problem on my small test case.

Which test case are you referring to. The one you made inline in 
https://reviews.llvm.org/D131335#3714567. Or the test attached to this patch? 
If it's the former, what part of the big test case is missing (and can that be 
fixed)?

> But this only works because the way we create `FunctionTemplateDecl`s is 
> incorrect (as I've described in my previous comments).
>
> So the options are any combination of the following:
>
> 1. carry this patch forward (and possibly remove the `asm()` hack for C++ 
> member functions)
> 2. Add the `asm()` attribute hack to all function declarations (or just when 
> we are dealing with template functions)
> 3. Fix the way we generate `FunctionTemplateDecl`s when parsing DWARF (this 
> likely needs a change to DWARF generation)
>
> @aprantl @labath Any preference? To me it seems 1 and 3 are the more "proper" 
> way to fix this issue. And once we fix 3 (which we should do anyway) #2 may 
> break.

I would like to understand what (if any) are the issues with extending the 
approach #2 to cover non-member functions as well. For what it's worth, I don't 
think #2 is a hack. I might actually say that's "using DWARF as it was meant to 
be used" -- the mangled name is there for us to use (*). Before LLDB came 
around, I doubt anybody on the DWARF committee imagined people would try 
round-tripping the DWARF information back into a "regular" C++ compiler (and 
expecting it to recreate "perfect" mangled names for templates and all).

I don't mean to offend, but if anything, I would say that #1 is a hack. What it 
essentially does is pretend that abi tags don't exist. The way I understand it, 
the main reason for the introduction of abi tags was to enable one to have two 
versions of the same function (class, etc.) co-exist in the same 
binary/program. This essentially defeats that. I haven't looked at the 
implementation too closely, but I don't see how one could properly disambiguate 
two differently-tagged versions of the same functions using this approach. It 
may be a hack that we have to live with, but I am not convinced of that (yet).

Regarding #3, yes, there are definite problems regarding the way we represent 
templates. The root cause is the fundamental mismatch between what clang wants 
(a completely accurate description of all objects "as if written in source") 
versus what DWARF provides (a description of the most important properties of 
entities that are present **in the binary**). Where the problem lies depends on 
your point of view. However, even if we come up with a better way to represent 
the  debug information in AST, it's not clear to me why that would be 
incompatible with #2. Since we don't have the actual source code for the 
template function, we will never be able to provide a fully generic definition 
of it -- we will always need to deal with specific instantiations of that 
template. So what we just need is the attach a (`asm`) label to a specific 
template instantiation. I would hope that wouldn't be too much to ask of clang 
to support.

The idea of encoding this information into dwarf (via 
`DW_TAG_template_type_parameter`) sounds interesting but it's not clear to me 
how one would extend it to more complicated scenarios. E.g., I'm not sure how 
I'd describe `template void 
f(std::enable_if_t, T>)` in this way, but the result 
would likely be huge. One could also consider "fishing" this information out of 
the mangled name (at least the itanium scheme should have all of this in there, 
and in a more compact scheme), but that does seem rather hacky.

> The good thing about #2 is that we avoid searching object files, improving 
> performance.

Speaking of performance, how does this searching impact the evaluation time of 
expressions?

(*) The use of mangled names is not particularly important here. What we really 
need is just some way to associate the entities in the clang AST with the 
symbols in llvm IR. Mangled names, due to their almost-guaranteed uniqueness, 
happen to be very good 

[Lldb-commits] [PATCH] D131335: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Thanks for taking the time to look at this

> I would like to understand what (if any) are the issues with extending the 
> approach #2 to cover non-member functions as well. For what it's worth, I 
> don't think #2 is a hack. I might actually say that's "using DWARF as it was 
> meant to be used" -- the mangled name is there for us to use (*). Before LLDB 
> came around, I doubt anybody on the DWARF committee imagined people would try 
> round-tripping the DWARF information back into a "regular" C++ compiler (and 
> expecting it to recreate "perfect" mangled names for templates and all).

There are no issues that I know of with this. My hesitation with this approach 
was just that if we eventually fix the way we construct template decls, we will 
need to make sure we continue attaching the labels correctly. Maybe this isn't 
a big deal if we note it in a comment

> I don't mean to offend, but if anything, I would say that #1 is a hack. What 
> it essentially does is pretend that abi tags don't exist. The way I 
> understand it, the main reason for the introduction of abi tags was to enable 
> one to have two versions of the same function (class, etc.) co-exist in the 
> same binary/program. This essentially defeats that. I haven't looked at the 
> implementation too closely, but I don't see how one could properly 
> disambiguate two differently-tagged versions of the same functions using this 
> approach. It may be a hack that we have to live with, but I am not convinced 
> of that (yet).

The way this patch allows distinguishing differently-tagged versions is by 
checking the `Attrs` Node of the Itanium mangle tree. But you're right in that 
this change does rely on a follow-up patch that attaches `AbiTagAttr` nodes to 
the `FunctionDecl`s we generate, which we were planning on doing. But I guess 
if we are going to attach labels to FunctionDecls anyway, we could just attach 
the `AsmLabel` instead and thus cover all types of attributes trivially.

> Regarding #3, yes, there are definite problems regarding the way we represent 
> templates. The root cause is the fundamental mismatch between what clang 
> wants (a completely accurate description of all objects "as if written in 
> source") versus what DWARF provides (a description of the most important 
> properties of entities that are present in the binary). Where the problem 
> lies depends on your point of view. However, even if we come up with a better 
> way to represent the debug information in AST, it's not clear to me why that 
> would be incompatible with #2. Since we don't have the actual source code for 
> the template function, we will never be able to provide a fully generic 
> definition of it -- we will always need to deal with specific instantiations 
> of that template. So what we just need is the attach a (asm) label to a 
> specific template instantiation. I would hope that wouldn't be too much to 
> ask of clang to support.

This mismatch currently causes problems with unqualified lookups, especially 
with import-std-module enabled, so it would be nice to fix eventually. We don't 
need a fully generic definition, just the generic prototype to get the mangled 
name correctly. But as you say, this can be done with an asm() label with less 
trouble.

I can open a separate diff with approach #2 and see from there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131335

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

[Lldb-commits] [PATCH] D131705: Don't create sections for SHN_ABS symbols in ELF files.

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: dsrbecky.
labath added a comment.

I have somewhat mixed feelings about this.

On one hand, I was never too happy with how we're creating these funny extra 
sections, so I can't say I would be sad to see that go. OTOH, I can't really 
agree with the assertion that these symbols do not represent "addresses".  All 
the ELF spec says about them is that they are not subject to relocation. That 
makes them rather ill-suited for describing the locations of objects in the 
usual scenarios. However, these symbols can be (and, I believe, are) used in 
the embedded world for describing objects that are architecturally defined to 
reside at a particular memory address (the 16-bit dos/bios world was full of 
those). However, one can imagine something similar being done in userspace as 
well (for example, if the kernel provides some data at a fixed virtual 
address). We envisioned something similar when we added support for this. In 
our case, the object/function in question was generated by a JIT, which knew 
the exact address at which it placed the jitted code.

However, AFAIK, that project never materialized, and we are not relying on 
these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the case, 
and given that gdb does not handle these symbols in this way either (at least, 
I haven't been able to make it do that), I think we can go forward with 
removing them.




Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:28-29
+and this caused problems as the new sections could interfere with
+with address resolution. Absolute symbols' values are not addresses
+and should not be treated this way.
+

As I said in the overall comment, I cannot agree with this statement without 
reservations. 

And in any case, I think prose like this is better left for the commit message. 
The test should just state what it does (although that's fairly obvious here), 
not what it used to do.



Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:76-80
+# Make sure no sections were created for the absolute symbol with a
+# prefix of ".absolute." followed by the symbol name as they interfere
+# with address lookups if they are treated like real sections.
+for section in module.sections:
+self.assertTrue(section.GetName() != ".absolute.absolute")

This check isn't particularly useful, since you've completely deleted the code 
which created those sections. If something similar is ever reintroduced, it's 
quite possible it would use a different section name, and so it wouldn't catch 
this.

I don't think it's necessary, but if you want, I think it'd be better to run 
something like `image lookup -a 0x8000` and check that it does not 
produce any matches, as that's what you're really interested in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131705

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

[Lldb-commits] [PATCH] D131335: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py:14
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):

labath wrote:
> This means the test is only expected to pass with the `dsym` debug info 
> flavour. Is that really what you wanted to say?
Yup this was intentional. All these suffer from issues with unqualified lookup 
due to the way we construct the template decls


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131335

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

[Lldb-commits] [PATCH] D131667: [LLDB][RISCV] Fix risc-v target build

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp:54
+NativeRegisterContextLinux::DetermineArchitecture(lldb::tid_t tid) {
+  return HostInfo::GetArchitecture();
+}

DavidSpickett wrote:
> On the assumption that riscv6 linux doesn't have a 32 bit compatibility mode, 
> this is fine.
> 
> (that's what this is trying to detect, for example running arm32 on arm64).
(even if it does, I don't think we have to support that mode right away)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131667

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

[Lldb-commits] [lldb] 95e2949 - [LLDB] Fix possible nullptr exception

[via lldb-commits]

Author: Emmmer
Date: 2022-08-16T23:41:00+08:00
New Revision: 95e2949a5352aea0182ba5295bf9ba46080c261e

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

LOG: [LLDB] Fix possible nullptr exception

Some architectures do not have a flag register (like riscv).
In this case, we should set it to `baton.m_register_values.end()` to avoid 
nullptr exception.

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp 
b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
index ee5295bf65651..f9520cf4cfb75 100644
--- a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
@@ -128,8 +128,10 @@ Status 
NativeProcessSoftwareSingleStep::SetupSoftwareSingleStepping(
 
   auto pc_it =
   baton.m_register_values.find(reg_info_pc->kinds[eRegisterKindDWARF]);
-  auto flags_it =
-  baton.m_register_values.find(reg_info_flags->kinds[eRegisterKindDWARF]);
+  auto flags_it = reg_info_flags == nullptr
+  ? baton.m_register_values.end()
+  : baton.m_register_values.find(
+reg_info_flags->kinds[eRegisterKindDWARF]);
 
   lldb::addr_t next_pc;
   lldb::addr_t next_flags;



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

[Lldb-commits] [PATCH] D131945: [LLDB] Fix possible nullptr exception

[Emmmer S via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG95e2949a5352: [LLDB] Fix possible nullptr exception 
(authored by Emmmer).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131945

Files:
  lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp


Index: lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
===
--- lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
@@ -128,8 +128,10 @@
 
   auto pc_it =
   baton.m_register_values.find(reg_info_pc->kinds[eRegisterKindDWARF]);
-  auto flags_it =
-  baton.m_register_values.find(reg_info_flags->kinds[eRegisterKindDWARF]);
+  auto flags_it = reg_info_flags == nullptr
+  ? baton.m_register_values.end()
+  : baton.m_register_values.find(
+reg_info_flags->kinds[eRegisterKindDWARF]);
 
   lldb::addr_t next_pc;
   lldb::addr_t next_flags;


Index: lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
===
--- lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
@@ -128,8 +128,10 @@
 
   auto pc_it =
   baton.m_register_values.find(reg_info_pc->kinds[eRegisterKindDWARF]);
-  auto flags_it =
-  baton.m_register_values.find(reg_info_flags->kinds[eRegisterKindDWARF]);
+  auto flags_it = reg_info_flags == nullptr
+  ? baton.m_register_values.end()
+  : baton.m_register_values.find(
+reg_info_flags->kinds[eRegisterKindDWARF]);
 
   lldb::addr_t next_pc;
   lldb::addr_t next_flags;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 8ed3e75 - [LLDB] Handle possible resume thread error

[via lldb-commits]

Author: Emmmer
Date: 2022-08-16T23:43:28+08:00
New Revision: 8ed3e75c96d987e294067e2fe0ab5b02a5bcc89d

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

LOG: [LLDB] Handle possible resume thread error

In this switch case we didn't handle possible errors in `ResumeThread()`, it's 
hard to get helpful information when it goes wrong.

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp 
b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index 5556f69c10194..a0f9f476a3c6f 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -933,8 +933,14 @@ Status NativeProcessLinux::Resume(const ResumeActionList 
&resume_actions) {
 case eStateStepping: {
   // Run the thread, possibly feeding it the signal.
   const int signo = action->signal;
-  ResumeThread(static_cast(*thread), action->state,
-   signo);
+  Status error = ResumeThread(static_cast(*thread),
+  action->state, signo);
+  if (error.Fail())
+return Status("NativeProcessLinux::%s: failed to resume thread "
+  "for pid %" PRIu64 ", tid %" PRIu64 ", error = %s",
+  __FUNCTION__, GetID(), thread->GetID(),
+  error.AsCString());
+
   break;
 }
 



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

[Lldb-commits] [PATCH] D131946: [LLDB] Handle possible resume thread error

[Emmmer S via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8ed3e75c96d9: [LLDB] Handle possible resume thread error 
(authored by Emmmer).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131946

Files:
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp


Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -933,8 +933,14 @@
 case eStateStepping: {
   // Run the thread, possibly feeding it the signal.
   const int signo = action->signal;
-  ResumeThread(static_cast(*thread), action->state,
-   signo);
+  Status error = ResumeThread(static_cast(*thread),
+  action->state, signo);
+  if (error.Fail())
+return Status("NativeProcessLinux::%s: failed to resume thread "
+  "for pid %" PRIu64 ", tid %" PRIu64 ", error = %s",
+  __FUNCTION__, GetID(), thread->GetID(),
+  error.AsCString());
+
   break;
 }
 


Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -933,8 +933,14 @@
 case eStateStepping: {
   // Run the thread, possibly feeding it the signal.
   const int signo = action->signal;
-  ResumeThread(static_cast(*thread), action->state,
-   signo);
+  Status error = ResumeThread(static_cast(*thread),
+  action->state, signo);
+  if (error.Fail())
+return Status("NativeProcessLinux::%s: failed to resume thread "
+  "for pid %" PRIu64 ", tid %" PRIu64 ", error = %s",
+  __FUNCTION__, GetID(), thread->GetID(),
+  error.AsCString());
+
   break;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 4fc7e9c - [LLDB][RISCV] Make software single stepping work

[via lldb-commits]

Author: Emmmer
Date: 2022-08-16T23:44:50+08:00
New Revision: 4fc7e9cba24b3f96f274126218a7985f31aa5861

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

LOG: [LLDB][RISCV] Make software single stepping work

Add:
- `EmulateInstructionRISCV`, which can be used for riscv32 and riscv64.
- Add unittests for EmulateInstructionRISCV.

Note: Compressed instructions set (RVC) was still not supported in this patch.

Reviewed By: DavidSpickett

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

Added: 
lldb/source/Plugins/Instruction/RISCV/CMakeLists.txt
lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp
lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp

Modified: 
lldb/source/Plugins/Instruction/CMakeLists.txt
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/SystemInitializerLLGS.cpp
lldb/unittests/Instruction/CMakeLists.txt

Removed: 
lldb/unittests/Instruction/TestAArch64Emulator.cpp



diff  --git a/lldb/source/Plugins/Instruction/CMakeLists.txt 
b/lldb/source/Plugins/Instruction/CMakeLists.txt
index 89771e8f46d14..631c0b307cac4 100644
--- a/lldb/source/Plugins/Instruction/CMakeLists.txt
+++ b/lldb/source/Plugins/Instruction/CMakeLists.txt
@@ -3,3 +3,4 @@ add_subdirectory(ARM64)
 add_subdirectory(MIPS)
 add_subdirectory(MIPS64)
 add_subdirectory(PPC64)
+add_subdirectory(RISCV)

diff  --git a/lldb/source/Plugins/Instruction/RISCV/CMakeLists.txt 
b/lldb/source/Plugins/Instruction/RISCV/CMakeLists.txt
new file mode 100644
index 0..ed05d4a3132a1
--- /dev/null
+++ b/lldb/source/Plugins/Instruction/RISCV/CMakeLists.txt
@@ -0,0 +1,11 @@
+add_lldb_library(lldbPluginInstructionRISCV PLUGIN
+  EmulateInstructionRISCV.cpp
+
+  LINK_LIBS
+lldbCore
+lldbInterpreter
+lldbSymbol
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+  )

diff  --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
new file mode 100644
index 0..458b996d6ff62
--- /dev/null
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -0,0 +1,356 @@
+//===-- EmulateInstructionRISCV.cpp 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include 
+
+#include "EmulateInstructionRISCV.h"
+#include "Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.h"
+#include "Plugins/Process/Utility/lldb-riscv-register-enums.h"
+
+#include "lldb/Core/Address.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Interpreter/OptionValueArray.h"
+#include "lldb/Interpreter/OptionValueDictionary.h"
+#include "lldb/Symbol/UnwindPlan.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/RegisterValue.h"
+#include "lldb/Utility/Stream.h"
+
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/MathExtras.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+LLDB_PLUGIN_DEFINE_ADV(EmulateInstructionRISCV, InstructionRISCV)
+
+namespace lldb_private {
+
+// Masks for detecting instructions types. According to riscv-spec Chap 26.
+constexpr uint32_t I_MASK = 0b1110111;
+constexpr uint32_t J_MASK = 0b111;
+// no funct3 in the b-mask because the logic executing B is quite similar.
+constexpr uint32_t B_MASK = 0b111;
+
+// The funct3 is the type of compare in B instructions.
+// funct3 means "3-bits function selector", which RISC-V ISA uses as minor
+// opcode. It reuses the major opcode encoding space.
+constexpr uint32_t BEQ = 0b000;
+constexpr uint32_t BNE = 0b001;
+constexpr uint32_t BLT = 0b100;
+constexpr uint32_t BGE = 0b101;
+constexpr uint32_t BLTU = 0b110;
+constexpr uint32_t BGEU = 0b111;
+
+constexpr uint32_t DecodeRD(uint32_t inst) { return (inst & 0xF80) >> 7; }
+constexpr uint32_t DecodeRS1(uint32_t inst) { return (inst & 0xF8000) >> 15; }
+constexpr uint32_t DecodeRS2(uint32_t inst) { return (inst & 0x1F0) >> 20; 
}
+constexpr uint32_t DecodeFunct3(uint32_t inst) { return (inst & 0x7000) >> 12; 
}
+
+constexpr int32_t SignExt(uint32_t imm) { return int32_t(imm); }
+
+constexpr uint32_t DecodeJImm(uint32_t inst) {
+  return (uint64_t(int64_t(int32_t(inst & 0x8000)) >> 11)) // imm[20]
+   

[Lldb-commits] [PATCH] D131759: [LLDB][RISCV] Make software single stepping work

[Emmmer S via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4fc7e9cba24b: [LLDB][RISCV] Make software single stepping 
work (authored by Emmmer).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131759

Files:
  lldb/source/Plugins/Instruction/CMakeLists.txt
  lldb/source/Plugins/Instruction/RISCV/CMakeLists.txt
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-server/SystemInitializerLLGS.cpp
  lldb/unittests/Instruction/ARM64/TestAArch64Emulator.cpp
  lldb/unittests/Instruction/CMakeLists.txt
  lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
  lldb/unittests/Instruction/TestAArch64Emulator.cpp

Index: lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
===
--- /dev/null
+++ lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
@@ -0,0 +1,196 @@
+//===-- TestRISCVEmulator.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Core/Address.h"
+#include "lldb/Core/Disassembler.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/RegisterValue.h"
+
+#include "Plugins/Instruction/RISCV/EmulateInstructionRISCV.h"
+#include "Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.h"
+#include "Plugins/Process/Utility/lldb-riscv-register-enums.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+struct RISCVEmulatorTester : public EmulateInstructionRISCV, testing::Test {
+  RegisterInfoPOSIX_riscv64::GPR gpr;
+
+  RISCVEmulatorTester()
+  : EmulateInstructionRISCV(ArchSpec("riscv64-unknown-linux-gnu")) {
+EmulateInstruction::SetReadRegCallback(ReadRegisterCallback);
+EmulateInstruction::SetWriteRegCallback(WriteRegisterCallback);
+  }
+
+  static bool ReadRegisterCallback(EmulateInstruction *instruction, void *baton,
+   const RegisterInfo *reg_info,
+   RegisterValue ®_value) {
+RISCVEmulatorTester *tester = (RISCVEmulatorTester *)instruction;
+uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
+if (reg == gpr_x0_riscv)
+  reg_value.SetUInt(0, reg_info->byte_size);
+else
+  reg_value.SetUInt(tester->gpr.gpr[reg], reg_info->byte_size);
+return true;
+  }
+
+  static bool WriteRegisterCallback(EmulateInstruction *instruction,
+void *baton, const Context &context,
+const RegisterInfo *reg_info,
+const RegisterValue ®_value) {
+RISCVEmulatorTester *tester = (RISCVEmulatorTester *)instruction;
+uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
+if (reg != gpr_x0_riscv)
+  tester->gpr.gpr[reg] = reg_value.GetAsUInt64();
+return true;
+  }
+};
+
+TEST_F(RISCVEmulatorTester, testJAL) {
+  lldb::addr_t old_pc = 0x114514;
+  WritePC(old_pc);
+  // jal x1, -6*4
+  uint32_t inst = 0b111010011110;
+  ASSERT_TRUE(DecodeAndExecute(inst, false));
+  auto x1 = gpr.gpr[1];
+
+  bool success = false;
+  auto pc = ReadPC(&success);
+
+  ASSERT_TRUE(success);
+  ASSERT_EQ(x1, old_pc + 4);
+  ASSERT_EQ(pc, old_pc + (-6 * 4));
+}
+
+constexpr uint32_t EncodeIType(uint32_t opcode, uint32_t funct3, uint32_t rd,
+   uint32_t rs1, uint32_t imm) {
+  return imm << 20 | rs1 << 15 | funct3 << 12 | rd << 7 | opcode;
+}
+
+constexpr uint32_t JALR(uint32_t rd, uint32_t rs1, int32_t offset) {
+  return EncodeIType(0b1100111, 0, rd, rs1, uint32_t(offset));
+}
+
+TEST_F(RISCVEmulatorTester, testJALR) {
+  lldb::addr_t old_pc = 0x114514;
+  lldb::addr_t old_x2 = 0x1024;
+  WritePC(old_pc);
+  gpr.gpr[2] = old_x2;
+  // jalr x1, x2(-255)
+  uint32_t inst = JALR(1, 2, -255);
+  ASSERT_TRUE(DecodeAndExecute(inst, false));
+  auto x1 = gpr.gpr[1];
+
+  bool success = false;
+  auto pc = ReadPC(&success);
+
+  ASSERT_TRUE(success);
+  ASSERT_EQ(x1, old_pc + 4);
+  // JALR always zeros the bottom bit of the target address.
+  ASSERT_EQ(pc, (old_x2 + (-255)) & (~1));
+}
+
+constexpr uint32_t EncodeBType(uint32_t opcode, uint32_t funct3, uint32_t rs1,
+   uint32_t rs2, uint32_t imm) {
+  uint32_t bimm = (imm & (0b1 << 11)) >> 4 

[Lldb-commits] [PATCH] D131539: [lldb] Make sure all Python API tests are marked as NO_DEBUG_INFO_TESTCASE

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131539#3712675 , @JDevlieghere 
wrote:

> In D131539#3711835 , @labath wrote:
>
>> In D131539#3711518 , @JDevlieghere 
>> wrote:
>>
>>> In D131539#3711488 , @kastiglione 
>>> wrote:
>>>
 this diff has made me wonder: should we have a `NoDebugInfoTestCase` that 
 can be used by any test, and would replace assigning to 
 `NO_DEBUG_INFO_TESTCASE`?
>>>
>>> I was wondering the same thing. I decided against it because we already 
>>> have `NO_DEBUG_INFO_TESTCASE` (test level) and `@no_debug_info_test` 
>>> (function level) and I didn't want to add yet another option. Additionally, 
>>> there's a few other patters that are common for the Python API tests (e.g. 
>>> the `self.source` and `self.line`) that could be moved up into the base 
>>> class in a follow up patch.
>>
>> Note that we already kind of have that. The `Base` test class is the base of 
>> all our tests and does not support automatic test replication. Tests which 
>> inherit directly from that (lldb-server and lldb-vscode tests) for instance, 
>> don't get the replication even though they don't use 
>> `NO_DEBUG_INFO_TESTCASE`. The `TestBase` class (which we use for the 
>> "normal" SB API tests) adds a bunch of SB utility functions *and* it adds 
>> the ability to replicate tests. I think that separating the two parts would 
>> be completely reasonable, and would remove the need for most/all of the uses 
>> of `NO_DEBUG_INFO_TESTCASE` and `@no_debug_info_test`.
>
> The ability to replicate tests is not even part of `TestBase` I think. Except 
> for the attribute, it's all handled by the `LLDBTestCaseFactory`.

Well.. yes, the actual code lives there, but if you think about it from the POV 
of a test case author, then he has the choice of either inheriting from Base 
(and getting no replication), or from TestBase (and getting the replication 
unless he disables it). I would say that the LLDBTestCaseFactory is an 
implementation detail that should not be visible to the outside world.

> If you think it's better to go the test case route (in favor of 
> NO_DEBUG_INFO_TESTCASE) we can pull up everything but the factory and 
> property from `TestBase` into `Base` (as you outlined) or keep them separate 
> and just move the factory and property down to a (third) child class.

I wouldn't want to put everything into Base, precisely because we have these 
gdb-server, vscode, etc., which don't operate on the SB API, and that 
functionality makes no sense there.

> Either way we should rename those 2 (3) classes to something more meaningful.

Oh yes.

> @labath: was your comment meant as a suggestion (i.e. we should make this 
> change) or just informative in reply to Dave's question (i.e. we could make 
> this change if we needed it)?

Something in between, I guess. I would like it to work that way, but I'm not 
going to make anyone do that right now. It's also a part of a larger discussion 
of whether the debug info replication should be opt-in or opt-out. A separate 
class makes more sense for an opt-in world, whereas the status quo is kind of 
geared towards an opt-out mechanism.


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

https://reviews.llvm.org/D131539

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

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: labath, aprantl, jingham.
Herald added a reviewer: shafik.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When resolving symbols during IR execution, lldb makes a last effort attempt
to resolve external symbols from object files by approximate name matching.
It currently uses `CPlusPlusNameParser` to parse the demangled function name
and arguments for the unresolved symbol and its candidates. However, this
hand-rolled C++ parser doesn’t support ABI tags which, depending on the 
demangler,
get demangled into `[abi:tag]`. This lack of parsing support causes lldb to 
never
consider a candidate mangled function name that has ABI tags.

The issue reproduces by calling an ABI-tagged template function from the
expression evaluator. This is particularly problematic with the recent
addition of ABI tags to numerous libcxx APIs.

The issue stems from the fact that `clang::CodeGen` emits function
function calls using the mangled name inferred from the `FunctionDecl`
LLDB constructs from DWARF. Debug info often lacks information for
us to construct a perfect FunctionDecl resulting in subtle mangled
name inaccuracies.

This patch side-steps the problem of inaccurate `FunctionDecl`s by
attaching an `asm()` label to each `FunctionDecl` LLDB creates from DWARF.
`clang::CodeGen` consults this label to get the mangled name as one of
the first courses of action when emitting a function call.

LLDB already does this for C++ member functions as of
675767a5910d2ec77ef8b51c78fe312cf9022896 

**Testing**

- Added API tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131974

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/abi_tag_lookup/Makefile
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp

Index: lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
@@ -0,0 +1,71 @@
+#include 
+
+namespace A {
+template  bool operator<(const T &, const T &) { return true; }
+
+template 
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template 
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template 
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template 
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+[[gnu::abi_tag("Baz")]] int withAbiTag(const int &, const int &) { return -3; }
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const &Value() const { return mem; }
+};
+
+template  struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const &Value() const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template  int withImplicitTag(T const &t) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+   A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+   A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+   withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}
Index: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
@@ -0,0 +1,52 @@
+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, 'Break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# Qualified/unqualified lookup to templates in namespace
+self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+# Qualified/unqualified lookup to templates with ABI tags in namespace
+self.

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

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

This implements @labath's suggestion from https://reviews.llvm.org/D131335

Following tests fail with this:

  lang/cpp/extern_c/TestExternCSymbols.py
  lang/cpp/namespace/TestNamespaceLookup.py
  macosx/rosetta/TestRosetta.py

Investigating currently...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131974#3726438 , @Michael137 
wrote:

>   lang/cpp/extern_c/TestExternCSymbols.py

I think that's because `extern "C"` symbols don't have a DW_AT_linkage_name 
attribute. Generally speaking, I don't think we can assume all debug info 
entries will have one (e.g. `-gsce` makes clang omit those), and should add 
them only if available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.



> Following tests fail with this:
>
>   lang/cpp/extern_c/TestExternCSymbols.py
>   lang/cpp/namespace/TestNamespaceLookup.py
>   macosx/rosetta/TestRosetta.py
>
> Investigating currently...

Fixed with latest diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D131974#3726451 , @labath wrote:

> In D131974#3726438 , @Michael137 
> wrote:
>
>>   lang/cpp/extern_c/TestExternCSymbols.py
>
> I think that's because `extern "C"` symbols don't have a DW_AT_linkage_name 
> attribute. Generally speaking, I don't think we can assume all debug info 
> entries will have one (e.g. `-gsce` makes clang omit those), and should add 
> them only if available.

Yup makes sense. Checking the others. Wonder if it needs a check for 
`attrs.storage == SC_Extern`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 453041.
Michael137 added a comment.

- Check mangled name and storage class before attaching label


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/abi_tag_lookup/Makefile
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp

Index: lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
@@ -0,0 +1,71 @@
+#include 
+
+namespace A {
+template  bool operator<(const T &, const T &) { return true; }
+
+template 
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template 
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template 
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template 
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+[[gnu::abi_tag("Baz")]] int withAbiTag(const int &, const int &) { return -3; }
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const &Value() const { return mem; }
+};
+
+template  struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const &Value() const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template  int withImplicitTag(T const &t) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+   A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+   A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+   withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}
Index: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
@@ -0,0 +1,52 @@
+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, 'Break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# Qualified/unqualified lookup to templates in namespace
+self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+# Qualified/unqualified lookup to templates with ABI tags in namespace
+self.expect_expr("operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 > b2", result_type="bool", result_value="true")
+
+# Call non-operator templates with ABI tags
+self.expect_expr("A::withAbiTagInNS(1, 1)", result_type="int", result_value="1")
+
+self.expect_expr("A::withAbiTagInNS(1.0, 1.0)", result_type="int", result_value="2")
+self.expect_expr("withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+self.expect_expr("A::withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+
+self.expect_expr("withAbiTag(b1, b2)", result_type="int", result_value="3")
+self.expect_expr("withAbiTag(0, 0)", result_type="int", result_value="-3")
+
+# Structures with ABI tags
+self.expect_expr("t.Value()", result_type="const int", result_value="4")
+self.expect_expr("tt.Value()", result_type="const int", result_value="5")
+
+self.expect_expr("Tagged{.mem = 6}", result_type="Tagged",
+ result_children=[ValueCheck(name="mem", value="6")])
+
+# Inline namespaces with ABI tags
+self.expect_expr("v1::withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+self.expect_expr("withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+
+self.expect_expr("v1::withImplicitTag(Tagged{.mem = 6})", result

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 453043.
Michael137 added a comment.

- Remove libcxx ABI-tag workaround from API test makefile


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/abi_tag_lookup/Makefile
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp

Index: lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
@@ -0,0 +1,71 @@
+#include 
+
+namespace A {
+template  bool operator<(const T &, const T &) { return true; }
+
+template 
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template 
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template 
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template 
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+[[gnu::abi_tag("Baz")]] int withAbiTag(const int &, const int &) { return -3; }
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const &Value() const { return mem; }
+};
+
+template  struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const &Value() const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template  int withImplicitTag(T const &t) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+   A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+   A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+   withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}
Index: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
@@ -0,0 +1,52 @@
+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, 'Break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# Qualified/unqualified lookup to templates in namespace
+self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+# Qualified/unqualified lookup to templates with ABI tags in namespace
+self.expect_expr("operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 > b2", result_type="bool", result_value="true")
+
+# Call non-operator templates with ABI tags
+self.expect_expr("A::withAbiTagInNS(1, 1)", result_type="int", result_value="1")
+
+self.expect_expr("A::withAbiTagInNS(1.0, 1.0)", result_type="int", result_value="2")
+self.expect_expr("withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+self.expect_expr("A::withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+
+self.expect_expr("withAbiTag(b1, b2)", result_type="int", result_value="3")
+self.expect_expr("withAbiTag(0, 0)", result_type="int", result_value="-3")
+
+# Structures with ABI tags
+self.expect_expr("t.Value()", result_type="const int", result_value="4")
+self.expect_expr("tt.Value()", result_type="const int", result_value="5")
+
+self.expect_expr("Tagged{.mem = 6}", result_type="Tagged",
+ result_children=[ValueCheck(name="mem", value="6")])
+
+# Inline namespaces with ABI tags
+self.expect_expr("v1::withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+self.expect_expr("withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+
+self.expec

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Adrian Prantl via Phabricator via lldb-commits]

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

Thanks! This looks like a fine incremental improvement. But we should aim at 
eventually getting rid of these XFAILs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [lldb] 7ebbef2 - [LLDB][NativePDB] Add nullptr checking.

[Zequan Wu via lldb-commits]

Author: Zequan Wu
Date: 2022-08-16T09:59:09-07:00
New Revision: 7ebbef2b303fc1c54950bd9047f79e353ee0129c

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

LOG: [LLDB][NativePDB] Add nullptr checking.

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp 
b/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
index 25425f9140880..b8848839f24f7 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -501,6 +501,8 @@ clang::Decl 
*PdbAstBuilder::GetOrCreateSymbolForId(PdbCompilandSymId id) {
 
   if (isLocalVariableType(cvs.kind())) {
 clang::DeclContext *scope = GetParentDeclContext(id);
+if (!scope)
+  return nullptr;
 clang::Decl *scope_decl = clang::Decl::castFromDeclContext(scope);
 PdbCompilandSymId scope_id =
 PdbSymUid(m_decl_to_status[scope_decl].uid).asCompilandSym();
@@ -1010,7 +1012,7 @@ PdbAstBuilder::GetOrCreateTypedefDecl(PdbGlobalSymId id) {
 
   PdbTypeSymId real_type_id{udt.Type, false};
   clang::QualType qt = GetOrCreateType(real_type_id);
-  if (qt.isNull())
+  if (qt.isNull() || !scope)
 return nullptr;
 
   std::string uname = std::string(DropNameScope(udt.Name));
@@ -1265,7 +1267,7 @@ PdbAstBuilder::CreateFunctionDeclFromId(PdbTypeSymId 
func_tid,
 lldbassert(false && "Invalid function id type!");
   }
   clang::QualType func_qt = GetOrCreateType(func_ti);
-  if (func_qt.isNull())
+  if (func_qt.isNull() || !parent)
 return nullptr;
   CompilerType func_ct = ToCompilerType(func_qt);
   uint32_t param_count =
@@ -1280,6 +1282,8 @@ PdbAstBuilder::GetOrCreateFunctionDecl(PdbCompilandSymId 
func_id) {
 return llvm::dyn_cast(decl);
 
   clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
+  if (!parent)
+return nullptr;
   std::string context_name;
   if (clang::NamespaceDecl *ns = llvm::dyn_cast(parent)) 
{
 context_name = ns->getQualifiedNameAsString();

diff  --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp 
b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index ada7a0f71fb55..c596d9c7a1ae7 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -345,10 +345,13 @@ Block &SymbolFileNativePDB::CreateBlock(PdbCompilandSymId 
block_id) {
 // This is a function.  It must be global.  Creating the Function entry
 // for it automatically creates a block for it.
 FunctionSP func = GetOrCreateFunction(block_id, *comp_unit);
-Block &block = func->GetBlock(false);
-if (block.GetNumRanges() == 0)
-  block.AddRange(Block::Range(0, func->GetAddressRange().GetByteSize()));
-return block;
+if (func) {
+  Block &block = func->GetBlock(false);
+  if (block.GetNumRanges() == 0)
+block.AddRange(Block::Range(0, func->GetAddressRange().GetByteSize()));
+  return block;
+}
+break;
   }
   case S_BLOCK32: {
 // This is a block.  Its parent is either a function or another block.  In
@@ -1024,11 +1027,13 @@ uint32_t SymbolFileNativePDB::ResolveSymbolContext(
 continue;
   if (type == PDB_SymType::Function) {
 sc.function = GetOrCreateFunction(csid, *sc.comp_unit).get();
-Block &block = sc.function->GetBlock(true);
-addr_t func_base =
-sc.function->GetAddressRange().GetBaseAddress().GetFileAddress();
-addr_t offset = file_addr - func_base;
-sc.block = block.FindInnermostBlockByOffset(offset);
+if (sc.function) {
+  Block &block = sc.function->GetBlock(true);
+  addr_t func_base =
+  sc.function->GetAddressRange().GetBaseAddress().GetFileAddress();
+  addr_t offset = file_addr - func_base;
+  sc.block = block.FindInnermostBlockByOffset(offset);
+}
   }
 
   if (type == PDB_SymType::Block) {
@@ -1908,6 +1913,8 @@ SymbolFileNativePDB::GetDeclContextForUID(lldb::user_id_t 
uid) {
 CompilerDeclContext
 SymbolFileNativePDB::GetDeclContextContainingUID(lldb::user_id_t uid) {
   clang::DeclContext *context = m_ast->GetParentDeclContext(PdbSymUid(uid));
+  if (!context)
+return CompilerDeclContext();
   return m_ast->ToCompilerDeclContext(*context);
 }
 
@@ -1929,6 +1936,8 @@ Type *SymbolFileNativePDB::ResolveTypeUID(lldb::user_id_t 
type_uid) {
 return nullptr;
 
   TypeSP type_sp = CreateAndCacheType(type_id);
+  if (!type_sp)
+return nullptr;
   return &*type_sp;
 }
 

[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

For me this wins on simplicity grounds alone.

In D131974#3726475 , @Michael137 
wrote:

> - Check mangled name and storage class before attaching label

I don't think that the storage class check is helping anything. Non-external 
functions can have DW_AT_linkage_names and ABI tags (although their usefulness 
is questionable) as well. Of course, that means that the local (indicated by 
the `L` in the name) mangled name is not necessarily unique (and that's why I 
said that going off of the address would be more correct). However, this is not 
something we can fix by letting clang deduce the name for itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974

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

[Lldb-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Ilya Biryukov via Phabricator via lldb-commits]

ilya-biryukov added a comment.

I agree that the change in behaviour is reasonable and have no objections to 
it. The code should not rely on particular output of `__PRETTY_FUNCTION__`.
I just wanted to point out that we still don't match GCC in other cases, not 
that is was a worthwhile goal to chase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

Ok, I've just realized that the pipe writing solution isn't going to work when 
the notification happens from the same thread (as the unittest does right now). 
@labath, do you happen to have an idea how to solve that reasonably cleanly?


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

https://reviews.llvm.org/D131160

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D131160#3726642 , @mgorny wrote:

> Ok, I've just realized that the pipe writing solution isn't going to work 
> when the notification happens from the same thread (as the unittest does 
> right now). @labath, do you happen to have an idea how to solve that 
> reasonably cleanly?

Ah, nevermind, for some reason I thought pipe writes are going to block until 
read happens. Perhaps we don't need to worry about that.


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

https://reviews.llvm.org/D131160

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

[Lldb-commits] [PATCH] D131900: [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[Slava Gurevich via Phabricator via lldb-commits]

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

All pending suggestions have been resolved


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131900

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 453067.
mgorny added a comment.

Add `m_event_pipe` handler to `m_read_fds` to avoid reinventing the wheel three 
times.


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

https://reviews.llvm.org/D131160

Files:
  lldb/include/lldb/Host/MainLoopBase.h
  lldb/include/lldb/Host/posix/MainLoopPosix.h
  lldb/include/lldb/Host/windows/MainLoopWindows.h
  lldb/source/Host/common/MainLoopBase.cpp
  lldb/source/Host/posix/MainLoopPosix.cpp
  lldb/source/Host/windows/MainLoopWindows.cpp
  lldb/unittests/Host/MainLoopTest.cpp

Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -148,6 +148,18 @@
   ASSERT_EQ(3u, callback_count);
 }
 
+TEST_F(MainLoopTest, Event) {
+  MainLoop loop;
+  bool event_called = false;
+  auto handle = loop.RegisterEvent([&](MainLoopBase &loop) {
+event_called = true;
+loop.RequestTermination();
+  });
+  handle->Notify();
+  ASSERT_THAT_ERROR(loop.Run().ToError(), llvm::Succeeded());
+  ASSERT_TRUE(event_called);
+}
+
 #ifdef LLVM_ON_UNIX
 TEST_F(MainLoopTest, DetectsEOF) {
 
Index: lldb/source/Host/windows/MainLoopWindows.cpp
===
--- lldb/source/Host/windows/MainLoopWindows.cpp
+++ lldb/source/Host/windows/MainLoopWindows.cpp
@@ -21,9 +21,19 @@
 using namespace lldb;
 using namespace lldb_private;
 
+MainLoopWindows::MainLoopWindows() {
+  m_event_event = WSACreateEvent();
+  assert(m_event_event != WSA_INVALID_EVENT);
+}
+
+MainLoopWindows::~MainLoopWindows() {
+  BOOL result = WSACloseEvent(m_event_event);
+  assert(result == TRUE);
+}
+
 llvm::Expected MainLoopWindows::Poll() {
-  std::vector read_events;
-  read_events.reserve(m_read_fds.size());
+  std::vector events;
+  events.reserve(m_read_fds.size() + 1);
   for (auto &fd : m_read_fds) {
 WSAEVENT event = WSACreateEvent();
 assert(event != WSA_INVALID_EVENT);
@@ -32,24 +42,25 @@
 WSAEventSelect(fd.first, event, FD_READ | FD_ACCEPT | FD_CLOSE);
 assert(result == 0);
 
-read_events.push_back(event);
+events.push_back(event);
   }
+  events.push_back(loop.m_event_event);
 
-  DWORD result = WSAWaitForMultipleEvents(
-  read_events.size(), read_events.data(), FALSE, WSA_INFINITE, FALSE);
+  DWORD result = WSAWaitForMultipleEvents(events.size(), events.data(), FALSE,
+  WSA_INFINITE, FALSE);
 
   for (auto &fd : m_read_fds) {
 int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
 assert(result == 0);
   }
 
-  for (auto &event : read_events) {
+  events.pop_back();
+  for (auto &event : events) {
 BOOL result = WSACloseEvent(event);
 assert(result == TRUE);
   }
 
-  if (result >= WSA_WAIT_EVENT_0 &&
-  result < WSA_WAIT_EVENT_0 + read_events.size())
+  if (result >= WSA_WAIT_EVENT_0 && result <= WSA_WAIT_EVENT_0 + events.size())
 return result - WSA_WAIT_EVENT_0;
 
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
@@ -73,16 +84,29 @@
   Status error;
 
   // run until termination or until we run out of things to listen to
-  while (!m_terminate_request && !m_read_fds.empty()) {
+  while (!m_terminate_request && (!m_read_fds.empty() || !m_events.empty())) {
 
 llvm::Expected signaled_event = Poll();
 if (!signaled_event)
   return Status(signaled_event.takeError());
 
-auto &fd_info = *std::next(m_read_fds.begin(), *signaled_event);
+if (*signaled_event < m_read_fds.size()) {
+  auto &fd_info = *std::next(m_read_fds.begin(), *signaled_event);
+  ProcessReadObject(fd_info.first);
+} else {
+  assert(*signaled_event == m_read_fds.size());
+  // Must be called before events are processed.
+  WSAResetEvent(m_event_event);
+  ProcessEvents();
+}
 
-ProcessReadObject(fd_info.first);
 ProcessPendingCallbacks();
   }
   return Status();
 }
+
+void MainLoopWindows::NotifyEvent(std::list::iterator info_it) {
+  // This must be set before we notify the event.
+  info_it->notified.store(true, std::memory_order_release);
+  WSASetEvent(m_event_event);
+}
Index: lldb/source/Host/posix/MainLoopPosix.cpp
===
--- lldb/source/Host/posix/MainLoopPosix.cpp
+++ lldb/source/Host/posix/MainLoopPosix.cpp
@@ -7,10 +7,12 @@
 //===--===//
 
 #include "lldb/Host/posix/MainLoopPosix.h"
+
 #include "lldb/Host/Config.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Utility/Status.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Errno.h"
 #include 
 #include 
 #include 
@@ -222,6 +224,17 @@
 #endif
 
 MainLoopPosix::MainLoopPosix() {
+  Status error = m_event_pipe.CreateNew(/*child_process_inherit=*/false);
+  assert(error.Success());
+  const int event_pipe_fd = m_event_pi

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 453071.
mgorny added a comment.

Add a test that notifying actually works on top of pending read.


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

https://reviews.llvm.org/D131160

Files:
  lldb/include/lldb/Host/MainLoopBase.h
  lldb/include/lldb/Host/posix/MainLoopPosix.h
  lldb/include/lldb/Host/windows/MainLoopWindows.h
  lldb/source/Host/common/MainLoopBase.cpp
  lldb/source/Host/posix/MainLoopPosix.cpp
  lldb/source/Host/windows/MainLoopWindows.cpp
  lldb/unittests/Host/MainLoopTest.cpp

Index: lldb/unittests/Host/MainLoopTest.cpp
===
--- lldb/unittests/Host/MainLoopTest.cpp
+++ lldb/unittests/Host/MainLoopTest.cpp
@@ -148,6 +148,39 @@
   ASSERT_EQ(3u, callback_count);
 }
 
+TEST_F(MainLoopTest, Event) {
+  MainLoop loop;
+  bool event_called = false;
+  auto handle = loop.RegisterEvent([&](MainLoopBase &loop) {
+event_called = true;
+loop.RequestTermination();
+  });
+  handle->Notify();
+  ASSERT_THAT_ERROR(loop.Run().ToError(), llvm::Succeeded());
+  ASSERT_TRUE(event_called);
+}
+
+TEST_F(MainLoopTest, EventAndPendingRead) {
+  MainLoop loop;
+  bool event_called = false;
+  auto handle = loop.RegisterEvent([&](MainLoopBase &loop) {
+event_called = true;
+loop.RequestTermination();
+  });
+  Status error;
+  auto socket_handle = loop.RegisterReadObject(
+  socketpair[1], [](MainLoopBase &) {}, error);
+  ASSERT_TRUE(error.Success());
+  ASSERT_TRUE(socket_handle);
+  std::thread notifier([&]() {
+sleep(1);
+handle->Notify();
+  });
+  ASSERT_THAT_ERROR(loop.Run().ToError(), llvm::Succeeded());
+  notifier.join();
+  ASSERT_TRUE(event_called);
+}
+
 #ifdef LLVM_ON_UNIX
 TEST_F(MainLoopTest, DetectsEOF) {
 
Index: lldb/source/Host/windows/MainLoopWindows.cpp
===
--- lldb/source/Host/windows/MainLoopWindows.cpp
+++ lldb/source/Host/windows/MainLoopWindows.cpp
@@ -21,9 +21,19 @@
 using namespace lldb;
 using namespace lldb_private;
 
+MainLoopWindows::MainLoopWindows() {
+  m_event_event = WSACreateEvent();
+  assert(m_event_event != WSA_INVALID_EVENT);
+}
+
+MainLoopWindows::~MainLoopWindows() {
+  BOOL result = WSACloseEvent(m_event_event);
+  assert(result == TRUE);
+}
+
 llvm::Expected MainLoopWindows::Poll() {
-  std::vector read_events;
-  read_events.reserve(m_read_fds.size());
+  std::vector events;
+  events.reserve(m_read_fds.size() + 1);
   for (auto &fd : m_read_fds) {
 WSAEVENT event = WSACreateEvent();
 assert(event != WSA_INVALID_EVENT);
@@ -32,24 +42,25 @@
 WSAEventSelect(fd.first, event, FD_READ | FD_ACCEPT | FD_CLOSE);
 assert(result == 0);
 
-read_events.push_back(event);
+events.push_back(event);
   }
+  events.push_back(loop.m_event_event);
 
-  DWORD result = WSAWaitForMultipleEvents(
-  read_events.size(), read_events.data(), FALSE, WSA_INFINITE, FALSE);
+  DWORD result = WSAWaitForMultipleEvents(events.size(), events.data(), FALSE,
+  WSA_INFINITE, FALSE);
 
   for (auto &fd : m_read_fds) {
 int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
 assert(result == 0);
   }
 
-  for (auto &event : read_events) {
+  events.pop_back();
+  for (auto &event : events) {
 BOOL result = WSACloseEvent(event);
 assert(result == TRUE);
   }
 
-  if (result >= WSA_WAIT_EVENT_0 &&
-  result < WSA_WAIT_EVENT_0 + read_events.size())
+  if (result >= WSA_WAIT_EVENT_0 && result <= WSA_WAIT_EVENT_0 + events.size())
 return result - WSA_WAIT_EVENT_0;
 
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
@@ -73,16 +84,29 @@
   Status error;
 
   // run until termination or until we run out of things to listen to
-  while (!m_terminate_request && !m_read_fds.empty()) {
+  while (!m_terminate_request && (!m_read_fds.empty() || !m_events.empty())) {
 
 llvm::Expected signaled_event = Poll();
 if (!signaled_event)
   return Status(signaled_event.takeError());
 
-auto &fd_info = *std::next(m_read_fds.begin(), *signaled_event);
+if (*signaled_event < m_read_fds.size()) {
+  auto &fd_info = *std::next(m_read_fds.begin(), *signaled_event);
+  ProcessReadObject(fd_info.first);
+} else {
+  assert(*signaled_event == m_read_fds.size());
+  // Must be called before events are processed.
+  WSAResetEvent(m_event_event);
+  ProcessEvents();
+}
 
-ProcessReadObject(fd_info.first);
 ProcessPendingCallbacks();
   }
   return Status();
 }
+
+void MainLoopWindows::NotifyEvent(std::list::iterator info_it) {
+  // This must be set before we notify the event.
+  info_it->notified.store(true, std::memory_order_release);
+  WSASetEvent(m_event_event);
+}
Index: lldb/source/Host/posix/MainLoopPosix.cpp
===
--- lldb/source/Host/posix/MainLoopPosix.cpp
+++ lldb/source/Host/posix/

[Lldb-commits] [PATCH] D131705: Don't create sections for SHN_ABS symbols in ELF files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D131705#3726248 , @labath wrote:

> I have somewhat mixed feelings about this.
>
> On one hand, I was never too happy with how we're creating these funny extra 
> sections, so I can't say I would be sad to see that go. OTOH, I can't really 
> agree with the assertion that these symbols do not represent "addresses".  
> All the ELF spec says about them is that they are not subject to relocation. 
> That makes them rather ill-suited for describing the locations of objects in 
> the usual scenarios. However, these symbols can be (and, I believe, are) used 
> in the embedded world for describing objects that are architecturally defined 
> to reside at a particular memory address (the 16-bit dos/bios world was full 
> of those). However, one can imagine something similar being done in userspace 
> as well (for example, if the kernel provides some data at a fixed virtual 
> address). We envisioned something similar when we added support for this. In 
> our case, the object/function in question was generated by a JIT, which knew 
> the exact address at which it placed the jitted code.

I am fine trying to see if these symbols exist within an existing section and 
if they do, then resolve the symbol as a section + offset within an existing 
section value if you think that is the way to go. If they don't exist in any 
sections, then do what I am doing by just making them have no section. But 
really, why would you ever use a SHN_ABS symbol when you can make a real symbol 
that _is_ defined to be in a section? That is what puzzled me. The 
documentation in the ELF was not helpful, as you stated it just says these 
symbols have no relocations. but if you do have such a symbol, then if the 
section it does exist in does get relocated or moved at runtime, what good is 
the symbol and why not create an actual symbol in an actual section?

> However, AFAIK, that project never materialized, and we are not relying on 
> these symbols for that. Tagging @dsrbecky to confirm. Assuming that's the 
> case, and given that gdb does not handle these symbols in this way either (at 
> least, I haven't been able to make it do that), I think we can go forward 
> with removing them.

When you say "we can go forward with removing them", do you mean not add them 
at all as symbols? I wouldn't want to remove a symbol, I would rather keep it 
like I have added it now if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131705

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

[Lldb-commits] [PATCH] D131705: Don't create sections for SHN_ABS symbols in ELF files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:28-29
+and this caused problems as the new sections could interfere with
+with address resolution. Absolute symbols' values are not addresses
+and should not be treated this way.
+

labath wrote:
> As I said in the overall comment, I cannot agree with this statement without 
> reservations. 
> 
> And in any case, I think prose like this is better left for the commit 
> message. The test should just state what it does (although that's fairly 
> obvious here), not what it used to do.
I can remove this part of the comment.



Comment at: lldb/test/API/python_api/absolute_symbol/TestAbsoluteSymbol.py:76-80
+# Make sure no sections were created for the absolute symbol with a
+# prefix of ".absolute." followed by the symbol name as they interfere
+# with address lookups if they are treated like real sections.
+for section in module.sections:
+self.assertTrue(section.GetName() != ".absolute.absolute")

labath wrote:
> This check isn't particularly useful, since you've completely deleted the 
> code which created those sections. If something similar is ever reintroduced, 
> it's quite possible it would use a different section name, and so it wouldn't 
> catch this.
> 
> I don't think it's necessary, but if you want, I think it'd be better to run 
> something like `image lookup -a 0x8000` and check that it does 
> not produce any matches, as that's what you're really interested in.
Sounds good as long as we don't start trying to resolve the symbol as mentioned


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131705

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

[Lldb-commits] [PATCH] D131900: [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks great, maybe fix the pass by value in the callback and this is good?




Comment at: 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp:230
+std::function const &callback) {
+  auto array_sp = std::make_shared();

Pass by reference to avoid an increment and decrement in the shared pointer due 
to a copy



Comment at: 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp:347-348
->GetValueAsUnsigned(0));
-  dict->AddItem("sleep_trace", StructuredData::ObjectSP(CreateStackTrace(
-   main_value, ".sleep_trace")));
+  dict->AddItem("sleep_trace", CreateStackTrace(
+   main_value, ".sleep_trace"));
 

combine this line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131900

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

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

[Slava Gurevich via Phabricator via lldb-commits]

fixathon created this revision.
fixathon added reviewers: clayborg, JDevlieghere, DavidSpickett, jasonmolenda.
Herald added a project: All.
fixathon published this revision for review.
fixathon added inline comments.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.



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

If getAsInteger() fails we trigger the error path.
If getAsInteger() succeeds, but the fetched value (side-effect) is negative we 
trigger the error path.

I'm not 100% happy with my own code here..
getAsInteger() somewhat counter-intuitively returns *true* on failure, and 
there's reliance on the short-circuit evaluation sequence-points for correct 
evaluation when the 1st part is setting value via side-effect, and the second 
validates it. This makes the expression somewhat hard to reason about.

Also mutating **m_count** directly may result in a non-atomic transition to its 
new valid state in case when getAsInteger() fetches negative input, which is 
less than perfect.

That said, this code is just parsing command options, in a single-threaded 
manner, so that shouldn't matter.



Comment at: lldb/source/Commands/CommandObjectThread.cpp:71
   "invalid integer value for option '%c'", short_option);
-} else if (input_count < 0)
-  m_count = UINT32_MAX;

Note how we check **input_count** that's never been set (beyond the initial 
value). This is broken.


- Improve reliability by checking return results for calls to 
FindLineEntryByAddress()
- Fix broken option parsing in SetOptionValue()


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131983

Files:
  lldb/source/Commands/CommandObjectThread.cpp


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c': {
-int32_t input_count = 0;
-if (option_arg.getAsInteger(0, m_count)) {
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '%c'", short_option);
-} else if (input_count < 0)
-  m_count = UINT32_MAX;
-  } break;
+}
+break;
   case 's':
 if (option_arg.getAsInteger(0, m_start))
   error.SetErrorStringWithFormat(
@@ -1004,8 +1002,15 @@
 
 AddressRange fun_addr_range = sc.function->GetAddressRange();
 Address fun_start_addr = fun_addr_range.GetBaseAddress();
-line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-   &index_ptr);
+
+if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+&index_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 Address fun_end_addr(fun_start_addr.GetSection(),
  fun_start_addr.GetOffset() +
@@ -1013,8 +1018,14 @@
 
 bool all_in_function = true;
 
-line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-   &end_ptr);
+if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+&end_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 // Since not all source lines will contribute code, check if we are
 // setting the breakpoint on the exact line number or the nearest


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c': {
-int32_t input_count = 0;
-if (option_arg.getAsInteger(0, m_count)) {
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '

[Lldb-commits] [PATCH] D126615: [lldb] Add an integration test for non-stop protocol

[Ed Maste via Phabricator via lldb-commits]

emaste added a comment.

Looks reasonable to me


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

https://reviews.llvm.org/D126615

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

[Lldb-commits] [lldb] 50630dc - [lldb] Fix warnings

[Kazu Hirata via lldb-commits]

Author: Kazu Hirata
Date: 2022-08-16T12:33:21-07:00
New Revision: 50630dcc4c8ab5de26e153a948577c4cc743b722

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

LOG: [lldb] Fix warnings

This patch fixes:

  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h:34:5:
  error: default label in switch which covers all enumeration values
  [-Werror,-Wcovered-switch-default]

and:

  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp:194:21:
  error: comparison of integers of different signs: 'int' and 'size_t'
  (aka 'unsigned long') [-Werror,-Wsign-compare]

Added: 


Modified: 
lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h

Removed: 




diff  --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 458b996d6ff62..10aefc6d9dc08 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -191,8 +191,7 @@ static InstrPattern PATTERNS[] = {
 bool EmulateInstructionRISCV::DecodeAndExecute(uint32_t inst,
bool ignore_cond) {
   Log *log = GetLog(LLDBLog::Process | LLDBLog::Breakpoints);
-  for (int i = 0; i < llvm::array_lengthof(PATTERNS); ++i) {
-const InstrPattern &pat = PATTERNS[i];
+  for (const InstrPattern &pat : PATTERNS) {
 if ((inst & pat.type_mask) == pat.eigen) {
   LLDB_LOGF(log, "EmulateInstructionRISCV::%s: inst(%x) was decoded to %s",
 __FUNCTION__, inst, pat.name);

diff  --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
index 6df9a7d409a75..39f7c429106bc 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
@@ -31,7 +31,6 @@ class EmulateInstructionRISCV : public EmulateInstruction {
   return true;
 case eInstructionTypePrologueEpilogue:
 case eInstructionTypeAll:
-default:
   return false;
 }
   }



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

[Lldb-commits] [PATCH] D131900: [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[Slava Gurevich via Phabricator via lldb-commits]

fixathon updated this revision to Diff 453109.
fixathon added a comment.

Address comments about passing-by-ref for perf


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131900

Files:
  lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

Index: lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -206,10 +206,10 @@
 t;
 )";
 
-static StructuredData::Array *
+static StructuredData::ArraySP
 CreateStackTrace(ValueObjectSP o,
  const std::string &trace_item_name = ".trace") {
-  StructuredData::Array *trace = new StructuredData::Array();
+  auto trace_sp = std::make_shared();
   ValueObjectSP trace_value_object =
   o->GetValueForExpressionPath(trace_item_name.c_str());
   size_t count = trace_value_object->GetNumChildren();
@@ -218,18 +218,18 @@
 trace_value_object->GetChildAtIndex(j, true)->GetValueAsUnsigned(0);
 if (trace_addr == 0)
   break;
-trace->AddItem(
-StructuredData::ObjectSP(new StructuredData::Integer(trace_addr)));
+trace_sp->AddItem(std::make_shared(trace_addr));
   }
-  return trace;
+  return trace_sp;
 }
 
-static StructuredData::Array *ConvertToStructuredArray(
+static StructuredData::ArraySP ConvertToStructuredArray(
 ValueObjectSP return_value_sp, const std::string &items_name,
 const std::string &count_name,
-std::function const
+std::function const
 &callback) {
-  StructuredData::Array *array = new StructuredData::Array();
+  auto array_sp = std::make_shared();
   unsigned int count =
   return_value_sp->GetValueForExpressionPath(count_name.c_str())
   ->GetValueAsUnsigned(0);
@@ -237,13 +237,13 @@
   return_value_sp->GetValueForExpressionPath(items_name.c_str());
   for (unsigned int i = 0; i < count; i++) {
 ValueObjectSP o = objects->GetChildAtIndex(i, true);
-StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+auto dict_sp = std::make_shared();
 
-callback(o, dict);
+callback(o, dict_sp);
 
-array->AddItem(StructuredData::ObjectSP(dict));
+array_sp->AddItem(dict_sp);
   }
-  return array;
+  return array_sp;
 }
 
 static std::string RetrieveString(ValueObjectSP return_value_sp,
@@ -263,8 +263,8 @@
std::map &thread_id_map) {
   ConvertToStructuredArray(
   data, ".threads", ".thread_count",
-  [process_sp, &thread_id_map](ValueObjectSP o,
-   StructuredData::Dictionary *dict) {
+  [process_sp, &thread_id_map](const ValueObjectSP &o,
+   const StructuredData::DictionarySP &dict) {
 uint64_t thread_id =
 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0);
 uint64_t thread_os_id =
@@ -338,31 +338,33 @@
   std::map thread_id_map;
   GetRenumberedThreadIds(process_sp, main_value, thread_id_map);
 
-  StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+  auto dict = std::make_shared();
   dict->AddStringItem("instrumentation_class", "ThreadSanitizer");
   dict->AddStringItem("issue_type",
   RetrieveString(main_value, process_sp, ".description"));
   dict->AddIntegerItem("report_count",
main_value->GetValueForExpressionPath(".report_count")
->GetValueAsUnsigned(0));
-  dict->AddItem("sleep_trace", StructuredData::ObjectSP(CreateStackTrace(
-   main_value, ".sleep_trace")));
+  dict->AddItem("sleep_trace", CreateStackTrace(
+   main_value, ".sleep_trace"));
 
-  StructuredData::Array *stacks = ConvertToStructuredArray(
+  StructuredData::ArraySP stacks = ConvertToStructuredArray(
   main_value, ".stacks", ".stack_count",
-  [thread_sp](ValueObjectSP o, StructuredData::Dictionary *dict) {
+  [thread_sp](const ValueObjectSP &o,
+  const StructuredData::DictionarySP &dict) {
 dict->AddIntegerItem(
 "index",
 o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
-dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+dict->AddItem("trace", CreateStackTrace(o));
 // "stacks" happen on the current thread
 dict->AddIntegerItem("thread_id", thread_sp->GetIndexID());
   });
-  dict->AddItem("stacks", StructuredData::ObjectSP(stacks));
+  dict->AddItem("stacks", stacks);
 
-  StructuredData::Array *mops = ConvertToStructuredArray(
+  StructuredData::ArraySP mops = ConvertToStructuredArray(
   main_value, ".mops", ".mop_count",
-  [&thread_id_map](ValueObjectSP o, StructuredData::D

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

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



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

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

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [PATCH] D131996: Use a SmallPtrSet rather than a SmallVector in ClusterManager.

[Jim Ingham via Phabricator via lldb-commits]

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

The m_objects store in ClusterManager holds pointers to all the objects
managed by the ClusterManager.  It is used to check whether
this ClusterManager already owns this pointer before adding it to
the ClusterManager's shared pointer or getting the shared pointer for the 
object.  
When the ClusterManager is deleted, we iterate over it to destroy all the 
managed objects.
The most common operation by far is "GetSharedPointer" on the cluster.

With a SmallVector and llvm::is_contained the performance was non-linear
in the number of elements.  For instance, printing all the elements of a 16M 
element std::vector
didn't complete in the time I was willing to wait for it (hours).

  

Since we are mostly doing insert & contains, some kind of set is likely to be a
better data structure.  In this patch I used SmallPtrSet.  With
that, the same array prints out in 30 seconds.  I couldn't see any noticeable 
difference
for ValueObjects with a small number of children.  I also tried a the same 
patch using a
std::unordered_set but that was slightly slower and used a fair bit
more memory than the SmallPtrSet.

  

Other than performance, this is NFC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131996

Files:
  lldb/include/lldb/Utility/SharedCluster.h


Index: lldb/include/lldb/Utility/SharedCluster.h
===
--- lldb/include/lldb/Utility/SharedCluster.h
+++ lldb/include/lldb/Utility/SharedCluster.h
@@ -11,7 +11,7 @@
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 #include 
 #include 
@@ -32,15 +32,15 @@
 
   void ManageObject(T *new_object) {
 std::lock_guard guard(m_mutex);
-assert(!llvm::is_contained(m_objects, new_object) &&
-   "ManageObject called twice for the same object?");
-m_objects.push_back(new_object);
+auto ret = m_objects.insert(new_object);
+assert(ret.second && "ManageObject called twice for the same object?");
   }
 
   std::shared_ptr GetSharedPointer(T *desired_object) {
 std::lock_guard guard(m_mutex);
 auto this_sp = this->shared_from_this();
-if (!llvm::is_contained(m_objects, desired_object)) {
+size_t count =  m_objects.count(desired_object);
+if (count == 0) {
   lldbassert(false && "object not found in shared cluster when expected");
   desired_object = nullptr;
 }
@@ -49,8 +49,7 @@
 
 private:
   ClusterManager() : m_objects() {}
-
-  llvm::SmallVector m_objects;
+  llvm::SmallPtrSet m_objects;
   std::mutex m_mutex;
 };
 


Index: lldb/include/lldb/Utility/SharedCluster.h
===
--- lldb/include/lldb/Utility/SharedCluster.h
+++ lldb/include/lldb/Utility/SharedCluster.h
@@ -11,7 +11,7 @@
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 #include 
 #include 
@@ -32,15 +32,15 @@
 
   void ManageObject(T *new_object) {
 std::lock_guard guard(m_mutex);
-assert(!llvm::is_contained(m_objects, new_object) &&
-   "ManageObject called twice for the same object?");
-m_objects.push_back(new_object);
+auto ret = m_objects.insert(new_object);
+assert(ret.second && "ManageObject called twice for the same object?");
   }
 
   std::shared_ptr GetSharedPointer(T *desired_object) {
 std::lock_guard guard(m_mutex);
 auto this_sp = this->shared_from_this();
-if (!llvm::is_contained(m_objects, desired_object)) {
+size_t count =  m_objects.count(desired_object);
+if (count == 0) {
   lldbassert(false && "object not found in shared cluster when expected");
   desired_object = nullptr;
 }
@@ -49,8 +49,7 @@
 
 private:
   ClusterManager() : m_objects() {}
-
-  llvm::SmallVector m_objects;
+  llvm::SmallPtrSet m_objects;
   std::mutex m_mutex;
 };
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 1fe7200 - [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[Slava Gurevich via lldb-commits]

Author: Slava Gurevich
Date: 2022-08-16T14:34:50-07:00
New Revision: 1fe72001e8d69cae1975a360fee055c2fd3730f4

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

LOG: [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

ConvertToStructuredArray() relies on its caller to deallocate the 
heap-allocated object pointer it returns. One of its call-sites, in 
GetRenumberedThreadIds(), fails to deallocate causing a memory/resource leak. 
Fix the memory leak by converting the return type to shared_ptr, and clean up 
the rest of the file to use the typedef-ed shared_ptr types for StructuredData 
for safety and consistency.

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

Added: 


Modified: 

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
 
b/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
index 55ef3d245411f..910992c48a7dc 100644
--- 
a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ 
b/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -206,10 +206,10 @@ for (int i = 0; i < t.unique_tid_count; i++) {
 t;
 )";
 
-static StructuredData::Array *
+static StructuredData::ArraySP
 CreateStackTrace(ValueObjectSP o,
  const std::string &trace_item_name = ".trace") {
-  StructuredData::Array *trace = new StructuredData::Array();
+  auto trace_sp = std::make_shared();
   ValueObjectSP trace_value_object =
   o->GetValueForExpressionPath(trace_item_name.c_str());
   size_t count = trace_value_object->GetNumChildren();
@@ -218,18 +218,18 @@ CreateStackTrace(ValueObjectSP o,
 trace_value_object->GetChildAtIndex(j, true)->GetValueAsUnsigned(0);
 if (trace_addr == 0)
   break;
-trace->AddItem(
-StructuredData::ObjectSP(new StructuredData::Integer(trace_addr)));
+trace_sp->AddItem(std::make_shared(trace_addr));
   }
-  return trace;
+  return trace_sp;
 }
 
-static StructuredData::Array *ConvertToStructuredArray(
+static StructuredData::ArraySP ConvertToStructuredArray(
 ValueObjectSP return_value_sp, const std::string &items_name,
 const std::string &count_name,
-std::function 
const
+std::function const
 &callback) {
-  StructuredData::Array *array = new StructuredData::Array();
+  auto array_sp = std::make_shared();
   unsigned int count =
   return_value_sp->GetValueForExpressionPath(count_name.c_str())
   ->GetValueAsUnsigned(0);
@@ -237,13 +237,13 @@ static StructuredData::Array *ConvertToStructuredArray(
   return_value_sp->GetValueForExpressionPath(items_name.c_str());
   for (unsigned int i = 0; i < count; i++) {
 ValueObjectSP o = objects->GetChildAtIndex(i, true);
-StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+auto dict_sp = std::make_shared();
 
-callback(o, dict);
+callback(o, dict_sp);
 
-array->AddItem(StructuredData::ObjectSP(dict));
+array_sp->AddItem(dict_sp);
   }
-  return array;
+  return array_sp;
 }
 
 static std::string RetrieveString(ValueObjectSP return_value_sp,
@@ -263,8 +263,8 @@ GetRenumberedThreadIds(ProcessSP process_sp, ValueObjectSP 
data,
std::map &thread_id_map) {
   ConvertToStructuredArray(
   data, ".threads", ".thread_count",
-  [process_sp, &thread_id_map](ValueObjectSP o,
-   StructuredData::Dictionary *dict) {
+  [process_sp, &thread_id_map](const ValueObjectSP &o,
+   const StructuredData::DictionarySP &dict) {
 uint64_t thread_id =
 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0);
 uint64_t thread_os_id =
@@ -338,31 +338,33 @@ StructuredData::ObjectSP 
InstrumentationRuntimeTSan::RetrieveReportData(
   std::map thread_id_map;
   GetRenumberedThreadIds(process_sp, main_value, thread_id_map);
 
-  StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+  auto dict = std::make_shared();
   dict->AddStringItem("instrumentation_class", "ThreadSanitizer");
   dict->AddStringItem("issue_type",
   RetrieveString(main_value, process_sp, ".description"));
   dict->AddIntegerItem("report_count",
main_value->GetValueForExpressionPath(".report_count")
->GetValueAsUnsigned(0));
-  dict->AddItem("sleep_trace", StructuredData::ObjectSP(CreateStackTrace(
-   main_value, ".sleep_trace")));
+  dict->AddItem("sleep_trace", CreateStackTrace(
+   main_value, ".sleep_trace"));
 
-  StructuredData::Array

[Lldb-commits] [PATCH] D131900: [LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[Slava Gurevich via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1fe72001e8d6: [LLDB][NFC] Fix memory leak in 
IntstumentationRuntimeTSan.cpp (authored by fixathon).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131900

Files:
  lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

Index: lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -206,10 +206,10 @@
 t;
 )";
 
-static StructuredData::Array *
+static StructuredData::ArraySP
 CreateStackTrace(ValueObjectSP o,
  const std::string &trace_item_name = ".trace") {
-  StructuredData::Array *trace = new StructuredData::Array();
+  auto trace_sp = std::make_shared();
   ValueObjectSP trace_value_object =
   o->GetValueForExpressionPath(trace_item_name.c_str());
   size_t count = trace_value_object->GetNumChildren();
@@ -218,18 +218,18 @@
 trace_value_object->GetChildAtIndex(j, true)->GetValueAsUnsigned(0);
 if (trace_addr == 0)
   break;
-trace->AddItem(
-StructuredData::ObjectSP(new StructuredData::Integer(trace_addr)));
+trace_sp->AddItem(std::make_shared(trace_addr));
   }
-  return trace;
+  return trace_sp;
 }
 
-static StructuredData::Array *ConvertToStructuredArray(
+static StructuredData::ArraySP ConvertToStructuredArray(
 ValueObjectSP return_value_sp, const std::string &items_name,
 const std::string &count_name,
-std::function const
+std::function const
 &callback) {
-  StructuredData::Array *array = new StructuredData::Array();
+  auto array_sp = std::make_shared();
   unsigned int count =
   return_value_sp->GetValueForExpressionPath(count_name.c_str())
   ->GetValueAsUnsigned(0);
@@ -237,13 +237,13 @@
   return_value_sp->GetValueForExpressionPath(items_name.c_str());
   for (unsigned int i = 0; i < count; i++) {
 ValueObjectSP o = objects->GetChildAtIndex(i, true);
-StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+auto dict_sp = std::make_shared();
 
-callback(o, dict);
+callback(o, dict_sp);
 
-array->AddItem(StructuredData::ObjectSP(dict));
+array_sp->AddItem(dict_sp);
   }
-  return array;
+  return array_sp;
 }
 
 static std::string RetrieveString(ValueObjectSP return_value_sp,
@@ -263,8 +263,8 @@
std::map &thread_id_map) {
   ConvertToStructuredArray(
   data, ".threads", ".thread_count",
-  [process_sp, &thread_id_map](ValueObjectSP o,
-   StructuredData::Dictionary *dict) {
+  [process_sp, &thread_id_map](const ValueObjectSP &o,
+   const StructuredData::DictionarySP &dict) {
 uint64_t thread_id =
 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0);
 uint64_t thread_os_id =
@@ -338,31 +338,33 @@
   std::map thread_id_map;
   GetRenumberedThreadIds(process_sp, main_value, thread_id_map);
 
-  StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+  auto dict = std::make_shared();
   dict->AddStringItem("instrumentation_class", "ThreadSanitizer");
   dict->AddStringItem("issue_type",
   RetrieveString(main_value, process_sp, ".description"));
   dict->AddIntegerItem("report_count",
main_value->GetValueForExpressionPath(".report_count")
->GetValueAsUnsigned(0));
-  dict->AddItem("sleep_trace", StructuredData::ObjectSP(CreateStackTrace(
-   main_value, ".sleep_trace")));
+  dict->AddItem("sleep_trace", CreateStackTrace(
+   main_value, ".sleep_trace"));
 
-  StructuredData::Array *stacks = ConvertToStructuredArray(
+  StructuredData::ArraySP stacks = ConvertToStructuredArray(
   main_value, ".stacks", ".stack_count",
-  [thread_sp](ValueObjectSP o, StructuredData::Dictionary *dict) {
+  [thread_sp](const ValueObjectSP &o,
+  const StructuredData::DictionarySP &dict) {
 dict->AddIntegerItem(
 "index",
 o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
-dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+dict->AddItem("trace", CreateStackTrace(o));
 // "stacks" happen on the current thread
 dict->AddIntegerItem("thread_id", thread_sp->GetIndexID());
   });
-  dict->AddItem("stacks", StructuredData::ObjectSP(stacks));
+  dict->AddItem("stacks", stacks);
 
-  StructuredData::Array *mops = ConvertToStructuredArray(
+  StructuredData::ArraySP mops = ConvertToStructuredArray(
   main_value, ".mops", 

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



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

hawkinsw wrote:
> fixathon wrote:
> > If getAsInteger() fails we trigger the error path.
> > If getAsInteger() succeeds, but the fetched value (side-effect) is negative 
> > we trigger the error path.
> > 
> > I'm not 100% happy with my own code here..
> > getAsInteger() somewhat counter-intuitively returns *true* on failure, and 
> > there's reliance on the short-circuit evaluation sequence-points for 
> > correct evaluation when the 1st part is setting value via side-effect, and 
> > the second validates it. This makes the expression somewhat hard to reason 
> > about.
> > 
> > Also mutating **m_count** directly may result in a non-atomic transition to 
> > its new valid state in case when getAsInteger() fetches negative input, 
> > which is less than perfect.
> > 
> > That said, this code is just parsing command options, in a single-threaded 
> > manner, so that shouldn't matter.
> I am not sure if this helps or not, but ...
> 
> `getAsInteger` is the exact interface that the LLVM command-line parsing 
> library uses for parsing numeric options. I looked through the codebase for 
> other places that do the same type of operation to see if they use a 
> different technique and could not find anything. 
> 
> To address your non-atomic comment, we could introduce a temporary? Given 
> your description of how this code will be used (single-threaded), that seems 
> like overkill? 
> 
> I am no expert and I hope that being part of the discussion is helpful. 
Can we rely on ordering here? Or do we need to have two if statements, first 
one for getAsInteger and a second nested on for "m_count  < 0"? I would be safe 
if we don't know the right answer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

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

[Greg Clayton via Phabricator via lldb-commits]

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



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

clayborg wrote:
> hawkinsw wrote:
> > fixathon wrote:
> > > If getAsInteger() fails we trigger the error path.
> > > If getAsInteger() succeeds, but the fetched value (side-effect) is 
> > > negative we trigger the error path.
> > > 
> > > I'm not 100% happy with my own code here..
> > > getAsInteger() somewhat counter-intuitively returns *true* on failure, 
> > > and there's reliance on the short-circuit evaluation sequence-points for 
> > > correct evaluation when the 1st part is setting value via side-effect, 
> > > and the second validates it. This makes the expression somewhat hard to 
> > > reason about.
> > > 
> > > Also mutating **m_count** directly may result in a non-atomic transition 
> > > to its new valid state in case when getAsInteger() fetches negative 
> > > input, which is less than perfect.
> > > 
> > > That said, this code is just parsing command options, in a 
> > > single-threaded manner, so that shouldn't matter.
> > I am not sure if this helps or not, but ...
> > 
> > `getAsInteger` is the exact interface that the LLVM command-line parsing 
> > library uses for parsing numeric options. I looked through the codebase for 
> > other places that do the same type of operation to see if they use a 
> > different technique and could not find anything. 
> > 
> > To address your non-atomic comment, we could introduce a temporary? Given 
> > your description of how this code will be used (single-threaded), that 
> > seems like overkill? 
> > 
> > I am no expert and I hope that being part of the discussion is helpful. 
> Can we rely on ordering here? Or do we need to have two if statements, first 
> one for getAsInteger and a second nested on for "m_count  < 0"? I would be 
> safe if we don't know the right answer
Actually this should work just fine. Otherwise things like "if (ptr && 
ptr->...)" wouldn't work. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [lldb] 461b410 - [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

[Slava Gurevich via lldb-commits]

Author: Slava Gurevich
Date: 2022-08-16T15:30:25-07:00
New Revision: 461b410159426fdc6da77e0fb653737e04e0ebe9

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

LOG: [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

* Improve reliability by checking return results for calls to 
FindLineEntryByAddress()
* Fix broken option parsing in SetOptionValue()

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectThread.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectThread.cpp 
b/lldb/source/Commands/CommandObjectThread.cpp
index 5d7f3c16f3894..1e317d8bfdbe8 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@ class CommandObjectThreadBacktrace : public 
CommandObjectIterateOverThreads {
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c': {
-int32_t input_count = 0;
-if (option_arg.getAsInteger(0, m_count)) {
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '%c'", short_option);
-} else if (input_count < 0)
-  m_count = UINT32_MAX;
-  } break;
+}
+break;
   case 's':
 if (option_arg.getAsInteger(0, m_start))
   error.SetErrorStringWithFormat(
@@ -1004,8 +1002,15 @@ class CommandObjectThreadUntil : public 
CommandObjectParsed {
 
 AddressRange fun_addr_range = sc.function->GetAddressRange();
 Address fun_start_addr = fun_addr_range.GetBaseAddress();
-line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-   &index_ptr);
+
+if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+&index_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 Address fun_end_addr(fun_start_addr.GetSection(),
  fun_start_addr.GetOffset() +
@@ -1013,8 +1018,14 @@ class CommandObjectThreadUntil : public 
CommandObjectParsed {
 
 bool all_in_function = true;
 
-line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-   &end_ptr);
+if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+&end_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 // Since not all source lines will contribute code, check if we are
 // setting the breakpoint on the exact line number or the nearest



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

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

[Slava Gurevich via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
fixathon marked 3 inline comments as done.
Closed by commit rG461b41015942: [LLDB][NFC] Fix optons parsing and misc. 
reliability in CommandObjectThread (authored by fixathon).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

Files:
  lldb/source/Commands/CommandObjectThread.cpp


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c': {
-int32_t input_count = 0;
-if (option_arg.getAsInteger(0, m_count)) {
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '%c'", short_option);
-} else if (input_count < 0)
-  m_count = UINT32_MAX;
-  } break;
+}
+break;
   case 's':
 if (option_arg.getAsInteger(0, m_start))
   error.SetErrorStringWithFormat(
@@ -1004,8 +1002,15 @@
 
 AddressRange fun_addr_range = sc.function->GetAddressRange();
 Address fun_start_addr = fun_addr_range.GetBaseAddress();
-line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-   &index_ptr);
+
+if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+&index_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 Address fun_end_addr(fun_start_addr.GetSection(),
  fun_start_addr.GetOffset() +
@@ -1013,8 +1018,14 @@
 
 bool all_in_function = true;
 
-line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-   &end_ptr);
+if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+&end_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 // Since not all source lines will contribute code, check if we are
 // setting the breakpoint on the exact line number or the nearest


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c': {
-int32_t input_count = 0;
-if (option_arg.getAsInteger(0, m_count)) {
+  case 'c':
+if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '%c'", short_option);
-} else if (input_count < 0)
-  m_count = UINT32_MAX;
-  } break;
+}
+break;
   case 's':
 if (option_arg.getAsInteger(0, m_start))
   error.SetErrorStringWithFormat(
@@ -1004,8 +1002,15 @@
 
 AddressRange fun_addr_range = sc.function->GetAddressRange();
 Address fun_start_addr = fun_addr_range.GetBaseAddress();
-line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-   &index_ptr);
+
+if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+&index_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to find line entry by address for "
+  "frame %u of thread id %" PRIu64 ".\n",
+  m_options.m_frame_idx, thread->GetID());
+  return false;
+}
 
 Address fun_end_addr(fun_start_addr.GetSection(),
  fun_start_addr.GetOffset() +
@@ -1013,8 +1018,14 @@
 
 bool all_in_function = true;
 
-line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-   &end_ptr);
+if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+&end_ptr)) {
+  result.AppendErrorWithFormat(
+  "Failed to

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

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [PATCH] D131996: Use a SmallPtrSet rather than a SmallVector in ClusterManager.

[Dave Lee via Phabricator via lldb-commits]

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

+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131996

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

[Lldb-commits] [PATCH] D131996: Use a SmallPtrSet rather than a SmallVector in ClusterManager.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM modulo inline comment.




Comment at: lldb/include/lldb/Utility/SharedCluster.h:52
   ClusterManager() : m_objects() {}
-
-  llvm::SmallVector m_objects;
+  llvm::SmallPtrSet m_objects;
   std::mutex m_mutex;

I assume pointers cannot be modified once they're in the set. Can this be 
`const T *`? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131996

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

[Lldb-commits] [PATCH] D131605: [lldb][tests] Test queue-specific breakpoints

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/test/API/macosx/queues/TestQueues.py:131
 
+def check_queue_breakpoints(self, queue1, queue2, queue_breakpoint):
+queue1_thread = queue1.GetThreadAtIndex(0)

Any reason this should be a separate function? Can this be inlined in 
`queue_specific_breakpoints`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131605

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

[Lldb-commits] [PATCH] D131605: [lldb][tests] Test queue-specific breakpoints

[Chelsea Cassanova via Phabricator via lldb-commits]

cassanova added inline comments.



Comment at: lldb/test/API/macosx/queues/TestQueues.py:131
 
+def check_queue_breakpoints(self, queue1, queue2, queue_breakpoint):
+queue1_thread = queue1.GetThreadAtIndex(0)

JDevlieghere wrote:
> Any reason this should be a separate function? Can this be inlined in 
> `queue_specific_breakpoints`?
It can be inlined, the reason I had it in a separate function is that it 
matched the rest of the test in having the assertions be in their own functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131605

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

[Lldb-commits] [PATCH] D131998: [LLDB][NFC] Suppress spurious static inspection warnings

[Slava Gurevich via Phabricator via lldb-commits]

fixathon created this revision.
fixathon added a reviewer: clayborg.
Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, 
s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, johnrusso, rbar, asb.
Herald added a project: All.
fixathon requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead.
Herald added a project: LLDB.

Suppress coverity false positives.
This diff contains comments only, including the hints for Coverity static code 
inspection
to suppress the warning originating at the next line after the comment.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131998

Files:
  lldb/include/lldb/Core/ThreadSafeValue.h
  lldb/source/Host/common/ProcessRunLock.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/tools/lldb-vscode/FifoFiles.cpp

Index: lldb/tools/lldb-vscode/FifoFiles.cpp
===
--- lldb/tools/lldb-vscode/FifoFiles.cpp
+++ lldb/tools/lldb-vscode/FifoFiles.cpp
@@ -62,6 +62,12 @@
   line = buffer;
   }));
   if (future->wait_for(timeout) == std::future_status::timeout || !line)
+/* Indeed this is a leak, but it's intentional. "future" obj destructor
+  will block in waiting for the worker thread to join. And the worker
+  thread might be stuck in blocking I/O. Intentionally leaking the  obj
+  as a hack to avoid blocking main thread, and adding annotation to
+  supress static code inspection warnings" */
+/* coverity[leaked_storage] */
 return createStringError(inconvertibleErrorCode(),
  "Timed out trying to get messages from the " +
  m_other_endpoint_name);
@@ -79,6 +85,12 @@
 done = true;
   }));
   if (future->wait_for(timeout) == std::future_status::timeout || !done) {
+/* Indeed this is a leak, but it's intentional. "future" obj destructor will
+ * block in waiting for the worker thread to join. And the worker thread
+ * might be stuck in blocking I/O. Intentionally leaking the  obj as a hack
+ * to avoid blocking main thread, and adding annotation to supress static
+ * code inspection warnings" */
+/* coverity[leaked_storage] */
 return createStringError(inconvertibleErrorCode(),
  "Timed out trying to send messages to the " +
  m_other_endpoint_name);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -304,7 +304,7 @@
 response.PutChar(';');
   }
 #endif // #if defined(__APPLE__)
-
+  /* coverity[unsigned_compare] */
   if (g_default_packet_timeout_sec > 0)
 response.Printf("default_packet_timeout:%u;", g_default_packet_timeout_sec);
 
Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp
@@ -126,6 +126,7 @@
 
 size_t RegisterInfoPOSIX_riscv64::GetRegisterSetFromRegisterIndex(
 uint32_t reg_index) const {
+  /* coverity[unsigned_compare]
   if (reg_index >= gpr_first_riscv && reg_index <= gpr_last_riscv)
 return GPRegSet;
   if (reg_index >= fpr_first_riscv && reg_index <= fpr_last_riscv)
Index: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
===
--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
+++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
@@ -105,6 +105,7 @@
   uint32_t ConfigureVectorLength(uint32_t sve_vq);
 
   bool VectorSizeIsValid(uint32_t vq) {
+/* coverity[unsigned_compare] */
 if (vq >= eVectorQuadwordAArch64 && vq <= eVectorQuadwordAArch64SVEMax)
   return true;
 return false;
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6345,6 +6345,7 @@
 // The segment name in a Mach-O LC_SEGMENT/LC_SEGMENT_64 is char[16] and
 // is not guaranteed to be nul-terminated if all 16 characters are
 // used.
+/* coverity[buffer_size_warning] */
 strncpy(seg_vmaddr.segname, name

[Lldb-commits] [PATCH] D131998: [LLDB][NFC] Suppress spurious static inspection warnings

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Can we use C++ comments instead of C comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131998

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

[Lldb-commits] [PATCH] D132004: [lldb][Tests] Skip static-only tests in TestConstStaticIntegralMember.py for dsym variant

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added reviewers: aprantl, fdeazeve.
Herald added a reviewer: JDevlieghere.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This test fails for Clang versions < 14.0 for `dsym` variants.
`dsymutil` strips debug info for classes with only static members.
Thus move the failing assertions into the XFAIL test case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132004

Files:
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py


Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -67,12 +67,6 @@
 self.expect_expr("A::scoped_ll_enum_val_neg", result_value="case0")
 self.expect_expr("A::scoped_ll_enum_val", result_value="case2")
 
-# Test an aliased enum with fixed underlying type.
-self.expect_expr("ClassWithEnumAlias::enum_alias",
- result_value="scoped_enum_case2")
-self.expect_expr("ClassWithEnumAlias::enum_alias_alias",
- result_value="scoped_enum_case1")
-
 # Test taking address.
 if lldbplatformutil.getPlatform() == "windows":
 # On Windows data members without the out-of-class definitions 
still have
@@ -102,3 +96,9 @@
 self.expect_expr("ClassWithConstexprs::member", result_value="2")
 self.expect_expr("ClassWithConstexprs::enum_val", 
result_value="enum_case2")
 self.expect_expr("ClassWithConstexprs::scoped_enum_val", 
result_value="scoped_enum_case2")
+
+# Test an aliased enum with fixed underlying type.
+self.expect_expr("ClassWithEnumAlias::enum_alias",
+ result_value="scoped_enum_case2")
+self.expect_expr("ClassWithEnumAlias::enum_alias_alias",
+ result_value="scoped_enum_case1")


Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -67,12 +67,6 @@
 self.expect_expr("A::scoped_ll_enum_val_neg", result_value="case0")
 self.expect_expr("A::scoped_ll_enum_val", result_value="case2")
 
-# Test an aliased enum with fixed underlying type.
-self.expect_expr("ClassWithEnumAlias::enum_alias",
- result_value="scoped_enum_case2")
-self.expect_expr("ClassWithEnumAlias::enum_alias_alias",
- result_value="scoped_enum_case1")
-
 # Test taking address.
 if lldbplatformutil.getPlatform() == "windows":
 # On Windows data members without the out-of-class definitions still have
@@ -102,3 +96,9 @@
 self.expect_expr("ClassWithConstexprs::member", result_value="2")
 self.expect_expr("ClassWithConstexprs::enum_val", result_value="enum_case2")
 self.expect_expr("ClassWithConstexprs::scoped_enum_val", result_value="scoped_enum_case2")
+
+# Test an aliased enum with fixed underlying type.
+self.expect_expr("ClassWithEnumAlias::enum_alias",
+ result_value="scoped_enum_case2")
+self.expect_expr("ClassWithEnumAlias::enum_alias_alias",
+ result_value="scoped_enum_case1")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 9fd54cf - [LLDB][NativePDB] Add nullptr checking.

[Zequan Wu via lldb-commits]

Author: Zequan Wu
Date: 2022-08-16T17:01:47-07:00
New Revision: 9fd54cf5c9620504c8c16cf885354b6f9d82f638

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

LOG: [LLDB][NativePDB] Add nullptr checking.

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp 
b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index c596d9c7a1ae7..cf17e616f659f 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -1718,6 +1718,8 @@ VariableSP 
SymbolFileNativePDB::CreateLocalVariable(PdbCompilandSymId scope_id,
   CompilandIndexItem *cii = m_index->compilands().GetCompiland(var_id.modi);
   CompUnitSP comp_unit_sp = GetOrCreateCompileUnit(*cii);
   TypeSP type_sp = GetOrCreateType(var_info.type);
+  if (!type_sp)
+return nullptr;
   std::string name = var_info.name.str();
   Declaration decl;
   SymbolFileTypeSP sftype =



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

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

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

This broke the windows lldb bot: 
https://lab.llvm.org/buildbot/#/builders/83/builds/22557/steps/7/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983

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

[Lldb-commits] [PATCH] D132004: [lldb][Tests] Skip static-only tests in TestConstStaticIntegralMember.py for dsym variant

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This might be a bit aggressive. If it's easy we could add a `if clang > 14` 
condition before those last two tests so we don't loose coverage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132004

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

[Lldb-commits] [PATCH] D51387: Allow Template argument accessors to automatically unwrap parameter packs

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 8 inline comments as done.
JDevlieghere added a comment.

Thanks for the review Michael. I've addressed all your comments in the version 
I landed.


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

https://reviews.llvm.org/D51387

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

[Lldb-commits] [lldb] b706f56 - [lldb] Automatically unwrap parameter packs in template argument accessors

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-08-16T18:10:14-07:00
New Revision: b706f56133a77f9d7c55270ac24ff59e6fce3fa4

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

LOG: [lldb]  Automatically unwrap parameter packs in template argument accessors

When looking at template arguments in LLDB, we usually care about what
the user passed in his code, not whether some of those arguments where
passed as a variadic parameter pack.

This patch extends all the C++ APIs to look at template parameters to
take an additional 'expand_pack' boolean that automatically unwraps the
potential argument packs. The equivalent SBAPI calls have been changed
to pass true for this parameter.

A byproduct of the patch is to also fix the support for template type
that have only a parameter pack as argument (like the OnlyPack type in
the test). Those were not recognized as template instanciations before.

The added test verifies that the SBAPI is able to iterate over the
arguments of a variadic template.

The original patch was written by Fred Riss almost 4 years ago.

Differential revision: https://reviews.llvm.org/D51387

Added: 
lldb/test/API/lang/cpp/class-template-parameter-pack/TestTemplatePackArgs.py

Modified: 
lldb/include/lldb/API/SBType.h
lldb/include/lldb/Symbol/CompilerType.h
lldb/include/lldb/Symbol/TypeSystem.h
lldb/source/API/SBType.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/source/Symbol/CompilerType.cpp
lldb/source/Symbol/TypeSystem.cpp
lldb/test/API/lang/cpp/class-template-parameter-pack/main.cpp
lldb/unittests/Symbol/TestTypeSystemClang.cpp

Removed: 




diff  --git a/lldb/include/lldb/API/SBType.h b/lldb/include/lldb/API/SBType.h
index 244d328b51f4..aa45aeeec476 100644
--- a/lldb/include/lldb/API/SBType.h
+++ b/lldb/include/lldb/API/SBType.h
@@ -182,6 +182,8 @@ class SBType {
 
   lldb::SBType GetTemplateArgumentType(uint32_t idx);
 
+  /// Return the TemplateArgumentKind of the template argument at index idx.
+  /// Variadic argument packs are automatically expanded.
   lldb::TemplateArgumentKind GetTemplateArgumentKind(uint32_t idx);
 
   lldb::SBType GetFunctionReturnType();

diff  --git a/lldb/include/lldb/Symbol/CompilerType.h 
b/lldb/include/lldb/Symbol/CompilerType.h
index 0ad05a27570e..aefd19d0a859 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -338,14 +338,28 @@ class CompilerType {
   GetIndexOfChildMemberWithName(const char *name, bool omit_empty_base_classes,
 std::vector &child_indexes) const;
 
-  size_t GetNumTemplateArguments() const;
-
-  lldb::TemplateArgumentKind GetTemplateArgumentKind(size_t idx) const;
-  CompilerType GetTypeTemplateArgument(size_t idx) const;
+  /// Return the number of template arguments the type has.
+  /// If expand_pack is true, then variadic argument packs are automatically
+  /// expanded to their supplied arguments. If it is false an argument pack
+  /// will only count as 1 argument.
+  size_t GetNumTemplateArguments(bool expand_pack = false) const;
+
+  // Return the TemplateArgumentKind of the template argument at index idx.
+  // If expand_pack is true, then variadic argument packs are automatically
+  // expanded to their supplied arguments. With expand_pack set to false, an
+  // arguement pack will count as 1 argument and return a type of Pack.
+  lldb::TemplateArgumentKind
+  GetTemplateArgumentKind(size_t idx, bool expand_pack = false) const;
+  CompilerType GetTypeTemplateArgument(size_t idx,
+   bool expand_pack = false) const;
 
   /// Returns the value of the template argument and its type.
+  /// If expand_pack is true, then variadic argument packs are automatically
+  /// expanded to their supplied arguments. With expand_pack set to false, an
+  /// arguement pack will count as 1 argument and it is invalid to call this
+  /// method on the pack argument.
   llvm::Optional
-  GetIntegralTemplateArgument(size_t idx) const;
+  GetIntegralTemplateArgument(size_t idx, bool expand_pack = false) const;
 
   CompilerType GetTypeForFormatters() const;
 

diff  --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index be5783596897..769449a4933b 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -346,14 +346,18 @@ class TypeSystem : public PluginInterface {
 const char *name, bool omit_empty_base_classes,
 std::vector &child_indexes) = 0;
 
-  virtual size_t GetNumTemplateArguments(lldb::opaque_compiler_type_t type);
+  virtual size_t GetNumTemplateArguments(lldb

[Lldb-commits] [PATCH] D51387: Allow Template argument accessors to automatically unwrap parameter packs

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb706f56133a7: [lldb]  Automatically unwrap parameter packs 
in template argument accessors (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D51387?vs=451714&id=453180#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D51387

Files:
  lldb/include/lldb/API/SBType.h
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/API/SBType.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  lldb/source/Symbol/TypeSystem.cpp
  lldb/test/API/lang/cpp/class-template-parameter-pack/TestTemplatePackArgs.py
  lldb/test/API/lang/cpp/class-template-parameter-pack/main.cpp
  lldb/unittests/Symbol/TestTypeSystemClang.cpp

Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -500,18 +500,24 @@
   for (CompilerType t : {type, typedef_type, auto_type}) {
 SCOPED_TRACE(t.GetTypeName().AsCString());
 
-EXPECT_EQ(m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 0),
-  eTemplateArgumentKindType);
-EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 0),
-  int_type);
-EXPECT_EQ(llvm::None,
-  m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 0));
-
-EXPECT_EQ(m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 1),
-  eTemplateArgumentKindIntegral);
-EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 1),
-  CompilerType());
-auto result = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1);
+const bool expand_pack = false;
+EXPECT_EQ(
+m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 0, expand_pack),
+eTemplateArgumentKindType);
+EXPECT_EQ(
+m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 0, expand_pack),
+int_type);
+EXPECT_EQ(llvm::None, m_ast->GetIntegralTemplateArgument(
+  t.GetOpaqueQualType(), 0, expand_pack));
+
+EXPECT_EQ(
+m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 1, expand_pack),
+eTemplateArgumentKindIntegral);
+EXPECT_EQ(
+m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 1, expand_pack),
+CompilerType());
+auto result = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1,
+ expand_pack);
 ASSERT_NE(llvm::None, result);
 EXPECT_EQ(arg, result->value);
 EXPECT_EQ(int_type, result->type);
Index: lldb/test/API/lang/cpp/class-template-parameter-pack/main.cpp
===
--- lldb/test/API/lang/cpp/class-template-parameter-pack/main.cpp
+++ lldb/test/API/lang/cpp/class-template-parameter-pack/main.cpp
@@ -26,7 +26,13 @@
   bool argsAre_Int_bool() { return true; }
 };
 
+template  struct OnlyPack {};
+template  struct EmptyPack {};
+
 int main(int argc, char const *argv[]) {
+  EmptyPack emptyPack;
+  OnlyPack> onlyPack;
+
   C myC;
   C myLesserC;
   myC.member = 64;
@@ -34,7 +40,7 @@
   (void)C().argsAre_16_32();
   (void)(myC.member != 64);
   D myD;
-  D myLesserD;
+  D myLesserD; // breakpoint here
   myD.member = 64;
   (void)D().argsAre_Int_bool();
   (void)D().argsAre_Int_bool();
Index: lldb/test/API/lang/cpp/class-template-parameter-pack/TestTemplatePackArgs.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/class-template-parameter-pack/TestTemplatePackArgs.py
@@ -0,0 +1,38 @@
+"""
+Test that the type of arguments to C++ template classes that have variadic
+parameters can be enumerated.
+"""
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TemplatePackArgsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test_template_argument_pack(self):
+self.build()
+(_, _, thread, _) = lldbutil.run_to_source_breakpoint(self,
+  'breakpoint here', lldb.SBFileSpec('main.cpp'), exe_name = 'a.out')
+frame = thread.GetSelectedFrame()
+
+empty_pack = frame.FindVariable('emptyPack')
+self.assertTrue(empty_pack.IsValid(),
+'make sure we find the emptyPack variable')
+
+only_pack = frame.FindVariable('onlyPack')
+self.assertTrue(only_pack.IsValid(),
+'make sure we find the onlyPack variable')
+self.assertEqual(

[Lldb-commits] [lldb] 0cbaed3 - Revert "[LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread"

[Stella Stamenova via lldb-commits]

Author: Stella Stamenova
Date: 2022-08-16T18:11:28-07:00
New Revision: 0cbaed3e14448de4b9ed32b7f531471bc6d4347f

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

LOG: Revert "[LLDB][NFC] Fix optons parsing and misc. reliability in 
CommandObjectThread"

This very much non-NFC change broke the windows lldb bot: 
https://lab.llvm.org/buildbot/#/builders/83/builds/22557

This reverts commit 461b410159426fdc6da77e0fb653737e04e0ebe9.

Added: 


Modified: 
lldb/source/Commands/CommandObjectThread.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectThread.cpp 
b/lldb/source/Commands/CommandObjectThread.cpp
index 1e317d8bfdbe8..5d7f3c16f3894 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -62,13 +62,15 @@ class CommandObjectThreadBacktrace : public 
CommandObjectIterateOverThreads {
   const int short_option = m_getopt_table[option_idx].val;
 
   switch (short_option) {
-  case 'c':
-if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
+  case 'c': {
+int32_t input_count = 0;
+if (option_arg.getAsInteger(0, m_count)) {
   m_count = UINT32_MAX;
   error.SetErrorStringWithFormat(
   "invalid integer value for option '%c'", short_option);
-}
-break;
+} else if (input_count < 0)
+  m_count = UINT32_MAX;
+  } break;
   case 's':
 if (option_arg.getAsInteger(0, m_start))
   error.SetErrorStringWithFormat(
@@ -1002,15 +1004,8 @@ class CommandObjectThreadUntil : public 
CommandObjectParsed {
 
 AddressRange fun_addr_range = sc.function->GetAddressRange();
 Address fun_start_addr = fun_addr_range.GetBaseAddress();
-
-if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-&index_ptr)) {
-  result.AppendErrorWithFormat(
-  "Failed to find line entry by address for "
-  "frame %u of thread id %" PRIu64 ".\n",
-  m_options.m_frame_idx, thread->GetID());
-  return false;
-}
+line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+   &index_ptr);
 
 Address fun_end_addr(fun_start_addr.GetSection(),
  fun_start_addr.GetOffset() +
@@ -1018,14 +1013,8 @@ class CommandObjectThreadUntil : public 
CommandObjectParsed {
 
 bool all_in_function = true;
 
-if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-&end_ptr)) {
-  result.AppendErrorWithFormat(
-  "Failed to find line entry by address for "
-  "frame %u of thread id %" PRIu64 ".\n",
-  m_options.m_frame_idx, thread->GetID());
-  return false;
-}
+line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+   &end_ptr);
 
 // Since not all source lines will contribute code, check if we are
 // setting the breakpoint on the exact line number or the nearest



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

[Lldb-commits] [lldb] 9fc6565 - fold assert-only variable into assert to address non-assert -Wunused-variable

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2022-08-17T04:10:59Z
New Revision: 9fc65658f5e5a0af5821f4945d78a9f6b8fd368f

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

LOG: fold assert-only variable into assert to address non-assert 
-Wunused-variable

Added: 


Modified: 
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f815f3ad7d41..e298d7fee421 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7188,8 +7188,7 @@ GetNthTemplateArgument(const 
clang::ClassTemplateSpecializationDecl *decl,
   // (including the ones preceding the parameter pack).
   const auto &pack = args[last_idx];
   const size_t pack_idx = idx - last_idx;
-  const size_t pack_size = pack.pack_size();
-  assert(pack_idx < pack_size && "parameter pack index out-of-bounds");
+  assert(pack_idx < pack.pack_size() && "parameter pack index out-of-bounds");
   return &pack.pack_elements()[pack_idx];
 }
 



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

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

[Tom Stellard via Phabricator via lldb-commits]

tstellar updated this revision to Diff 453194.
tstellar added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131919

Files:
  clang/CMakeLists.txt
  compiler-rt/CMakeLists.txt
  lld/CMakeLists.txt
  lldb/cmake/modules/LLDBStandalone.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/utils/unittest/CMakeLists.txt
  llvm/utils/unittest/UnitTestMain/CMakeLists.txt
  llvm/utils/unittest/UnitTestMain/TestMain.cpp
  llvm/utils/unittest/googlemock/LICENSE.txt
  llvm/utils/unittest/googlemock/README.LLVM
  llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-cardinalities.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-function-mocker.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-generated-actions.h
  
llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-generated-matchers.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-more-actions.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-more-matchers.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-nice-strict.h
  llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h
  llvm/utils/unittest/googlemock/include/gmock/gmock.h
  
llvm/utils/unittest/googlemock/include/gmock/internal/custom/gmock-generated-actions.h
  llvm/utils/unittest/googlemock/include/gmock/internal/custom/gmock-matchers.h
  llvm/utils/unittest/googlemock/include/gmock/internal/custom/gmock-port.h
  llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h
  llvm/utils/unittest/googlemock/include/gmock/internal/gmock-port.h
  llvm/utils/unittest/googlemock/include/gmock/internal/gmock-pp.h
  llvm/utils/unittest/googlemock/src/gmock-all.cc
  llvm/utils/unittest/googlemock/src/gmock-cardinalities.cc
  llvm/utils/unittest/googlemock/src/gmock-internal-utils.cc
  llvm/utils/unittest/googlemock/src/gmock-matchers.cc
  llvm/utils/unittest/googlemock/src/gmock-spec-builders.cc
  llvm/utils/unittest/googlemock/src/gmock.cc
  llvm/utils/unittest/googletest/LICENSE.TXT
  llvm/utils/unittest/googletest/README.LLVM
  llvm/utils/unittest/googletest/include/gtest/gtest-death-test.h
  llvm/utils/unittest/googletest/include/gtest/gtest-matchers.h
  llvm/utils/unittest/googletest/include/gtest/gtest-message.h
  llvm/utils/unittest/googletest/include/gtest/gtest-param-test.h
  llvm/utils/unittest/googletest/include/gtest/gtest-printers.h
  llvm/utils/unittest/googletest/include/gtest/gtest-spi.h
  llvm/utils/unittest/googletest/include/gtest/gtest-test-part.h
  llvm/utils/unittest/googletest/include/gtest/gtest-typed-test.h
  llvm/utils/unittest/googletest/include/gtest/gtest.h
  llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h
  llvm/utils/unittest/googletest/include/gtest/gtest_prod.h
  llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest-port.h
  llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h
  llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest.h
  llvm/utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h
  
llvm/utils/unittest/googletest/include/gtest/internal/gtest-death-test-internal.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-filepath.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-internal.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-param-util.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-port-arch.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-string.h
  llvm/utils/unittest/googletest/include/gtest/internal/gtest-type-util.h
  llvm/utils/unittest/googletest/src/gtest-all.cc
  llvm/utils/unittest/googletest/src/gtest-death-test.cc
  llvm/utils/unittest/googletest/src/gtest-filepath.cc
  llvm/utils/unittest/googletest/src/gtest-internal-inl.h
  llvm/utils/unittest/googletest/src/gtest-matchers.cc
  llvm/utils/unittest/googletest/src/gtest-port.cc
  llvm/utils/unittest/googletest/src/gtest-printers.cc
  llvm/utils/unittest/googletest/src/gtest-test-part.cc
  llvm/utils/unittest/googletest/src/gtest-typed-test.cc
  llvm/utils/unittest/googletest/src/gtest.cc
  mlir/CMakeLists.txt
  polly/CMakeLists.txt
  third-party/unittest/CMakeLists.txt
  third-party/unittest/UnitTestMain/CMakeLists.txt
  third-party/unittest/UnitTestMain/TestMain.cpp
  third-party/unittest/googlemock/LICENSE.txt
  third-party/unittest/googlemock/README.LLVM
  third-party/unittest/googlemock/include/gmock/gmock-actions.h
  third-party/unittest/googlemock/include/gmock/gmock-cardinalities.h
  third-party/unittest/googlemock/include/gmock/gmock-function-mocker.h
  third-party/unittest/googlemock/include

[Lldb-commits] [PATCH] D131998: [LLDB][NFC] Suppress spurious static inspection warnings

[Slava Gurevich via Phabricator via lldb-commits]

fixathon added a comment.

In D131998#3727664 , @clayborg wrote:

> Can we use C++ comments instead of C comments?

We can, and I'll make the change. Do you think it would be a good idea to keep 
the coverity annotations only as C-style comments (comments for the machine)  
to disambiguate it from the regular comments (comments for the humans) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131998

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

[Lldb-commits] [lldb] ff9efe2 - [LLDB][JIT] Set processor for ARM architecture

[Pavel Kosov via lldb-commits]

Author: Pavel Kosov
Date: 2022-08-17T09:10:21+03:00
New Revision: ff9efe240c4711572d2892f9058fd94a8bd5336e

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

LOG: [LLDB][JIT] Set processor for ARM architecture

Patch sets ARM cpu, before compiling JIT code. This enables FastISel for armv6 
and higher CPUs and allows using hardware FPU

~~~

OS Laboratory. Huawei RRI. Saint-Petersburg

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Utility/ArchSpec.cpp
lldb/test/API/lang/c/fpeval/TestFPEval.py

Removed: 




diff  --git a/lldb/source/Utility/ArchSpec.cpp 
b/lldb/source/Utility/ArchSpec.cpp
index b61d180bca1e9..348a62dd0df42 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -583,7 +583,6 @@ void ArchSpec::SetFlags(const std::string &elf_abi) {
 
 std::string ArchSpec::GetClangTargetCPU() const {
   std::string cpu;
-
   if (IsMIPS()) {
 switch (m_core) {
 case ArchSpec::eCore_mips32:
@@ -630,6 +629,9 @@ std::string ArchSpec::GetClangTargetCPU() const {
   break;
 }
   }
+
+  if (GetTriple().isARM())
+cpu = GetTriple().getARMCPUForArch("").str();
   return cpu;
 }
 

diff  --git a/lldb/test/API/lang/c/fpeval/TestFPEval.py 
b/lldb/test/API/lang/c/fpeval/TestFPEval.py
index 6a3dc955ebf66..694d1ed7aa99d 100644
--- a/lldb/test/API/lang/c/fpeval/TestFPEval.py
+++ b/lldb/test/API/lang/c/fpeval/TestFPEval.py
@@ -19,7 +19,6 @@ def setUp(self):
 # Find the line number to break inside main().
 self.line = line_number('main.c', '// Set break point at this line.')
 
-@skipIf(archs=no_match(['amd64', 'x86_64', 'arm64'])) # lldb jitter 
incorrectly evals function with FP args on 32 bit arm
 def test(self):
 """Test floating point expressions while jitter is disabled."""
 self.build()



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

[Lldb-commits] [PATCH] D131783: [LLDB][JIT] Set processor for ARM architecture

[Pavel Kosov via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGff9efe240c47: [LLDB][JIT] Set processor for ARM architecture 
(authored by kpdev42).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131783

Files:
  lldb/source/Utility/ArchSpec.cpp
  lldb/test/API/lang/c/fpeval/TestFPEval.py


Index: lldb/test/API/lang/c/fpeval/TestFPEval.py
===
--- lldb/test/API/lang/c/fpeval/TestFPEval.py
+++ lldb/test/API/lang/c/fpeval/TestFPEval.py
@@ -19,7 +19,6 @@
 # Find the line number to break inside main().
 self.line = line_number('main.c', '// Set break point at this line.')
 
-@skipIf(archs=no_match(['amd64', 'x86_64', 'arm64'])) # lldb jitter 
incorrectly evals function with FP args on 32 bit arm
 def test(self):
 """Test floating point expressions while jitter is disabled."""
 self.build()
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -583,7 +583,6 @@
 
 std::string ArchSpec::GetClangTargetCPU() const {
   std::string cpu;
-
   if (IsMIPS()) {
 switch (m_core) {
 case ArchSpec::eCore_mips32:
@@ -630,6 +629,9 @@
   break;
 }
   }
+
+  if (GetTriple().isARM())
+cpu = GetTriple().getARMCPUForArch("").str();
   return cpu;
 }
 


Index: lldb/test/API/lang/c/fpeval/TestFPEval.py
===
--- lldb/test/API/lang/c/fpeval/TestFPEval.py
+++ lldb/test/API/lang/c/fpeval/TestFPEval.py
@@ -19,7 +19,6 @@
 # Find the line number to break inside main().
 self.line = line_number('main.c', '// Set break point at this line.')
 
-@skipIf(archs=no_match(['amd64', 'x86_64', 'arm64'])) # lldb jitter incorrectly evals function with FP args on 32 bit arm
 def test(self):
 """Test floating point expressions while jitter is disabled."""
 self.build()
Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -583,7 +583,6 @@
 
 std::string ArchSpec::GetClangTargetCPU() const {
   std::string cpu;
-
   if (IsMIPS()) {
 switch (m_core) {
 case ArchSpec::eCore_mips32:
@@ -630,6 +629,9 @@
   break;
 }
   }
+
+  if (GetTriple().isARM())
+cpu = GetTriple().getARMCPUForArch("").str();
   return cpu;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits