[Lldb-commits] [PATCH] D48337: Refactor OnExit utility class in ClangUserExpression

labath added a comment. How is this class different from `lldb_private::CleanUp`? Shouldn't we just delete it and replace all uses with the pre-existing class? Repository: rL LLVM https://reviews.llvm.org/D48337 ___ lldb-commits mailing list lld

[Lldb-commits] [PATCH] D48215: Remove dependency from Host to python

This revision was automatically updated to reflect the committed changes. Closed by commit rL335104: Remove dependency from Host to python (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48215 Files: lldb/trunk/incl

[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class

labath created this revision. labath added reviewers: zturner, jingham, clayborg. Herald added a subscriber: mgorny. The dump function was the only part of this class which depended on high-level functionality. This was due to the DumpDataExtractor function, which uses info from a running target t

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

labath added a comment. I am not sure this will actually solve the problems you are seeing. This may avoid corrupting the internal DenseMap data structures, but it will not make the algorithm using them actually correct. For example the pattern in `ParseTypeFromDWARF` is: 1. check the "already

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

labath added a comment. In https://reviews.llvm.org/D48393#1139270, @clayborg wrote: > In https://reviews.llvm.org/D48393#1138989, @labath wrote: > > > I am not sure this will actually solve the problems you are seeing. This > > may avoid corrupting the internal DenseMap data structures, but it

[Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command

labath added a comment. I think this would be a very nice feature for lldb. Thank you for working on this. However, I am somewhat worried about how you're hooking the expression completer into the completion machinery. I think this should be cleaned up first. Comment at: p

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath created this revision. labath added reviewers: clayborg, sas, lemo, davide. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. During the previous attempt to generalize the UUID class, it was suggested that we represent invalid UUIDs as length zero (previousl

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath added a comment. In https://reviews.llvm.org/D48479#1140927, @lemo wrote: > > The slight complication here is that > > some clients (MachO) actually use the all-zero notation to mean "no UUID > > has been set". To keep this use case working, I have introduced an > > additional argument

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath updated this revision to Diff 152530. labath added a comment. Delete copy constructor. https://reviews.llvm.org/D48479 Files: include/lldb/Utility/UUID.h source/API/SBModuleSpec.cpp source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp source/Plugins/DynamicLoa

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath added a comment. In https://reviews.llvm.org/D48479#1141067, @lemo wrote: > One solution might be to encapsulate the MachO convention in the MachO > > code: check in there (maybe through a helper function) if the UUID is > "000...0" and map it to the empty UUID in that case. The UUID in

[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync

labath added a comment. In https://reviews.llvm.org/D48463#1140861, @teemperor wrote: > Adding Pavel because he wrote the PrintAsync code. > > Also @labath: Can you tell me what variables/functionality the > `m_output_mutex` in Editline.cpp is supposed to shield? I don't see any > documentation

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath added a comment. In https://reviews.llvm.org/D48479#1141274, @clayborg wrote: > Would love to remove the "accept_zeroes" argument everywhere. Too much > matching happens in LLDB and we can't have multiple shared libraries claiming > zeros as their UUID A zero UUID is a problem only if

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath updated this revision to Diff 152699. labath added a comment. - Use static factory functions instead of the extra argument (the best names I could come up with is fromData and fromOptionalData). https://reviews.llvm.org/D48479 Files: include/lldb/Utility/UUID.h source/API/SBModuleSp

[Lldb-commits] [PATCH] D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`

labath added a comment. I suppose this is fine. My main worry is whether llvm-gcc is the *only* situation where we rely on these tweaks, but I guess the best way to find out is to try removing them. https://reviews.llvm.org/D48500 ___ lldb-commits

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

labath added inline comments. Comment at: include/lldb/Utility/UUID.h:109 + + uint32_t m_num_uuid_bytes = 0; // Should be 0, 16 or 20 ValueType m_uuid; clayborg wrote: > Do we need this comment here? We currently take a 4 byte debug info CRC and > call it a

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

This revision was automatically updated to reflect the committed changes. Closed by commit rL335612: Represent invalid UUIDs as UUIDs with length zero (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D48479?vs=152699

[Lldb-commits] [PATCH] D48596: [SymbolFile] Implement GetCompleteObjCClass for .debug_names

labath added a comment. How are you planning on testing this? Most of the DWARFIndex functionality is tested in terms of lldb-test, but I think this function is not reachable from there. Maybe you could write a specific dotest-test which targets this? https://reviews.llvm.org/D48596 ___

[Lldb-commits] [PATCH] D48596: [SymbolFile] Implement GetCompleteObjCClass for .debug_names

labath accepted this revision. labath added a comment. looks good to me https://reviews.llvm.org/D48596 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48633: UUID: Add support for arbitrary-sized module IDs

labath created this revision. labath added reviewers: clayborg, lemo, sas, davide. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. The data structure is optimized for the case where the UUID size is <= 20 bytes (standard length emitted by the GNU linkers), but la

[Lldb-commits] [PATCH] D48641: Skip core file tests on build configurations lacking necessary components

labath created this revision. labath added reviewers: clayborg, jingham. To successfully open a core file, we need to have LLVM built with support for the relevant target. Right now, if one does not have the appropriate targets configured, the tests will fail. This patch uses the GetBuildConfigur

[Lldb-commits] [PATCH] D48646: Add a test for reading lld-generated build-ids

labath created this revision. labath added reviewers: aprantl, zturner. This test makes sure we are able to read the shorter build-ids which are generated by lld. To make this work, I've extended lldb-test to print the UUID of the loaded object file. I've renamed the lldb-test subcommand from "mo

[Lldb-commits] [PATCH] D48665: Added test case for: r334978 - Fixed file completion for paths that start with '~'

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good. Thank you. https://reviews.llvm.org/D48665 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync

labath requested changes to this revision. labath added a comment. This revision now requires changes to proceed. In https://reviews.llvm.org/D48463#1145434, @teemperor wrote: > Well in theory we could poke more holes in our guard from some nested > function, but this only fixes the deadlock sym

[Lldb-commits] [PATCH] D48641: Skip core file tests on build configurations lacking necessary components

This revision was automatically updated to reflect the committed changes. Closed by commit rL335859: Skip core file tests on build configurations lacking necessary components (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.

[Lldb-commits] [PATCH] D48633: UUID: Add support for arbitrary-sized module IDs

labath added inline comments. Comment at: source/Interpreter/OptionValueUUID.cpp:82 + llvm::SmallVector uuid_bytes; + UUID::DecodeUUIDBytesFromString(s, uuid_bytes); for (size_t i = 0; i < num_modules; ++i) { clayborg wrote: > Probably should hav

[Lldb-commits] [PATCH] D48633: UUID: Add support for arbitrary-sized module IDs

labath updated this revision to Diff 153321. labath added a comment. Add a check to the tab-completion function. https://reviews.llvm.org/D48633 Files: include/lldb/Utility/UUID.h source/Interpreter/OptionValueUUID.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Utility/UUID.

[Lldb-commits] [PATCH] D48633: UUID: Add support for arbitrary-sized module IDs

This revision was automatically updated to reflect the committed changes. Closed by commit rL335963: UUID: Add support for arbitrary-sized module IDs (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48633 Files: lldb

[Lldb-commits] [PATCH] D48646: Add a test for reading lld-generated build-ids

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 rL335967: Add a test for reading lld-generated build-ids (authored by labath, committed by ). Herald added a subscriber: llv

[Lldb-commits] [PATCH] D48796: Refactoring for for the internal command line completion API (NFC)

labath added a comment. Thank you for working on this. Overall, I like the direction this is going, but I'm OOO this week and the next one, so I'll defer to other reviewers on this one. https://reviews.llvm.org/D48796 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

labath added a subscriber: jankratochvil. labath added a comment. In https://reviews.llvm.org/D48782#1149051, @plotfi wrote: > In https://reviews.llvm.org/D48782#1148498, @alexshap wrote: > > > @labath > > > > > I am not denying that there is value in running the dotest suite in all > > > of the

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

labath added a comment. Could you elaborate on how/why is this wrong? E.g. in which situations does it matter whether we clear the PC list? https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.

[Lldb-commits] [PATCH] D49110: [testsuite] Implement a category to skip libstdcxx tests

labath accepted this revision. labath added a comment. Sorry for the slow response, I am OOO officially :P. I think this is fine in general. As for the list of platforms, android should be on that list but not for long (which is why one of the things I was planning to do next week was to implem

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

labath added a comment. Could you also add a test case for this? I think it should be possible to test this via the gdb-client (`test/testcases/functionalities/gdb_remote_client/`) test suite. If I understood the previous comments correctly, you'll need to mock a server that sends a `thread-pcs

[Lldb-commits] [PATCH] D49307: Fix some crashes and deadlocks in FormatAnsiTerminalCodes

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This looks straight-forward enough. Comment at: unittests/Utility/AnsiTerminalTest.cpp:18 + std::string format = ansi::FormatAnsiTerminalCodes(""); + EXPECT_STREQ("", forma

[Lldb-commits] [PATCH] D49334: [LLDB} Added syntax highlighting support

labath added a comment. We're also trying to avoid adding new clang-specific code to the debugger core. I think it would make more sense if the (clang-based) c++ highlighter was provided by some plugin. I see a couple of options: - the c++ language plugin: I think this is the most low-level plu

[Lldb-commits] [PATCH] D49282: [cmake] Add option to skip building lldb-server

labath added a comment. I think this is fine (though the meaning of SKIP_LLDB_SERVER is subtly different than SKIP_DEBUGSERVER), but while looking at this I got an idea for a possible improvement. How do you currently convince lldb to use ds2 instead of lldb-server? Do you just set the LLDB_DE

[Lldb-commits] [PATCH] D49377: Move pretty stack trace printer into driver.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I think this makes perfect sense. Could you also add the same thing to the other binaries in the tools subfolder? https://reviews.llvm.org/D49377 __

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

labath added a comment. Could you please add a test for the new behavior as well? https://reviews.llvm.org/D48865 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49038: [CMake] Give lldb tools functional install targets when building LLDB.framework

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Makes sense to me. https://reviews.llvm.org/D49038 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

labath added a comment. I don't agree with the two-stage initialization of the MinidumpParser class being introduced here. We deliberately introduced the `Create` static function to avoid this. If this `Initialize` function in checking invariants which are assumed to be hold by other parser met

[Lldb-commits] [PATCH] D49282: [cmake] Add option to skip building lldb-server

labath added a comment. In https://reviews.llvm.org/D49282#1163853, @xiaobai wrote: > In https://reviews.llvm.org/D49282#1163517, @labath wrote: > > > I think this is fine (though the meaning of SKIP_LLDB_SERVER is subtly > > different than SKIP_DEBUGSERVER), but while looking at this I got an i

[Lldb-commits] [PATCH] D49406: Invert dependency between lldb-framework and lldb-suite

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/lld

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

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 *array[90];

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

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 pe

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

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 c

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

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

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

labath added a comment. In https://reviews.llvm.org/D49415#1165179, @teemperor wrote: > - Added a PrintTo function as otherwise the gtest comparison macros won't > compile. Sorry, I did not anticipate that. It looks like the `iterator` typedef inside the VMRange is confusing gtest's universal

[Lldb-commits] [PATCH] D49435: Added unit tests for Flags

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. lgtm. Thanks. Comment at: unittests/Utility/CMakeLists.txt:10-11 EnvironmentTest.cpp + FlagsTest.cpp FileSpecTest.cpp JSONTest.cpp Please keep this

[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

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 usin

[Lldb-commits] [PATCH] D49579: Support parsing minidump files that are created by Breakpad.

labath added a subscriber: lemo. labath added a comment. @markmentovai, @lemo, do you know under which circumstances do these extra 4 bytes get emitted? Is there any chance we could document this better than just saying "sometimes"? Comment at: unittests/Process/minidump/Mini

[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class

labath added a comment. In https://reviews.llvm.org/D48351#1168384, @jingham wrote: > 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 ro

[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class

labath added subscribers: clayborg, jingham, zturner. labath added a comment. Well, if it depends on everything, then it can only used from places that also depend on everything. Right now I don't think it matters now (though it will create a new Dump <-> "everything calling Dump" cycle). However,

[Lldb-commits] [PATCH] D49579: Support parsing minidump files that are created by Breakpad.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. The extra padding is unfortunate, but I guess we have to live with it now. Looks good. Thanks. https://reviews.llvm.org/D49579 ___ lldb-commits

[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB

labath added inline comments. Comment at: cmake/modules/LLDBConfig.cmake:417-423 +option(LLDB_USE_BUILTIN_DEMANGLER "Use lldb's builtin demangler" OFF) +option(LLDB_USE_LLVM_DEMANGLER "Use llvm's new partial demangler" ON) endif() if(LLDB_USE_BUILTIN_DEMANGLER) add

[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB

labath added a comment. In https://reviews.llvm.org/D49612#1171363, @sgraenitz wrote: > > this new demangler should also be available in the MSVC case, should it not? > > I don't think the Itanium mangler supports MSVC mangling. That's fine. It just needs to be able to demangle itanium names wh

[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB

labath added a comment. In https://reviews.llvm.org/D49612#1171395, @sgraenitz wrote: > > That's fine. It just needs to be able to demangle itanium names when > > running on an MSVC platform. > > IIUC `cstring_mangling_scheme()` should return `eManglingSchemeItanium` in > this case and switch t

[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class

labath added a comment. In https://reviews.llvm.org/D48351#1169959, @jingham wrote: > Dump is really meant to be the private description of the object that you > would use for logging and the like - Description was the public face of a > class. So while the Description-like functionality could

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. Which version is this patch based on? The line numbers don't seem to match what I see on master. Could you rebase the patch to master, and upload the patch with a full diff (e.g. via `git diff -U`, see https://llvm.org/docs/Phabricator.html#id3).

[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB

labath added a comment. The patch makes sense to me. It's nice to get a performance boost, while reducing the number of demanglers at the same time. Comment at: source/Core/Mangled.cpp:310 +#elif defined(LLDB_USE_LLVM_DEMANGLER) +llvm::ItaniumPartialDemangler IPD; +

[Lldb-commits] [PATCH] D48351: Move dumping code out of RegisterValue class

This revision was automatically updated to reflect the committed changes. Closed by commit rL337832: Move dumping code out of RegisterValue class (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48351 Files: lldb/tru

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. I am sure that particular test is worth trying to implement in lit. That script of yours is full of operating system specifics, and it's going to be super flaky. I'd suggest keeping this as a python test, as there you can abstract the platform specifics much easier, and

[Lldb-commits] [PATCH] D49755: Remove unused History class

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Yes, please. https://reviews.llvm.org/D49755 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

labath added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:179 +auto platform_sp = target.GetPlatform(); +if (platform_sp && !platform_sp->IsCompatibleArchitecture(arch, false, nullptr)) { + ArchSpec platform_arch;

[Lldb-commits] [PATCH] D49334: [LLDB} Added syntax highlighting support

labath added a comment. The patch is bigger than ideal for a single change, but I like the way it is structured. Thank you for abstracting the clang specifics. The one high-level question that came to mind when looking this over is whether we really need to do all this file name matching to get

[Lldb-commits] [PATCH] D49779: Make framework-header-fix process copied headers

labath added a reviewer: xiaobai. labath added a comment. Adding Alex, as he was doing a lot of framework work lately. https://reviews.llvm.org/D49779 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. In https://reviews.llvm.org/D49739#1174744, @apolyakov wrote: > What do you think about running tests with a hardcoded port number(as it's > done in a web-services). Doing this, we get rid of additional scripts and > os-specific things. AFAIK, debugserver even has its ow

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. Could you also add a test for this? https://reviews.llvm.org/D49685 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. In https://reviews.llvm.org/D49739#1174769, @apolyakov wrote: > `packages/Python/lldbsuite/test/tools/lldb-mi/signal/TestMiSignal.py` test > contains `port = 12000 + random.randint(0, 3999)`. Ok, that's bad. I don't know why the other lldb-mi tests are flaky, but these

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. In https://reviews.llvm.org/D49739#1174810, @apolyakov wrote: > Thanks, I used this `lldb-server gdbserver --pipe 0 localhost:0` and got a > port chosen by debugserver. But to let lldb-mi know about this port we need > an additional script, is python a good choice for th

[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility

labath added a comment. In https://reviews.llvm.org/D49740#1173655, @teemperor wrote: > Did you test that with lldb's C++ modules? If not I can test it locally. I didn't, but I don't expect there to be any issues, as I've made sure that the moved files don't include anything outside of lldb/U

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added inline comments. Comment at: lit/lit.cfg:59 -debugserver = lit.util.which('debugserver', lldb_tools_dir) +if platform.system() in ['Darwin']: +debugserver = lit.util.which('debugserver', lldb_tools_dir) apolyakov wrote: > Do we have `debugserve

[Lldb-commits] [PATCH] D49831: Don't print two errors for unknown commands.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good. It would be nice to mention the name of that other function in the commit message. https://reviews.llvm.org/D49831 ___ lldb-commits

[Lldb-commits] [PATCH] D49334: [LLDB} Added syntax highlighting support

labath added a comment. Thanks for the explanation. This looks fine to me, though I would feel better if someone else gave it a look too. Comment at: source/Core/Highlighter.cpp:29 + if (!m_prefix.empty()) +s << lldb_utility::ansi::FormatAnsiTerminalCodes(m_prefix); + s

[Lldb-commits] [PATCH] D49322: Narrow the CompletionRequest API to being append-only.

labath added a comment. The patch makes sense to me, though I can't say I'm that familiar with this code. The thing I'd really like to get rid of is the need to have `return request.GetNumberOfMatches();` everywhere, but that's a fight for another day.. Comment at: include/l

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. In https://reviews.llvm.org/D49685#1175413, @EugeneBi wrote: > In https://reviews.llvm.org/D49685#1174770, @labath wrote: > > > Could you also add a test for this? > > > I never ran LLDB tests, not sure where they are and what they are. > Also, how would you test that? I

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added inline comments. Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11 +def get_free_port(): +s = socket.socket() +s.bind(('', 0)) +return s.getsockname()[1] apolyakov wrote: > labath wrote: > > This is still racy, bec

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added inline comments. Comment at: lit/tools/lldb-mi/target/inputs/target-select-so-path.py:8-11 +def get_free_port(): +s = socket.socket() +s.bind(('', 0)) +return s.getsockname()[1] apolyakov wrote: > labath wrote: > > apolyakov wrote: > > >

[Lldb-commits] [PATCH] D49888: Stop building liblldb with CMake's framework functionality

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Can't say I fully understand what's going on, but the changes seem reasonable to me. If there is some public cmake bug describing the issue you ran into, it would be nice, for the sake of fut

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. In https://reviews.llvm.org/D49685#1177046, @EugeneBi wrote: > It is specific to shared libraries. Opening the executable and core dump > follows different code path. Which path is that? Is the path different even when you don't specify an executable when opening the c

[Lldb-commits] [PATCH] D49909: Unit test for Symtab::InitNameIndexes

labath added a comment. I am wondering whether a test like this wouldn't be better of as a lit test instead of a unit test. Right now, if you do a `lldb-test symbols foo.o` (and foo.o contains debug info), we will dump out a very similar index which is built from the information in the debug_i

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. In https://reviews.llvm.org/D49685#1178528, @EugeneBi wrote: > Hmm... I never thought I can do that :) > Anyway, with the change as it is now, LLDB would try to load executable in > the sysroot, fail that, then open it without the sysroot. Does that mean it is possible

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

labath added a comment. In https://reviews.llvm.org/D49685#1178730, @EugeneBi wrote: > I looked at the tests - is it all in Python? Not sure I have time to learn a > new language... Is there anything in C++? We have unit tests in c++, but it's going to be quite hard to tickle this code path f

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

labath added a comment. Thanks for looking out for the bots. I am afraid the compiler check will not be enough here. After that, you'll run into the issue of the system libc++ not being recent enough to contain std::optional. I suppose this could be handled by including some other header first

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

labath added a comment. I haven't read through this in detail yet, but I think this is a good start! The part I'm not sure about is whether the RichManglingInfo vs. RichManglingSpec distinction brings any value. I mean, the lifetime of the first is tied to the lifetime of the second, and the Sp

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This is really great. Thank you for doing this. I have some small ideas for improvement, but I don't think we have to go through another review cycle for that. Comment at:

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

labath added a comment. I am glad filing the cmake bug has paid off. :) I just have one small question about this patch. Comment at: cmake/modules/AddLLDB.cmake:81-87 + # install-liblldb{,-stripped} is the actual target that will install the + # framework, so it must rely on

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

labath added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21 +@add_test_categories(["libc++"]) +@skipIf(oslist=no_match(["macosx"]), compiler="clang", com

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

labath added a comment. I think there is still something wrong with the diff. I can't see any of the callers of e.g. `createItaniumInfo` but I can see the function on both LHS and RHS of the diff (which shouldn't be the case as it's a new function). It looks like you uploaded just an ammending

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. Sorry for not responding, I've been a bit busy these days. I've wanted to make a proof-of-concept to show you that reading the port number from stdout is possible, but I haven't gotten around to it yet. However, I am happy with the current mechanism of choosing the port

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

labath added a comment. Thank you for updating the patch. If I understand things correctly, we could avoid circular deps and untyped pointers (or `llvm::Any`, which is essentially the same thing), by moving `CPlusPlusLanguage::MethodName` to a separate file. Could we do that as a preparatory s

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

labath added inline comments. Comment at: include/lldb/Core/RichManglingInfo.h:83-84 +public: + RichManglingInfo *SetItaniumInfo(); + RichManglingInfo *SetLegacyCxxParserInfo(const ConstString &mangled); + I find it odd that one of these functions is stateless

[Lldb-commits] [PATCH] D50087: Add doxygen comments to the StackFrameList API (NFC)

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I am not too familiar with this code, but the descriptions seem to make sense. However, since you have kind of opened up the `Optional` discussion, I'll use this opportunity to give my take on

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D50038 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lld

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

labath added a comment. Sounds like a nice feature to have. In addition to the other feedback you've received, I'd suggest splitting out the addition of new format entities (frame count and friends) and the core interpolation feature into separate patches. Repository: rL LLVM https://review

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks great. I only noticed some typos when looking this over again. We can continue the register shuffling discussion offline. Comment at: include/lldb/Target/Target.h:929-

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

labath added a comment. Thanks for the patience. I've tried the patch out on our end. You shouldn't have any problems now. Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21 +@

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

labath added inline comments. Comment at: unittests/Utility/StreamTest.cpp:38-41 +TEST_F(StreamTest, ChangingByteOrder) { + s.SetByteOrder(lldb::eByteOrderPDP); + EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder()); +} probinson wrote: > teemperor wrote: > > labat

[Lldb-commits] [PATCH] D50159: Add byte counting mechanism to LLDB's Stream class.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I think there's a small difference in semantics between this and the `tell` function on llvm streams. This tells the number of bytes written, while the other one an absolute position within th

[Lldb-commits] [PATCH] D50161: Add raw_ostream wrapper to the Stream class

labath added a comment. Wouldn't it be even better to actually expose the llvm class via some accessor or something? This way we could slowly migrate existing code by changing it to write to `stream.accessor()` instead of `stream` ? (I am not saying to do that now, but it opens up possibilities

<    1   2   3   4   5   6   7   8   9   10   >