labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

labath added a reviewer for D47275: 1/3: DWARFDIE split out to DWARFBasicDIE: aprantl.

I don't think a name like DWARFUnitDIE is a good one bacause it would make a weird is-a relationship (a DWARFDIE represetning a DW_TAG_variable is certainly not a "unit DIE" yet you could assign it to a DWARFUnitDIE&). We could have a DWARFUnitDIE type if we wanted to, but that would have to be a special type in addition to DWARFBasicDIE. However, I think that would be overkill.

Thu, May 24, 2:32 AM

Yesterday

labath added a comment to D47235: Move ModuleList's dependency on clangDriver into Host.

I guess it would be nice to encapsulate this in some sort of a plugin (since the setting is used from the clang expression parser plugin, I guess this would be the natural home for it) , but I haven't looked in detail at could that work. What I do know is that we already have the ability to inject settings from within a plugin (see SymbolFileDWARF::DebuggerInitialize). Maybe that would work here too?

I agree that a clang plugin seems like the "real" solution, I had come to the same conclusion yesterday. But that is a significant amount of work obviously.

Wed, May 23, 9:16 AM
labath added a comment to D46005: [test] Add a utility for dumping all tests in the dotest suite.

Thank you for the support Jonas. I've quickly prototyped a patch (D47265) of how would this integrate with lit. The nice part about it is that it is very small. If you consider the inline test refactor it even has negative LOC. It would definitely have negative LOC impact if this would allow us to remote test driver parts from dotest.

Wed, May 23, 8:58 AM
labath added a dependency for D47265: WIP: lit: Run each test separately: D46005: [test] Add a utility for dumping all tests in the dotest suite.
Wed, May 23, 8:48 AM
labath added a dependent revision for D46005: [test] Add a utility for dumping all tests in the dotest suite: D47265: WIP: lit: Run each test separately.
Wed, May 23, 8:48 AM
labath created D47265: WIP: lit: Run each test separately.
Wed, May 23, 8:46 AM
labath created D47253: DWARF: Move indexing code from DWARFUnit to ManualDWARFIndex.
Wed, May 23, 5:29 AM
labath created D47250: Move ObjectFile initialization out of SystemInitializerCommon.
Wed, May 23, 3:43 AM
labath committed rL333074: Fix PathMappingList tests on windows.
Fix PathMappingList tests on windows
Wed, May 23, 3:36 AM
labath committed rL333073: ProcessLauncherPosixFork: move setgid call into the if(debug) branch.
ProcessLauncherPosixFork: move setgid call into the if(debug) branch
Wed, May 23, 3:14 AM
labath added a comment to D47235: Move ModuleList's dependency on clangDriver into Host.

In a way, this makes sense, but in a lot of other ways, it actually makes things worse :)

Wed, May 23, 2:33 AM
labath added a comment to D47232: Break dependency from Expression -> Commands.

Both of the solutions sound plausible to me (extra struct vs. moving REPL to Command). Maybe that's because I don't know enough about the REPL to have formed an opinion on it.

Wed, May 23, 1:46 AM
labath accepted D47228: Break dependency from Core to ObjectFileJIT.

The change sounds reasonable to me. I should just point out that this is the Core <-> ObjectFileJIT cycle you are breaking.

Wed, May 23, 1:34 AM

Tue, May 22

labath committed rL332997: Enable ProcessMachCore plugin on non-apple platforms.
Enable ProcessMachCore plugin on non-apple platforms
Tue, May 22, 9:37 AM
labath closed D47133: Enable ProcessMachCore plugin on non-apple platforms.
Tue, May 22, 9:37 AM
labath added a comment to D47147: DWARFIndex: Reduce duplication in the GetFunctions methods.

Changing the code to filter based on the dwarf information instead of the going through CompilerDeclContexts sounds like a good idea. I've been wondering why we are doing it this way -- the explanation I gave to myself was that this would allow the individual language plugins to decide what represents a "class" instead of just searching for DW_TAG_structure_type and similar. However, it's very likely that this is extra flexibility we don't need (and indeed, the manual index just checks the dwarf tags while building the index).

Tue, May 22, 9:21 AM
labath accepted D46726: build: use cmake to find the libedit content.

Ok, let's give this a try.

Tue, May 22, 2:03 AM

Mon, May 21

