[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added a comment. I thought about what my "ideal" design would look like. I'm not suggesting anyone should actually go off and do this, but since we're brainstorming design anyway, it doesn't hurt to consider what an optimal one should look like (and for the record, there may be things

[Lldb-commits] [PATCH] D56010: [NativePDB] Fix setting breakpoint by file and line

zturner created this revision. zturner added reviewers: aleksandr.urakov, lemo, labath, amccarth. Herald added a subscriber: arphaman. There were several problems preventing this from working. The first is that when the PDB had an absolute path to the main source file, we would construct an

[Lldb-commits] [PATCH] D56124: PECOFF: Fix section name computation

zturner added a comment. What about just making this function return a StringRef? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56124/new/ https://reviews.llvm.org/D56124 ___ lldb-commits mailing list lldb-commits@

[Lldb-commits] [PATCH] D56126: [NativePDB] Add basic support of methods recostruction in AST

zturner added a comment. > This info is contained in the Symbols stream, not in the TPI stream. As far > as I understand, we can't find in a simple way the link between a > LF_ONEMETHOD record and a corresponding S_GPROC32 record. I think it's the > place, where we need to use the approach simi

[Lldb-commits] [PATCH] D56126: [NativePDB] Add basic support of methods recostruction in AST

zturner added inline comments. Comment at: lit/SymbolFile/NativePDB/ast-methods.cpp:28 +// CHECK: |-CXXRecordDecl {{.*}} struct Struct definition +// CHECK: | |-CXXMethodDecl {{.*}} simple_method 'void () __attribute__((thiscall))' +// CHECK: | |-CXXMethodDecl {{.*}} virtual_met

[Lldb-commits] [PATCH] D56237: Implement GetFileLoadAddress for the Windows process plugin

zturner added a comment. We should probably have a test for this. It sounds like all you need to do to test this is create a target, set a breakpoint, run the process, then verify that you see some text that says the breakpoint was resolved. With the new build.py script this should be able to

[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

zturner added a comment. Should we use the GUID from the COFF Debug Directory instead? It certainly seems more appropriate, if it's there. The UUID's purpose is to match symbol to the executable, so if you use a hash of the path it might solve this one problem, but won't solve the general cas

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

zturner requested changes to this revision. zturner added a comment. This revision now requires changes to proceed. I don't think we can check in an executable file, we should try to compile it on the spot. We have 1-2 existing unit tests that check in an exe and we occasionally get reports tha

[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

zturner added a comment. In D56229#1346869 , @Hui wrote: > Not quite sure but correct me if i am wrong. > > (1) I think the Debug Directory is optional for COFF if it does have debug > information and pdb to match with. > > (2) The Debug Directory does no

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

zturner added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138 + // Double quote the string. + ::snprintf(arg_cstr, sizeof(arg_cstr), "--log-channels=\"%s\"", + env_debugserver_log_channels.c_str());

[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

zturner added inline comments. Comment at: source/Host/windows/PipeWindows.cpp:34-38 m_read = INVALID_HANDLE_VALUE; m_write = INVALID_HANDLE_VALUE; - m_read_fd = -1; - m_write_fd = -1; + m_read_fd = PipeWindows::kInvalidDescriptor; + m_write_fd = PipeWindows::kInvalid

[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for building lldb-server on Windows

zturner added inline comments. Comment at: include/lldb/Host/windows/PosixApi.h:78-79 +#define WNOHANG 1 +#define WUNTRACED 2 + krytarowski wrote: > I think that these symbols should not be leaked here in the first place. In general we should avoid mocking posi

[Lldb-commits] [PATCH] D56232: [lldb-server] Add remote platform capabilities for Windows

zturner added a comment. In D56232#1344652 , @labath wrote: > All of these functions seem identical to their PlatformPOSIX counterparts. Is > that right? And I seem to remember already seeing a lot of code duplication > between PlatformPOSIX and Platform

[Lldb-commits] [PATCH] D56232: [lldb-server] Add remote platform capabilities for Windows

zturner added inline comments. Comment at: source/Plugins/Platform/Windows/PlatformWindows.cpp:189-195 + else { +if (m_remote_platform_sp) + return m_remote_platform_sp->RunShellCommand( + command, working_dir, status_ptr, signo_ptr, command_output, timeout);

[Lldb-commits] [PATCH] D56231: [lldb-server] Improve support on Windows

zturner added inline comments. Comment at: source/Host/common/File.cpp:606-607 #else long cur = ::lseek(m_descriptor, 0, SEEK_CUR); +SeekFromStart(offset); error = Write(buf, num_bytes); Be careful here. `pwrite` on posix is atomic, which means th

[Lldb-commits] [PATCH] D56315: Add a verbose mode to "image dump line-table" and use it to write a .debug_line test

zturner accepted this revision. zturner added a comment. Ok, that makes sense. BTW, it would be nice if someone ever decided to make `llvm-mc` recognize more symbolic constants so we didn't have to use magic numbers everywhere. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56315/new/

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

zturner created this revision. zturner added reviewers: labath, clayborg, aleksandr.urakov, asmith, amccarth. Herald added subscribers: JDevlieghere, aprantl. `ParseDeclsForContext` was originally created to serve the very specific case where the context is a function block. It was never intende

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:318 + + TypeSystem *type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus); + if (!type_system) amccarth w

[Lldb-commits] [PATCH] D56461: [NativePDB] Add support for parsing typedefs and make lldb-test work with the native reader.

zturner created this revision. zturner added reviewers: aleksandr.urakov, amccarth, lemo, labath. Herald added a subscriber: aprantl. Typedefs are represented as `S_UDT` records in the globals stream. This creates a strange situation where "types" are actually represented as "symbols", so they

[Lldb-commits] [PATCH] D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

zturner created this revision. zturner added reviewers: clayborg, labath, davide, jingham. Herald added a subscriber: JDevlieghere. The function `SymbolFile::ParseTypes` previously accepted a `SymbolContext`. This makes it extremely difficult to implement faithfully, because you have to account

[Lldb-commits] [PATCH] D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes

zturner added inline comments. Comment at: source/Target/PathMappingList.cpp:76 ++m_mod_id; - m_pairs.push_back(pair(path, replacement)); + m_pairs.push_back(pair(NormalizePath(path), NormalizePath(replacement))); if (notify && m_callback) Slightly more i

[Lldb-commits] [PATCH] D43984: Make the clang module cache setting available without a target

zturner added subscribers: aprantl, labath, zturner. zturner added a comment. This change has introduced a dependency from Core -> clang Driver (due to #include "clang/Driver/Driver.h" in ModuleList.cpp). Can you please try to find a way to remove this dependency? Repository: rL LLVM https:/

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

zturner created this revision. zturner added reviewers: labath, jingham. Herald added a subscriber: mgorny. This was responsible for the cycle Core > ObjectFile > Core. The only reason this dependency was here was so that `Module` could have a function called `CreateJITModule` which created thin

[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

<    1   2   3   4   5   6   7   8   >