labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

labath added a comment to D48351: Move dumping code out of RegisterValue class.

The Stream part isn't the problem. The problem is that the dumping code is implemented in terms of ExecutionContextScope, which then pulls in pretty much everything. If it was just the object "dumping itself" then I would be fine with it as a method, but here it's using the whole world to achieve that goal. At that point I would prefer it living in a separate place (though we can debate on whether it should be a completely separate file, or merged to a single file with the DumpDataExtractor code)

Thu, Jul 19, 10:22 AM
labath added a comment to D48351: Move dumping code out of RegisterValue class.

Does anyone have an opinion on this?

Thu, Jul 19, 8:01 AM
labath resigned from D48306: Make TaskQueue support tasks that return values.
Thu, Jul 19, 7:50 AM
labath committed rL337459: ELF: Replace the header-extension unit test with a lit one.
ELF: Replace the header-extension unit test with a lit one
Thu, Jul 19, 7:43 AM
labath committed rC337456: [CodeGen] Disable aggressive structor optimizations at -O0, take 3.
[CodeGen] Disable aggressive structor optimizations at -O0, take 3
Thu, Jul 19, 7:10 AM
labath committed rL337456: [CodeGen] Disable aggressive structor optimizations at -O0, take 3.
[CodeGen] Disable aggressive structor optimizations at -O0, take 3
Thu, Jul 19, 7:10 AM
labath closed D46889: [DWARF] Extract indexing code into a separate class hierarchy.
Thu, Jul 19, 6:46 AM
labath committed rL337452: Fix whitespace formatting in DWARFExpression::DumpLocation.
Fix whitespace formatting in DWARFExpression::DumpLocation
Thu, Jul 19, 6:36 AM
labath added a comment to D49542: DwarfDebug: Reduce duplication in addAccel*** methods.

This technically isn't NFC, because it changes the StringPool used for
apple tables in the DWO case (now it uses the main file like DWARF v5
instead of the DWO file). However, that shouldn't matter, as DWO is not
a thing on apple targets (clang frontend simply ignores -gsplit-dwarf).

Thu, Jul 19, 5:56 AM
labath added inline comments to D49493: [DebugInfo] Reduce debug_str_offsets section size.
Thu, Jul 19, 5:53 AM
labath created D49542: DwarfDebug: Reduce duplication in addAccel*** methods.
Thu, Jul 19, 5:49 AM
labath updated the diff for D49493: [DebugInfo] Reduce debug_str_offsets section size.

Fix typos and add the extra test.

Thu, Jul 19, 3:52 AM
labath added a comment to D49493: [DebugInfo] Reduce debug_str_offsets section size.

I would like a test that did the following: add string "A" not indexed; add string "B" indexed; add string "A" indexed.
Then show that the string section has "A" followed by "B", and the offsets table is correct (entry 0 points to "B", entry 1 points to "A").
Is that feasible?

Thu, Jul 19, 3:51 AM

Yesterday

labath created D49493: [DebugInfo] Reduce debug_str_offsets section size.
Wed, Jul 18, 9:27 AM
labath added a comment to D49485: CMake: use new policy for CMP0051.

I am not sure what kinds of problems this can cause, but I can volunteer to help with the lldb side of things in case of any fallout.

Wed, Jul 18, 7:57 AM
labath added a comment to D46685: [CodeGen] Disable structor optimizations at -O0.

This was reverted in r333482 because it was causing "definition with same mangled name as another definition" errors in some internal google builds.

Wed, Jul 18, 2:16 AM
labath accepted D49435: Added unit tests for Flags.

lgtm. Thanks.

Wed, Jul 18, 1:19 AM
labath added a comment to D49415: Add unit tests for VMRange.
  • Added a PrintTo function as otherwise the gtest comparison macros won't compile.
Wed, Jul 18, 1:14 AM
labath accepted D49451: [windows] Use a well-known path for ComSpec if we fail to retrieve it.
Wed, Jul 18, 12:52 AM

