[Lldb-commits] [PATCH] D47232: Break dependency from Expression -> Commands

zturner created this revision. zturner added reviewers: labath, davide. The REPL (which lives in Expression) was making use of the command options for the expression command. It's arguable whether `REPL` should even live in `Expression` to begin with, but it makes more sense for Command to depe

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added a subscriber: aprantl. zturner added a comment. Yea I don’t think this addresses the problem. We should be able to link against parts of lldb without a dependency on clang. Since this is about configuring something related to clang, it seems like it should be isolated to some part of

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added a comment. In https://reviews.llvm.org/D47235#1109219, @labath wrote: > In a way, this makes sense, but in a lot of other ways, it actually makes > things worse :) > > My long-term goal is to be able to build lldb-server without even having > clang checked out (because it doesn't

[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

zturner added a comment. If the majority of active contributors think this is the right direction then don't let me hold it up. I mostly just wanted to raise the concern, but if you've evaluated the pros and cons and made a decision, I'm fine with that :) https://reviews.llvm.org/D46005 __

[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

zturner added a comment. FWIW, I think the single biggest improvement one could make to the LLDB test suite runtime is to compile all inferiors up front as part of the CMake step. If you run the test suite twice every inferior is unnecessarily compiled twice. https://reviews.llvm.org/D46005

[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

zturner added a comment. In https://reviews.llvm.org/D46005#1109761, @JDevlieghere wrote: > In https://reviews.llvm.org/D46005#1109693, @zturner wrote: > > > FWIW, I think the single biggest improvement one could make to the LLDB > > test suite runtime is to compile all inferiors up front as par

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added a comment. Can you revert the changes to `HostInfoBase.h`? It looks like now it's only whitespace changes. Comment at: source/Core/ModuleList.cpp:16 #include "lldb/Host/Symbols.h" +#include "lldb/Host/HostInfoBase.h" #include "lldb/Interpreter/OptionValuePrope

[Lldb-commits] [PATCH] D46005: [test] Add a utility for dumping all tests in the dotest suite

zturner added a comment. In https://reviews.llvm.org/D46005#1109911, @aprantl wrote: > In https://reviews.llvm.org/D46005#1109817, @zturner wrote: > > > In https://reviews.llvm.org/D46005#1109761, @JDevlieghere wrote: > > > > > In https://reviews.llvm.org/D46005#1109693, @zturner wrote: > > > > >

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added inline comments. Comment at: source/Core/ModuleList.cpp:94 - llvm::SmallString<128> path; - clang::driver::Driver::getDefaultModuleCachePath(path); - SetClangModulesCachePath(path); + assert(!g_default_clang_modules_cache_path.empty()); + SetClangModulesCache

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added a comment. Actually, now I’m starting to wonder why can’t ClangModulesDeclVendor.cpp just call this function in clangDriver to get the default if the accessor returns an empty string? That sidesteps all of this initialization funny business and lets the client just handle it. Wo

[Lldb-commits] [PATCH] D47228: Break dependency from Core to ObjectFileJIT

This revision was automatically updated to reflect the committed changes. Closed by commit rL333143: Break dependency from Core to ObjectFileJIT. (authored by zturner, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D47228?vs=148100&id=

[Lldb-commits] [PATCH] D47232: Break dependency from Expression -> Commands

zturner added a comment. In https://reviews.llvm.org/D47232#1108865, @jingham wrote: > Perhaps a better way to handle this is to think of REPL.cpp/h as adjuncts of > CommandObjectExpression.cpp/h - they mostly define the fancy IOHandler that > gets pushed when you run the command in this mode.

[Lldb-commits] [PATCH] D47232: Break dependency from Expression -> Commands

zturner added a comment. FWIW `REPL` only uses 4 of the 10+ fields of `CommandObjectExpression::Options`. If you don't like the separate struct idea, I can just pass them directly as as independent arguments to `REPL::SetCommandOptions`. https://reviews.llvm.org/D47232 ___

[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

zturner added a comment. That sounds good to me as well. https://reviews.llvm.org/D47235 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D47384: Remove dependency from Host to clang

zturner created this revision. zturner added reviewers: labath, davide. Herald added a subscriber: mgorny. The idea is to move the discovery of the clang resource directory into a more appropriate place, such as the clang expression parser. By continuing with this pattern we can isolate all cla

[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC

zturner added a subscriber: JDevlieghere. zturner added a comment. +1 I’d like to get rid of all EnumerateXXX with callback functions and replace with iterator based equivalents. Given that in this case the iterator version already exists, I definitely think we should try to use it instead Repos

[Lldb-commits] [PATCH] D47384: Remove dependency from Host to clang

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 rL333933: Remove dependency from Host to clang. (authored by zturner, committed by ). Herald added a subscriber: llvm-commit

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

zturner added a comment. The problem is that Phabricator doesn't preserve binary in diffs https://reviews.llvm.org/D47708 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

zturner added a subscriber: aleksandr.urakov. zturner added a comment. Do you just need a pdb, or does it really need to be a vs pdb? lld can generate high quality pdbs now. So it might be possible to use lld to link and produce a pdb when you run the test. Pavel’s suggestion is equally viable, y

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

zturner added a comment. As a general rule, lld-link is command line compatible with MSVC and clang-cl is command line compatible with cl. So, the /order option should work exactly the same with lld-link as it does with link. https://reviews.llvm.org/D47708 __

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

zturner added a comment. In https://reviews.llvm.org/D47708#1126669, @lemo wrote: > Doesn't the LIT based test drop the split-function case (originally > produced with PGO)? > > Sorry for being late to the party, but it seems beneficial to have both LIT > *and* checked in binaries since in gene

[Lldb-commits] [PATCH] D48084: [FileSpec] Delegate common operations to llvm::sys::path

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Looks like good cleanup to me, thanks! Comment at: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:2593-2596 +if (::strcmp(extension.GetCString

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

zturner added a subscriber: labath. zturner added a comment. The internal api has no guarantees as to its stability. https://reviews.llvm.org/D48215 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

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

zturner added a comment. It can assert when we pass the python or clang directory enumeration. We could also remove the values from the internal enumeration but keep them in the public enumeration. However, we can’t be forced into providing a stable internal api just because we must provide a stab

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

zturner added a comment. Long term I think one potentially interesting possibility for solving a lot of these threading and lazy evaluation issues is to make a task queue that runs all related operations on a single thread and returns a future you can wait on. This way, the core data structure

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

zturner added a subscriber: friss. zturner added a comment. Related question: Is the laziness done to save memory, startup time, or both? https://reviews.llvm.org/D48393 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org

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

zturner added a subscriber: teemperor. zturner added a comment. Ansi escape codes do not work on Windows. LLVM’s stream output classes have color support that does work in a cross platform manner. Can we use those instead? Repository: rL LLVM https://reviews.llvm.org/D49334 ___

[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

zturner added a comment. Fwiw I’ve seen cases where tests have passed even though they shouldn’t have — the functionality being tested was broken. The one that comes to mind was where we were doing a backtrace and then checking that it matched the regex “main\(argc=3” to make sure the local variab

[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 +

[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

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 +

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

zturner added a comment. I think the problem is in lld, we don’t emit S_UDT symbols because it caused weird problems in WinDbg. There’s a comment explaining it in PDB.cpp. But after some recent fixes this may just work. So we should probably try again to emit the S_UDT in lld, I think that shou

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

zturner added a subscriber: labath. zturner added a comment. I had previously thought of making a top level project called Dump that depends on everything. Also makes it very obvious where all the dumpers are. It can have overloaded functions called lldb_private::dump(T&) for every value of T, the

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

zturner added a subscriber: asmith. zturner added a comment. When you have a DIA interface for struct S, can you just call findChildren()? Will that enumerate tge unnamed struct? The fact that pdbutil doesn’t is only an indication of how the printing code behaves, you shouldn’t interpret anything

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

zturner added a comment. Wouldn’t the location of the unnamed struct be the location of its first member? We already print the offsets of the individual members, so that part is solvable (although that code is horrendously complex). The second part is figuring out how to correlate the member back

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

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. I am not sure what are the drawbacks of such a solution are. I guess the best way to find out is to try it and see if you observe any strange behavior :) I'm ok with that, for now. https

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

zturner added a comment. In the meantime, perhaps you could request commit access :) 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] D49990: Use rich mangling information in Symtab::InitNameIndexes()

zturner added inline comments. Comment at: include/lldb/Symbol/Symtab.h:58 + /// dependency. Keep a void* here instead and cast it on-demand on the cpp. + void *m_legacy_parser = nullptr; + sgraenitz wrote: > This is the hackiest point I guess. We have `llvm::A

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

zturner added inline comments. Comment at: include/lldb/Symbol/Symtab.h:58 + /// dependency. Keep a void* here instead and cast it on-demand on the cpp. + void *m_legacy_parser = nullptr; + sgraenitz wrote: > sgraenitz wrote: > > zturner wrote: > > > sgraenitz

[Lldb-commits] [PATCH] D50025: Don't ignore byte_order in Stream::PutMaxHex64

zturner added a comment. When is the Stream unit test coming? Maybe we should just add it first, then add this? https://reviews.llvm.org/D50025 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

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

zturner added reviewers: labath, jasonmolenda. zturner added inline comments. Comment at: include/lldb/Host/Editline.h:187 + /// Register a callback to retrieve the prompt. + void SetPromptCallback(PromptCallbackType callback, void *baton); + I'd love to stop

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

zturner added a comment. Please remember to test with the cmake build when you add or remove files, as that is the build that all of the buildbots use. I almost reverted this since it broke every LLDB buildbot, but I noticed that it's just forgetting to remove the files from the CMakeLists.txt so

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

zturner added a subscriber: labath. zturner added a comment. No objections here https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

zturner added a comment. Several months ago when this came up, i hypothesized that Utility would eventually contain a mix of random stuff some of which may not make sense in the long term. But that in the meantime, it’s useful to have some sort of layering abstraction for “has no other dependencie

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

zturner added a comment. For the Register stuff, for example, I think it could make sense for it to be in a project such as `HAL` (Hardware Abstraction Layer) or something similarly named. Everything that describes properties of specific CPUs could go there, perhaps even including `ArchSpec`.

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

zturner added a subscriber: lemo. zturner added a comment. I think we have llvm::contains() which would allow you to make this slightly better. Other than that, good find! Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits ma

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

zturner added a comment. Looks like i was wrong, nevermind! Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49980: [PDB] Parse UDT symbols and pointers to members (combined patch)

zturner added a subscriber: aleksandr.urakov. zturner added a comment. I am OOO this week and only have access to mobile, so hopefully someone else can review it https://reviews.llvm.org/D49980 ___ lldb-commits mailing list lldb-commits@lists.llvm.o

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

zturner added a subscriber: clayborg. zturner added a comment. Did you see my comments on the first round about how the CMake build didn’t work? Because I don’t see any changes to CMakeLists.txt here, which means it still won’t work. The easiest way to make sure you get all the fixes that may hav

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added a comment. How does this differ from VS Code's builtin LLDB MI support? https://reviews.llvm.org/D50365 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added a comment. To elaborate, if you install the C/C++ plugin, and you go to Debug -> Configurations, and you go down to the MICmdMode property, it is by default set to "gdb". But you can change this to "lldb" and it works out of the box. https://reviews.llvm.org/D50365

[Lldb-commits] [PATCH] D50384: Move Predicate.h from Host to Utility

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. lgtm, although Predicate is simple enough that I wonder if one day we should try to just delete it entirely. https://reviews.llvm.org/D50384 _

[Lldb-commits] [PATCH] D49980: [PDB] Parse UDT symbols and pointers to members (combined patch)

zturner added a comment. Ok I’ll take a look later today then when i get in https://reviews.llvm.org/D49980 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added inline comments. Comment at: tools/lldb-vscode/BreakpointBase.cpp:13-21 +uint64_t string_to_unsigned(const char *s, int base, uint64_t fail_value) { + if (s && s[0]) { +char *end = nullptr; +uint64_t uval = strtoull(s, &end, base); +if (*end == '\0') +

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added inline comments. Comment at: tools/lldb-vscode/JSONUtils.h:142 +std::vector GetStrings(const llvm::json::Object *obj, +llvm::StringRef key); + clayborg wrote: > Need to keep as is because as noted in the descripti

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added a comment. Looking pretty good, I went over it again and found a few more things. There's a bit more `auto` than I'm comfortable with, but I'm not gonna make a big deal about it. it does make the code a bit hard to read when it's used for trivial return values though. ===

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added a comment. I had a couple of other comments, but since I responded from email since I was on the go and I guess they didn't show up inline. Sorry about that. If you prefer I can resubmit them all as inline comments, or I guess you can just respond to the email thread. ===

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added inline comments. Comment at: tools/lldb-vscode/SourceBreakpoint.h:24 + // Set this breakpoint in LLDB as a new breakpoint + void SetBreakpoint(const char *source_path); +}; clayborg wrote: > zturner wrote: > > clayborg wrote: > > > zturner wrote:

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner added inline comments. Comment at: tools/lldb-vscode/lldb-vscode.cpp:2646 +g_vsc.out = fdopen(socket_fd, "w"); +if (g_vsc.in == nullptr || g_vsc.out == nullptr) { + if (g_vsc.log) clayborg wrote: > The mutex isn't the problem, it

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Thanks for being patient. Looking forward to actually using this on my Linux box. https://reviews.llvm.org/D50365 ___ lldb-commits mailing li

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

zturner accepted this revision. zturner added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:399-404 +JITLoaderList &ProcessMinidump::GetJITLoaders() { + if (!m_jit_loaders_ap) { +m_jit_loaders_ap = llvm::make_unique(); + } + return *m_jit

[Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

zturner added a subscriber: clayborg. zturner added a comment. For PE/COFF files, the Age is also in the executable and Guid+Age actually constitute a 20-byte UUID. Is this not the case on Apple? What object file format are you dealing with? https://reviews.llvm.org/D51442 ___

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a subscriber: aleksandr.urakov. zturner added a comment. I don’t think I’m really a good person to look at AST stuff. I can look for general style comments, obvious flaws, and test coverage. but you may be the best person regarding on the content of the patch. Does that sound ok? h

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a comment. I think in clang they have a method to dump the AST. Could we add something like that to `lldb-test`? We're using the `symbols` subcommand, maybe it would be worth it to add a `--dump-ast` flag to that subcommand? That seems like a generally useful testing feature.

[Lldb-commits] [PATCH] D50299: Migrate to llvm::unique_function instead of static member functions for callbacks

zturner added a comment. In https://reviews.llvm.org/D50299#1220983, @nealsid wrote: > Fix 80 char line length violations Just curious, how did you fix them? Was it by running clang-format, or manually? Repository: rLLDB LLDB https://reviews.llvm.org/D50299 ___

[Lldb-commits] [PATCH] D51557: Replace uses of LazyBool with LazyBool template

zturner added a comment. I think we should put this in `llvm/ADT` as it does not seem specific to debugging, and could be useful for others. Repository: rLLDB LLDB https://reviews.llvm.org/D51557 ___ lldb-commits mailing list lldb-commits@lists.

[Lldb-commits] [PATCH] D51557: Replace uses of LazyBool with LazyBool template

zturner added a comment. Also I think it doesn't need to be specific to member variables. We could just have template class Lazy { std::function Update; Optional Value; public: LazyValue(std::function Update) : Update(std::move(Update)) {} }; Then, to get the same member fun

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a comment. Lgtm, thanks! https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51615: Set Windows console mode to enable support for ansi escape codes

zturner added inline comments. Comment at: source/Core/Debugger.cpp:819 + consoleMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING; + SetUseColor(SetConsoleMode(hConsole, consoleMode)); +#endif xbolva00 wrote: > Should I rewrite it to more clear version like > > bool

[Lldb-commits] [PATCH] D51615: Set Windows console mode to enable support for ansi escape codes

zturner accepted this revision. zturner added a comment. lgtm after the one suggested change to the pre-processor conditional. Comment at: source/Core/Debugger.cpp:815 +#if defined(_WIN32) + HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE); + DWORD consoleMode; -

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:268-269 + +std::unique_ptr +GetClassOrFunctionParent(const llvm::pdb::PDBSymbol &symbol) { + const IPDBSession &session = symbol.getSession(); All file local fun

[Lldb-commits] [PATCH] D51966: Do not create new terminals when launching process on Windows with --no-stdio

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. I'm fine with this. https://reviews.llvm.org/D51966 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[Lldb-commits] [PATCH] D51967: [PDB] Use the raw PDB symbol interface more accurately

zturner accepted this revision. zturner added inline comments. This revision is now accepted and ready to land. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:274 - auto class_parent_id = raw.getClassParentId(); - if (auto class_parent = session.getSymbolById(clas

[Lldb-commits] [PATCH] D51967: [PDB] Use the raw PDB symbol interface more accurately

zturner added a comment. I've been experimenting with DIA locally and after some investigation I'm not sure this is going to be reliable. Let's say we have a class, we want the decl context containing the class. For example, on line 366. So we call `GetDeclContextContainingSymbol`. Despite

[Lldb-commits] [PATCH] D51967: [PDB] Use the raw PDB symbol interface more accurately

zturner accepted this revision. zturner added a comment. Maybe I can improve this in the native implementation of our PDB reader which I'm currently working on, so that the results can be more consistent. https://reviews.llvm.org/D51967 ___ lldb-co

[Lldb-commits] [PATCH] D39689: Add a dependency from check-lldb on lld

zturner accepted this revision. zturner added inline comments. Comment at: test/CMakeLists.txt:116-122 + if (CMAKE_SYSTEM_NAME MATCHES "Windows") +if (TARGET lld) + add_dependencies(check-lldb lld) +else () + message(WARNING "lld required to test LLDB on Window

[Lldb-commits] [PATCH] D39844: CompilerType: Add ability to retrieve an integral template argument

zturner added inline comments. Comment at: unittests/Symbol/TestClangASTContext.cpp:418-419 -arg = m_ast->GetTemplateArgument(t.GetOpaqueQualType(), 1, kind); -EXPECT_EQ(kind, eTemplateArgumentKindIntegral); -EXPECT_EQ(arg, int_type); +EXPECT_EQ(m_ast->GetTempla

[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

zturner added a comment. I'd be open to having another organizational component that isn't Utility. But as Jim said, there isn't a critical mass of stuff yet in Utility to figure out where it makes sense to draw the line. In all honesty, that second component might just end up being "Core" aga

[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

zturner added a comment. In https://reviews.llvm.org/D39896#922381, @probinson wrote: > Drive by comment: > > In https://reviews.llvm.org/D39896#94, @jingham wrote: > > > You're right, the Triple stuff is in ADT! I was using it as an example of > > something you clearly wouldn't do so I did

[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable

zturner added a comment. On second thought I actually think the strategy used here is fine. But we have `llvm::WritableBinaryStream` which could server as a single base interface for a `mapped_file_region` based implementation as well as a malloc-based implementation. For the malloc based imp

[Lldb-commits] [PATCH] D40519: Remove some duplicated code in UUID.cpp

zturner added inline comments. Comment at: source/Utility/UUID.cpp:77-78 void UUID::Dump(Stream *s) const { - const uint8_t *u = (const uint8_t *)GetBytes(); - s->Printf("%2.2X%2.2X%2.2X%2.2X-%2.2X%2.2X-%2.2X%2.2X-%2.2X%2.2X-%2.2X%2.2X%" -"2.2X%2.2X%2.2X%2.2X", -

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

zturner added a comment. You could use llvm's range adapters to make this slightly better. auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes); return llvm::find(Bytes, 0) != Bytes.end(); Another option would just be `return *this != UUID(m_num_uuid_bytes);` https://reviews.llvm.org/D40537

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

zturner added a comment. In https://reviews.llvm.org/D40537#937866, @sas wrote: > In https://reviews.llvm.org/D40537#937196, @zturner wrote: > > > You could use llvm's range adapters to make this slightly better. > > > > auto Bytes = makeArrayRef(m_uuid, m_num_uuid_bytes); > > return llvm::fi

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added a comment. It's too bad this has to be written as a unit test, because this would be the perfect candidate for a FileCheck style test. Probably a long shot, but have you tried the llvm-lit project? Last time I tried it, it basically worked, but there were only a handful of tests

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added a comment. For the same reason that the entire rest of the LLVM project and all other subprojects do it, when it makes sense and the nature of the test lends itself to it. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added a comment. Note that there's no interactivity here. This is "feed some input, get some output, make sure the output is correct". That's exactly what FileCheck is designed for. This isn't even testing the public API, it's testing the private API. We should prefer testing the ac

[Lldb-commits] [PATCH] D40636: Create a trivial lldb-test program

zturner created this revision. Herald added subscribers: krytarowski, mgorny, srhines. This is basically a proof-of-concept and starting point for having a testing-centric tool in LLDB. I'm sure this leaves a lot of room to be desired, but this at least allows us to have something to build on.

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added inline comments. Comment at: lit/Modules/compressed-sections.yaml:1 +# REQUIRES: zlib +# RUN: yaml2obj %s > %t labath wrote: > It's right here. (I'm open to suggestions where to place it). I see. I think part of the reason I didn't notice it is bec

[Lldb-commits] [PATCH] D40636: Create a trivial lldb-test program

zturner updated this revision to Diff 124960. zturner added a comment. Updated based on labath@'s suggestions. Also added a new class `LinePrinter`, shamelessly ripped and stripped down from a copy used in one of LLVM's dumpers, but that makes it possible to do some nice formatting easily. ht

[Lldb-commits] [PATCH] D40475: DWZ 12/12: DWZ test mode

zturner added a comment. In https://reviews.llvm.org/D40475#935615, @labath wrote: > The thing to be aware of with this testing strategy is that you will probably > be the only person who will be able to run these tests and verify dwz > functionality. If you don't setup a buildbot testing this

[Lldb-commits] [PATCH] D40636: Create a trivial lldb-test program

This revision was automatically updated to reflect the committed changes. Closed by commit rL319504: Add lldb-test. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D40636?vs=124960&id=125052#toc Repository: rL LLVM https://reviews.llvm.org/D40636 Files: lldb/trunk

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added inline comments. Comment at: lit/Modules/compressed-sections.yaml:20 + - Name:.bogus +# CHECK-NOT: .bogus +Type:SHT_PROGBITS labath wrote: > zturner wrote: > > You should probably put this as the very first check stateme

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added inline comments. Comment at: lit/Modules/compressed-sections.yaml:1 +# REQUIRES: zlib +# RUN: yaml2obj %s > %t labath wrote: > zturner wrote: > > labath wrote: > > > It's right here. (I'm open to suggestions where to place it). > > I see. I think p

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

zturner added inline comments. Comment at: lit/Modules/compressed-sections.yaml:20 + - Name:.bogus +# CHECK-NOT: .bogus +Type:SHT_PROGBITS labath wrote: > zturner wrote: > > labath wrote: > > > zturner wrote: > > > > You should probab

<    21   22   23   24   25   26   27   28   29   30   >