[Lldb-commits] [PATCH] D39314: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD

zturner added a comment. Ahh, you might also run the exact same test again but where you've loaded this inside of main with `LoadLibrary` instead of relying on early binding by the loader. https://reviews.llvm.org/D39314 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D39315: Correct the start address for exported windows functions in arm

zturner added a comment. In https://reviews.llvm.org/D39315#908246, @sas wrote: > Same thing here, I have no idea how to go about testing something specific > like this. Given that this is Windows Phone, it's even worse than simply > Windows Desktop because the only way to test is by doing remo

[Lldb-commits] [PATCH] D39315: Correct the start address for exported windows functions in arm

zturner added a comment. So when you're using ds2 as the remote, are you still using the normal lldb test suite? `dotest.py`? If so, then we could have a test decorator that says `@expectedFailure(not(debugserver=ds2))` or something. Then, even though you're the only one that can run it, at

[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

zturner added inline comments. Comment at: include/lldb/Host/HostInfoBase.h:85 + //--- + /// If the triple contains not specify the vendor, os, and environment parts, + /// we "augment" these using informa

[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

zturner added inline comments. Comment at: source/Target/Platform.cpp:986-991 +if (normalized_triple.getVendorName().empty()) + normalized_triple.setVendor(compatible_triple.getVendor()); +if (normalized_triple.getOSName().empty()) + normalized_triple.setOS(comp

[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

zturner added inline comments. Comment at: source/Target/Platform.cpp:986-991 +if (normalized_triple.getVendorName().empty()) + normalized_triple.setVendor(compatible_triple.getVendor()); +if (normalized_triple.getOSName().empty()) + normalized_triple.setOS(comp

[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

zturner added inline comments. Comment at: source/Core/ArchSpec.cpp:890 +bool ArchSpec::ContainsOnlyArch(const llvm::Triple &triple) { + return triple.getOSName().empty() && + triple.getVendorName().empty() && Should you also add `!triple.getArchName().e

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. Would you mind deleting and re-creating this revision with lldb-commits added as a subscriber? I don't think it's sufficient to just "add" it as a subscriber after the fact, I think it has to be done as part of the initial creation, or for some reason it won't show up

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. Hmm, weird. Maybe it already is? Even though I didn't see it on the original email. Anyway, ignore my last suggestion. On to actual comments: I'm not really crazy about the `regex:` prefix. Seems ugly and non-intuitive. Couldn't we just treat every filename as an a

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. In https://reviews.llvm.org/D39436#911254, @hintonda wrote: > In https://reviews.llvm.org/D39436#911237, @zturner wrote: > > > Hmm, weird. Maybe it already is? Even though I didn't see it on the > > original email. Anyway, ignore my last suggestion. > > > > On to actu

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. In https://reviews.llvm.org/D39436#911264, @jingham wrote: > Zachary's suggestion is better than adding regex patterns to FileSpec, but I > still don't like the idea of encoding option types in the option values. > > Moreover, this doesn't really apply to the main use of

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. In https://reviews.llvm.org/D39436#911302, @hintonda wrote: > In https://reviews.llvm.org/D39436#911264, @jingham wrote: > > > Zachary's suggestion is better than adding regex patterns to FileSpec, but > > I still don't like the idea of encoding option types in the optio

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > I was unhappy when we went over two pointers for a FileSpec when m_syntax was > added due to the extra size. Anything we can do to make this smaller would be > great, so the type on the enum would work, but a

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added a comment. In https://reviews.llvm.org/D39436#912902, @hintonda wrote: > In https://reviews.llvm.org/D39436#912829, @clayborg wrote: > > > In https://reviews.llvm.org/D39436#912828, @zturner wrote: > > > > > In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > > > > > > > I

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added inline comments. Comment at: include/lldb/Utility/FileSpec.h:65-69 + enum PathSyntax : unsigned char { ePathSyntaxPosix, ePathSyntaxWindows, ePathSyntaxHostNative }; This is actually a very nice change, as it reduces the size of `

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

zturner added inline comments. Comment at: include/lldb/Utility/FileSpec.h:65-69 + enum PathSyntax : unsigned char { ePathSyntaxPosix, ePathSyntaxWindows, ePathSyntaxHostNative }; hintonda wrote: > zturner wrote: > > This is actually a very nic

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

zturner added a comment. Ok, I wasn't aware of the libedit problem. I still don't like this approach, because it causes the design of the `File` class to be influenced by a limitation of a 3rd party library. What about adding a method to `IOObject` called `FILE *GetFileStream()`. Then the met

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto

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

zturner added a comment. In D56462#1351391 , @clayborg wrote: > So I like the ability to specify any symbol context to parse all types for. > If you pass in a symbol context with only a module filled in, we parse all > types. If we pass in a symbol conte

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

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB350764: Change lldb-test to use ParseAllDebugSymbols. (authored by zturner, committed by ). Herald added subscribers: teemperor, abidh. Changed prior to commit: https://reviews.llvm.org/D56418?vs=180

[Lldb-commits] [PATCH] D56511: [Python] Update PyString_FromString() to work for python 2 and 3.

zturner requested changes to this revision. zturner added a comment. This revision now requires changes to proceed. In D56511#1351768 , @JDevlieghere wrote: > There's two more mentions of this function: > > - `unittests/ScriptInterpreter/Python/PythonData

[Lldb-commits] [PATCH] D56511: [Python] Update PyString_FromString() to work for python 2 and 3.

zturner added inline comments. Comment at: lldb/scripts/Python/python-wrapper.swig:834 +#endif +PyObject* result = PyObject_CallMethodObjArgs(implementor, str_arg, arg, NULL); return result; Note that there appears to be an object leak here, as the `str

[Lldb-commits] [PATCH] D56511: [Python] Update PyString_FromString() to work for python 2 and 3.

zturner accepted this revision. zturner added inline comments. This revision is now accepted and ready to land. Comment at: lldb/scripts/Python/python-wrapper.swig:827 lldb::SBFrame frame_sb(frame_sp); PyObject *arg = SBTypeToSWIGWrapper(frame_sb); Th

[Lldb-commits] [PATCH] D56517: [Python] Update checkDsymForUUIDIsOn to be compatible with Python 3.

zturner added a comment. Another way of dealing with this is to pass `universal_newlines=True` to `subprocess.Popen`. Despite the silly name, all this really means is "redirected pipes are text mode". As long as you expect the command being run to always return text rather than binary, it's s

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

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. This looks much nicer with the `pipe_t` type. Comment at: source/Host/posix/PipePosix.cpp:70 +PipePosix::PipePosix(lldb::pipe_t read, lldb::pipe_t write) +: m_fds{(int)

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

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/lit/SymbolFile/NativePDB/typedefs.cpp:3 + +// REQUIRES: system-windows +// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s labath wrote: > What's the reason f

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

zturner marked 2 inline comments as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:626 +// a series of namespaces. +// FIXME: do this. +CVSymbol global = m_index.ReadSymbolRecord(uid.asGlobalSym());

[Lldb-commits] [PATCH] D56517: [Python] Update checkDsymForUUIDIsOn to be compatible with Python 3.

zturner added a comment. It's probably fine the way it is. The biggest advantage of using `universal_newlines=True` is that the result will always be a `string` in both Python 2 and Python 3. This makes it easier to reason about subsequent code , but if you're not doing much with the subseque

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

zturner added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:255 + void ComputePathStyle(); + aprantl wrote: > I would probably call this DetectPathStyle() since it's more a heuristic? Maybe even `Guess` since `Compute` implies absolu

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

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 rL350889: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit. (authored by zturner, committed by ). Herald added a su

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

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 rL350888: [NativePDB] Add support for parsing typedef records. (authored by zturner, committed by ). Herald added a subscrib

[Lldb-commits] [PATCH] D56564: [SymbolFile]

zturner created this revision. zturner added reviewers: labath, clayborg, davide. Previously all of these functions accepted a SymbolContext&. While a CompileUnit is one member of a SymbolContext, there are also many others, and by passing such a monolithic parameter in this way it makes t

lldb-commits@lists.llvm.org

zturner added a comment. In D56564#1353514 , @clayborg wrote: > All the change to the symbol vendor make sense, but it seems like all of the > call sites should be: > > cu->GetLanguage(); > cu->ParseFunctions(); > cu->GetLineTable(); > cu->ParseDe

lldb-commits@lists.llvm.org

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB350943: [SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&. (authored by zturner, committed by ). Herald added subscribers: teemperor, abidh. Changed prior to commit: https://reviews.llvm.or

lldb-commits@lists.llvm.org

zturner created this revision. zturner added reviewers: clayborg, labath, davide. This limits the API surface by providing only the necessary and sufficient information through the parameter list, while also making the expectations of the API self-documenting, since we include the word `Recursiv

[Lldb-commits] [PATCH] D56615: [SymbolFile] Remove the SymbolContext parameter from FindNamespace

zturner created this revision. zturner added reviewers: clayborg, davide, labath. Herald added a subscriber: aprantl. Every callsite was passing an empty SymbolContext, so this parameter had no effect. Inside the DWARF implementation of this function, however, there was one codepath that checke

[Lldb-commits] [PATCH] D56618: [SymbolFile] Remove the SymbolContext parameter from FindTypes

zturner created this revision. zturner added reviewers: labath, clayborg, davide. This parameter was only ever used with the Module set, and since a SymbolFile is tied to a module, the parameter turns out to be entirely unnecessary. Furthermore, it doesn't make a lot of sense to ask a caller to

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

zturner added a comment. In D56230#1354829 , @Hui wrote: > I think the key problem here is to make sure the argument will be treated as > a single argument to the process launcher. > > To be specific to this case only, could we just provide a quote char t

[Lldb-commits] [PATCH] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

zturner added a comment. I had to stop commenting because there's too many occurrences, but please remove `auto` pretty much throughout the patch. A reader of the code will not necessarily be aware or remember that the function returns an `Optional`, and they will read the code as "if the size

[Lldb-commits] [PATCH] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

zturner added inline comments. Comment at: include/lldb/Target/ProcessStructReader.h:70 } -size_t total_size = struct_type.GetByteSize(nullptr); -lldb::DataBufferSP buffer_sp(new DataBufferHeap(total_size, 0)); +auto total_size = struct_type.GetByteSize(nullptr);

[Lldb-commits] [PATCH] D56737: [SymbolFile] Split ParseVariablesForContext into two functions.

zturner created this revision. zturner added reviewers: clayborg, labath, jingham. This function was being used in 2 separate use cases. The first is parsing variables in a function, and the second is parsing global variables. To clarify the intent, I've split this into two functions, ParseLo

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

zturner added a comment. In D56230#1358328 , @labath wrote: > In D56230#1358269 , @Hui wrote: > > > > - drop the `if (m_entries[i].quote)` branch. You don't need it here, and > > > I don't believe it will be correc

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

zturner added a comment. Why do we need to link against user32? What kind of unresolved external are you getting? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56741/new/ https://reviews.llvm.org/D56741 ___ lldb-commits mailing list lldb-

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

zturner added a comment. It seems like a pretty bad idea to have lldb-mi show a dialog box :-/ I don't actually work on or have anything to do with lldb-mi though, but I don't know how well-maintained or supported it is these days anyway. But if nobody chimes in and says why this is important

[Lldb-commits] [PATCH] D57213: [Scalar] Add support for 512-bits values.

zturner added inline comments. Comment at: lldb/source/Utility/Scalar.cpp:161 +if (endian::InlHostByteOrder() == eByteOrderBig) { + swapped_words[0] = apint_words[7]; + swapped_words[1] = apint_words[6]; I'm confused. You say it returns a pointer t

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

zturner added a comment. Thanks for the reminder! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56126/new/ https://reviews.llvm.org/D56126 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/list

[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

zturner added a comment. The above suggestion is admittedly minor, but since it's both a minor performance improvement **and** a minor readability/maintainability improvement, I think it's probably worth doing. Comment at: lit/SymbolFile/NativePDB/tag-types.cpp:5 // Test tha

[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

zturner added a comment. Sorry, due to the way Phabricator re-orders feedback across files when sending the email notification, if you're reading my comments in email you have to read the previous email from the bottom up. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.l

[Lldb-commits] [PATCH] D56602: Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Seems I had forgotten about this. It looks pretty good to me, and I'm glad we're very close to breaking this dependency. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56602/new/ h

[Lldb-commits] [PATCH] D57413: Fix some warnings with gcc on Linux

zturner created this revision. zturner added reviewers: davide, JDevlieghere, jingham. Most of these are surrounding the ObjectiveC formatters. They use a lot of unions containing unnamed structs, which are not ISO C++ compliant and which gcc warns about. There are several other minor warnings

[Lldb-commits] [PATCH] D57413: Fix some warnings with gcc on Linux

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:304-314 -union { - struct { -uint64_t _mutations; - }; - struct { -uint32_t _muts; -uint32_t _used:25

[Lldb-commits] [PATCH] D57413: Fix some warnings with gcc on Linux

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:304-314 -union { - struct { -uint64_t _mutations; - }; - struct { -uint32_t _muts; -uint32_t _used:25

[Lldb-commits] [PATCH] D57413: Fix some warnings with gcc on Linux

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB352557: Fix some warnings in building LLDB. (authored by zturner, committed by ). Herald added a subscriber: abidh. Changed prior to commit: https://reviews.llvm.org/D57413?vs=184163&id=184186#toc R

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. This is really cool, thanks for doing this! Comment at: source/Expression/DWARFExpression.cpp:3253 - return false; + return true; } Why do we change t

[Lldb-commits] [PATCH] D57213: [Scalar] Add support for 512-bits values.

zturner added inline comments. Comment at: lldb/source/Utility/Scalar.cpp:168 + swapped_words[6] = apint_words[1]; + swapped_words[7] = apint_words[0]; + apint_words = swapped_words; davide wrote: > davide wrote: > > aprantl wrote: > > > std::rever

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

zturner added inline comments. Comment at: source/Target/PathMappingList.cpp:204 FileSpec &new_spec) const { if (!m_pairs.empty()) { +std::string orig_path = orig_spec.GetPath(); Can we put some early returns in here? ``` n

[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

zturner added a comment. In D56904#1383148 , @aleksandr.urakov wrote: > Ping! What do you think about it? Generally the change looks good, but I'm not sure why the compiler can't properly compile the source file. I know it's something to do with the

[Lldb-commits] [PATCH] D57780: Don't include UnixSignals.h from Host

zturner created this revision. zturner added a reviewer: labath. Herald added a reviewer: jfb. This file lives in Target, which makes sense since it is supposed to represent the various types of signals that can occur on any platform (since you might be remote debugging a platform with different

[Lldb-commits] [PATCH] D57840: Convert TestCommandRegex to lit (from expect)

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Can you rename these two tests to `command-regex-delete.test` and `command-regex-unalias.test`? Otherwise, lgtm Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[Lldb-commits] [PATCH] D58172: Embed swig version into lldb.py in a different way

zturner added a comment. Long term, I wonder if we can just delete this entire `modify-python-lldb.py` script. it seems like the entire purpose is to make sure that the SB methods support iteration, equality, and some other basic stuff, and it does this by inserting lines of python code into t

[Lldb-commits] [PATCH] D57780: Don't include UnixSignals.h from Host

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB354168: Don't include UnixSignals.h from Host. (authored by zturner, committed by ). Herald added subscribers: abidh, arichardson, emaste. Herald added a reviewer: espindola. Herald added a project: LLD

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner created this revision. zturner added reviewers: labath, jgorbe, JDevlieghere, davide. Herald added subscribers: mgorny, emaste. `Host/Config.h` is where we have platform specific preprocessor defines that are configured at CMake time. Then, we can include this file `lldb/Host/Config.h`

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner added a comment. In D58654#1409974 , @jingham wrote: > Utility is supposed to be a bunch of stand-alone utility files & headers that > we gather together for convenience's sake. > > Host is where we put all the code that is specific to one or anot

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner added a comment. In D58654#1410080 , @jingham wrote: > There aren't many more platforms (OpenVMS?) that are likely to show up and > need support for lldb, so maybe this is making too much of the matter. But > having there be a well defined set o

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner updated this revision to Diff 188393. zturner added a comment. Moved to `System` instead of `Utility` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58654/new/ https://reviews.llvm.org/D58654 Files: lldb/cmake/XcodeHeaderGenerator/CMakeLists.txt lldb/cmake/modules/LLDBGenera

[Lldb-commits] [PATCH] D58167: Refactor user/group name resolving code

zturner added inline comments. Comment at: include/lldb/Host/UserIDResolver.h:9 + +#ifndef LLDB_HOST_USERIDRESOLVER_H +#define LLDB_HOST_USERIDRESOLVER_H I wonder if this class should actually be in Host. While some specific implementation of it might be host-d

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner added a comment. In D58654#1410817 , @JDevlieghere wrote: > I'm not a fan of introducing another library for this. Without a clear > timeline of "fixing" Host it's basically as temporary as anything else in > software development. I also think i

[Lldb-commits] [PATCH] D58678: Improve step over performance by not stopping at branches that are function calls and stepping into and them out of each one

zturner added a comment. > Since we are stepping over we can safely ignore these calls since they will > return to the next instruction What if the call throws an exception? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58678/new/ https://reviews.llvm.org/D58

[Lldb-commits] [PATCH] D58350: Insert random blocks of python code with swig instead of modify-python-lldb.py

zturner accepted this revision. zturner added a comment. Also lgtm. Don't call the commit message "Insert random blocks of python code" though. Hopefully the code we're inserting is not actually random :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58350/new/ https://reviews.llvm.o

[Lldb-commits] [PATCH] D58654: Move Config.h from Host to Utility

zturner added a comment. In D58654#1411084 , @labath wrote: > BTW, what's the reason that you have to have this split now? I understand its > attractiveness from a Zen perspective, but it seems to me that at the end of > the day, it doesn't have that muc

[Lldb-commits] [PATCH] D58730: Rename Symbols.cpp from Host to Symbols/LocateSymbolFile.{h, cpp}

zturner created this revision. zturner added reviewers: labath, JDevlieghere, jingham. Herald added subscribers: jdoerfert, MaskRay, arichardson, mgorny, emaste. Herald added a reviewer: espindola. After this change, there is only 1 remaining dependency in Host to Target. While this change might

[Lldb-commits] [PATCH] D58730: Rename Symbols.cpp from Host to Symbols/LocateSymbolFile.{h, cpp}

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB355032: Move Host/Symbols.cpp to Symbols/LocateSymbolFile.cpp (authored by zturner, committed by ). Herald added subscribers: abidh, krytarowski. Herald added a project: LLDB. Changed prior to commit:

[Lldb-commits] [PATCH] D58833: Fix embedded Python initialization according to changes in version 3.7

zturner added reviewers: davide, stella.stamenova. zturner added a comment. Davide ran into this issue recently and tried to fix it, so I'm adding him here as a reviewer. Also Stella is interested in Python 3 support as well, so adding her also. Repository: rLLDB LLDB CHANGES SINCE LAST AC

[Lldb-commits] [PATCH] D58842: Move ProcessInstanceInfo and similar to Utility

zturner created this revision. zturner added reviewers: jingham, labath, JDevlieghere. Herald added subscribers: jdoerfert, mgorny, emaste. There are set of classes in Target that describe the parameters of a process - e.g. it's PID, name, user id, and similar. However, since it is a bare descr

[Lldb-commits] [PATCH] D58842: Move ProcessInstanceInfo and similar to Utility

zturner added a comment. In D58842#1415526 , @labath wrote: > Hmm.. I think I've thought of (or remembered, because I have been thinking > about Utility too) a reason why it might be better to keep these in Host. > ProcessLaunchInfo cannot be trivially m

[Lldb-commits] [PATCH] D58792: Add "operator bool" to SB APIs

zturner added a comment. Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58792/new/ https://reviews.llvm.org/D58792 ___ lldb-commits m

[Lldb-commits] [PATCH] D58838: Remove tautological #ifdefs

zturner added a comment. Yea it would be nice if we could remove all of the `LLDB_CONFIGURATION_xxx` macros and just use either the LLVM ones or standard ones such as NDEBUG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58838/new/ https://reviews.llvm.org/D58838

[Lldb-commits] [PATCH] D58842: Move ProcessInstanceInfo and similar to Utility

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 rL355342: Move ProcessInfo from Host to Utility. (authored by zturner, committed by ). Herald added a project: LLVM. Herald

[Lldb-commits] [PATCH] D58976: Introduce core2yaml tool

zturner added a comment. In D58976#1418621 , @clayborg wrote: > Strong ownership is needed for this class IMHO because it hands out pointers > to native data I disagree here, see my previous comment. Binaries grow large very quickly, and if we always

[Lldb-commits] [PATCH] D58972: Introduce the "Formats" module and move LinuxProcMaps parser to it

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Great idea, this is a nice analogue to LLVM/BinaryFormat. But, "Formats" is a little generic. What about FileFormats or something? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D589

[Lldb-commits] [PATCH] D58971: Move MemoryRegionInfo into the Utility module

zturner added a comment. One idea for a new module name could be `AbstractProcess`, but I can't think of anything else better at the moment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58971/new/ https://reviews.llvm.org/D58971 ___ lldb-

[Lldb-commits] [PATCH] D58985: Fix core files for 32 bit architectures that are supported in ProcessELFCore.cpp

zturner added inline comments. Comment at: source/Plugins/Process/elf-core/ThreadElfCore.cpp:276 +else + return sizeof(ELFLinuxPrStatus) - 10 * 4; } subtracting 40 from the header size seems a bit magical to those not in the know (such as myself). C

[Lldb-commits] [PATCH] D59043: [lldb-vscode] Correctly propagate errors back to VS Code

zturner created this revision. zturner added a reviewer: clayborg. I tried installing this, and I couldn't get it working. VS Code would launch the adapter and then it would get stuck waiting for something to happen. Eventually, I tracked this down to the fact that it couldn't locate lldb-serv

[Lldb-commits] [PATCH] D59104: [lldb-vscode] Make server mode work on Windows

zturner created this revision. zturner added a reviewer: clayborg. Herald added a subscriber: mgorny. For historical reasons, Windows unfortunately doesn't support using sockets in standard system calls like read/write, which means that they also can't be buffered with a `FILE*`. This violates

[Lldb-commits] [PATCH] D59104: [lldb-vscode] Make server mode work on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rL355637: [lldb-vscode] Support running in server mode on Windows. (authored by zturner, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: htt

[Lldb-commits] [PATCH] D59114: [lldb-vscode] Don't hang indefinitely on invalid program

zturner created this revision. zturner added a reviewer: clayborg. When you configure your launch settings, if you pass a path to an invalid program, we would previously hand indefinitely. This is because a combination of two bugs working together. The first is that we did not attempt to detec

[Lldb-commits] [PATCH] D59114: [lldb-vscode] Don't hang indefinitely on invalid program

This revision was automatically updated to reflect the committed changes. Closed by commit rL355656: [lldb-vscode] Report an error if an invalid program is specified. (authored by zturner, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to comm

[Lldb-commits] [PATCH] D59158: Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands

zturner added inline comments. Comment at: lldb/include/lldb/Commands/CommandObjectBreakpoint.h:36 - static void VerifyBreakpointOrLocationIDs(Args &args, Target *target, -CommandReturnObject &result, I think the cla

[Lldb-commits] [PATCH] D59158: Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands

zturner added a comment. I think maybe part of the problem is that this patch looks like actually 2 things. 1) A move of the include files from `lldb/source/Commands` to `lldb/Include/lldb/Commands`, and 2) The dependency changes. So it makes it hard to see what changes are actually needed fo

[Lldb-commits] [PATCH] D59158: Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands

zturner added a comment. Interesting, I had looked at fixing this one once before but I didn't realize we had a class named `EvaluateExpressionOptions` that contained just the right set of information, and it felt gross to invent a new one just for this purpose (It's a good thing I didn't too,

[Lldb-commits] [PATCH] D59158: Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Ahh yea, sorry. Got confused for a second :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59158/new/ https://reviews.llvm.org/D59158 ___

[Lldb-commits] [PATCH] D59164: [SymbolFileDWARF] Move ElaboratingDIEIterator into implementation file

zturner created this revision. zturner added a reviewer: clayborg. This is not used outside of the private implementation of the class, so hiding in the implementation file is a nice way of simplifying the external interface. https://reviews.llvm.org/D59164 Files: lldb/source/Plugins/Sy

[Lldb-commits] [PATCH] D59165: Remove DWARFDIECollection

zturner created this revision. zturner added reviewers: clayborg, aprantl. Herald added subscribers: jdoerfert, mgorny. This is a very thin wrapper over a std::vector and does not seem to provide any real value over just using a container directly. https://reviews.llvm.org/D59165 Files:

[Lldb-commits] [PATCH] D59200: Fix a crasher in StackFrame::GetValueForVariableExpressionPath()

zturner added inline comments. Comment at: source/Target/StackFrame.cpp:644-645 if (error.Fail()) { error.SetErrorStringWithFormatv( "Failed to dereference sythetic value: %s", deref_error); return ValueObjectSP(); No

[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison

zturner added a comment. SBType storing a pair of CompilerType and TypeSP seems like a very confusing interface and like it will be very easy to misuse or violate assumptions (perhaps even assumptions that nobody knows exists). Why exactly is this necessary? As far as I can tell, `SBType` use

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner created this revision. zturner added reviewers: clayborg, jasonmolenda, aprantl. Herald added a subscriber: jdoerfert. LLVM doesn't produce DWARF64, and neither does GCC. LLDB's support for DWARF64 is only partial, and if enabled appears to also not work. It also appears to have no tes

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner added a subscriber: jankratochvil. zturner added a comment. In D59235#1425417 , @clayborg wrote: > What part of parsing DWARF64 wasn't working? I think the SPARC compiler was > the only one producing DWARF64. This patch originated from a thread

<    1   2   3   4   5   6   7   8   >