Tue, Jul 17

labath added inline comments to D49420: [DebugInfo] Generate .debug_names section when it makes sense.
Tue, Jul 17, 8:50 AM
labath updated the diff for D49420: [DebugInfo] Generate .debug_names section when it makes sense.

Updated the test to include the two extra strings that are used from the
debug_names table.

Tue, Jul 17, 8:42 AM
labath added a comment to D49379: lldbsuite: ignore decoding errors in _encoded_write.

Also I think we can report about decoding errors like gdb does:

Thread 3 "a.out" hit Breakpoint 1, do_bad_thing_with_location (char_ptr=0x619c20 "\001$\255", <incomplete sequence \373>, new_val=1 '\001') at main.cpp:40
40	}
(gdb) x/10xb char_ptr
0x619c20:	0x01	0x24	0xad	0xfb	0x00	0x00	0x00	0x00
0x619c28:	0x00	0x00
Tue, Jul 17, 8:15 AM · Restricted Project
labath added a comment to D49379: lldbsuite: ignore decoding errors in _encoded_write.

Thank you for comments, I've updated patch. I agree that it is not complete solution, maybe better to make all tests unicode safe.

Tue, Jul 17, 6:23 AM · Restricted Project
labath accepted D49137: [dsymutil] Implement DWARF5 accelerator tables..

Update logic for picking the default accelerator table kind:

If we've only seen either Apple or DWARF accelerator tables, we pick the
respective one. If we've seen neither or both, the choice is determined
based on the lower DWARF version encountered.

Tue, Jul 17, 5:43 AM
labath created D49420: [DebugInfo] Generate .debug_names section when it makes sense.
Tue, Jul 17, 5:28 AM
labath added a comment to D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP). .

I suppose we can add an off-by-default DWP mode so that it can be used for integration testing.

Tue, Jul 17, 3:40 AM
labath added a comment to D49234: [DebugInfo] Refactor DWARFDie::iterator to prevent UB.

"this is the reverse iterator type for this sequence"

Tue, Jul 17, 2:04 AM
labath added a comment to D49415: Add unit tests for VMRange.

The tests seem fine, but as a matter of style I would suggest two changes:

  • replace ASSERT_XXX with EXPECT_XXX: EXPECT macros will not terminate the test, so you'll be able to see additional failures, which is helpful in pinpointing the problem. ASSERT is good for cases where the subsequent checks make no sense or will crash if the previous check is not satisfied (but that is not the case for your checks AFAICT).
  • replace ASSERT_TRUE(foo == bar) with ASSERT_EQ(foo, bar) (and similar for other relational operators). Right not that does not matter much, but if someone later implements operator<< for this class, you will automatically get a helpful error message describing the objects instead of a useless "false is not true" message.
Tue, Jul 17, 1:46 AM
labath added a comment to D49411: Move from StringRef to std::string in the ScriptInterpreter API.

Normally this would be clearly a good thing, but the added complication here is that this function is part of a class hierarchy, and so this way you are forcing every implementation to take a std::string, even though only one of them cares about null-termination.

Tue, Jul 17, 1:37 AM
labath added a comment to D49410: [PDB] Parse UDT symbols and pointers to members.

I have no opinion on the implementation, but I like that you wrote a non-execution test for this, as this will enable us one day to run it on non-windows hosts too.

Tue, Jul 17, 1:26 AM
labath accepted D49406: Invert dependency between lldb-framework and lldb-suite.

lgtm

Tue, Jul 17, 1:19 AM

Mon, Jul 16

labath added a comment to D49282: [cmake] Add option to skip building lldb-server.

I think this is fine (though the meaning of SKIP_LLDB_SERVER is subtly different than SKIP_DEBUGSERVER), but while looking at this I got an idea for a possible improvement.

How is it subtly different? Admittedly I haven't looked in excruciating detail, but I didn't notice any large differences.