labath created D47147: DWARFIndex: Reduce duplication in the GetFunctions methods.
Mon, May 21, 9:27 AM
labath committed rL332841: Reland "[DWARF] Extract indexing code into a separate class hierarchy".
Reland "[DWARF] Extract indexing code into a separate class hierarchy"
Mon, May 21, 7:16 AM
labath committed rL332839: [CodeGen] Disable aggressive structor optimizations at -O0, take 2.
[CodeGen] Disable aggressive structor optimizations at -O0, take 2
Mon, May 21, 4:53 AM
labath committed rC332839: [CodeGen] Disable aggressive structor optimizations at -O0, take 2.
[CodeGen] Disable aggressive structor optimizations at -O0, take 2
Mon, May 21, 4:53 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Mon, May 21, 4:53 AM
labath created D47133: Enable ProcessMachCore plugin on non-apple platforms.
Mon, May 21, 3:29 AM
labath committed rL332833: Improve coverage of the apple-tables test.
Improve coverage of the apple-tables test
Mon, May 21, 3:13 AM
labath committed rL332831: Add some apple-tables lookup tests.
Add some apple-tables lookup tests
Mon, May 21, 2:31 AM
labath closed D47064: Add some apple-tables lookup tests.
Mon, May 21, 2:31 AM
labath accepted D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes.

looks good. I don't know if anyone else has an opinion on how should we treat "" for mapping purposes, but treating it as "." seems fine to me.

Mon, May 21, 1:57 AM
labath added a comment to D47062: Suggest lldb-dotest to reproduce a failure..

Looks fine to me, just make the path computation windows-compatible.

Mon, May 21, 1:21 AM

Fri, May 18

labath added a comment to D46889: [DWARF] Extract indexing code into a separate class hierarchy.

Thanks for jumping on this Amara — I just wanted to point out that we ususally don't revert lldb changes that only break the lldb-xcode bot if they pass on the lldb-cmake bot at the same time. When this happens it usually means that the lldb Xcode project must be updated and it's too much to ask from all open source contributors to get access to a machine running Xcode to do this. Instead one of the Apple LLDB developers usually goes in and updates the Xcode project for them.

  • adrian

Ah ok. Does that include cases like this one with the link error:

Undefined symbols for architecture x86_64:
  "lldb_private::AppleDWARFIndex::Create(lldb_private::Module&, lldb_private::DWARFDataExtractor, lldb_private::DWARFDataExtractor, lldb_private::DWARFDataExtractor, lldb_private::DWARFDataExtractor, lldb_private::DWARFDataExtractor)", referenced from:
      SymbolFileDWARF::InitializeObject() in liblldb-core.a(SymbolFileDWARF.o)
  "vtable for lldb_private::ManualDWARFIndex", referenced from:
      SymbolFileDWARF::InitializeObject() in liblldb-core.a(SymbolFileDWARF.o)
  NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
Fri, May 18, 12:12 PM
labath added inline comments to D47064: Add some apple-tables lookup tests.
Fri, May 18, 7:22 AM
labath committed rL332719: [DWARF] Extract indexing code into a separate class hierarchy.
[DWARF] Extract indexing code into a separate class hierarchy
Fri, May 18, 7:19 AM
labath closed D46889: [DWARF] Extract indexing code into a separate class hierarchy.
Fri, May 18, 7:19 AM
labath created D47064: Add some apple-tables lookup tests.
Fri, May 18, 6:59 AM
labath committed rL332710: Add back #ifdef __APPLE__ to RegisterContextDarwin_xxx….
Add back #ifdef __APPLE__ to RegisterContextDarwin_xxx…
Fri, May 18, 5:58 AM
labath committed rL332702: Make ObjectFileMachO work on non-darwin platforms.
Make ObjectFileMachO work on non-darwin platforms
Fri, May 18, 4:39 AM
labath closed D46934: Make ObjectFileMachO work on non-darwin platforms.
Fri, May 18, 4:39 AM
labath added a comment to D47021: Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes.

Although it may not seem that way from the number of comments, the change looks good to me. The main thing is the moving of the test file, as that will fail in the cmake build. And it also looks like some code can be simplified if my assumption about not converting "" to "." is true.

Fri, May 18, 2:40 AM

Thu, May 17

labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/23447

Thu, May 17, 10:18 AM
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

Sorry, I was wrong about the cause, but the bots are red nonetheless. My bet is it's the last {"", "."}, test, which is not working because of an early return in SetFile. TBH, I am not sure we would want that to work anyway, as we too treat an empty filespec specially in a lot of places.

