[Lldb-commits] [PATCH] D52403: [LLDB] - Support the single file split DWARF.

labath added a comment. I believe you should be able to use `lldb-test` to write this kind of a test. I am thinking of a sequence like: yaml2obj %p/Inputs/test.yaml > %t/test.yaml yaml2obj %p/Inputs/test.o.yaml > %t/test.o.yaml lldb-test breakpoint %t/test.yaml %s | FileCheck %s b main

[Lldb-commits] [PATCH] D52152: Add NativeProcessProtocol unit tests

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 rLLDB342875: Add NativeProcessProtocol unit tests (authored by labath, committed by ). Changed prior to commit: https://r

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

labath added a comment. In https://reviews.llvm.org/D48393#1243195, @JDevlieghere wrote: > In https://reviews.llvm.org/D48393#1139327, @labath wrote: > > > The only sane algorithm I can come up right now is to make the list of > > parsed dies local to each thread/parsing entity (e.g. via a "visi

[Lldb-commits] [PATCH] D52139: [lldb-mi] Fix hanging of target-select-so-path.test

labath added a comment. In https://reviews.llvm.org/D52139#1243170, @teemperor wrote: > Anyway, the actual issue is related Python 3: Arch Linux (and probably > some other distributions that "already" upgraded to Python 3 as > default) will launch Python 3 when the test script calls `python ...

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

labath added a comment. I think you should look at CPlusPlusLanguage::MethodName. It already contains a parser (in fact, two of them) of c++ names, and I think it should be easy to extend it to do what you want. Repository: rLLDB LLDB https://reviews.llvm.org/D52461 _

[Lldb-commits] [PATCH] D52498: [lldb-mi] Fix bugs in target-select-so-path.test