Mon, Jul 16, 12:36 PM
labath added a comment to D49137: [dsymutil] Implement DWARF5 accelerator tables..

Are you worried about the case where the dSYM bundle contains CUs with different dwarf versions?

Mon, Jul 16, 9:41 AM
labath committed rL337188: Fix typo in find-basic-function test.
Fix typo in find-basic-function test
Mon, Jul 16, 9:24 AM
labath added a comment to D49193: [CMake] Export the LLVM_LINK_LLVM_DYLIB setting.

This looks straight-forward enough to me, though I would feel more comfortable if someone else (@mgorny ?) took a look at this too, to make sure I am not missing anything.

Mon, Jul 16, 9:12 AM
labath added a comment to D49202: Restructure the minidump loading path and add early & explicit consistency checks.

I don't agree with the two-stage initialization of the MinidumpParser class being introduced here. We deliberately introduced the Create static function to avoid this. If this Initialize function in checking invariants which are assumed to be hold by other parser methods, then it should be done by the Create function. Ideally this would be done before even constructing the parser object, but if this is impractical for some reason then you can make the Initialize function private and call it directly from Create. This way a user will never be able to see an malformed parser object. To make sure you propagate the error, you can change the return type of Create to llvm::Expected<MinidumpParser (the only reason we did not do this back then was that there was no precedent for using Expected in LLDB, but this is no longer the case).

Mon, Jul 16, 9:04 AM · Restricted Project
labath accepted D49038: [CMake] Give lldb tools functional install targets when building LLDB.framework.

Makes sense to me.

Mon, Jul 16, 8:21 AM
labath added a comment to D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID.

Could you please add a test for the new behavior as well?

Mon, Jul 16, 8:18 AM
labath accepted D49377: Move pretty stack trace printer into driver..

I think this makes perfect sense. Could you also add the same thing to the other binaries in the tools subfolder?

Mon, Jul 16, 7:55 AM
labath added a comment to D49282: [cmake] Add option to skip building lldb-server.

I think this is fine (though the meaning of SKIP_LLDB_SERVER is subtly different than SKIP_DEBUGSERVER), but while looking at this I got an idea for a possible improvement.

Mon, Jul 16, 7:50 AM
labath committed rL337173: Fix TestDataFormatterUnordered for older libc++ versions.
Fix TestDataFormatterUnordered for older libc++ versions
Mon, Jul 16, 7:43 AM
labath added a comment to D49334: [LLDB} Added syntax highlighting support.

We're also trying to avoid adding new clang-specific code to the debugger core. I think it would make more sense if the (clang-based) c++ highlighter was provided by some plugin. I see a couple of options:

  • the c++ language plugin: I think this is the most low-level plugin that is still language specific. However, it is specific to c(++), whereas here you format other languages too.
  • the clang expression parser plugin: it's a bit of a stretch, but syntax higlighting is a kind of expression parsing
  • a completely new plugin
Mon, Jul 16, 7:17 AM · Restricted Project
labath accepted D49307: Fix some crashes and deadlocks in FormatAnsiTerminalCodes.

This looks straight-forward enough.

Mon, Jul 16, 4:14 AM
labath added a comment to D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet.

Could you also add a test case for this?
I think it should be possible to test this via the gdb-client (test/testcases/functionalities/gdb_remote_client/) test suite. If I understood the previous comments correctly, you'll need to mock a server that sends a thread-pcs field, but does not implement a jThreadsInfo packet.

Mon, Jul 16, 3:15 AM
labath accepted D49102: [AccelTable] Provide DWARF5AccelTableStaticData for dsymutil..

This looks good to me. There are some different tradeoffs we could make (speed/templates vs. code size/virtualization), but I don't have an opinion on that, so if noone else does either, I guess this is fine.

Mon, Jul 16, 3:08 AM

Tue, Jul 10

labath accepted D49110: [testsuite] Implement a category to skip libstdcxx tests.

