Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (319 w, 6 d)

Recent Activity

Today

labath committed rGa3189a032a14: ELF: Fix a "memset clearing object of non-trivial type" warning (authored by labath).
ELF: Fix a "memset clearing object of non-trivial type" warning
Mon, Jul 22, 7:30 AM
labath committed rL366692: ELF: Fix a "memset clearing object of non-trivial type" warning.
ELF: Fix a "memset clearing object of non-trivial type" warning
Mon, Jul 22, 7:30 AM
labath created D65089: SymbolVendor: Move compile unit handling into the SymbolFile class.
Mon, Jul 22, 6:46 AM

Fri, Jul 19

labath added a comment to D64995: [lldb] Fix crash when tab-completing in multi-line expr.

This looks like a good opportunity to unleash pexpect.

Fri, Jul 19, 7:43 AM · Restricted Project
labath added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Fri, Jul 19, 3:15 AM · Restricted Project

Thu, Jul 18

labath added a comment to D63165: Initial support for native debugging of x86/x64 Windows processes.

I'm trying to think through the implications of this always-use-an-lldb-server approach on cross-platform postmortem debugging. I'll have to do some studying, but I guess this patch doesn't change anything in that regard.

Thu, Jul 18, 8:33 AM · Restricted Project
labath added a comment to D63165: Initial support for native debugging of x86/x64 Windows processes.

I am sorry, but the code still seems a lot more verbose to me than it should be needed to implement the given functionality. I'd like to understand why/if it's that necessary..

Thu, Jul 18, 8:10 AM · Restricted Project
labath updated the diff for D63713: Add error handling to the DataExtractor class.
  • address feedback from @dblaikie
  • waiting to hear more opinions before updating the patch to use size_t
Thu, Jul 18, 7:12 AM · Restricted Project
labath added inline comments to D63713: Add error handling to the DataExtractor class.
Thu, Jul 18, 7:12 AM · Restricted Project
labath added a comment to D63713: Add error handling to the DataExtractor class.

I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)

Thu, Jul 18, 6:53 AM · Restricted Project
labath added a comment to D64881: [Cmake] Use the modern way to find Python when possible.

I haven't been keeping up with all the python cmake patches, but I'm not sure this will help the situation, as it will add another dimension to the configuration matrix. If we're still going to support cmake<3.12, then we're going to need to make the other branch of this work anyway... Maybe once cmake 3.12 gets a bit older (widely available), we could just switch to the other method and say that cmake>=3.12 is required if you're going to build lldb...

Thu, Jul 18, 2:38 AM · Restricted Project
labath accepted D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4.

lgtm

Thu, Jul 18, 2:11 AM · Restricted Project

Wed, Jul 17

labath added inline comments to D64853: Fix CommandInterpreter for _regex-break with options.
Wed, Jul 17, 8:24 AM · Restricted Project
labath updated the diff for D63713: Add error handling to the DataExtractor class.

Add doxygen comments and fix typo.

Wed, Jul 17, 1:53 AM · Restricted Project
labath added inline comments to D63713: Add error handling to the DataExtractor class.
Wed, Jul 17, 1:53 AM · Restricted Project
labath added a comment to D64769: [lldb][test_suite] change the test main.cpp to avoid expression reschedule.

The test set the breakpoint to the return statement, even thought the compiler flag -O0 was set, the first hit to that bp showed that g_common_1 = 0 which actually should be 21. The test assume when we hit the return statement, the value should already be calculated, but that's not the case for NDK20 clang.

So we reproduce the test manually, turned out that the value was calculated later. lldb first hit the return statement then jump back to line 20. By looking at the assembly that clang provided, it indeed jump back and forth.

It's possible that the jumping behavior is caused by optimization, considering there're some other tests having similar problem, and the behavior changed a lot from NDK 19 to 20. I made this change because this is irrelevant to testing global variables.

Wed, Jul 17, 1:02 AM · Restricted Project