Thu, May 17, 10:17 AM
labath updated subscribers of D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

This has broken the unit tests. Looks like a bad merge that did not take
into account the refactoring in r332088.

Thu, May 17, 10:09 AM
labath added a comment to D47018: [lit, lldbsuite] Disable tests that are failing because of pr21765 and pr24489.

I think you're the only one looking into test health on windows right now, so feel free to manage their decorators as you see fit.

Thu, May 17, 10:02 AM
labath updated the diff for D46934: Make ObjectFileMachO work on non-darwin platforms.
  • Use #ifndef approach for handling the constants.
Thu, May 17, 10:01 AM
labath updated the diff for D46889: [DWARF] Extract indexing code into a separate class hierarchy.
  • s/AppleIndex/AppleDWARFIndex
  • move things to separate files
Thu, May 17, 8:19 AM
labath added a comment to D46889: [DWARF] Extract indexing code into a separate class hierarchy.

Thank you for the review. I'll have the updated diff shortly. In the mean time, here are my responses.

Thu, May 17, 7:48 AM
labath requested review of D46685: [CodeGen] Disable structor optimizations at -O0.
Thu, May 17, 7:21 AM
labath updated the diff for D46685: [CodeGen] Disable structor optimizations at -O0.

Updating with a new version of the patch. The problem with the previous version
was that it caused us to use C5/D5 comdats very aggressively (at -O0), and this
is not always correct. This uses a more nuanced approach:

Thu, May 17, 7:19 AM
labath reopened D46685: [CodeGen] Disable structor optimizations at -O0.
Thu, May 17, 7:19 AM
labath committed rL332596: [DWARF] Have HashedNameToDIE store a DataExtractor by value.
[DWARF] Have HashedNameToDIE store a DataExtractor by value
Thu, May 17, 4:40 AM
labath closed D46888: [DWARF] Have HashedNameToDIE store a DataExtractor by value.
Thu, May 17, 4:39 AM
labath updated the diff for D46934: Make ObjectFileMachO work on non-darwin platforms.

This is the version with new symbolic constants. I put them in a new file, as I
couldn't think of a better place for them.

Thu, May 17, 4:13 AM
labath added a comment to D46934: Make ObjectFileMachO work on non-darwin platforms.

Does that mean we can now also remove the #ifdef APPLE from the objectfile unit tests?

Thu, May 17, 4:06 AM
labath accepted D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

It would still be interesting to align remove_dots with the "standard practice", but it looks like that would end up being a big project. I think we can live with tweak on our side in this case. If someone ever comes around to changing remote_dots behavior, at least it we will be ready.

Thu, May 17, 3:47 AM
labath added a comment to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
The advantage of the second one is that we will have the ability to inject commands which depend on the results of previous commands (something that I think we will need, sooner or later).

That is worth considering. To write good tests that depend on previous results, we probably want to have SBAPI-like Python scripting for lldb-mi available. To make that work we would need to introduce an lldbmi.HandleCommand("-mycmd...") interface that runs one command, blocks on it, and returns the output in a string. And then we would need to export that to Python. Since the interface is string-in-string-out, we could probably do it via C without even involving SWIG. As an escape hatch to access features outside of the mi command set, we could implement the -interpreter-exec mi command that passes its input to SBAPI's HandleCommand() if need be.
Alternatively we could use gtest to write unittest-style lldb-mi tests directly in C++, if going via Python is too messy. Again, we would do that in a blocking or synchronized I/O mode.

Thu, May 17, 3:22 AM · Restricted Project

Wed, May 16

labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Wed, May 16, 9:34 AM · Restricted Project
labath added inline comments to D46934: Make ObjectFileMachO work on non-darwin platforms.
Wed, May 16, 9:02 AM
labath created D46934: Make ObjectFileMachO work on non-darwin platforms.
Wed, May 16, 3:00 AM
labath added a comment to D46887: Fix llvm::sys::path::remove_dots() to return "." instead of an empty path..

I believe this function was written based on Rob Pike's Clean() function for Go, and the Go's function also returns "." (instead of "") for the current directory. So this change makes sense to me.

https://golang.org/pkg/path/#example_Clean

Maybe you should return "." for "". as well. An empty string as a path component makes sense, as "/foo/bar", "/foo//bar" and "/foo/./bar" are all interpreted as the same path. Go's Clean() returns "." for "" too.

Wed, May 16, 12:57 AM
labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Wed, May 16, 12:34 AM · Restricted Project