Sorry for the slow response, I am OOO officially :P.

Tue, Jul 10, 11:35 AM

Mon, Jul 9

labath added a comment to D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet.

Could you elaborate on how/why is this wrong? E.g. in which situations does it matter whether we clear the PC list?

Mon, Jul 9, 6:20 AM
labath added a comment to D49033: [test-suite] Add a decorator for the lack of libstdcxx on the system..

Besides the problems I mention in inline comments, the other issue I have with this patch is that it does things completely different than how we implement the check for libc++, which is an identical problem. (libc++ detection is done by marking all libc++ tests as with a special category, and then deciding in dotest.py (checkLibcxxSupport) whether to run the category as a whole).

Mon, Jul 9, 6:12 AM

Mon, Jul 2

labath updated subscribers of D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP). .

@labath

I am not denying that there is value in running the dotest suite in all of these modes. In fact, I think that (the fact that we can use the same tests to exercise a lot of different scenarios) is one of the strengths ?>of our test suite. However, I don't believe all of these modes should be inflicted onto everyone running lldb tests or be our first and only line of defense against regressions.

for what it's worth - not sure how much you care about my opinion, but i think it's an important point but it doesn't actually contradict or prevent your second point regarding adding regression tests using lldb-test, however i think those should be added over time (sadly no tests were added when the support for .dwp was implemented / introduced) (not in this patch).
I think that the approach of this patch is still useful, this mode can be off by default, but if smb needs to run all the tests with dwps - it's easy to do by passing or setting a variable (for example).

yes, thats the near term solution.

Mon, Jul 2, 11:24 AM
labath added a comment to D48796: Refactoring for for the internal command line completion API (NFC).

Thank you for working on this. Overall, I like the direction this is going, but I'm OOO this week and the next one, so I'll defer to other reviewers on this one.

Mon, Jul 2, 11:04 AM

Fri, Jun 29

labath updated subscribers of D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP). .
Fri, Jun 29, 12:47 PM
labath added a reviewer for D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP). : davide.

Is your plan to add dwp as another dimension in the test matrix (an equal citizen of DWARF, dSYM, DWO) or something that would be on or off for an entire run of the suite, or something only exercised by few specialized testcases?

Fri, Jun 29, 12:25 PM
labath committed rL335967: Add a test for reading lld-generated build-ids.
Add a test for reading lld-generated build-ids
Fri, Jun 29, 5:20 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Fri, Jun 29, 5:20 AM
labath committed rL335963: UUID: Add support for arbitrary-sized module IDs.
UUID: Add support for arbitrary-sized module IDs
Fri, Jun 29, 4:25 AM
labath closed D48633: UUID: Add support for arbitrary-sized module IDs.
Fri, Jun 29, 4:25 AM
labath committed rL335960: Fix use-after-free in CommandCompletions.cpp.
Fix use-after-free in CommandCompletions.cpp
Fri, Jun 29, 3:32 AM
labath committed rL335956: Fix TestLoadUsingPaths on linux.
Fix TestLoadUsingPaths on linux
Fri, Jun 29, 2:26 AM
labath committed rL335955: Modernize completion tests.
Modernize completion tests
Fri, Jun 29, 2:11 AM

Thu, Jun 28

labath updated the diff for D48633: UUID: Add support for arbitrary-sized module IDs.

Add a check to the tab-completion function.

Thu, Jun 28, 7:40 AM
labath added inline comments to D48633: UUID: Add support for arbitrary-sized module IDs.
Thu, Jun 28, 7:39 AM
labath committed rL335859: Skip core file tests on build configurations lacking necessary components.
Skip core file tests on build configurations lacking necessary components
Thu, Jun 28, 7:27 AM
labath closed D48641: Skip core file tests on build configurations lacking necessary components.
Thu, Jun 28, 7:27 AM
labath added a comment to D48632: ADT: Move ArrayRef comparison operators into the class.