Tue, Jul 16

labath added inline comments to D64777: Fix CreateFunctionTemplateSpecialization to prevent dangling poiner to stack memory.
Tue, Jul 16, 9:23 AM · Restricted Project
labath added a comment to D63713: Add error handling to the DataExtractor class.

I think this should be ready for a proper review now. To see the new API in action, please take a look at D64798.

Tue, Jul 16, 7:09 AM · Restricted Project
labath retitled D63713: Add error handling to the DataExtractor class from WIP: DataExtractor error handling to Add error handling to the DataExtractor class.
Tue, Jul 16, 7:09 AM · Restricted Project
labath created D64798: WIP: Refactor accel table and debug_loc parsers to demonstrate the new DataExtractor API.
Tue, Jul 16, 7:07 AM · Restricted Project
labath updated the diff for D63713: Add error handling to the DataExtractor class.
  • update the patch to reflect our offline conversation with @dblaikie. We decided to go for the API which makes most sense (i.e. skip all reads as soon as one of them returns an error), at least until there is evidence that this makes a difference in practice (one can always prove that there is a slowdown here with a suitable micro-benchmark).
  • add tests for the new APIs
  • move the refactoring of other parsing classes into a separate patch (keeping this patch solely about DataExtractor).
Tue, Jul 16, 7:03 AM · Restricted Project
labath added inline comments to D64777: Fix CreateFunctionTemplateSpecialization to prevent dangling poiner to stack memory.
Tue, Jul 16, 4:42 AM · Restricted Project
labath added a comment to D64782: [SWIG] Deprecate SWIG 1.x .

I've updated swig on GreenDragon, and the Windows bot is running 3.0. I'm not sure about the Debian bot, as it's incremental it doesn't print the version number. Regardless, I'll keep an eye on the bots of course.

Tue, Jul 16, 4:38 AM · Restricted Project, Restricted Project
labath accepted D64771: [lldb][test_suite] skip tests of `libstdcpp` on Android and clean up.
Tue, Jul 16, 4:28 AM · Restricted Project
labath accepted D64769: [lldb][test_suite] change the test main.cpp to avoid expression reschedule.

The change is fine, but for my own education, could you elaborate on what this "delayed calculation" is, and how does it make the test fail?

Tue, Jul 16, 4:26 AM · Restricted Project
labath accepted D64767: [lldb][test_suite] Update tests with unexpected pass on Android aarch64.

Whether a lot of these tests (e.g. all watchpoint related tests) pass or fail may depend on the exact android device you are running the tests against. However, you are the only ones running android tests ATM, so that does not matter much right now. If that ever changes, we'll have to do something smarter here...

Tue, Jul 16, 4:23 AM · Restricted Project
labath added inline comments to D62570: Use LLVM's debug line parser in LLDB.
Tue, Jul 16, 3:35 AM · Restricted Project, Restricted Project
labath added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

Sorry for the delay. The patch looks fine to me. As far as testing goes, there's a "gdb-client" test suite in test/testcases/functionalities/gdb_remote_client/, which should allow you to mock gdb-server responses and verify that the right packets are being sent.

Tue, Jul 16, 2:42 AM · Restricted Project, Restricted Project

Sat, Jul 13

labath added inline comments to D64647: [lldb] [Process/NetBSD] Implement per-thread execution control.
Sat, Jul 13, 7:02 AM · Restricted Project

Fri, Jul 12

labath accepted D64661: [ObjectContainerBSDArchive] Simplify a few things (NFC).
Fri, Jul 12, 1:10 PM · Restricted Project, Restricted Project
labath added inline comments to D64647: [lldb] [Process/NetBSD] Implement per-thread execution control.
Fri, Jul 12, 12:58 PM · Restricted Project
labath added a comment to D64647: [lldb] [Process/NetBSD] Implement per-thread execution control.

Shouldn't there be some tests that come along with this patch?