Tue, May 15

labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

Thank you.

Tue, May 15, 9:37 AM
labath added a comment to D46887: Fix llvm::sys::path::remove_dots() to return "." instead of an empty path..

As I said on the lldb patch, I believe this behavior makes more sense for a function like this (i.e., it should never return an invalid path ("") if the input path is valid, even if it means leaving some dots around. (We already keep a leading ../ component for the remove_dot_dot=true case as it is not possible to preserve the path semantics without this.)

Tue, May 15, 9:27 AM
labath created D46889: [DWARF] Extract indexing code into a separate class hierarchy.
Tue, May 15, 9:22 AM
labath created D46888: [DWARF] Have HashedNameToDIE store a DataExtractor by value.
Tue, May 15, 9:19 AM
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

If those are the only tests that fail, then I'd still go for it, as I still firmly believe this is the correct behavior for such a function. Such a low level test does not have to mean that someone really cares about this, it could be more of a case of documenting existing behavior (the svn history is not really clear on this). If there are other llvm/clang tests failing as a result of changing that, then we can reconsider.

Tue, May 15, 9:10 AM
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

(Also, the function already does not remove leading .. components, so there is precedent already for not removing stuff when it causes the path to become not equivalent.)

Tue, May 15, 8:25 AM
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

So the function in llvm is called llvm::sys::path::remove_dots(...) and it is removing the dots. Not sure it is correct to be changing a function that says "remove_dots" to not remove dots and actually return something with a . in it... Seems like this should be taken care of in LLDB. Thoughts?

Tue, May 15, 8:22 AM
labath committed rL332353: Reapply "Remove Process references from the Host module".
Reapply "Remove Process references from the Host module"
Tue, May 15, 6:46 AM
labath committed rL332349: Reapply "DWARFVerifier: Check "completeness" of .debug_names section".
Reapply "DWARFVerifier: Check "completeness" of .debug_names section"
Tue, May 15, 6:28 AM
labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 5:42 AM · Restricted Project
labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 4:00 AM · Restricted Project
labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 3:38 AM · Restricted Project
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

Adding a test for that in lldb is fine. If we're relying on some behavior we want to make sure it doesn't change without us noticing. However, I don't think we should make any special allowments for people using lldb with older versions of llvm. It sends the wrong message. Making this work in this case is easy enough, but this is just the tip of the iceberg. We have made many changes in the past where we add/modify/extend an llvm interface to make it work for our needs, and then almost immediately start using it in lldb (the last example of that is the switch to llvm path handling functions in FileSpec, if anyone tries use that with a older version of llvm, he'll get a bunch of errors). We're a single project with a single release schedule (I know a lot don't follow the official release schedule, but then they should at least cut from the same trunk revision; all the people that I know about do just that) and we don't support mixing revisions. If anyone tries to do things differently, he's on his own. If he's lucky, he'll get a build error. If not, he gets to track down subtle behavior differences.

Tue, May 15, 2:01 AM
labath added a comment to D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer.

Trying to be smart while being lazy and multithreaded is going to make the code complicated (possibility for bugs) and/or introduce a lot of locking overhead.

Tue, May 15, 1:44 AM
labath added inline comments to D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target..
Tue, May 15, 1:24 AM · Restricted Project

Mon, May 14

labath committed rL332261: Revert "Remove Process references from the Host module".
Revert "Remove Process references from the Host module"
Mon, May 14, 9:58 AM
labath committed rL332255: Fix macosx build broken by r332250.
Fix macosx build broken by r332250
Mon, May 14, 9:05 AM
labath committed rL332250: Remove Process references from the Host module.
Remove Process references from the Host module
Mon, May 14, 8:17 AM
labath closed D46395: Remove Process references from the Host module.
Mon, May 14, 8:17 AM
labath committed rL332247: FileSpec: Remove PathSyntax enum and use llvm version instead.
FileSpec: Remove PathSyntax enum and use llvm version instead
Mon, May 14, 7:56 AM
labath closed D46753: FileSpec: Remove PathSyntax enum and use llvm version instead.
Mon, May 14, 7:56 AM
labath added a comment to D46685: [CodeGen] Disable structor optimizations at -O0.

I've reverted the patch as it has caused miscompiles on non-trivial inputs. I'll update the patch when I gain a better understanding of what is going on.

Mon, May 14, 7:54 AM
labath added a comment to D46753: FileSpec: Remove PathSyntax enum and use llvm version instead.