I've had to revert this because it causes compilation errors on MSVC. I expected this could be a problem, though the error itself is rather puzzling:

C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): error C2893: Failed to specialize function template 'unknown-type std::equal_to<void>::operator ()(_Ty1 &&,_Ty2 &&) const'
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: With the following template arguments:
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: '_Ty1=const llvm::MDOperand &'
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\xutility(2919): note: '_Ty2=const llvm::MDOperand &'

So it MSVC is picking up this friend declaration (or at least, getting confused by it) even when neither of the types are ArrayRefs. That seems like a bug to me, though we may not be able to do anything about it.

Thu, Jun 28, 5:47 AM
labath committed rL335844: Revert "ADT: Move ArrayRef comparison operators into the class".
Revert "ADT: Move ArrayRef comparison operators into the class"
Thu, Jun 28, 5:15 AM
labath committed rL335839: ADT: Move ArrayRef comparison operators into the class.
ADT: Move ArrayRef comparison operators into the class
Thu, Jun 28, 4:51 AM
labath closed D48632: ADT: Move ArrayRef comparison operators into the class.
Thu, Jun 28, 4:51 AM
labath requested changes to D48463: Prevent dead locking when calling PrintAsync.

Well in theory we could poke more holes in our guard from some nested function, but this only fixes the deadlock symptom. PrintAsync would still be blocking whenever the mutex is hold by someone.

Thu, Jun 28, 3:43 AM
labath committed rL335822: Retrieve a function PDB symbol correctly from nested blocks.
Retrieve a function PDB symbol correctly from nested blocks
Thu, Jun 28, 3:08 AM
labath closed D47939: Retrieve a function PDB symbol correctly from nested blocks.
Thu, Jun 28, 3:08 AM
labath accepted D48665: Added test case for: r334978 - Fixed file completion for paths that start with '~'.

Looks good. Thank you.

Thu, Jun 28, 2:49 AM

Wed, Jun 27

labath added a comment to D47532: [ASTImporter] Import the whole redecl chain of functions.

The broken lldb tests are fixed with a minor change. We no longer load the Decls from the
external source during the call of DeclContext::containsDecl. A new function
DeclContext::containsDeclAndLoad is added which does a load and calls
containsDecl.

Re-apply commit: r335731

Wed, Jun 27, 7:09 AM
labath created D48646: Add a test for reading lld-generated build-ids.
Wed, Jun 27, 7:08 AM
labath created D48641: Skip core file tests on build configurations lacking necessary components.
Wed, Jun 27, 5:56 AM
labath created D48633: UUID: Add support for arbitrary-sized module IDs.
Wed, Jun 27, 5:08 AM
labath created D48632: ADT: Move ArrayRef comparison operators into the class.
Wed, Jun 27, 5:05 AM
labath accepted D48596: [SymbolFile] Implement GetCompleteObjCClass for .debug_names.

looks good to me

Wed, Jun 27, 4:28 AM
labath accepted D47939: Retrieve a function PDB symbol correctly from nested blocks.

This looks fine to me, but let's also wait for Aaron.

Wed, Jun 27, 4:23 AM
labath added a comment to D48596: [SymbolFile] Implement GetCompleteObjCClass for .debug_names.

How are you planning on testing this? Most of the DWARFIndex functionality is tested in terms of lldb-test, but I think this function is not reachable from there. Maybe you could write a specific dotest-test which targets this?

Wed, Jun 27, 1:52 AM
labath added inline comments to D48623: WIP: Move architecture-specific address adjustment to architecture plugins..
Wed, Jun 27, 1:44 AM
labath added a comment to D48623: WIP: Move architecture-specific address adjustment to architecture plugins..

I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?

Wed, Jun 27, 1:19 AM
labath added inline comments to D48623: WIP: Move architecture-specific address adjustment to architecture plugins..
Wed, Jun 27, 1:15 AM

Tue, Jun 26

