aleksandr.urakov added a comment.
It seems that you forgot to add the testing `*.cpp` files from the `Input`
directory to the patch...
Repository:
rL LLVM
https://reviews.llvm.org/D49410
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
dblaikie added a comment.
In https://reviews.llvm.org/D49411#1164680, @labath wrote:
> Normally this would be clearly a good thing, but the added complication here
> is that this function is part of a class hierarchy, and so this way you are
> forcing every implementation to take a
stella.stamenova added a comment.
When you send a review for the fix, please add me to it. Could you also take
care of the extra space in the output? Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
Author: jdevlieghere
Date: Tue Jul 17 08:11:15 2018
New Revision: 337291
URL: http://llvm.org/viewvc/llvm-project?rev=337291=rev
Log:
[CMake] Have check-lldb-unit use CMAKE_CURRENT_BINARY_DIR
llvm-lit uses `map_config` directives (generated at configuration-time) to
make it possible to pass a
aleksandr.urakov added a comment.
I've removed spaces in such places, but i'm not sure if this won't break
anything (may be something relies on that spaces). @clayborg, are these spaces
have some special purpose? I think it's a decor and not really
important.F6716378: patch.diff
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
clayborg added inline comments.
Comment at:
lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:860
PyRefType::Owned,
-Py_BuildValue("(Os)", session_dict.get(), command));
+Py_BuildValue("(Os)",
teemperor created this revision.
Herald added a subscriber: mgorny.
https://reviews.llvm.org/D49435
Files:
unittests/Utility/CMakeLists.txt
unittests/Utility/FlagsTest.cpp
Index: unittests/Utility/FlagsTest.cpp
===
---
clayborg added a comment.
In https://reviews.llvm.org/D49202#1165455, @lemo wrote:
> Great timing! ARM support would be most welcome. Are you planning to
> support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
> Mentovai just reminded me that the ARM support was added
teemperor updated this revision to Diff 155899.
teemperor added a comment.
- ASSERT_ -> EXPECT_
- Now using gtest's comparison macros where possible.
- Added a PrintTo function as otherwise the gtest comparison macros won't
compile.
https://reviews.llvm.org/D49415
Files:
aleksandr.urakov added a comment.
I think that this patch is more solid and covers more cases, than
https://reviews.llvm.org/D49368, especially in `CreateLLDBTypeFromPDBType`
part. But https://reviews.llvm.org/D49368 has also following:
- Filling of a layout info. It allows use of packed
teemperor added a comment.
@clayborg Seems reasonable, even though the Python docs say that we should use
Py_ssize_t instead of int. I'll update the patch.
https://reviews.llvm.org/D49411
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added inline comments.
Comment at: unittests/Utility/VMRangeTest.cpp:42
+ VMRange range1(0x100, 0x200);
+ VMRange range2(0x100, 0x200);
+ EXPECT_EQ(range1, range2);
Seems like a few more edge cases might be good:
```
EXPECT_NE(range1, VMRange(0x100,
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337311: Invert dependency between lldb-framework and
lldb-suite (authored by xiaobai, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D49406
Author: xiaobai
Date: Tue Jul 17 11:28:51 2018
New Revision: 337311
URL: http://llvm.org/viewvc/llvm-project?rev=337311=rev
Log:
Invert dependency between lldb-framework and lldb-suite
Summary:
Currently, if you build lldb-framework the entire framework doesn't
actually build. In order to build
aleksandr.urakov added a comment.
The expression `location = {{.*}},` should process that space correctly. It is
possible to remove that space in `DWARFExpression::DumpLocation` function, but
I don't know how necessary is it.
But what about a compiler version? I want to fix the issue if
stella.stamenova added a comment.
Please fix the space. The test will process it fine, but that doesn't mean we
shouldn't fix it. I'll email you with the setup we use for tests.
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
asmith updated this revision to Diff 155904.
asmith added a comment.
Adding missing inputs
https://reviews.llvm.org/D49410
Files:
lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
lit/SymbolFile/PDB/Inputs/PointerTypeTest.cpp
lit/SymbolFile/PDB/class-layout.test
Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)
Regarding the invalid minidumps, I used my
lemo added a comment.
Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)
Regarding the
clayborg added a comment.
I have an upcoming patch for adding ARM and ARM64 support to the minidump
parser and i was curious how you created the invalid minidump files that are
part of this patch?
Repository:
rL LLVM
https://reviews.llvm.org/D49202
davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.
Probably last round of comments. Thanks for your patience!
Comment at:
jingham accepted this revision.
jingham added a comment.
This looks fine. I agree with Davide that putting some description of the type
you are formatting in comments somewhere would be make things easier for
somebody else who might have to fix this (or to you when you've totally
forgotten
Author: jmolenda
Date: Tue Jul 17 16:44:09 2018
New Revision: 337335
URL: http://llvm.org/viewvc/llvm-project?rev=337335=rev
Log:
Link the lldb driver ("lldb") against the llvm static
libraries because of the new prettystackprinter dependency.
Modified:
aleksandr.urakov added a comment.
Yes, you are right, extra `location = DW_OP_addr(000140004114)` appeared
after this commit. But I hadn't noticed that, because the test was already
broken for me on `0fa537f42f1af238c74bf41998dc1af31195839a`. Now I have checked
one out and ran the test to
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
lgtm
https://reviews.llvm.org/D49406
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
labath added a comment.
I have no opinion on the implementation, but I like that you wrote a
non-execution test for this, as this will enable us one day to run it on
non-windows hosts too.
Comment at: lit/SymbolFile/PDB/class-layout.test:50-53
+CHECK-DAG:_List
labath added a comment.
Normally this would be clearly a good thing, but the added complication here is
that this function is part of a class hierarchy, and so this way you are
forcing every implementation to take a std::string, even though only one of
them cares about null-termination.
In
labath added a comment.
The tests seem fine, but as a matter of style I would suggest two changes:
- replace `ASSERT_XXX` with `EXPECT_XXX`: EXPECT macros will not terminate the
test, so you'll be able to see additional failures, which is helpful in
pinpointing the problem. ASSERT is good for
teemperor created this revision.
Herald added a subscriber: mgorny.
https://reviews.llvm.org/D49415
Files:
unittests/Utility/CMakeLists.txt
unittests/Utility/VMRangeTest.cpp
Index: unittests/Utility/VMRangeTest.cpp
===
---
aleksandr.urakov added a comment.
Ah, we need some way to synchronize our work :)
I think in this case it will be better to appreciate the pros and cons of our
patches, choose the better variant and improve that with the features available
in another variant. I will investigate your patch to
labath added a comment.
I suppose we can add an off-by-default DWP mode so that it can be used for
integration testing.
However, I wouldn't consider any future DWP changes "tested" if the code is
only exercised via this mode. As I said above, I think the majority of DWP code
can be exercised
JDevlieghere accepted this revision.
JDevlieghere added a comment.
LGTM, Thanks!
https://reviews.llvm.org/D49271
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Author: jdevlieghere
Date: Tue Jul 17 03:04:19 2018
New Revision: 337261
URL: http://llvm.org/viewvc/llvm-project?rev=337261=rev
Log:
Move pretty stack trace printer into driver.
We used to have a pretty stack trace printer in SystemInitializerCommon.
This was disabled on Apple because we didn't
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337261: Move pretty stack trace printer into driver.
(authored by JDevlieghere, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D49377?vs=155684=155834#toc
Repository:
rL LLVM
35 matches
Mail list logo