I was actually hoping that the test suite already covers what needs to be covered, and buildbot would tell me which tests were fixed ;-).

Fri, Jul 12, 11:27 AM · Restricted Project
labath added a comment to D64647: [lldb] [Process/NetBSD] Implement per-thread execution control.

Shouldn't there be some tests that come along with this patch?

Fri, Jul 12, 10:56 AM · Restricted Project

Thu, Jul 11

labath added inline comments to D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin.
Thu, Jul 11, 2:38 PM · Restricted Project
labath added a comment to D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin.

My model for this was that there was a CPPLanguageRuntime.cpp that contains everything you can implement about the CPP runtime that is independent of any particular implementation of the CPP language runtime, and then a plugin instance (in this case the ItaniumABILanguageRuntime) that contains all the bits that are specific to a particular implementation. Then putting the CPPLanguageRuntime.cpp in Target lets you know that this has to only contain the generic parts of the runtime behavior. That still seems to me a useful distinction.

Thu, Jul 11, 2:34 PM · Restricted Project
labath added a comment to D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin.

What is the indented relationship between CPPLanguage and CPPLanguageRuntime plugins (and generally between any Language and its LanguageRuntime)? Right now you're having the CPPLanguage depend on the CPPLanguageRuntime plugin. There is no reverse dependency, so this may be fine, if that's how we intend things to be. However, it's not clear to me whether that's the right way to organize things. Intuitively, I'd expect the LanguageRuntime to depend on Language, and not the other way around...

Thu, Jul 11, 2:26 PM · Restricted Project
labath added a comment to D64546: [lldb] Make TestDeletedExecutable more reliable.

This might mitigate the issue, but timeouts like this are bound to fail in some circumstances (machine load, ...). It's more work, but can we instead have the inferior produce an observable side effect (eg, print some output) and synchronize on this?

Thu, Jul 11, 8:04 AM · Restricted Project, Restricted Project

Wed, Jul 10

labath added a comment to D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4.

Looks mostly good. Just a couple of style comments.

Wed, Jul 10, 1:49 PM · Restricted Project
labath committed rG1abaeece719c: Options: Reduce code duplication (authored by labath).
Options: Reduce code duplication
Wed, Jul 10, 10:12 AM
labath committed rL365665: Options: Reduce code duplication.
Options: Reduce code duplication
Wed, Jul 10, 10:10 AM
labath closed D63770: Options: Reduce code duplication.
Wed, Jul 10, 10:10 AM · Restricted Project
labath committed rG0ace98c9df70: ObjectFileELF: Add support for gnu-style compressed sections (authored by labath).
ObjectFileELF: Add support for gnu-style compressed sections
Wed, Jul 10, 9:12 AM
labath committed rL365654: ObjectFileELF: Add support for gnu-style compressed sections.
ObjectFileELF: Add support for gnu-style compressed sections
Wed, Jul 10, 9:11 AM

Tue, Jul 9

labath committed rG9eb4b96be02a: Add lldb type unit support to the release notes (authored by labath).
Add lldb type unit support to the release notes
Tue, Jul 9, 3:40 PM
labath committed rL365568: Add lldb type unit support to the release notes.
Add lldb type unit support to the release notes
Tue, Jul 9, 3:40 PM
labath closed D64366: Add lldb type unit support to the release notes.
Tue, Jul 9, 3:40 PM · Restricted Project
labath added a comment to D64408: [CMake] `install-distribution` for LLDB on Darwin.

I'm not very familiar with the details here, but this approach seems reasonable to me...

Tue, Jul 9, 8:05 AM · Restricted Project, Restricted Project

Mon, Jul 8

labath added inline comments to D64163: Change LaunchThread interface to return an expected..
Mon, Jul 8, 2:03 PM · Restricted Project, Restricted Project
labath added a comment to D64365: [lldb] Let table gen create command option initializers..