labath committed rL335612: Represent invalid UUIDs as UUIDs with length zero.
Represent invalid UUIDs as UUIDs with length zero
Tue, Jun 26, 8:17 AM
labath closed D48479: Represent invalid UUIDs as UUIDs with length zero.
Tue, Jun 26, 8:17 AM

Mon, Jun 25

labath added inline comments to D48479: Represent invalid UUIDs as UUIDs with length zero.
Mon, Jun 25, 8:41 AM
labath added a comment to D48500: [DWARFASTParser] Remove special cases for `llvm-gcc`.

I suppose this is fine. My main worry is whether llvm-gcc is the *only* situation where we rely on these tweaks, but I guess the best way to find out is to try removing them.

Mon, Jun 25, 8:35 AM
labath updated subscribers of D47532: [ASTImporter] Import the whole redecl chain of functions.

This has broken the LLDB bot http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/25132. Could you take a look?

Mon, Jun 25, 8:30 AM
labath updated the diff for D48479: Represent invalid UUIDs as UUIDs with length zero.
  • Use static factory functions instead of the extra argument (the best names I could come up with is fromData and fromOptionalData).
Mon, Jun 25, 8:22 AM
labath added a comment to D48479: Represent invalid UUIDs as UUIDs with length zero.

Would love to remove the "accept_zeroes" argument everywhere. Too much matching happens in LLDB and we can't have multiple shared libraries claiming zeros as their UUID

Mon, Jun 25, 8:21 AM
labath committed rL335476: Fix TestThreadExit for gcc&libc++ combo.
Fix TestThreadExit for gcc&libc++ combo
Mon, Jun 25, 7:33 AM

Fri, Jun 22

labath added a comment to D48463: Prevent dead locking when calling PrintAsync.

Adding Pavel because he wrote the PrintAsync code.

Also @labath: Can you tell me what variables/functionality the m_output_mutex in Editline.cpp is supposed to shield? I don't see any documentation for that.

The m_output_mutex name suggests its related to console output, but we actually take the lock also when reading *input*. Especially in EditLine::GetLine we take a guard on the lock but then somehow unlock the guarded mutex from inside Editline::GetCharacter that we call afterwards (which completely breaks this patch):

Fri, Jun 22, 2:04 PM
labath added a comment to D48479: Represent invalid UUIDs as UUIDs with length zero.
One solution might be to encapsulate the MachO convention in the MachO

code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?

Fri, Jun 22, 12:29 PM
labath updated the diff for D48479: Represent invalid UUIDs as UUIDs with length zero.

Delete copy constructor.

Fri, Jun 22, 12:03 PM
labath added a comment to D48479: Represent invalid UUIDs as UUIDs with length zero.

The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working, I have introduced an
additional argument to the UUID constructor, which specifies whether an
all-zero vector should be considered a valid UUID. For the usages where
the UUID data comes from a MachO file, I set this argument to false.

What is the distinction between "no UUID has been set" and "invalid UUID"? I find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?

Fri, Jun 22, 12:02 PM
labath committed rL335344: Android.rules: Use libc++ by default.
Android.rules: Use libc++ by default
Fri, Jun 22, 6:18 AM
labath created D48479: Represent invalid UUIDs as UUIDs with length zero.
Fri, Jun 22, 6:00 AM
labath added a comment to D48240: Try again to implement a FIFO task queue.

I think I noticed one more bug in the !threads implementation. Other than that, this looks fine to me, but I'll leave the final ok to Chandler.

Fri, Jun 22, 3:51 AM
labath added a comment to D48465: Added initial code completion support for the `expr` command.

I think this would be a very nice feature for lldb. Thank you for working on this.

Fri, Jun 22, 3:19 AM

Thu, Jun 21

labath committed rL335260: Partially revert r335236.
Partially revert r335236
Thu, Jun 21, 10:41 AM
labath committed rL335247: Fix macos build for r335244.
Fix macos build for r335244
Thu, Jun 21, 8:45 AM