[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
aleksandr.urakov added a comment. Can you explain me, please, why do you think that we should remove these spaces? I have the following considerations: - For eight years (the spaces were commited in 2010) some tests may have relied on these spaces. It's a good case - we can run tests and see it; - Some formatting may rely on these spaces and it will be broken. It's a worse case - if it wasn't covered with tests, then we will not see it; - Several people in several places, not only in DWARFExpressions, use such a formatting style. May be it's an approved way of formatting such things? - Format-only commits make the use of `git blame` more difficult. It's not very horrible, but still adds some inconvenients; - The only thing I can find to justify such a commit is the beauty. But the beauty is the matter of opinion and what some peoples find beautiful, other find not. So peoples who commited such spaces found them beautiful (or used the approved formatting style, or used them for some special formatting cases), and I can't find the reason to fix that. Please, correct me if I am missing something? Repository: rL LLVM https://reviews.llvm.org/D49018 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
On Thu, 19 Jul 2018 at 10:04, Aleksandr Urakov via Phabricator wrote: > So peoples who commited such spaces found them beautiful (or used the > approved formatting style, or used them for some special formatting cases), Or it's simply a typo. I don't want to make a bigger fuss of this than it already is, but I think removing that space is a good idea. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
aleksandr.urakov added inline comments.
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:696
+ base_ast_type.GetOpaqueQualType(), access,
+ pdb_base_class->isVirtualBaseClass(), /*base_of_class*/ true);
+ base_classes.push_back(base_spec);
Hui wrote:
> aleksandr.urakov wrote:
> > If I understand correctly, `base_of_class` must be `false` for structs. May
> > be check the kind of `pdb_udt` here?
> Struct/Union can't have base class. Only class udt is handled. I think it is
> safe to set it ture.
I think that it must be `false` for following code:
```
class B {
char _b;
};
struct S : B {
int _s;
};
```
https://reviews.llvm.org/D49410
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
But can you, please, explain me why? Excuse me for so much fuss, I just wanted to explain my position... We have two cases now: - Just commit such a patch without an additional investigation. But it's not a good idea, because we can't be 100% sure that it is a typo (and even if it is, we still may break something from I listed above); - Make an additional investigation and commit this. But I think that it's unconstructive to do it now, because there are many problems with a higher priority. So I suggest to defer it for a better times. We may report a bug and assign it to me, I'll do it when I will finish other more important work. What do you think about it? On Thu, Jul 19, 2018 at 12:13 PM Pavel Labath wrote: > On Thu, 19 Jul 2018 at 10:04, Aleksandr Urakov via Phabricator > wrote: > > So peoples who commited such spaces found them beautiful (or used the > approved formatting style, or used them for some special formatting cases), > > Or it's simply a typo. > > I don't want to make a bigger fuss of this than it already is, but I > think removing that space is a good idea. > -- Aleksandr Urakov Software Developer JetBrains http://www.jetbrains.com The Drive to Develop ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
I knew I should have stayed quiet :P, but now that I am in, here's my reasoning: > - For eight years (the spaces were commited in 2010) some tests may have > relied on these spaces. It's a good case - we can run tests and see it; For seven of those eight years we haven't been using this function to write tests. It was only used to produce debugging dumps to be viewed by humans, and even that I'm not sure how often.. > - Some formatting may rely on these spaces and it will be broken. It's a > worse case - if it wasn't covered with tests, then we will not see it; What kind of "formatting"? If this is not some automated test (in which case it would fall under your first item), then I don't think it will care whether you have the extra space or not. Overall, I think you're overestimating the importance of this function. > - Format-only commits make the use of `git blame` more difficult. It's not > very horrible, but still adds some inconvenients; Yes, but without them you cannot make forward progress. (And the point of my email here was to show that it's not just Stella and you(?) who think removing the space looks better. I think it's an improvement too). Moreoever, I think that if there ever was a time to change the format of the dump functions, it is now. A year ago we did not have any tests based on this function. Now we have maybe two dozen. Changing it now is easier than next year when it may be hundreds. And I don't mean just the spaces. If you see *anything* in the dump format that makes writing a test, I encourage you to change it. I don't think we should be afraid of change. LLVM shows is it's possible to change the textual format even when you have tens of thousands of tests.. > - Several people in several places, not only in DWARFExpressions, use such a > formatting style. May be it's an approved way of formatting such things? > - The only thing I can find to justify such a commit is the beauty. But the > beauty is the matter of opinion and what some peoples find beautiful, other > find not. Writing a space before a comma does not follow any stylistic rules I know of and I would argue we should change all of those places. On Thu, 19 Jul 2018 at 11:24, Aleksandr Urakov wrote: > > But can you, please, explain me why? > > Excuse me for so much fuss, I just wanted to explain my position... We have > two cases now: > - Just commit such a patch without an additional investigation. But it's not > a good idea, because we can't be 100% sure that it is a typo (and even if it > is, we still may break something from I listed above); > - Make an additional investigation and commit this. But I think that it's > unconstructive to do it now, because there are many problems with a higher > priority. So I suggest to defer it for a better times. We may report a bug > and assign it to me, I'll do it when I will finish other more important work. > > What do you think about it? > > On Thu, Jul 19, 2018 at 12:13 PM Pavel Labath wrote: >> >> On Thu, 19 Jul 2018 at 10:04, Aleksandr Urakov via Phabricator >> wrote: >> > So peoples who commited such spaces found them beautiful (or used the >> > approved formatting style, or used them for some special formatting cases), >> >> Or it's simply a typo. >> >> I don't want to make a bigger fuss of this than it already is, but I >> think removing that space is a good idea. > > > > -- > Aleksandr Urakov > Software Developer > JetBrains > http://www.jetbrains.com > The Drive to Develop ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
On Thu, Jul 19, 2018 at 2:14 PM Pavel Labath wrote: > I knew I should have stayed quiet :P, but now that I am in, here's my > reasoning: > Thank you for not staying quiet, I think it's the only way to have a dialog and solve the problems :) I find your argumentation convincing. The problem is that I'm very new in lldb, and I admit that my misgivings may be in vain. So I have no objections if some more experienced lldb developer will commit this (I have no commit access). Or, if you want that it to be exactly my patch, I can make it (test it, find another such places etc.) some later, after current work, and will send a review. What do you think about it? ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
aleksandr.urakov added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:186 +return lldb::eAccessProtected; + return lldb::eAccessNone; +} zturner wrote: > I wonder if this would be better as an `llvm_unreachable`. Being able to > assume this function returns a sane value would simplify calling code. PDBSymbolTypeUDT::getType() returns PDB_MemberAccess that equals 0, so we can't make it `llvm_unreachable`. https://reviews.llvm.org/D49410 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
aleksandr.urakov added inline comments.
Comment at: lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp:37
+ };
+ union { // Test unnamed union. MSVC treats it as `int a; float b;`
+int a;
Here is a problem. `MicrosoftRecordLayoutBuilder` asserts every field or base
offset, but in our case fields `a` and `b` are treated as `struct Complex`'s
fields, not `union`'s, so lldb crashes in debug on this. I can't find enough
info in PDB to restore the unnamed union here. Do you have any ideas about it?
https://reviews.llvm.org/D49410
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r337452 - Fix whitespace formatting in DWARFExpression::DumpLocation
Author: labath
Date: Thu Jul 19 06:30:56 2018
New Revision: 337452
URL: http://llvm.org/viewvc/llvm-project?rev=337452&view=rev
Log:
Fix whitespace formatting in DWARFExpression::DumpLocation
we were printing an extra space before the start for the expression and
an extra space after some dwarf operators. This makes sure we only print
exactly one space **between** operators and nowhere else.
Modified:
lldb/trunk/source/Expression/DWARFExpression.cpp
Modified: lldb/trunk/source/Expression/DWARFExpression.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Expression/DWARFExpression.cpp?rev=337452&r1=337451&r2=337452&view=diff
==
--- lldb/trunk/source/Expression/DWARFExpression.cpp (original)
+++ lldb/trunk/source/Expression/DWARFExpression.cpp Thu Jul 19 06:30:56 2018
@@ -148,13 +148,13 @@ void DWARFExpression::DumpLocation(Strea
break;
case lldb::eDescriptionLevelBrief:
- if (offset > start_offset)
+ if (op_offset > start_offset)
s->PutChar(' ');
break;
case lldb::eDescriptionLevelFull:
case lldb::eDescriptionLevelVerbose:
- if (offset > start_offset)
+ if (op_offset > start_offset)
s->EOL();
s->Indent();
if (level == lldb::eDescriptionLevelFull)
@@ -173,34 +173,34 @@ void DWARFExpression::DumpLocation(Strea
*s << "DW_OP_deref";
break; // 0x06
case DW_OP_const1u:
- s->Printf("DW_OP_const1u(0x%2.2x) ", m_data.GetU8(&offset));
+ s->Printf("DW_OP_const1u(0x%2.2x)", m_data.GetU8(&offset));
break; // 0x08 1 1-byte constant
case DW_OP_const1s:
- s->Printf("DW_OP_const1s(0x%2.2x) ", m_data.GetU8(&offset));
+ s->Printf("DW_OP_const1s(0x%2.2x)", m_data.GetU8(&offset));
break; // 0x09 1 1-byte constant
case DW_OP_const2u:
- s->Printf("DW_OP_const2u(0x%4.4x) ", m_data.GetU16(&offset));
+ s->Printf("DW_OP_const2u(0x%4.4x)", m_data.GetU16(&offset));
break; // 0x0a 1 2-byte constant
case DW_OP_const2s:
- s->Printf("DW_OP_const2s(0x%4.4x) ", m_data.GetU16(&offset));
+ s->Printf("DW_OP_const2s(0x%4.4x)", m_data.GetU16(&offset));
break; // 0x0b 1 2-byte constant
case DW_OP_const4u:
- s->Printf("DW_OP_const4u(0x%8.8x) ", m_data.GetU32(&offset));
+ s->Printf("DW_OP_const4u(0x%8.8x)", m_data.GetU32(&offset));
break; // 0x0c 1 4-byte constant
case DW_OP_const4s:
- s->Printf("DW_OP_const4s(0x%8.8x) ", m_data.GetU32(&offset));
+ s->Printf("DW_OP_const4s(0x%8.8x)", m_data.GetU32(&offset));
break; // 0x0d 1 4-byte constant
case DW_OP_const8u:
- s->Printf("DW_OP_const8u(0x%16.16" PRIx64 ") ", m_data.GetU64(&offset));
+ s->Printf("DW_OP_const8u(0x%16.16" PRIx64 ")", m_data.GetU64(&offset));
break; // 0x0e 1 8-byte constant
case DW_OP_const8s:
- s->Printf("DW_OP_const8s(0x%16.16" PRIx64 ") ", m_data.GetU64(&offset));
+ s->Printf("DW_OP_const8s(0x%16.16" PRIx64 ")", m_data.GetU64(&offset));
break; // 0x0f 1 8-byte constant
case DW_OP_constu:
- s->Printf("DW_OP_constu(0x%" PRIx64 ") ", m_data.GetULEB128(&offset));
+ s->Printf("DW_OP_constu(0x%" PRIx64 ")", m_data.GetULEB128(&offset));
break; // 0x10 1 ULEB128 constant
case DW_OP_consts:
- s->Printf("DW_OP_consts(0x%" PRId64 ") ", m_data.GetSLEB128(&offset));
+ s->Printf("DW_OP_consts(0x%" PRId64 ")", m_data.GetSLEB128(&offset));
break; // 0x11 1 SLEB128 constant
case DW_OP_dup:
s->PutCString("DW_OP_dup");
@@ -212,7 +212,7 @@ void DWARFExpression::DumpLocation(Strea
s->PutCString("DW_OP_over");
break; // 0x14
case DW_OP_pick:
- s->Printf("DW_OP_pick(0x%2.2x) ", m_data.GetU8(&offset));
+ s->Printf("DW_OP_pick(0x%2.2x)", m_data.GetU8(&offset));
break; // 0x15 1 1-byte stack index
case DW_OP_swap:
s->PutCString("DW_OP_swap");
@@ -254,7 +254,7 @@ void DWARFExpression::DumpLocation(Strea
s->PutCString("DW_OP_plus");
break;// 0x22
case DW_OP_plus_uconst: // 0x23 1 ULEB128 addend
- s->Printf("DW_OP_plus_uconst(0x%" PRIx64 ") ",
+ s->Printf("DW_OP_plus_uconst(0x%" PRIx64 ")",
m_data.GetULEB128(&offset));
break;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
Thanks. I agree you're not in the best position to make these kinds of changes (and I don't think I would have asked you to do them). In fact, I was already considering just changing that function myself, so I went ahead and did that now in r337452. As far as I can tell, no tests need to be updated as a result of this, though I haven't tried running the test suite on non-linux platforms. On Thu, 19 Jul 2018 at 12:49, Aleksandr Urakov wrote: > > On Thu, Jul 19, 2018 at 2:14 PM Pavel Labath wrote: >> >> I knew I should have stayed quiet :P, but now that I am in, here's my >> reasoning: > > Thank you for not staying quiet, I think it's the only way to have a dialog > and solve the problems :) > > I find your argumentation convincing. The problem is that I'm very new in > lldb, and I admit that my misgivings may be in vain. So I have no objections > if some more experienced lldb developer will commit this (I have no commit > access). Or, if you want that it to be exactly my patch, I can make it (test > it, find another such places etc.) some later, after current work, and will > send a review. > > What do you think about it? ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression
Thank you a lot for explanations and commit! On Thu, Jul 19, 2018 at 4:40 PM Pavel Labath wrote: > Thanks. > > I agree you're not in the best position to make these kinds of changes > (and I don't think I would have asked you to do them). In fact, I was > already considering just changing that function myself, so I went > ahead and did that now in r337452. As far as I can tell, no tests need > to be updated as a result of this, though I haven't tried running the > test suite on non-linux platforms. > On Thu, 19 Jul 2018 at 12:49, Aleksandr Urakov > wrote: > > > > On Thu, Jul 19, 2018 at 2:14 PM Pavel Labath wrote: > >> > >> I knew I should have stayed quiet :P, but now that I am in, here's my > reasoning: > > > > Thank you for not staying quiet, I think it's the only way to have a > dialog and solve the problems :) > > > > I find your argumentation convincing. The problem is that I'm very new > in lldb, and I admit that my misgivings may be in vain. So I have no > objections if some more experienced lldb developer will commit this (I have > no commit access). Or, if you want that it to be exactly my patch, I can > make it (test it, find another such places etc.) some later, after current > work, and will send a review. > > > > What do you think about it? > -- Aleksandr Urakov Software Developer JetBrains http://www.jetbrains.com The Drive to Develop ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
aleksandr.urakov added inline comments.
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:273
+udt->getClassParent()) {
+ access = GetDefaultAccessibilityForUdtKind(udt_kind);
+}
Yes, we can occur here, as I have mentioned at the line 186. But if I
understand correctly for following code:
```
class C {
struct S {
};
};
```
we will retrieve `public` as the access for `S`, but must retrieve `private`,
right? May be we need to get the default accessibility of the class parent?
https://reviews.llvm.org/D49410
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r337459 - ELF: Replace the header-extension unit test with a lit one
Author: labath
Date: Thu Jul 19 07:38:30 2018
New Revision: 337459
URL: http://llvm.org/viewvc/llvm-project?rev=337459&view=rev
Log:
ELF: Replace the header-extension unit test with a lit one
The new test checks that we are actually able to read data from these
kinds of elf headers correctly instead of just that we read the section
number correctly. It is also easier to figure out what's going on in the
test.
Added:
lldb/trunk/lit/Modules/elf-many-sections.s
Removed:
lldb/trunk/unittests/ObjectFile/ELF/TestELFHeader.cpp
Modified:
lldb/trunk/lit/Modules/lit.local.cfg
lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt
Added: lldb/trunk/lit/Modules/elf-many-sections.s
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/elf-many-sections.s?rev=337459&view=auto
==
--- lldb/trunk/lit/Modules/elf-many-sections.s (added)
+++ lldb/trunk/lit/Modules/elf-many-sections.s Thu Jul 19 07:38:30 2018
@@ -0,0 +1,72 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+## Check that we are able to parse ELF files with more than SHN_LORESERVE
+## sections. This generates a file that contains 64k sections from
+## .., plus a couple of standard ones (.strtab, etc.)
+## Check the number is correct plus the names of a couple of chosen sections.
+
+# CHECK: Showing 65541 sections
+# CHECK: Name:
+# CHECK: Name:
+# CHECK: Name:
+# CHECK: Name: abcdabcd
+# CHECK: Name:
+
+.macro gen_sections4 x
+ .section a\x
+ .section b\x
+ .section c\x
+ .section d\x
+.endm
+
+.macro gen_sections16 x
+ gen_sections4 a\x
+ gen_sections4 b\x
+ gen_sections4 c\x
+ gen_sections4 d\x
+.endm
+
+.macro gen_sections64 x
+ gen_sections16 a\x
+ gen_sections16 b\x
+ gen_sections16 c\x
+ gen_sections16 d\x
+.endm
+
+.macro gen_sections256 x
+ gen_sections64 a\x
+ gen_sections64 b\x
+ gen_sections64 c\x
+ gen_sections64 d\x
+.endm
+
+.macro gen_sections1024 x
+ gen_sections256 a\x
+ gen_sections256 b\x
+ gen_sections256 c\x
+ gen_sections256 d\x
+.endm
+
+.macro gen_sections4096 x
+ gen_sections1024 a\x
+ gen_sections1024 b\x
+ gen_sections1024 c\x
+ gen_sections1024 d\x
+.endm
+
+.macro gen_sections16384 x
+ gen_sections4096 a\x
+ gen_sections4096 b\x
+ gen_sections4096 c\x
+ gen_sections4096 d\x
+.endm
+
+gen_sections16384 a
+gen_sections16384 b
+gen_sections16384 c
+gen_sections16384 d
+
+.global _start
+_start:
Modified: lldb/trunk/lit/Modules/lit.local.cfg
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/lit.local.cfg?rev=337459&r1=337458&r2=337459&view=diff
==
--- lldb/trunk/lit/Modules/lit.local.cfg (original)
+++ lldb/trunk/lit/Modules/lit.local.cfg Thu Jul 19 07:38:30 2018
@@ -1 +1 @@
-config.suffixes = ['.yaml']
+config.suffixes = ['.s', '.yaml']
Modified: lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt?rev=337459&r1=337458&r2=337459&view=diff
==
--- lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt (original)
+++ lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt Thu Jul 19 07:38:30 2018
@@ -1,5 +1,4 @@
add_lldb_unittest(ObjectFileELFTests
- TestELFHeader.cpp
TestObjectFileELF.cpp
LINK_LIBS
Removed: lldb/trunk/unittests/ObjectFile/ELF/TestELFHeader.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/ObjectFile/ELF/TestELFHeader.cpp?rev=337458&view=auto
==
--- lldb/trunk/unittests/ObjectFile/ELF/TestELFHeader.cpp (original)
+++ lldb/trunk/unittests/ObjectFile/ELF/TestELFHeader.cpp (removed)
@@ -1,62 +0,0 @@
-//===-- TestELFHeader.cpp ---*- C++
-*-===//
-//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "Plugins/ObjectFile/ELF/ELFHeader.h"
-#include "lldb/Utility/DataExtractor.h"
-#include "gtest/gtest.h"
-
-using namespace lldb;
-using namespace lldb_private;
-
-
-TEST(ELFHeader, ParseHeaderExtension) {
- uint8_t data[] = {
- // e_ident
- 0x7f, 0x45, 0x4c, 0x46, 0x02, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00,
-
- // e_type, e_machine, e_version, e_entry
- 0x03, 0x00, 0x3e, 0x00, 0x01, 0x00, 0x00, 0x00, 0x90, 0x48, 0x40, 0x00,
- 0x00, 0x00, 0x00, 0x00,
-
- // e_phoff, e_shoff
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00,
-
- // e_fl
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
labath added a comment. Does anyone have an opinion on this? https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
davide added a comment. I guess this is fine. @jingham? https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
zturner added a comment. Seems good to me. Separating dump code from core logic is always helpful. https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
aleksandr.urakov added inline comments. Comment at: lit/SymbolFile/PDB/class-layout.test:3 +RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/ClassLayoutTest.cpp /o %T/ClassLayoutTest.cpp.obj +RUN: link %T/ClassLayoutTest.cpp.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:%T/ClassLayoutTest.cpp.exe +RUN: lldb-test symbols %T/ClassLayoutTest.cpp.exe | FileCheck %s zturner wrote: > If you change this to `lld-link` this test may be able to run on non windows > today. It seems that with `lld-link` the `GetDeclarationForSymbol` function can't find declarations for UDTs. https://reviews.llvm.org/D49410 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members
zturner added inline comments. Comment at: lit/SymbolFile/PDB/class-layout.test:3 +RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/ClassLayoutTest.cpp /o %T/ClassLayoutTest.cpp.obj +RUN: link %T/ClassLayoutTest.cpp.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:%T/ClassLayoutTest.cpp.exe +RUN: lldb-test symbols %T/ClassLayoutTest.cpp.exe | FileCheck %s aleksandr.urakov wrote: > zturner wrote: > > If you change this to `lld-link` this test may be able to run on non > > windows today. > It seems that with `lld-link` the `GetDeclarationForSymbol` function can't > find declarations for UDTs. Heh, I know exactly what that is actually. Ignore my request, it's easy to fix in lld-link but I don't have the time to do it right now. https://reviews.llvm.org/D49410 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49062: [lldb-mi] Re-implement data-info-line command.
apolyakov updated this revision to Diff 156299.
apolyakov added a comment.
Converted data-info-line python test to a lit one.
https://reviews.llvm.org/D49062
Files:
lit/tools/lldb-mi/data/data-info-line.test
lit/tools/lldb-mi/data/inputs/data-info-line.c
lit/tools/lldb-mi/data/lit.local.cfg
packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
tools/lldb-mi/MICmdCmdData.cpp
tools/lldb-mi/MICmdCmdData.h
Index: tools/lldb-mi/MICmdCmdData.h
===
--- tools/lldb-mi/MICmdCmdData.h
+++ tools/lldb-mi/MICmdCmdData.h
@@ -34,14 +34,14 @@
#pragma once
// Third party headers:
-#include "lldb/API/SBCommandReturnObject.h"
#include "lldb/API/SBError.h"
// In-house headers:
#include "MICmdBase.h"
#include "MICmnLLDBDebugSessionInfoVarObj.h"
#include "MICmnMIValueList.h"
#include "MICmnMIValueTuple.h"
+#include "MICmnMIResultRecord.h"
//++
//
@@ -377,6 +377,6 @@
// Attributes:
private:
- lldb::SBCommandReturnObject m_lldbResult;
const CMIUtilString m_constStrArgLocation;
+ CMICmnMIResultRecord m_resultRecord;
};
Index: tools/lldb-mi/MICmdCmdData.cpp
===
--- tools/lldb-mi/MICmdCmdData.cpp
+++ tools/lldb-mi/MICmdCmdData.cpp
@@ -19,15 +19,13 @@
// CMICmdCmdDataInfoLine implementation.
// Third Party Headers:
-#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBInstruction.h"
#include "lldb/API/SBInstructionList.h"
#include "lldb/API/SBStream.h"
#include "lldb/API/SBThread.h"
-#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Regex.h"
-#include // For PRIx64
+#include
// In-house headers:
#include "MICmdArgValConsume.h"
@@ -46,6 +44,12 @@
#include "MICmnMIResultRecord.h"
#include "MICmnMIValueConst.h"
+namespace {
+CMIUtilString IntToHexAddrStr(uint32_t number) {
+ return CMIUtilString("0x" + llvm::Twine::utohexstr(number).str());
+}
+} // namespace
+
//++
//
// Details: CMICmdCmdDataEvaluateExpression constructor.
@@ -1588,7 +1592,9 @@
// Throws: None.
//--
CMICmdCmdDataInfoLine::CMICmdCmdDataInfoLine()
-: m_constStrArgLocation("location") {
+: m_constStrArgLocation("location"),
+ m_resultRecord(m_cmdData.strMiCmdToken,
+ CMICmnMIResultRecord::eResultClass_Done) {
// Command factory matches this name with that received from the stdin stream
m_strMiCmd = "data-info-line";
@@ -1604,7 +1610,7 @@
// Return: None.
// Throws: None.
//--
-CMICmdCmdDataInfoLine::~CMICmdCmdDataInfoLine() {}
+CMICmdCmdDataInfoLine::~CMICmdCmdDataInfoLine() = default;
//++
//
@@ -1637,98 +1643,84 @@
bool CMICmdCmdDataInfoLine::Execute() {
CMICMDBASE_GETOPTION(pArgLocation, String, m_constStrArgLocation);
+ lldb::SBLineEntry line;
+ bool found_line = false;
const CMIUtilString &strLocation(pArgLocation->GetValue());
- CMIUtilString strCmdOptionsLocation;
+ lldb::SBTarget target = CMICmnLLDBDebugSessionInfo::Instance().GetTarget();
+
if (strLocation.at(0) == '*') {
// Parse argument:
// *0x12345
-// ^^^ -- address
-const CMIUtilString strAddress(strLocation.substr(1));
-strCmdOptionsLocation =
-CMIUtilString::Format("--address %s", strAddress.c_str());
+// ^ -- address
+lldb::addr_t address = 0x0;
+if (llvm::StringRef(strLocation.substr(1)).getAsInteger(0, address)) {
+ SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SOME_ERROR),
+ m_cmdData.strMiCmd.c_str(),
+ "Failed to parse address."));
+ return MIstatus::failure;
+}
+line = target.ResolveFileAddress(address).GetLineEntry();
+// Check that found line is valid.
+if (line.GetLine())
+ found_line = true;
} else {
const size_t nLineStartPos = strLocation.rfind(':');
if ((nLineStartPos == std::string::npos) || (nLineStartPos == 0) ||
(nLineStartPos == strLocation.length() - 1)) {
- SetError(
- CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_LOCATION_FORMAT),
-m_cmdData.strMiCmd.c_str(), strLocation.c_str())
- .c_str());
+ SetError(CMIUtilString::Format(
+ MIRSRC(IDS_CMD_ERR_INVALID_LOCATION_FORMAT),
+ m_cmdData.strMiCmd.c_str(), strLocation.c_str()));
return MIstatus::failure;
}
// Parse argument:
// hello.cpp:5
// ^ -- file
// ^ -- line
-const CMIUtilString strFile(strLocation.substr(0, nLineStartPos));
-const CMIUtilString strLine(strLocation.substr(nLineStartPos + 1));
-
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
clayborg added a comment. Is this a case of keeping RegisterValue from needing to link against the stream stuff so code inside a directory doesn't depend on code from other directories? Not sure why we wouldn't want to just ask the object itself to dump itself... https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I don't agree that moving dump routines away from the class that they are dumping is a good idea in general. It makes it harder for somebody new to the code to find out how to print out the objects, which is often handy in development as well as for logging and the like. OTOH, it seems like in this case the motivation (RegisterValue is useful on the server side so its reducing its dependencies is desirable) outweighs these downsides. If this pattern becomes more common, then we have to deal with how somebody would find these dump routines. If RegisterValue gets moved out of Core, would you really look to a file in Core for the dump routine, especially as many other classes have an obvious Dump method? But this change is well motivated, so as a one-off this is okay. https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
labath added a comment. The Stream part isn't the problem. The problem is that the dumping code is implemented in terms of `ExecutionContextScope`, which then pulls in pretty much everything. If it was just the object "dumping itself" then I would be fine with it as a method, but here it's using the whole world to achieve that goal. At that point I would prefer it living in a separate place (though we can debate on whether it should be a completely separate file, or merged to a single file with the DumpDataExtractor code) https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class
clayborg added a comment. I am fine if this helps with not pulling in the world. Seems like it should move out of Core then no? Seems weird to make the change yet keep it in the same directory. https://reviews.llvm.org/D48351 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r337475 - Added unit tests for Flags
Author: teemperor
Date: Thu Jul 19 10:45:51 2018
New Revision: 337475
URL: http://llvm.org/viewvc/llvm-project?rev=337475&view=rev
Log:
Added unit tests for Flags
Reviewers: labath
Reviewed By: labath
Subscribers: labath, mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D49435
Added:
lldb/trunk/unittests/Utility/FlagsTest.cpp
Modified:
lldb/trunk/unittests/Utility/CMakeLists.txt
Modified: lldb/trunk/unittests/Utility/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/CMakeLists.txt?rev=337475&r1=337474&r2=337475&view=diff
==
--- lldb/trunk/unittests/Utility/CMakeLists.txt (original)
+++ lldb/trunk/unittests/Utility/CMakeLists.txt Thu Jul 19 10:45:51 2018
@@ -8,6 +8,7 @@ add_lldb_unittest(UtilityTests
CompletionRequestTest.cpp
EnvironmentTest.cpp
FileSpecTest.cpp
+ FlagsTest.cpp
JSONTest.cpp
LogTest.cpp
NameMatchesTest.cpp
Added: lldb/trunk/unittests/Utility/FlagsTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/FlagsTest.cpp?rev=337475&view=auto
==
--- lldb/trunk/unittests/Utility/FlagsTest.cpp (added)
+++ lldb/trunk/unittests/Utility/FlagsTest.cpp Thu Jul 19 10:45:51 2018
@@ -0,0 +1,199 @@
+//===-- FlagsTest.cpp ---===-*- C++
-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Flags.h"
+
+using namespace lldb_private;
+
+enum DummyFlags {
+ eFlag0 = 1 << 0,
+ eFlag1 = 1 << 1,
+ eFlag2 = 1 << 2,
+ eAllFlags = (eFlag0 | eFlag1 | eFlag2)
+};
+
+TEST(Flags, GetBitSize) {
+ Flags f;
+ // Methods like ClearCount depend on this specific value, so we test
+ // against it here.
+ EXPECT_EQ(32U, f.GetBitSize());
+}
+
+TEST(Flags, Reset) {
+ Flags f;
+ f.Reset(0x3);
+ EXPECT_EQ(0x3U, f.Get());
+ EXPECT_EQ(2U, f.SetCount());
+}
+
+TEST(Flags, Clear) {
+ Flags f;
+ f.Reset(0x3);
+ EXPECT_EQ(2U, f.SetCount());
+
+ f.Clear(0x5);
+ EXPECT_EQ(1U, f.SetCount());
+
+ f.Clear();
+ EXPECT_EQ(0U, f.SetCount());
+}
+
+TEST(Flags, AllSet) {
+ Flags f;
+
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnySet) {
+ Flags f;
+
+ EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, Test) {
+ Flags f;
+
+ EXPECT_FALSE(f.Test(eFlag0));
+ EXPECT_FALSE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.Test(eFlag0));
+ EXPECT_FALSE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.Test(eFlag0));
+ EXPECT_TRUE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.Test(eFlag0));
+ EXPECT_TRUE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ // FIXME: Should Flags assert on Test(eFlag0 | eFlag1) (more than one bit)?
+}
+
+TEST(Flags, AllClear) {
+ Flags f;
+
+ EXPECT_TRUE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnyClear) {
+ Flags f;
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, IsClear) {
+ Flags f;
+
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_FALSE(f.IsClear(eFlag0));
+ EXPECT_FALSE(f.IsClear(eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_FALSE(f.IsClear(eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+}
+
+TEST(Flags, ClearCount) {
+ Flags f;
+ EXPECT_EQ(32U, f.ClearCount());
+
+ f.Set(eFlag0);
+ EXPECT_EQ(31U, f.ClearCount());
+
[Lldb-commits] [PATCH] D49435: Added unit tests for Flags
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337475: Added unit tests for Flags (authored by teemperor,
committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D49435?vs=155914&id=156319#toc
Repository:
rL LLVM
https://reviews.llvm.org/D49435
Files:
lldb/trunk/unittests/Utility/CMakeLists.txt
lldb/trunk/unittests/Utility/FlagsTest.cpp
Index: lldb/trunk/unittests/Utility/FlagsTest.cpp
===
--- lldb/trunk/unittests/Utility/FlagsTest.cpp
+++ lldb/trunk/unittests/Utility/FlagsTest.cpp
@@ -0,0 +1,199 @@
+//===-- FlagsTest.cpp ---===-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Flags.h"
+
+using namespace lldb_private;
+
+enum DummyFlags {
+ eFlag0 = 1 << 0,
+ eFlag1 = 1 << 1,
+ eFlag2 = 1 << 2,
+ eAllFlags = (eFlag0 | eFlag1 | eFlag2)
+};
+
+TEST(Flags, GetBitSize) {
+ Flags f;
+ // Methods like ClearCount depend on this specific value, so we test
+ // against it here.
+ EXPECT_EQ(32U, f.GetBitSize());
+}
+
+TEST(Flags, Reset) {
+ Flags f;
+ f.Reset(0x3);
+ EXPECT_EQ(0x3U, f.Get());
+ EXPECT_EQ(2U, f.SetCount());
+}
+
+TEST(Flags, Clear) {
+ Flags f;
+ f.Reset(0x3);
+ EXPECT_EQ(2U, f.SetCount());
+
+ f.Clear(0x5);
+ EXPECT_EQ(1U, f.SetCount());
+
+ f.Clear();
+ EXPECT_EQ(0U, f.SetCount());
+}
+
+TEST(Flags, AllSet) {
+ Flags f;
+
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AllSet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnySet) {
+ Flags f;
+
+ EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnySet(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AnySet(eFlag0 | eFlag1));
+}
+
+TEST(Flags, Test) {
+ Flags f;
+
+ EXPECT_FALSE(f.Test(eFlag0));
+ EXPECT_FALSE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.Test(eFlag0));
+ EXPECT_FALSE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Set(eFlag1);
+ EXPECT_TRUE(f.Test(eFlag0));
+ EXPECT_TRUE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.Test(eFlag0));
+ EXPECT_TRUE(f.Test(eFlag1));
+ EXPECT_FALSE(f.Test(eFlag2));
+
+ // FIXME: Should Flags assert on Test(eFlag0 | eFlag1) (more than one bit)?
+}
+
+TEST(Flags, AllClear) {
+ Flags f;
+
+ EXPECT_TRUE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ f.Clear(eFlag0);
+ EXPECT_FALSE(f.AllClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, AnyClear) {
+ Flags f;
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Set(eFlag1);
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_TRUE(f.AnyClear(eFlag0 | eFlag1));
+}
+
+TEST(Flags, IsClear) {
+ Flags f;
+
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+
+ f.Set(eFlag0);
+ EXPECT_FALSE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+
+ f.Set(eFlag1);
+ EXPECT_FALSE(f.IsClear(eFlag0));
+ EXPECT_FALSE(f.IsClear(eFlag1));
+
+ f.Clear(eFlag0);
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_FALSE(f.IsClear(eFlag1));
+
+ f.Clear(eFlag1);
+ EXPECT_TRUE(f.IsClear(eFlag0));
+ EXPECT_TRUE(f.IsClear(eFlag1));
+}
+
+TEST(Flags, ClearCount) {
+ Flags f;
+ EXPECT_EQ(32U, f.ClearCount());
+
+ f.Set(eFlag0);
+ EXPECT_EQ(31U, f.ClearCount());
+
+ f.Set(eFlag0);
+ EXPECT_EQ(31U, f.ClearCount());
+
+ f.Set(eFlag1);
+ EXPECT_EQ(30U, f.ClearCount());
+
+ f.Set(eAllFlags);
+ EXPECT_EQ(29U, f.ClearCount());
+}
+
+TEST(Flags, SetCount) {
+ Flags f;
+ EXPECT_EQ(0U, f.SetCount());
+
+ f.Set(eFlag0);
+ EXPECT_EQ(1U, f.SetCount());
+
+ f.Set(eFlag0);
+ EXPECT_EQ(1U, f.SetCount());
+
+ f.Set(eFlag1);
+ EXPECT_EQ(2U, f.SetCount());
+
+ f.Set(eAllFlags);
+ EXPECT_EQ(3U, f.SetCount());
+}
Index: lldb/trunk/unittests/Utility/CMakeLists.txt
===
--- lldb/trunk/unittests/Utility/CMakeLists.txt
+++ lldb/trunk/unittests/Utility/CMakeLists.txt
@@ -8,6 +8,7 @@
CompletionReq
[Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command
teemperor updated this revision to Diff 156341.
teemperor added a comment.
- Test case for completing when we only have a forward decl in another file (as
suggested by Fred).
https://reviews.llvm.org/D48465
Files:
include/lldb/Expression/ExpressionParser.h
include/lldb/Expression/UserExpression.h
packages/Python/lldbsuite/test/expression_command/completion/.categories
packages/Python/lldbsuite/test/expression_command/completion/Makefile
packages/Python/lldbsuite/test/expression_command/completion/TestExprCompletion.py
packages/Python/lldbsuite/test/expression_command/completion/main.cpp
packages/Python/lldbsuite/test/expression_command/completion/other.cpp
packages/Python/lldbsuite/test/functionalities/completion/TestCompletion.py
packages/Python/lldbsuite/test/lldbtest.py
source/Commands/CommandObjectExpression.cpp
source/Commands/CommandObjectExpression.h
source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -143,6 +143,9 @@
lldb_private::ExecutionPolicy execution_policy,
bool keep_result_in_memory, bool generate_debug_info) override;
+ bool Complete(ExecutionContext &exe_ctx, StringList &matches,
+unsigned complete_pos) override;
+
ExpressionTypeSystemHelper *GetTypeSystemHelper() override {
return &m_type_system_helper;
}
@@ -198,6 +201,10 @@
lldb::TargetSP m_target_sp;
};
+ /// The absolute character position in the transformed source code where the
+ /// user code (as typed by the user) starts. If the variable is empty, then we
+ /// were not able to calculate this position.
+ llvm::Optional m_user_expression_start_pos;
ResultDelegate m_result_delegate;
};
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -402,6 +402,16 @@
"couldn't construct expression body");
return llvm::Optional();
}
+
+// Find and store the start position of the original code inside the
+// transformed code. We need this later for the code completion.
+std::size_t original_start;
+std::size_t original_end;
+bool found_bounds = source_code->GetOriginalBodyBounds(
+m_transformed_text, lang_type, original_start, original_end);
+if (found_bounds) {
+ m_user_expression_start_pos = original_start;
+}
}
return lang_type;
}
@@ -591,6 +601,119 @@
return true;
}
+//--
+/// Converts an absolute position inside a given code string into
+/// a column/line pair.
+///
+/// @param[in] abs_pos
+/// A absolute position in the code string that we want to convert
+/// to a column/line pair.
+///
+/// @param[in] code
+/// A multi-line string usually representing source code.
+///
+/// @param[out] line
+/// The line in the code that contains the given absolute position.
+/// The first line in the string is indexed as 1.
+///
+/// @param[out] column
+/// The column in the line that contains the absolute position.
+/// The first character in a line is indexed as 0.
+//--
+static void AbsPosToLineColumnPos(unsigned abs_pos, llvm::StringRef code,
+ unsigned &line, unsigned &column) {
+ // Reset to code position to beginning of the file.
+ line = 0;
+ column = 0;
+
+ assert(abs_pos <= code.size() && "Absolute position outside code string?");
+
+ // We have to walk up to the position and count lines/columns.
+ for (std::size_t i = 0; i < abs_pos; ++i) {
+// If we hit a line break, we go back to column 0 and enter a new line.
+// We only handle \n because that's what we internally use to make new
+// lines for our temporary code strings.
+if (code[i] == '\n') {
+ ++line;
+ column = 0;
+ continue;
+}
+++column;
+ }
+}
+
+bool ClangUserExpression::Complete(ExecutionContext &exe_ctx,
+ StringList &matches, unsigned complete_pos) {
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+
+ // We don't want any visible feedback when completing an expression. Mostly
+ // because the results we get from an incomplete invocation ar
[Lldb-commits] [PATCH] D49579: Support parsing minidump files that are created by Breakpad.
clayborg created this revision.
clayborg added reviewers: labath, zturner.
Breakpad files sometimes extra 4 bytes of padding after the 32 bit count for
memory, module and thread lists. I also created bare minimum minidump files
that contain both padded and not padded files that test each functionality
works correctly.
https://reviews.llvm.org/D49579
Files:
source/Plugins/Process/minidump/MinidumpTypes.cpp
unittests/Process/minidump/Inputs/memory-list-not-padded.dmp
unittests/Process/minidump/Inputs/memory-list-padded.dmp
unittests/Process/minidump/Inputs/module-list-not-padded.dmp
unittests/Process/minidump/Inputs/module-list-padded.dmp
unittests/Process/minidump/Inputs/thread-list-not-padded.dmp
unittests/Process/minidump/Inputs/thread-list-padded.dmp
unittests/Process/minidump/MinidumpParserTest.cpp
Index: unittests/Process/minidump/MinidumpParserTest.cpp
===
--- unittests/Process/minidump/MinidumpParserTest.cpp
+++ unittests/Process/minidump/MinidumpParserTest.cpp
@@ -84,6 +84,79 @@
EXPECT_EQ(1232UL, context.size());
}
+TEST_F(MinidumpParserTest, GetThreadListNotPadded) {
+ // Verify that we can load a thread list that doesn't have 4 bytes of padding
+ // after the thread count.
+ SetUpData("thread-list-padded.dmp");
+ llvm::ArrayRef thread_list;
+
+ thread_list = parser->GetThreads();
+ ASSERT_EQ(2UL, thread_list.size());
+ EXPECT_EQ(0x11223344UL, thread_list[0].thread_id);
+ EXPECT_EQ(0x55667788UL, thread_list[1].thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetThreadListPadded) {
+ // Verify that we can load a thread list that has 4 bytes of padding
+ // after the thread count as found in breakpad minidump files.
+ SetUpData("thread-list-padded.dmp");
+ auto thread_list = parser->GetThreads();
+ ASSERT_EQ(2UL, thread_list.size());
+ EXPECT_EQ(0x11223344UL, thread_list[0].thread_id);
+ EXPECT_EQ(0x55667788UL, thread_list[1].thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetModuleListNotPadded) {
+ // Verify that we can load a module list that doesn't have 4 bytes of padding
+ // after the module count.
+ SetUpData("module-list-not-padded.dmp");
+ auto module_list = parser->GetModuleList();
+ ASSERT_EQ(2UL, module_list.size());
+ EXPECT_EQ(0x1000UL, module_list[0].base_of_image);
+ EXPECT_EQ(0x2000UL, module_list[0].size_of_image);
+ EXPECT_EQ(0x5000UL, module_list[1].base_of_image);
+ EXPECT_EQ(0x3000UL, module_list[1].size_of_image);
+}
+
+TEST_F(MinidumpParserTest, GetModuleListPadded) {
+ // Verify that we can load a module list that has 4 bytes of padding
+ // after the module count as found in breakpad minidump files.
+ SetUpData("module-list-padded.dmp");
+ auto module_list = parser->GetModuleList();
+ ASSERT_EQ(2UL, module_list.size());
+ EXPECT_EQ(0x1000UL, module_list[0].base_of_image);
+ EXPECT_EQ(0x2000UL, module_list[0].size_of_image);
+ EXPECT_EQ(0x5000UL, module_list[1].base_of_image);
+ EXPECT_EQ(0x3000UL, module_list[1].size_of_image);
+}
+
+TEST_F(MinidumpParserTest, GetMemoryListNotPadded) {
+ // Verify that we can load a memory list that doesn't have 4 bytes of padding
+ // after the memory range count.
+ SetUpData("memory-list-not-padded.dmp");
+
+ if (auto mem = parser->FindMemoryRange(0x8000)) {
+EXPECT_EQ((lldb::addr_t)0x8000, mem->start);
+ }
+ if (auto mem = parser->FindMemoryRange(0x8010)) {
+EXPECT_EQ((lldb::addr_t)0x8010, mem->start);
+ }
+}
+
+TEST_F(MinidumpParserTest, GetMemoryListPadded) {
+ // Verify that we can load a memory list that has 4 bytes of padding
+ // after the memory range count as found in breakpad minidump files.
+ SetUpData("memory-list-padded.dmp");
+ auto mem = parser->FindMemoryRange(0x8000);
+ EXPECT_TRUE(mem.hasValue());
+ if (mem.hasValue())
+EXPECT_EQ((lldb::addr_t)0x8000, mem->start);
+ mem = parser->FindMemoryRange(0x8010);
+ EXPECT_TRUE(mem.hasValue());
+ if (mem.hasValue())
+EXPECT_EQ((lldb::addr_t)0x8010, mem->start);
+}
+
TEST_F(MinidumpParserTest, TruncatedMinidumps) {
InvalidMinidump("linux-x86_64.dmp", 32);
InvalidMinidump("linux-x86_64.dmp", 100);
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -81,11 +81,17 @@
llvm::ArrayRef
MinidumpThread::ParseThreadList(llvm::ArrayRef &data) {
+ const auto orig_size = data.size();
+
const llvm::support::ulittle32_t *thread_count;
Status error = consumeObject(data, thread_count);
if (error.Fail() || *thread_count * sizeof(MinidumpThread) > data.size())
return {};
+ // Sometimes there is padding after the count
+ if (4 + *thread_count * sizeof(MinidumpThread) < orig_size)
+data = data.drop_front(4);
+
return llvm::ArrayRef(
reinterpret_cast(data.data()), *thread_count);
}
@@ -158,8 +164,15 @@
llvm::ArrayRef
Minidu
[Lldb-commits] [lldb] r337515 - Defend LoadImageUsingPaths against a path list
Author: jingham
Date: Thu Jul 19 18:20:18 2018
New Revision: 337515
URL: http://llvm.org/viewvc/llvm-project?rev=337515&view=rev
Log:
Defend LoadImageUsingPaths against a path list
with empty paths on it.
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_using_paths/TestLoadUsingPaths.py
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_using_paths/TestLoadUsingPaths.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_using_paths/TestLoadUsingPaths.py?rev=337515&r1=337514&r2=337515&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_using_paths/TestLoadUsingPaths.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/functionalities/load_using_paths/TestLoadUsingPaths.py
Thu Jul 19 18:20:18 2018
@@ -103,10 +103,27 @@ class LoadUsingPathsTestCase(TestBase):
out_spec = lldb.SBFileSpec()
token = process.LoadImageUsingPaths(relative_spec, paths, out_spec,
error)
-self.assertNotEqual(token, lldb.LLDB_INVALID_IMAGE_TOKEN, "Got a valid
token")
-self.assertEqual(out_spec, lldb.SBFileSpec(self.hidden_lib), "Found
the expected library")
+self.assertNotEqual(token, lldb.LLDB_INVALID_IMAGE_TOKEN, "Got a valid
token with relative path")
+self.assertEqual(out_spec, lldb.SBFileSpec(self.hidden_lib), "Found
the expected library with relative path")
process.UnloadImage(token)
+
+# Make sure the presence of an empty path doesn't mess anything up:
+paths.Clear()
+paths.AppendString("")
+paths.AppendString(os.path.join(self.wd, "no_such_dir"))
+paths.AppendString(self.wd)
+relative_spec = lldb.SBFileSpec(os.path.join("hidden", self.lib_name))
+
+out_spec = lldb.SBFileSpec()
+token = process.LoadImageUsingPaths(relative_spec, paths, out_spec,
error)
+
+self.assertNotEqual(token, lldb.LLDB_INVALID_IMAGE_TOKEN, "Got a valid
token with included empty path")
+self.assertEqual(out_spec, lldb.SBFileSpec(self.hidden_lib), "Found
the expected library with included empty path")
+
+process.UnloadImage(token)
+
+
# Finally, passing in an absolute path should work like the basename:
# This should NOT work because we've taken hidden_dir off the paths:
Modified: lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp?rev=337515&r1=337514&r2=337515&view=diff
==
--- lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp Thu Jul 19
18:20:18 2018
@@ -1155,6 +1155,10 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb
size_t buffer_size = 0;
std::string path_array;
for (auto path : *paths) {
+ // Don't insert empty paths, they will make us abort the path
+ // search prematurely.
+ if (path.empty())
+continue;
size_t path_size = path.size();
path_array.append(path);
path_array.push_back('\0');
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
