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.

In D47235#1109580, @zturner wrote:

In D47235#1109219, @labath wrote:

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.

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.

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

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.

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

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).

Ok, let's give this a try.

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.

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

In D46889#1104882, @aemerson wrote:

In D46889#1104823, @aprantl wrote:

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.

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.

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

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.

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

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.

• Use #ifndef approach for handling the constants.

• s/AppleIndex/AppleDWARFIndex
• move things to separate files

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

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:

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.

In D46934#1101963, @aprantl wrote:

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

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.

In D46588#1102363, @aprantl wrote:

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.

In D46887#1100111, @ruiu wrote:

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.

Thank you.

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.)

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.

(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.)

In D46783#1099561, @clayborg wrote:

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?

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.

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.

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.

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.

Sounds like a plan.

In D46748#1097654, @JDevlieghere wrote:

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.

In D46810#1097740, @jankratochvil wrote:

In D46810#1097503, @labath wrote:

(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.

Thank you for the quick review.

In D46748#1097598, @JDevlieghere wrote:

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

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.

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

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.