We don't export the style right now. At some point we may need to teach SBFileSpec about path styles, but at that point I'd just do the translation at the SB level, and let everyone else use the llvm enum.

Mon, May 14, 7:41 AM
labath committed rL332246: [CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly.
[CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly
Mon, May 14, 7:20 AM
labath closed D46748: [CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly.
Mon, May 14, 7:20 AM
labath added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Sounds like a plan.

Mon, May 14, 7:02 AM
labath added a comment to D46748: [CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly.

I meant for the missing ++NumErrors;. But on checking the existing test (debug-names-verify-entries.s) we already check the return value, which just happens to be non-zero because of the other failures. I'm okay with keeping it as is.

Mon, May 14, 6:44 AM
labath added a comment to D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer.

(If that is true, then I think this is workable, but there are still some details which need to be ironed out; e.g., m_first_die.GetFirstChild())

The current code calling GetUnitDIEOnly() (returning DWARFDIE) or GetUnitDIEPtrOnly (returning DWARFDebugInfoEntry *) is never dealing with child DIEs of what it gets - it also makes sense as there is no guarantee they have been read in.

I am not sure it's that simple. I've found at least one case (SymbolFileDWARF::ParseImportedModuleswhere we call GetFirstChild() on the value returned by GetUnitDIEOnly` (there may be others which are not easily greppable). Previously that would work if one would call this only after he'd know that all DIEs have been parsed. Now this will never work because GetFirstChild will return whatever is in the memory after m_first_die. I am not sure if this would be caught by asan straight away, though it will most likely cause a crash very soon.

Mon, May 14, 6:36 AM
labath committed rL332232: Revert "[CodeGen] Disable aggressive structor optimizations at -O0".
Revert "[CodeGen] Disable aggressive structor optimizations at -O0"
Mon, May 14, 4:39 AM
labath committed rC332232: Revert "[CodeGen] Disable aggressive structor optimizations at -O0".
Revert "[CodeGen] Disable aggressive structor optimizations at -O0"
Mon, May 14, 4:39 AM
labath committed rL332228: [CodeGen] Disable aggressive structor optimizations at -O0.
[CodeGen] Disable aggressive structor optimizations at -O0
Mon, May 14, 4:06 AM
labath committed rC332228: [CodeGen] Disable aggressive structor optimizations at -O0.
[CodeGen] Disable aggressive structor optimizations at -O0
Mon, May 14, 4:06 AM
labath closed D46685: [CodeGen] Disable structor optimizations at -O0.
Mon, May 14, 4:06 AM
labath added a comment to D46685: [CodeGen] Disable structor optimizations at -O0.

Thank you for the quick review.

Mon, May 14, 4:04 AM
labath added a comment to D46748: [CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly.

LGTM. I wonder if we should we have a negative test as well?

Mon, May 14, 3:42 AM
labath added a comment to D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty..

Preserving the dot if it is the only path component is perfectly reasonable -- "" is not a valid path and we shouldn't convert a valid path into an invalid one. However, I think this should be done on the llvm side, in the remote_dots function and not as a workaround on top of it.

Mon, May 14, 3:01 AM
labath added inline comments to D46726: build: use cmake to find the libedit content.
Mon, May 14, 2:56 AM
labath added a comment to D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer.

I think I understand what is going on here, but I can't say for certain. Could you elaborate on the implementation details of this somewhere. It would be good to keep a note of this for future maintainers. My understanding of this is:

  • if nothing has been parsed, m_die_array is empty and m_first_die is empty
  • if the cu die has been parsed, m_die_array us empty and m_first_die is full
  • if everything has been parsed m_first_die and m_die_array are full, and the first element of m_die_array contains a copy of m_first_die
Mon, May 14, 2:24 AM
labath accepted D46773: [lit] Fix several tests that fail when using Python 3 or on Windows.
Mon, May 14, 1:35 AM

Fri, May 11

labath created D46753: FileSpec: Remove PathSyntax enum and use llvm version instead.
Fri, May 11, 6:13 AM
labath updated the diff for D46685: [CodeGen] Disable structor optimizations at -O0.

Yes we can do that. Only the RAUW optimization is nasty, I've checked that the
debugger is able to use the alias with no problem.

Fri, May 11, 5:38 AM
labath created D46748: [CodeGen/AccelTable]: Handle -dwarf-linkage-names=Abstract correctly.
Fri, May 11, 5:16 AM