I like this, though I am not really familiar with details of implementing a tablegen backend. This should make it easier to migrate to the llvmOption library, once we arrive to that point.

Mon, Jul 8, 1:57 PM · Restricted Project, Restricted Project
labath accepted D64255: Remove lldb-mi.
Mon, Jul 8, 1:41 PM · Restricted Project
labath added a comment to D64267: lldb_assert: abort when assertions are enabled..

lldbassert(x) already expands to assert(x) in a debug (LLDB_CONFIGURATION_DEBUG) build. It sounds like you want this to assert in a non-debug build which has assertions enabled (Release+Assertions) ? If that's the case, then I think we should just change the #ifdef LLDB_CONFIGURATION_DEBUG in LLDBAssert.h into #ifdef _DEBUG..

Mon, Jul 8, 1:33 PM · Restricted Project, Restricted Project
labath created D64366: Add lldb type unit support to the release notes.
Mon, Jul 8, 1:28 PM · Restricted Project
labath added a comment to D64254: Add lldb-mi deprecation to the release notes.

Random idea: Should we force the user to pass something like -DLLDB_TEMPORARILY_ENABLE_LLDB_MI to cmake to make sure it gets noticed that it is going away (I don't know how many people read release notes).

Mon, Jul 8, 1:20 PM · Restricted Project, Restricted Project

Thu, Jul 4

labath added inline comments to D63166: Move common functionality from processwindows into processdebugger.
Thu, Jul 4, 11:30 PM · Restricted Project
labath accepted D64163: Change LaunchThread interface to return an expected..

Looks fine to me.

Thu, Jul 4, 11:21 PM · Restricted Project, Restricted Project
labath added inline comments to D64154: [Docs] Unify build instructions .
Thu, Jul 4, 11:05 PM · Restricted Project
labath accepted D64118: [lldb_test_suite] Fix lldb test suite targeting remote Android.

This patch looks fine to me overall. I'm unsure if LLDB has a guarantee about which NDK version its compatible with, but I'm alright with staying up to date with the latest release

Yeah, that been my general objective while I was still working on android...

Thu, Jul 4, 10:54 PM · Restricted Project, Restricted Project

Wed, Jul 3

labath accepted D64112: Add plugin.process.gdb-remote.use-libraries-svr4 option.

Thank you.

Wed, Jul 3, 6:19 AM · Restricted Project, Restricted Project

Tue, Jul 2

labath added a comment to D63713: Add error handling to the DataExtractor class.

Ok, I got some numbers now. I went for a micro-benchmark-like setup to show some kind of a worst case scenario. The test setup is as follows:

Tue, Jul 2, 6:42 AM · Restricted Project
labath added inline comments to D63713: Add error handling to the DataExtractor class.
Tue, Jul 2, 3:01 AM · Restricted Project
labath committed rGa1c64dcdecb9: [DWARF] Add one more type unit test (authored by labath).
[DWARF] Add one more type unit test
Tue, Jul 2, 1:01 AM
labath committed rL364890: [DWARF] Add one more type unit test.
[DWARF] Add one more type unit test
Tue, Jul 2, 12:57 AM

Mon, Jul 1

labath added inline comments to D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4.
Mon, Jul 1, 11:46 AM · Restricted Project
labath added a comment to D64013: Correctly use GetLoadedModuleList to take advantage of libraries-svr4.

I don't have time to do a full review of this today, but this seems pretty good at a first glance...

Mon, Jul 1, 8:45 AM · Restricted Project
labath added a comment to D62931: [lldb-server] Add setting to force 'g' packet use.

I think it will be pretty hard for a single person to gather all of these stats for all targets. So perhaps the best solution is to have the setting for the g packet, defaulting to true. After a while, if nobody notices a regression, we can just remove it...

Mon, Jul 1, 6:26 AM · Restricted Project, Restricted Project
labath committed rGc12dfcf1f56b: Don't check the validity of newly contructed data buffers (authored by labath).
Don't check the validity of newly contructed data buffers
Mon, Jul 1, 6:22 AM
labath added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Mon, Jul 1, 6:22 AM · Restricted Project
labath committed rL364754: Don't check the validity of newly contructed data buffers.
Don't check the validity of newly contructed data buffers
Mon, Jul 1, 6:18 AM
labath committed rG77c04c3a5774: @skipIfXmlSupportMissing TestRecognizeBreakpoint (authored by labath).
@skipIfXmlSupportMissing TestRecognizeBreakpoint
Mon, Jul 1, 6:14 AM
labath committed rL364753: @skipIfXmlSupportMissing TestRecognizeBreakpoint.
@skipIfXmlSupportMissing TestRecognizeBreakpoint
Mon, Jul 1, 6:14 AM
labath added a comment to D62503: Add ReadCStringFromMemory for faster string reads.

I've also reverted the preceeding patch in this series as reverting this one has caused one of the other tests to start failing (I've tried to make a more surgical fix first, but @jankratochvil pointed out that this basically reintroduced the problem that we were trying to solve by reverting this).