labath accepted this revision. labath added a comment. LGTM. (BTW, python 3.1's EOL was 6 years ago) https://reviews.llvm.org/D52498 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commit

[Lldb-commits] [PATCH] D52516: [lldbinline] Set directory attribute on test-specific classes

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Ooh, nice catch. This must have been fun to debug. https://reviews.llvm.org/D52516 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

labath created this revision. labath added reviewers: krytarowski, zturner. This function encodes the knowledge of whether the PC points to the breakpoint instruction of the one following it after the breakpoint is "hit". This behavior mainly(*) depends on the architecture and not on the OS, so it

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

labath added a comment. In https://reviews.llvm.org/D52532#1246173, @krytarowski wrote: > I was wondering whether we want to normalize this inside the kernel and > always advance the Program Counter.. but it's easier to manage it in userland. I am generally in favour of keeping the kernel simp

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

This revision was automatically updated to reflect the committed changes. Closed by commit rL343409: Pull GetSoftwareBreakpointPCOffset into base class (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D52532 Files: ll

[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer

labath added inline comments. Herald added a subscriber: arphaman. Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179 + if (!m_die_array.empty()) { +lldbassert(!m_first_die || m_first_die == m_die_array.front()); +m_first_die = m_die_array.front();

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

labath added a comment. In https://reviews.llvm.org/D52461#1249259, @aleksandr.urakov wrote: > It seems that `CPlusPlusLanguage::MethodName` is backed by LLDB > `CPlusPlusNameParser`, which can't parse demangled names... What makes you say that? If you look at the MethodName unit tests (`unit

[Lldb-commits] [PATCH] D52719: Pull FixupBreakpointPCAsNeeded into base class

labath created this revision. labath added a reviewer: krytarowski. This function existed (with identical code) in both NativeProcessLinux and NativeProcessNetBSD, and it is likely that it would be useful to any future implementation of NativeProcessProtocol. Therefore I move it to the base class

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

labath added inline comments. Comment at: lit/Expr/TestIRMemoryMapWindows.test:1-12 +# REQUIRES: windows + +# RUN: clang-cl /Zi %p/Inputs/call-function.cpp -o %t + +# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic +# RUN: lldb-test ir-memory-map -host-only %t %S/In

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

labath added a comment. In https://reviews.llvm.org/D52461#1250555, @aleksandr.urakov wrote: > I've tried to parse with it a name like > > N0::`unnamed namespase'::Name > > > and it can't parse it correctly. May be it just can't parse MSVC demangled > names? I expect the backqoutes are confu

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

labath added a comment. In https://reviews.llvm.org/D52618#1250909, @zturner wrote: > One idea would be to define some lit substitutions like %debuginfo. It’s > true you can produce a gcc style command line that will be equivalent to a > clang-cl invocation but it won’t be easy. eg you’ll needi

[Lldb-commits] [PATCH] D52719: Pull FixupBreakpointPCAsNeeded into base class

This revision was automatically updated to reflect the committed changes. Closed by commit rL343683: Pull FixupBreakpointPCAsNeeded into base class (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D52719 Files: lldb/t

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

labath created this revision. labath added reviewers: krytarowski, zturner, clayborg. Herald added a subscriber: mgorny. A fairly simple operation as setting a breakpoint (writing a breakpoint opcode) at a given address was going through three classes: NativeProcessProtocol which called NativeBrea

[Lldb-commits] [PATCH] D52404: Prevent double import of _lldb module

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Sorry for the delay. I *think* this should be fine, but with these things its hard to tell whether it won't break someone's build/release setup. However, it does solve a real problem (I've hit

[Lldb-commits] [PATCH] D52404: Prevent double import of _lldb module

labath added a comment. In https://reviews.llvm.org/D52404#1257347, @vadimcn wrote: > Thanks, > What changed in SWIG 3.0.11? That's the version which added support for the `moduleimport` attribute. Before that, the modules were imported using a fixed algorithm (I am not sure whether that alg

[Lldb-commits] [PATCH] D53090: [Windows] Fix a bug that causes lldb to freeze

labath added a comment. Do we have a test for this? It sounds like it shouldn't be hard to replicate the dependent library load failure in a test... Repository: rLLDB LLDB https://reviews.llvm.org/D53090 ___ lldb-commits mailing list lldb-commit

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

labath added a comment. In https://reviews.llvm.org/D53086#1261704, @aleksandr.urakov wrote: > As for aligned stack cross-platform problems, I mean also problems with stack > unwinding. They seem to appear on non-Windows too. It's because > `x86AssemblyInspectionEngine` doesn't support stack al

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

labath added a comment. In https://reviews.llvm.org/D52461#1266302, @aleksandr.urakov wrote: > `operator<'::`2'::B::operator> > The reason we had to use clang lexer for parsing itanium names is because parsing itanium demangled names is tricky precisely for cases like these. If the MSVC dem

[Lldb-commits] [PATCH] D53255: Fix: Assertion failed: (!m_first_die || m_first_die == m_die_array.front()), function ExtractDIEsRWLocked

labath added a comment. It looks like it should be easy to write a test for this by hand-crafting a minimal .s file and feeding it to lldb-test. Repository: rL LLVM https://reviews.llvm.org/D53255 ___ lldb-commits mailing list lldb-commits@list

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

labath added a comment. That's an interesting code snippet. Thanks for sharing that. The thing that's not clear to me is: are you specifically interested in unwinding from these kinds of functions **without debug info**? Because it sounds to me like the info Zachary provided is enough to unwind

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

labath added a comment. Yes it sounds like the situation is very much similar here as it is in dwarf land -- the compilers there also don't tend to emit unwind info for prologues and epilogues. The thing to realize about the instruction emulation is that it is always going to be a best effort

[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support

labath added a comment. Back when the FileSystem class was introduced, the idea was that *it* would eventually become the gateway to the real filesystem, and FileSpec would just deal with abstract path manipulation. I still think that is a good idea, particularly in light of this patch. So I w

[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

labath added a comment. I also think it we should try to keep FileSpec in the Utility module. We should be able to do that by deleting the "utility" functions in the FileSpec class, and updating everything to go through the FileSystem class. After the VFS introduction, something like `file_spec

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

labath added a comment. Now that I know your use case (I forgot that you're working on the replay now, and the mention of VFS threw me off in a completely different direction), I agree that having a single global filesystem is fine. As for the increased verbosity, I wonder if we should make the

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

labath added a comment. > For the purposes of testing I decided to bypass lldb-test and go with a > scripted lldbinit file. I'm sticking with this approach wherever possible to > prove that this is "true" cross-platform debugger functionality. However, > there are definite limitations. For exam

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

labath added a comment. It sounds like at least part of this could be tested by adding the ability to dump dependent modules to `lldb-test object-file` (which I think would fit very nicely within the current information printed by that command). I seem to recall seeing code like that in some pa

[Lldb-commits] [PATCH] D53677: Fix a bug PlatformDarwin::SDKSupportsModule

labath accepted this revision. labath added a comment. lgtm. Sorry about the trouble. https://reviews.llvm.org/D53677 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53785: [FileSystem] Move EnumerateDirectory from FileSpec to FileSystem.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. lgtm Repository: rLLDB LLDB https://reviews.llvm.org/D53785 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cg

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

labath added a comment. In https://reviews.llvm.org/D53532#1279022, @JDevlieghere wrote: > Address Pavel's feedback: > > - Change to `Initialize` method which can only be called once. I suppose this is slightly better, but what I meant was to have a `static` (like all other functions with the

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

labath added a comment. Ok, if that's how you guys feel, then I won't stand in your way. I am fine with this patch then. https://reviews.llvm.org/D52618 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

labath added a comment. In https://reviews.llvm.org/D53532#1280734, @JDevlieghere wrote: > In https://reviews.llvm.org/D53532#1280018, @labath wrote: > > > In https://reviews.llvm.org/D53532#1279022, @JDevlieghere wrote: > > > > > Address Pavel's feedback: > > > > > > - Change to `Initialize` met

[Lldb-commits] [PATCH] D53834: [FileSystem] Remove ResolveExecutableLocation() from FileSpec

labath added a comment. Why did you change the function name? I think it would be nice to keep "executable" or something to that effect in the name (e.g. the llvm function has "program"), given that the underlying function checks for the executable bit, and so it cannot be used for general sear

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Thanks for bearing with me. I am very happy with how this turned out in the end. I have more suggestion on how to clean up the initialization, which you can incorporate if you like it, but I d

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `MSVCUndecoratedNameParser`

labath added a comment. It's not fully clear to me from the previous comments if you are proceeding with this or not, but in case you are, I have made comments inline. I see that you've added some lit tests, but I also think you it would be good add some unit tests for the name parser functiona

[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

labath added a comment. I am not sure what do you mean by "translating paths" in the VFS. If you mean something like "return a `FILE *` for `/whatever/my_reproducer/vfs/bin/ls` when I ask for `/bin/ls`", then I think that's a good idea, as it's most likely to work with all of our existing code

[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D54020#1285539, @JDevlieghere wrote: > In https://reviews.llvm.org/D54020#1285201, @labath wrote: > > > I am not sure what do you mean by "translating paths" in the

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

labath added a comment. lldb-server is used on architectures that don't have python (readily) available, and it uses the same codebase as the rest of lldb. (Granted, it only needs a small subset of that codebase, and this subset doesn't/shouldn't care about python, but we aren't able to split o

[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

labath added a comment. A very nice use of the structured demangler. https://reviews.llvm.org/D54053 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `MSVCUndecoratedNameParser`

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Thanks for your patience. This looks good to me now. https://reviews.llvm.org/D52461 ___ lldb-commits mailing list lldb-commits@lists.llvm.org ht

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

labath added a comment. Kamil, I think you're the best qualified person to look at the lldb-server changes right now, so I'd like to hear your opinion on the other changes too. Otherwise, I'll just commit this some time next week. https://reviews.llvm.org/D52941

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

labath created this revision. labath added reviewers: sgraenitz, erik.pilkington, JDevlieghere. Now that llvm demangler supports more generic customization, we can implement type substitution directly on top of this API. This will allow us to remove the specialized hooks which were added to the de

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

labath added inline comments. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2275 +SymbolFile *sf = m->GetSymbolVendor()->GetSymbolFile(); +sf->DumpClangAST(); + } The dump function should take the output stream from the `result` ob

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

labath added a comment. thanks. https://reviews.llvm.org/D52941 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

This revision was automatically updated to reflect the committed changes. Closed by commit rL346093: NativeProcessProtocol: Simplify breakpoint setting code (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D52941 Files:

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

labath added a comment. In https://reviews.llvm.org/D54072#1286748, @zturner wrote: > Unfortunately then color output is impossible. Where else would the output > be expected to go? If you execute the command over the SB API (SBCommandInterpreter::HandleCommand) then it will go into the provi

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

labath added a comment. If it comes down to choosing between colored output going to stderr and plain output going where it should, i'd go with the second option. The way I'd implement this to support both things is approximately this: - add color (`has_colors`, `changeColor` and friends) suppo

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

labath updated this revision to Diff 172583. labath added a comment. Thanks for the review. I think that creating the new BumpPtrAllocator every call doesn't matter much here, this function get's called only on a few symbols for each evaluated expression. However, it also wasn't too hard to reu

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

labath updated this revision to Diff 172764. labath added a comment. Thanks for the review. I have added some comments, and moved appending of the unmodified portion of the name into a separate function to aid readability. https://reviews.llvm.org/D54074 Files: source/Plugins/Language/CPlusP

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

This revision was automatically updated to reflect the committed changes. Closed by commit rL346233: CPlusPlusLanguage: Use new demangler API to implement type substitution (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.or

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

labath added a comment. Could we reverse the dependencies between the two? I.e., first add the necessary functionality to lldb-test and then write the test using that? Or just merge that into a single patch? Some of the information tested here is already available there (list of sections) othe

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

labath added a comment. I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automaticall

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

labath added a comment. In https://reviews.llvm.org/D54221#1290638, @JDevlieghere wrote: > In https://reviews.llvm.org/D54221#1290572, @labath wrote: > > > I recall something about linux on arm having a magic unmodifiable (even by > > ptrace) page of memory, so this could be useful there too. Ho

[Lldb-commits] [PATCH] D54272: Extract construction of DataBufferLLVM into FileSystem

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks fine to me, I'd just ask you to consider shortening the method names a bit. Comment at: include/lldb/Host/FileSystem.h:119-122 + CreateDataBufferFromPath(const llvm::

[Lldb-commits] [PATCH] D54417: Fix a crash when parsing incorrect DWARF

labath created this revision. labath added reviewers: clayborg, jankratochvil, JDevlieghere. While parsing a childless compile unit DIE we could crash if the DIE was followed by any extra data (such as a superfluous end-of-children marker). This happened because the break-on-depth=0 check was perf

[Lldb-commits] [PATCH] D54417: Fix a crash when parsing incorrect DWARF

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB346849: Fix a crash when parsing incorrect DWARF (authored by labath, committed by ). Herald added a subscriber: abidh. Changed prior to commit: https://reviews.llvm.org/D54417?vs=173634&id=174006#to

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

labath added inline comments. Comment at: tools/debugserver/source/CMakeLists.txt:101 +option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't try to codesign it" OFF) +option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. Use the syste

[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider

labath added a comment. I am confused by differing scopes of various objects that are interacting here. It seems you are implementing capture/replay as something that can be flicked on/off at any point during a debug session (`Debugger` lifetime). However, you are modifying global state (the fi

[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.

labath added a comment. If by //"it would cause errors to no longer appear in the same order as specified by the user, but in an arbitrary order specified by the driver implementation"//, you mean that now for a command line like: $ new/lldb -S /tmp/sadg --file /tmp/qwr I get error: file

[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.

labath added inline comments. Comment at: include/lldb/Utility/Reproducer.h:59-64 + void InitializeFileInfo(llvm::StringRef name, + llvm::ArrayRef files) { +m_info.name = name; +for (auto file : files) + m_info.files.push_back(file); + } --

[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider

labath added a comment. In https://reviews.llvm.org/D54617#1302376, @JDevlieghere wrote: > In https://reviews.llvm.org/D54617#1302366, @labath wrote: > > > I am confused by differing scopes of various objects that are interacting > > here. It seems you are implementing capture/replay as somethin

[Lldb-commits] [PATCH] D54731: [lit] Enable the use of custom user-defined lit commands

labath added a comment. I think that something like this would go a long way towards solving the problems with lit tests we're having in lldb. However, the part that is not clear to me is whether it is actually necessary to modify lit (shtest) to achieve this. It seems to me an equivalent effec

[Lldb-commits] [PATCH] D54731: [lit] Enable the use of custom user-defined lit commands

labath added a comment. I'd go with the "conservative" approach first. The idea of having lldb loaded inside a lit process does not excite me. One of the problems we have with dotest is that when lldb crashes during the test, it takes a part of the test driver with it which causes some tests to

[Lldb-commits] [PATCH] D54843: [Expr] Check the language before ignoring Objective C keywords

labath added inline comments. Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:401-404 -// FIXME: the following language option is a temporary workaround, -// to "ask for C++, get ObjC++". Apple hopes to remove this requirement on -// non-

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

labath accepted this revision. labath added a reviewer: clayborg. labath added a comment. The change looks pretty safe to me. Adding Greg in case he has any concerns. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54863/new/ https://reviews.llvm.org/D54863 __

[Lldb-commits] [PATCH] D28305: [Host] Handle short reads and writes, take 3

labath added a comment. This is a stale patch which I am abandoning. I am just cleaning up my queue. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D28305/new/ https://reviews.llvm.org/D28305 ___ lldb-commits mailing list lldb-commits@lists.l

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

labath added a comment. > There’s actually been a slow push away from cl::opt. It’s less flexible > and doesn’t support some things that the TableGen approach does. > Recently there’s been a few efforts to port existing tools onto TableGen > options from cl::opt. > > I don’t think cl::opt is g

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

labath added a comment. In D54692#1308207 , @zturner wrote: > In D54692#1308190 , @labath wrote: > > > Another reason for using libOption is that it is also usable as a parser > > for the lldb command line, whereas

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

labath added inline comments. Comment at: tools/driver/Driver.cpp:876-888 + usage << indent << tool_name +<< " -a -f [-c ] [-s ] [-o " + "] [-S ] [-O ] [-k ] [-K ] " + "[-Q] [-b] [-e] [-x] [-X] [-l ] [-d] [-z " + "] [[--] [ ...]]\n"; + u

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

labath added a comment. I didn't look at the code in detail, as most of it deals with windows stuff, and I don't know much about those anyway. However, the interesting question for me would be how to make this useful for cross-compiling. Right now that sort of works for NativePDB tests because

[Lldb-commits] [PATCH] D53759: [PDB] Support PDB-backed expressions evaluation

labath added inline comments. Comment at: source/Core/RichManglingContext.cpp:134-135 get(m_cxx_method_parser)->GetBasename(); +if (!m_buffer.data()) + m_buffer = llvm::StringRef("", 0); return; Why is this necessary? It looks like somebody

[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.

labath added a comment. I haven't been following the reproducer work in detail, but this seems reasonable to me. Thanks for incorporating my drive-by suggestions. Comment at: include/lldb/Utility/Reproducer.h:104 + template T *Create() { +std::unique_ptr provider(new T(m

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

labath added a comment. In D54914#1310337 , @zturner wrote: > In D54914#1309700 , @labath wrote: > > > I didn't look at the code in detail, as most of it deals with windows > > stuff, and I don't know much about th

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

labath added a comment. In D54914#1309901 , @aprantl wrote: > I would like to ask a general question that I (indirectly) also asked in > D54731 : Why do we want to implement support > for building inferiors in LIT-based t

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

labath added a comment. I have a lot of comments, but I think they're mostly cosmetic. I think the general idea here is good. Comment at: include/lldb/API/SBInitializerOptions.h:15 +#include "lldb/API/SBFileSpec.h" +#include "lldb/Initialization/SystemInitializer.h" +

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

labath added a comment. In D54914#1311492 , @zturner wrote: > I think it would be good to use the way dotest works as a starting point. > You can specify --arch, and then you can run the test on multiple arches this > way by running dotest several times

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

labath added a comment. I've recently started looking at adding a new symbol file format (breakpad symbols). While researching the best way to achieve that, I started comparing the operation of PDB and DWARF symbol files. I noticed a very important difference there, and I think that is the caus

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

labath added a comment. In D53368#1313145 , @zturner wrote: > In D53368#1313124 , @labath wrote: > > > I've recently started looking at adding a new symbol file format (breakpad > > symbols). While researching the

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

labath added a comment. > Great observations Pavel! I think it's really important to have > orthogonal/composable abstractions here: the symbols should be decoupled > from the container format IMO. > > You know more about the ObjectFile than me so I can't say if > ObjectFileBreakpad is the

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

labath added a comment. On 29/11/2018 21:29, Leonard Mosescu wrote: > Hi Aleksandr, yes, no objections to this patch. > > I was responding to Pavel's comments, which I also assume are > forward-looking as well, not strictly related to this patch. Agreed, and I apologise for hijacking your rev

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

labath added a comment. In D55038#1314026 , @JDevlieghere wrote: > Test didn't run. Is there a way to REQUIRE either darwin or linux? I think the canonical way to do that would be to define a new feature in lit, which gets set when the target supports

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

labath added a comment. I like how you've separated out the conversion function doing the actual conversion. That should make it easy to write unit tests for it (including tests for malformed input). Can you add something like that? I am particularly interested in what will the merging code do

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Thanks for adding the test. looks good to me. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h:289-291 + mutable std::unique_ptr m_filespec_ap; + typedef llv

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

labath added a comment. I don't see any tests :(. Also, the three bullet points in the description sound like rather independent pieces of functionality. Would it be possible to split them up into separate patches? That would make things easier to review, particularly for those who don't look

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In D55038#1314968 , @JDevlieghere wrote: > In D55038#1314336 , @labath wrote: > > > I think the canonical way

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath created this revision. labath added reviewers: clayborg, zturner, lemo, amccarth. Herald added subscribers: fedor.sergeev, mgorny. This patch adds the scaffolding necessary for lldb to recognise symbol files generated by breakpad. These (textual) files contain just enough information to be

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:66-84 + if (os == llvm::Triple::Win32) { +// In binary form, the module id should have 20 bytes: 16 bytes for UUID, +// and 4 bytes for the "age". However, in the textual

[Lldb-commits] [PATCH] D55240: [FileSystem] Migrate CommandCompletions

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good, modulo the filesystem copy comment. Comment at: source/Commands/CommandCompletions.cpp:180 + FileSystem fs = FileSystem::Instance(); std::error_code EC; ---

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

labath added a comment. Thank you. I like the tests a lot. I think I'll steal the implementation of this when I get around to parsing breakpad unwind instructions. ;) This looks fine to me, but one of the windows folks should take a look at it too. Comment at: source/Plugin

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

labath added a comment. In D55122#1318295 , @leonid.mashinskiy wrote: > > Is this true? Is it not possible for a program to depend on a value of a > > register which will be defined later? > > I am not totally sure about this, but all valid fpo programs

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath updated this revision to Diff 176615. labath added a comment. - implement the module_id/code_id logic suggested by Mark Mentovai - fix module_id endianness handling to make sure the UUID matches the one we get from the minidump files CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath added a reviewer: markmentovai. labath marked 3 inline comments as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:66-84 + if (os == llvm::Triple::Win32) { +// In binary form, the module id should have 20 bytes

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath updated this revision to Diff 176781. labath marked 17 inline comments as done. labath added a comment. Updated according to review comments. Also added a couple of tests for invalid inputs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55214/new/ https://reviews.llvm.org/D55214

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath added inline comments. Comment at: lit/Modules/Breakpad/lit.local.cfg:1 +config.suffixes = ['.test'] zturner wrote: > This shouldn't be necessary, the top-level `lit.cfg.py` already recognizes > `.test` extension. You only need a lit.local.cfg if you're

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

labath marked 3 inline comments as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74 + + bool IsStripped() override { return false; } + markmentovai wrote: > labath wrote: > > zturner wrote: > > > Is this

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

labath added a comment. Sounds like an interesting feature to me. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55318/new/ https://reviews.llvm.org/D55318 ___ lldb-commits mailing list lldb-commits@lists.llvm.org h

  1   2   3   4   5   6   7   8   9   10   >