Mon, Jul 1, 6:09 AM · Restricted Project, Restricted Project
labath committed rG08c38f77c5fb: Revert "Implement xfer:libraries-svr4:read packet" (authored by labath).
Revert "Implement xfer:libraries-svr4:read packet"
Mon, Jul 1, 5:44 AM
labath committed rL364751: Revert "Implement xfer:libraries-svr4:read packet".
Revert "Implement xfer:libraries-svr4:read packet"
Mon, Jul 1, 5:41 AM
labath committed rG4f0a37728052: Fix TestGdbRemoteLibrariesSvr4Support (authored by labath).
Fix TestGdbRemoteLibrariesSvr4Support
Mon, Jul 1, 5:04 AM
labath committed rL364748: Fix TestGdbRemoteLibrariesSvr4Support.
Fix TestGdbRemoteLibrariesSvr4Support
Mon, Jul 1, 5:00 AM
labath committed rG0f73709cb716: Remove null checks of results of new expressions (authored by labath).
Remove null checks of results of new expressions
Mon, Jul 1, 4:11 AM
labath committed rL364744: Remove null checks of results of new expressions.
Remove null checks of results of new expressions
Mon, Jul 1, 4:09 AM
labath added a comment to D63868: Unify+fix remote XML libraries handling with the legacy one.

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

OK, I will try to reproduce it - you are right GDB documentation says for the m packet: "The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory. "

And LLDB is using ReadMemory in NativeProcessProtocol::ReadMemoryWithoutTrap from GDBRemoteCommunicationServerLLGS::Handle_memory_read.

Mon, Jul 1, 3:09 AM · Restricted Project
labath accepted D63791: [lldb] [Process/NetBSD] Fix segfault when handling watchpoint.

LGTM. I don't know what Kamil is up to these days. It might be better to have him review patches like this, but if he's unable to do that, I'm happy to stamp them too...

Mon, Jul 1, 3:00 AM · Restricted Project
labath accepted D63792: [lldb] [Process/NetBSD] Use global enable bits for watchpoints.

Sorry about the delay, I was OOO last week. BTW, the usual rate of pinging on a patch is one week https://llvm.org/docs/DeveloperPolicy.html#code-reviews.

Mon, Jul 1, 2:53 AM · Restricted Project
labath accepted D63545: [lldb] [Process/NetBSD] Support reading YMM registers via PT_*XSTATE.

I don't know whether the code is correct for NetBSD, but it looks ok from a general readability perspective.

Mon, Jul 1, 2:39 AM · Restricted Project
labath accepted D63540: Fix lookup of symbols at the same address with no size vs. size.

Looks good to me, but I'd suggest waiting a while to give @clayborg a chance to respond...

Mon, Jul 1, 2:13 AM · Restricted Project, Restricted Project
labath added a comment to D63868: Unify+fix remote XML libraries handling with the legacy one.

That is because it fixed failing ReadMemory (due to PAGE_SIZE size reading shared library name overlapping to a next page which is accidentally unmapped)

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

Mon, Jul 1, 1:58 AM · Restricted Project
labath resigned from D63853: [Symbol] Add DeclVendor::FindTypes.

Besides picking at code style details, I don't believe I have anything useful to add here. :)

Mon, Jul 1, 1:22 AM · Restricted Project
labath added a comment to D61233: Refactor ObjectFile::GetSDKVersion.

looks good to me too...

Mon, Jul 1, 1:09 AM · Restricted Project, Restricted Project

Tue, Jun 25

labath created D63770: Options: Reduce code duplication.
Tue, Jun 25, 7:50 AM · Restricted Project
labath added a comment to D63110: Fix a crash in option parsing..

BTW, the new version was now failing on linux for a change.

Tue, Jun 25, 7:21 AM · Restricted Project, Restricted Project
labath committed rL364317: Options: Correctly check for missing arguments.
Options: Correctly check for missing arguments
Tue, Jun 25, 7:06 AM
labath committed rG34cac0955d7f: Options: Correctly check for missing arguments (authored by labath).
Options: Correctly check for missing arguments
Tue, Jun 25, 7:06 AM
labath added a comment to D63713: Add error handling to the DataExtractor class.

I'm kind of thinking a change to the cursor as Greg suggested might be better than wrapping the stream further - and probably using llvm::Error.

& yeah, I think using Error and saying "parsing failures are problems that must be handled" rather than allowing for accidentally ignoring failures,

Amounting to something like this:

Cursor C;
DataExtractor D = ...;
D.getU32(C);
D.getU64(C);
...
if (Error E = C.getError())
  ...;

That way early exits (if your record type has optional fields, etc) you can't accidentally forget your error checking (because if there's an Error inside the Cursor it'd have to be checked otherwise it'll assert that it's unchecked, etc)

Tue, Jun 25, 6:49 AM · Restricted Project
labath updated the diff for D63713: Add error handling to the DataExtractor class.

Cursor implementation.

Tue, Jun 25, 6:42 AM · Restricted Project
labath committed rG7ada1c530094: Remove core loading timeout (authored by labath).
Remove core loading timeout
Tue, Jun 25, 12:17 AM
labath committed rL364276: Remove core loading timeout.
Remove core loading timeout
Tue, Jun 25, 12:15 AM
labath closed D63730: Remove core loading timeout.
Tue, Jun 25, 12:15 AM · Restricted Project
labath added a comment to D63540: Fix lookup of symbols at the same address with no size vs. size.

So I am fine with symbols having zero size being in the symbol table. I would be fine not changing anything in the sorting and leaving some symbols with zero size, we just need to fix:

Symbol *Symtab::FindSymbolAtFileAddress(addr_t file_addr);

To ignore zero sized symbols when it finds them _if_ there is another symbol that has a size for that address. Wouldn't that fix the issue here?

Tue, Jun 25, 12:12 AM · Restricted Project, Restricted Project
labath committed rGfcad3bc41541: DWARF: Add support for type units+split dwarf combo (authored by labath).
DWARF: Add support for type units+split dwarf combo
Tue, Jun 25, 12:01 AM
labath committed rL364274: DWARF: Add support for type units+split dwarf combo.
DWARF: Add support for type units+split dwarf combo
Tue, Jun 25, 12:01 AM
labath closed D63643: DWARF: Add support for type units+split dwarf combo.
Tue, Jun 25, 12:01 AM · Restricted Project

Mon, Jun 24

labath added a comment to D63745: [CMake] Check that a certificate for lldb is present at build time..

This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...

Mon, Jun 24, 11:45 PM · Restricted Project, Restricted Project
labath created D63730: Remove core loading timeout.
Mon, Jun 24, 10:56 AM · Restricted Project